| Message ID | 1408618794-28497-4-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | New |
| Headers | show |
Hi On Thu, Aug 21, 2014 at 12:59 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > From: Sudeep Holla <sudeep.holla@arm.com> > > This patch creates a new class called "cpu" and assigns it to all the > cpu devices. This helps in grouping all the cpu devices and associated > child devices under the same class. > > This patch also: > 1. modifies the get_parent_device to return the legacy path > (/sys/devices/system/cpu/..) for the cpu class devices to support > existing sysfs ABI > 2. avoids creating link in the class directory pointing to the device as > there would be per-cpu instance of these devices with the same name > 3. makes sure subsystem symlink continues pointing to cpu bus instead of > cpu class for cpu devices This patch lacks any explanation _why_ you add a class for CPUs. With this patch applied, these directories are effectively the same: /sys/bus/cpu/devices/ /sys/class/cpu/ Why do we need a cpu-class if the same set of information is already available on the cpu-bus? Furthermore, classes are deprecated anyway. Everything you can do with a class can be solved with a bus. And we already have a bus for cpus. Thanks David > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/base/core.c | 39 +++++++++++++++++++++++++++++++++------ > drivers/base/cpu.c | 7 +++++++ > include/linux/cpu.h | 2 ++ > 3 files changed, 42 insertions(+), 6 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 20da3ad1696b..fe622e2a48d0 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -10,6 +10,7 @@ > * > */ > > +#include <linux/cpu.h> > #include <linux/device.h> > #include <linux/err.h> > #include <linux/init.h> > @@ -742,6 +743,12 @@ static struct kobject *get_device_parent(struct device *dev, > return &block_class.p->subsys.kobj; > } > #endif > + /* > + * if the device is in cpu class, then use the default/legacy > + * /sys/devices/system/cpu/.. path > + */ > + if (dev->class == cpu_class) > + return &parent->kobj; > > /* > * If we have no parent, we live in "virtual". > @@ -808,11 +815,17 @@ static int device_add_class_symlinks(struct device *dev) > if (!dev->class) > return 0; > > - error = sysfs_create_link(&dev->kobj, > - &dev->class->p->subsys.kobj, > - "subsystem"); > - if (error) > - goto out; > + /* > + * the subsystem symlink in each cpu device needs to continue > + * pointing to cpu bus > + */ > + if (dev->bus != &cpu_subsys) { > + error = sysfs_create_link(&dev->kobj, > + &dev->class->p->subsys.kobj, > + "subsystem"); > + if (error) > + goto out; > + } > > if (dev->parent && device_is_not_partition(dev)) { > error = sysfs_create_link(&dev->kobj, &dev->parent->kobj, > @@ -826,6 +839,13 @@ static int device_add_class_symlinks(struct device *dev) > if (sysfs_deprecated && dev->class == &block_class) > return 0; > #endif > + /* > + * don't create a link in the cpu class directory pointing to the > + * device as there would be per-cpu instance of these devices with > + * the same name > + */ > + if (dev->class == cpu_class) > + return 0; > > /* link in the class directory pointing to the device */ > error = sysfs_create_link(&dev->class->p->subsys.kobj, > @@ -851,11 +871,18 @@ static void device_remove_class_symlinks(struct device *dev) > > if (dev->parent && device_is_not_partition(dev)) > sysfs_remove_link(&dev->kobj, "device"); > - sysfs_remove_link(&dev->kobj, "subsystem"); > + > + /* if subsystem points to cpu bus, bus_remove_device will remove it */ > + if (dev->bus != &cpu_subsys) > + sysfs_remove_link(&dev->kobj, "subsystem"); > #ifdef CONFIG_BLOCK > if (sysfs_deprecated && dev->class == &block_class) > return; > #endif > + /* symlinks are not created for cpu class devices, nothing to remove */ > + if (dev->class == cpu_class) > + return; > + > sysfs_delete_link(&dev->class->p->subsys.kobj, &dev->kobj, dev_name(dev)); > } > > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > index 277a9cfa9040..8e380214625d 100644 > --- a/drivers/base/cpu.c > +++ b/drivers/base/cpu.c > @@ -319,6 +319,7 @@ static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env) > } > #endif > > +struct class *cpu_class; > /* > * register_cpu - Setup a sysfs device for a CPU. > * @cpu - cpu->hotpluggable field set to 1 will generate a control file in > @@ -335,6 +336,8 @@ int register_cpu(struct cpu *cpu, int num) > memset(&cpu->dev, 0x00, sizeof(struct device)); > cpu->dev.id = num; > cpu->dev.bus = &cpu_subsys; > + cpu->dev.parent = cpu_subsys.dev_root; > + cpu->dev.class = cpu_class; > cpu->dev.release = cpu_device_release; > cpu->dev.offline_disabled = !cpu->hotpluggable; > cpu->dev.offline = !cpu_online(num); > @@ -420,5 +423,9 @@ void __init cpu_dev_init(void) > if (subsys_system_register(&cpu_subsys, cpu_root_attr_groups)) > panic("Failed to register CPU subsystem"); > > + cpu_class = class_create(THIS_MODULE, "cpu"); > + if (IS_ERR(cpu_class)) > + panic("Failed to register CPU class"); > + > cpu_dev_register_generic(); > } > diff --git a/include/linux/cpu.h b/include/linux/cpu.h > index 95978ad7fcdd..8c0fc9b0acad 100644 > --- a/include/linux/cpu.h > +++ b/include/linux/cpu.h > @@ -39,6 +39,8 @@ extern void cpu_remove_dev_attr(struct device_attribute *attr); > extern int cpu_add_dev_attr_group(struct attribute_group *attrs); > extern void cpu_remove_dev_attr_group(struct attribute_group *attrs); > > +extern struct class *cpu_class; > + > #ifdef CONFIG_HOTPLUG_CPU > extern void unregister_cpu(struct cpu *cpu); > extern ssize_t arch_cpu_probe(const char *, size_t); > -- > 1.8.3.2 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 21/08/14 12:20, David Herrmann wrote: > Hi > > On Thu, Aug 21, 2014 at 12:59 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> From: Sudeep Holla <sudeep.holla@arm.com> >> >> This patch creates a new class called "cpu" and assigns it to all the >> cpu devices. This helps in grouping all the cpu devices and associated >> child devices under the same class. >> >> This patch also: >> 1. modifies the get_parent_device to return the legacy path >> (/sys/devices/system/cpu/..) for the cpu class devices to support >> existing sysfs ABI >> 2. avoids creating link in the class directory pointing to the device as >> there would be per-cpu instance of these devices with the same name >> 3. makes sure subsystem symlink continues pointing to cpu bus instead of >> cpu class for cpu devices > > This patch lacks any explanation _why_ you add a class for CPUs. With > this patch applied, these directories are effectively the same: > /sys/bus/cpu/devices/ > /sys/class/cpu/ > Yes that's the intention, so that we don't break any existing sysfs/ABI > Why do we need a cpu-class if the same set of information is already > available on the cpu-bus? Furthermore, classes are deprecated anyway. > Everything you can do with a class can be solved with a bus. And we > already have a bus for cpus. > This was suggested[1] by GregKH. The main reason it was added is to reuse the device attributes rather than creating the raw kobjects. It helps to move few other cpu related subsystems using raw kobjects to the device attribute groups. Regards, Sudeep [1] https://lkml.org/lkml/2014/2/10/766 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Hi On Thu, Aug 21, 2014 at 2:30 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 21/08/14 12:20, David Herrmann wrote: >> Why do we need a cpu-class if the same set of information is already >> available on the cpu-bus? Furthermore, classes are deprecated anyway. >> Everything you can do with a class can be solved with a bus. And we >> already have a bus for cpus. >> > > This was suggested[1] by GregKH. The main reason it was added is to > reuse the device attributes rather than creating the raw kobjects. > > It helps to move few other cpu related subsystems using raw kobjects to > the device attribute groups. So the only reason to add a class is to get attributes registered properly with the device? Just set dev->groups before calling device_add()? This works just fine on buses, too. Thanks David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 21/08/14 13:37, David Herrmann wrote: > Hi > > On Thu, Aug 21, 2014 at 2:30 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 21/08/14 12:20, David Herrmann wrote: >>> Why do we need a cpu-class if the same set of information is already >>> available on the cpu-bus? Furthermore, classes are deprecated anyway. >>> Everything you can do with a class can be solved with a bus. And we >>> already have a bus for cpus. >>> Sorry I wasn't aware that classes are deprecated. IIUC its class_device struct and related APIs that are deprecated not struct class. I may be wrong but Greg would not have suggested to use class if it was deprecated. >> >> This was suggested[1] by GregKH. The main reason it was added is to >> reuse the device attributes rather than creating the raw kobjects. >> >> It helps to move few other cpu related subsystems using raw kobjects to >> the device attribute groups. > > So the only reason to add a class is to get attributes registered > properly with the device? Just set dev->groups before calling > device_add()? This works just fine on buses, too. > Do you mean to say create bus for each cpu and add cpu devices like cache under that ? We can't reuse the cpu bus i.e cpu_subsys as we can't add per-cpu devices with same name under /sys/bus/cpu/devices/ Also adding bus for each cpu device might be more complex than creating cpu class. That's the reason why I added checks to avoid creating link in the class directory pointing to the device. It would be difficult to do that with bus as it can't be unconditional. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Thu, Aug 21, 2014 at 2:30 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > On 21/08/14 12:20, David Herrmann wrote: >> On Thu, Aug 21, 2014 at 12:59 PM, Sudeep Holla <sudeep.holla@arm.com> >> wrote: >>> >>> From: Sudeep Holla <sudeep.holla@arm.com> >>> >>> This patch creates a new class called "cpu" and assigns it to all the >>> cpu devices. This helps in grouping all the cpu devices and associated >>> child devices under the same class. >>> >>> This patch also: >>> 1. modifies the get_parent_device to return the legacy path >>> (/sys/devices/system/cpu/..) for the cpu class devices to support >>> existing sysfs ABI >>> 2. avoids creating link in the class directory pointing to the device as >>> there would be per-cpu instance of these devices with the same name >>> 3. makes sure subsystem symlink continues pointing to cpu bus instead of >>> cpu class for cpu devices >> >> >> This patch lacks any explanation _why_ you add a class for CPUs. With >> this patch applied, these directories are effectively the same: >> /sys/bus/cpu/devices/ >> /sys/class/cpu/ >> > > Yes that's the intention, so that we don't break any existing sysfs/ABI > > >> Why do we need a cpu-class if the same set of information is already >> available on the cpu-bus? Furthermore, classes are deprecated anyway. >> Everything you can do with a class can be solved with a bus. And we >> already have a bus for cpus. >> > > This was suggested[1] by GregKH. The main reason it was added is to > reuse the device attributes rather than creating the raw kobjects. > > It helps to move few other cpu related subsystems using raw kobjects to > the device attribute groups. No, nothing should ever create a bus and a class with the same name. This is not supported by userspace tools. Your problem needs to be addressed by adding things to the existing "cpu" bus, not by adding a new class. Kay -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/drivers/base/core.c b/drivers/base/core.c index 20da3ad1696b..fe622e2a48d0 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -10,6 +10,7 @@ * */ +#include <linux/cpu.h> #include <linux/device.h> #include <linux/err.h> #include <linux/init.h> @@ -742,6 +743,12 @@ static struct kobject *get_device_parent(struct device *dev, return &block_class.p->subsys.kobj; } #endif + /* + * if the device is in cpu class, then use the default/legacy + * /sys/devices/system/cpu/.. path + */ + if (dev->class == cpu_class) + return &parent->kobj; /* * If we have no parent, we live in "virtual". @@ -808,11 +815,17 @@ static int device_add_class_symlinks(struct device *dev) if (!dev->class) return 0; - error = sysfs_create_link(&dev->kobj, - &dev->class->p->subsys.kobj, - "subsystem"); - if (error) - goto out; + /* + * the subsystem symlink in each cpu device needs to continue + * pointing to cpu bus + */ + if (dev->bus != &cpu_subsys) { + error = sysfs_create_link(&dev->kobj, + &dev->class->p->subsys.kobj, + "subsystem"); + if (error) + goto out; + } if (dev->parent && device_is_not_partition(dev)) { error = sysfs_create_link(&dev->kobj, &dev->parent->kobj, @@ -826,6 +839,13 @@ static int device_add_class_symlinks(struct device *dev) if (sysfs_deprecated && dev->class == &block_class) return 0; #endif + /* + * don't create a link in the cpu class directory pointing to the + * device as there would be per-cpu instance of these devices with + * the same name + */ + if (dev->class == cpu_class) + return 0; /* link in the class directory pointing to the device */ error = sysfs_create_link(&dev->class->p->subsys.kobj, @@ -851,11 +871,18 @@ static void device_remove_class_symlinks(struct device *dev) if (dev->parent && device_is_not_partition(dev)) sysfs_remove_link(&dev->kobj, "device"); - sysfs_remove_link(&dev->kobj, "subsystem"); + + /* if subsystem points to cpu bus, bus_remove_device will remove it */ + if (dev->bus != &cpu_subsys) + sysfs_remove_link(&dev->kobj, "subsystem"); #ifdef CONFIG_BLOCK if (sysfs_deprecated && dev->class == &block_class) return; #endif + /* symlinks are not created for cpu class devices, nothing to remove */ + if (dev->class == cpu_class) + return; + sysfs_delete_link(&dev->class->p->subsys.kobj, &dev->kobj, dev_name(dev)); } diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c index 277a9cfa9040..8e380214625d 100644 --- a/drivers/base/cpu.c +++ b/drivers/base/cpu.c @@ -319,6 +319,7 @@ static int cpu_uevent(struct device *dev, struct kobj_uevent_env *env) } #endif +struct class *cpu_class; /* * register_cpu - Setup a sysfs device for a CPU. * @cpu - cpu->hotpluggable field set to 1 will generate a control file in @@ -335,6 +336,8 @@ int register_cpu(struct cpu *cpu, int num) memset(&cpu->dev, 0x00, sizeof(struct device)); cpu->dev.id = num; cpu->dev.bus = &cpu_subsys; + cpu->dev.parent = cpu_subsys.dev_root; + cpu->dev.class = cpu_class; cpu->dev.release = cpu_device_release; cpu->dev.offline_disabled = !cpu->hotpluggable; cpu->dev.offline = !cpu_online(num); @@ -420,5 +423,9 @@ void __init cpu_dev_init(void) if (subsys_system_register(&cpu_subsys, cpu_root_attr_groups)) panic("Failed to register CPU subsystem"); + cpu_class = class_create(THIS_MODULE, "cpu"); + if (IS_ERR(cpu_class)) + panic("Failed to register CPU class"); + cpu_dev_register_generic(); } diff --git a/include/linux/cpu.h b/include/linux/cpu.h index 95978ad7fcdd..8c0fc9b0acad 100644 --- a/include/linux/cpu.h +++ b/include/linux/cpu.h @@ -39,6 +39,8 @@ extern void cpu_remove_dev_attr(struct device_attribute *attr); extern int cpu_add_dev_attr_group(struct attribute_group *attrs); extern void cpu_remove_dev_attr_group(struct attribute_group *attrs); +extern struct class *cpu_class; + #ifdef CONFIG_HOTPLUG_CPU extern void unregister_cpu(struct cpu *cpu); extern ssize_t arch_cpu_probe(const char *, size_t);