| Message ID | 1408706963-23195-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi On Fri, Aug 22, 2014 at 1:29 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > From: Sudeep Holla <sudeep.holla@arm.com> > > This patch adds a new function to create per-cpu devices. > This helps in: > 1. reusing the device infrastructure to create any cpu related > attributes and corresponding sysfs instead of creating and > dealing with raw kobjects directly > 2. retaining the legacy path(/sys/devices/system/cpu/..) to support > existing sysfs ABI > 3. avoiding to create links in the bus directory pointing to the > device as there would be per-cpu instance of these devices with > the same name since dev->bus is not populated to cpu_sysbus on > purpose > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/base/cpu.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/cpu.h | 4 ++++ > 2 files changed, 58 insertions(+) > > Hi Greg, > > Here is the alternate solution I could come up with instead of > creating cpu class. cpu_device_create is very similar to > device_create_groups_vargs w/o class support, but I could not > reuse anything else to avoid creating similar function. > > Let me know your thoughts/suggestions on this. > > Regards, > Sudeep > > > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > index 277a9cfa9040..53f0c4141d05 100644 > --- a/drivers/base/cpu.c > +++ b/drivers/base/cpu.c > @@ -363,6 +363,60 @@ struct device *get_cpu_device(unsigned cpu) > } > EXPORT_SYMBOL_GPL(get_cpu_device); > > +static void device_create_release(struct device *dev) > +{ > + kfree(dev); > +} > + > +static struct device * > +__cpu_device_create(struct device *parent, void *drvdata, > + const struct attribute_group **groups, > + const char *fmt, va_list args) > +{ > + struct device *dev = NULL; > + int retval = -ENODEV; > + > + dev = kzalloc(sizeof(*dev), GFP_KERNEL); > + if (!dev) { > + retval = -ENOMEM; > + goto error; > + } > + > + device_initialize(dev); > + dev->parent = parent; > + dev->groups = groups; > + dev->release = device_create_release; > + dev_set_drvdata(dev, drvdata); > + > + retval = kobject_set_name_vargs(&dev->kobj, fmt, args); > + if (retval) > + goto error; > + > + retval = device_add(dev); > + if (retval) > + goto error; Exactly! As I said, simply setting dev->groups before calling device_add(). However, I really don't understand why we need this as global API. Skimming over the other patches, you use cpu_device_create() only in one place. Why not hard-code this all there? It is totally OK to do device initialization in drivers. All the helpers (like device_create(), device_create_with_groups(), and so on) are just convenience functions. The driver-core API explicitly allows drivers to initialize devices manually. Nevertheless, this patch looks fine. Thanks David > + > + return dev; > + > +error: > + put_device(dev); > + return ERR_PTR(retval); > +} > + > +struct device *cpu_device_create(struct device *parent, void *drvdata, > + const struct attribute_group **groups, > + const char *fmt, ...) > +{ > + va_list vargs; > + struct device *dev; > + > + va_start(vargs, fmt); > + dev = __cpu_device_create(parent, drvdata, groups, fmt, vargs); > + va_end(vargs); > + return dev; > +} > +EXPORT_SYMBOL_GPL(cpu_device_create); > + > #ifdef CONFIG_GENERIC_CPU_AUTOPROBE > static DEVICE_ATTR(modalias, 0444, print_cpu_modalias, NULL); > #endif > diff --git a/include/linux/cpu.h b/include/linux/cpu.h > index 95978ad7fcdd..bb790a5621c0 100644 > --- a/include/linux/cpu.h > +++ b/include/linux/cpu.h > @@ -19,6 +19,7 @@ > > struct device; > struct device_node; > +struct attribute_group; > > struct cpu { > int node_id; /* The node which contains the CPU */ > @@ -39,6 +40,9 @@ 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 device *cpu_device_create(struct device *parent, void *drvdata, > + const struct attribute_group **groups, > + const char *fmt, ...); > #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/
Hi On Fri, Aug 22, 2014 at 1:37 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Fri, Aug 22, 2014 at 1:29 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> From: Sudeep Holla <sudeep.holla@arm.com> >> >> This patch adds a new function to create per-cpu devices. >> This helps in: >> 1. reusing the device infrastructure to create any cpu related >> attributes and corresponding sysfs instead of creating and >> dealing with raw kobjects directly >> 2. retaining the legacy path(/sys/devices/system/cpu/..) to support >> existing sysfs ABI >> 3. avoiding to create links in the bus directory pointing to the >> device as there would be per-cpu instance of these devices with >> the same name since dev->bus is not populated to cpu_sysbus on >> purpose >> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> --- >> drivers/base/cpu.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/cpu.h | 4 ++++ >> 2 files changed, 58 insertions(+) >> >> Hi Greg, >> >> Here is the alternate solution I could come up with instead of >> creating cpu class. cpu_device_create is very similar to >> device_create_groups_vargs w/o class support, but I could not >> reuse anything else to avoid creating similar function. >> >> Let me know your thoughts/suggestions on this. >> >> Regards, >> Sudeep >> >> >> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c >> index 277a9cfa9040..53f0c4141d05 100644 >> --- a/drivers/base/cpu.c >> +++ b/drivers/base/cpu.c >> @@ -363,6 +363,60 @@ struct device *get_cpu_device(unsigned cpu) >> } >> EXPORT_SYMBOL_GPL(get_cpu_device); >> >> +static void device_create_release(struct device *dev) >> +{ >> + kfree(dev); >> +} >> + >> +static struct device * >> +__cpu_device_create(struct device *parent, void *drvdata, >> + const struct attribute_group **groups, >> + const char *fmt, va_list args) >> +{ >> + struct device *dev = NULL; >> + int retval = -ENODEV; >> + >> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >> + if (!dev) { >> + retval = -ENOMEM; >> + goto error; >> + } >> + >> + device_initialize(dev); >> + dev->parent = parent; >> + dev->groups = groups; >> + dev->release = device_create_release; >> + dev_set_drvdata(dev, drvdata); >> + >> + retval = kobject_set_name_vargs(&dev->kobj, fmt, args); >> + if (retval) >> + goto error; >> + >> + retval = device_add(dev); >> + if (retval) >> + goto error; > > Exactly! As I said, simply setting dev->groups before calling device_add(). > > However, I really don't understand why we need this as global API. > Skimming over the other patches, you use cpu_device_create() only in > one place. Why not hard-code this all there? It is totally OK to do > device initialization in drivers. All the helpers (like > device_create(), device_create_with_groups(), and so on) are just > convenience functions. The driver-core API explicitly allows drivers > to initialize devices manually. > > Nevertheless, this patch looks fine. Wait, no. Why don't you set dev->bus to cpu_subsys? Is this thing supposed to create child-devices of CPUs? Can you describe what your topology is supposed to look like? 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 22/08/14 12:37, David Herrmann wrote: > Hi > > On Fri, Aug 22, 2014 at 1:29 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> From: Sudeep Holla <sudeep.holla@arm.com> >> >> This patch adds a new function to create per-cpu devices. >> This helps in: >> 1. reusing the device infrastructure to create any cpu related >> attributes and corresponding sysfs instead of creating and >> dealing with raw kobjects directly >> 2. retaining the legacy path(/sys/devices/system/cpu/..) to support >> existing sysfs ABI >> 3. avoiding to create links in the bus directory pointing to the >> device as there would be per-cpu instance of these devices with >> the same name since dev->bus is not populated to cpu_sysbus on >> purpose >> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> --- >> drivers/base/cpu.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/cpu.h | 4 ++++ >> 2 files changed, 58 insertions(+) >> >> Hi Greg, >> >> Here is the alternate solution I could come up with instead of >> creating cpu class. cpu_device_create is very similar to >> device_create_groups_vargs w/o class support, but I could not >> reuse anything else to avoid creating similar function. >> >> Let me know your thoughts/suggestions on this. >> >> Regards, >> Sudeep >> >> >> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c >> index 277a9cfa9040..53f0c4141d05 100644 >> --- a/drivers/base/cpu.c >> +++ b/drivers/base/cpu.c >> @@ -363,6 +363,60 @@ struct device *get_cpu_device(unsigned cpu) >> } >> EXPORT_SYMBOL_GPL(get_cpu_device); >> >> +static void device_create_release(struct device *dev) >> +{ >> + kfree(dev); >> +} >> + >> +static struct device * >> +__cpu_device_create(struct device *parent, void *drvdata, >> + const struct attribute_group **groups, >> + const char *fmt, va_list args) >> +{ >> + struct device *dev = NULL; >> + int retval = -ENODEV; >> + >> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >> + if (!dev) { >> + retval = -ENOMEM; >> + goto error; >> + } >> + >> + device_initialize(dev); >> + dev->parent = parent; >> + dev->groups = groups; >> + dev->release = device_create_release; >> + dev_set_drvdata(dev, drvdata); >> + >> + retval = kobject_set_name_vargs(&dev->kobj, fmt, args); >> + if (retval) >> + goto error; >> + >> + retval = device_add(dev); >> + if (retval) >> + goto error; > > Exactly! As I said, simply setting dev->groups before calling device_add(). > > However, I really don't understand why we need this as global API. Yes right now it's used only in cacheinfo. The main reason why I am exporting it is that we can reuse it in some places like cpuidle/freq which deals with raw kobjects directly and can be moved to device attributes. > Skimming over the other patches, you use cpu_device_create() only in > one place. Why not hard-code this all there? It is totally OK to do > device initialization in drivers. All the helpers (like > device_create(), device_create_with_groups(), and so on) are just > convenience functions. The driver-core API explicitly allows drivers > to initialize devices manually. > > Nevertheless, this patch looks fine. > Thanks for having a look at this. 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 22/08/14 12:41, David Herrmann wrote: > Hi > > On Fri, Aug 22, 2014 at 1:37 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >> Hi >> >> On Fri, Aug 22, 2014 at 1:29 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> From: Sudeep Holla <sudeep.holla@arm.com> >>> >>> This patch adds a new function to create per-cpu devices. >>> This helps in: >>> 1. reusing the device infrastructure to create any cpu related >>> attributes and corresponding sysfs instead of creating and >>> dealing with raw kobjects directly >>> 2. retaining the legacy path(/sys/devices/system/cpu/..) to support >>> existing sysfs ABI >>> 3. avoiding to create links in the bus directory pointing to the >>> device as there would be per-cpu instance of these devices with >>> the same name since dev->bus is not populated to cpu_sysbus on >>> purpose >>> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> --- >>> drivers/base/cpu.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/cpu.h | 4 ++++ >>> 2 files changed, 58 insertions(+) >>> >>> Hi Greg, >>> >>> Here is the alternate solution I could come up with instead of >>> creating cpu class. cpu_device_create is very similar to >>> device_create_groups_vargs w/o class support, but I could not >>> reuse anything else to avoid creating similar function. >>> >>> Let me know your thoughts/suggestions on this. >>> >>> Regards, >>> Sudeep >>> >>> >>> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c >>> index 277a9cfa9040..53f0c4141d05 100644 >>> --- a/drivers/base/cpu.c >>> +++ b/drivers/base/cpu.c >>> @@ -363,6 +363,60 @@ struct device *get_cpu_device(unsigned cpu) >>> } >>> EXPORT_SYMBOL_GPL(get_cpu_device); >>> >>> +static void device_create_release(struct device *dev) >>> +{ >>> + kfree(dev); >>> +} >>> + >>> +static struct device * >>> +__cpu_device_create(struct device *parent, void *drvdata, >>> + const struct attribute_group **groups, >>> + const char *fmt, va_list args) >>> +{ >>> + struct device *dev = NULL; >>> + int retval = -ENODEV; >>> + >>> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >>> + if (!dev) { >>> + retval = -ENOMEM; >>> + goto error; >>> + } >>> + >>> + device_initialize(dev); >>> + dev->parent = parent; >>> + dev->groups = groups; >>> + dev->release = device_create_release; >>> + dev_set_drvdata(dev, drvdata); >>> + >>> + retval = kobject_set_name_vargs(&dev->kobj, fmt, args); >>> + if (retval) >>> + goto error; >>> + >>> + retval = device_add(dev); >>> + if (retval) >>> + goto error; >> >> Exactly! As I said, simply setting dev->groups before calling device_add(). >> >> However, I really don't understand why we need this as global API. >> Skimming over the other patches, you use cpu_device_create() only in >> one place. Why not hard-code this all there? It is totally OK to do >> device initialization in drivers. All the helpers (like >> device_create(), device_create_with_groups(), and so on) are just >> convenience functions. The driver-core API explicitly allows drivers >> to initialize devices manually. >> >> Nevertheless, this patch looks fine. > > Wait, no. Why don't you set dev->bus to cpu_subsys? Is this thing > supposed to create child-devices of CPUs? Can you describe what your > topology is supposed to look like? > Yes, it's not done on purpose as mentioned in the commit log. E.g.: cacheinfo topology will be as below /sys/devices/system/cpu/cpuX/cache/index0/<attributes> /sys/devices/system/cpu/cpuX/cache/index1/<attributes> .. /sys/devices/system/cpu/cpuX/cache/index<Y/<attributes> In this case 'cache' is cpuX's child and index<0..Y> are children of cache in cpuX. The main problem with per-cpu device is that they have same name for each cpu's instance and when the bus is set to this devices, the driver model tries to create symlink to each of these devices in /sys/bus/cpu/... which fails. 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/
Hi David, On 22/08/14 13:33, Sudeep Holla wrote: > > > On 22/08/14 12:41, David Herrmann wrote: >> Hi >> >> On Fri, Aug 22, 2014 at 1:37 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >>> Hi >>> >>> On Fri, Aug 22, 2014 at 1:29 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>> From: Sudeep Holla <sudeep.holla@arm.com> >>>> >>>> This patch adds a new function to create per-cpu devices. >>>> This helps in: >>>> 1. reusing the device infrastructure to create any cpu related >>>> attributes and corresponding sysfs instead of creating and >>>> dealing with raw kobjects directly >>>> 2. retaining the legacy path(/sys/devices/system/cpu/..) to support >>>> existing sysfs ABI >>>> 3. avoiding to create links in the bus directory pointing to the >>>> device as there would be per-cpu instance of these devices with >>>> the same name since dev->bus is not populated to cpu_sysbus on >>>> purpose >>>> >>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>> --- >>>> drivers/base/cpu.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> include/linux/cpu.h | 4 ++++ >>>> 2 files changed, 58 insertions(+) >>>> >>>> Hi Greg, >>>> >>>> Here is the alternate solution I could come up with instead of >>>> creating cpu class. cpu_device_create is very similar to >>>> device_create_groups_vargs w/o class support, but I could not >>>> reuse anything else to avoid creating similar function. >>>> >>>> Let me know your thoughts/suggestions on this. >>>> >>>> Regards, >>>> Sudeep >>>> >>>> >>>> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c >>>> index 277a9cfa9040..53f0c4141d05 100644 >>>> --- a/drivers/base/cpu.c >>>> +++ b/drivers/base/cpu.c >>>> @@ -363,6 +363,60 @@ struct device *get_cpu_device(unsigned cpu) >>>> } >>>> EXPORT_SYMBOL_GPL(get_cpu_device); >>>> >>>> +static void device_create_release(struct device *dev) >>>> +{ >>>> + kfree(dev); >>>> +} >>>> + >>>> +static struct device * >>>> +__cpu_device_create(struct device *parent, void *drvdata, >>>> + const struct attribute_group **groups, >>>> + const char *fmt, va_list args) >>>> +{ >>>> + struct device *dev = NULL; >>>> + int retval = -ENODEV; >>>> + >>>> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >>>> + if (!dev) { >>>> + retval = -ENOMEM; >>>> + goto error; >>>> + } >>>> + >>>> + device_initialize(dev); >>>> + dev->parent = parent; >>>> + dev->groups = groups; >>>> + dev->release = device_create_release; >>>> + dev_set_drvdata(dev, drvdata); >>>> + >>>> + retval = kobject_set_name_vargs(&dev->kobj, fmt, args); >>>> + if (retval) >>>> + goto error; >>>> + >>>> + retval = device_add(dev); >>>> + if (retval) >>>> + goto error; >>> >>> Exactly! As I said, simply setting dev->groups before calling device_add(). >>> >>> However, I really don't understand why we need this as global API. >>> Skimming over the other patches, you use cpu_device_create() only in >>> one place. Why not hard-code this all there? It is totally OK to do >>> device initialization in drivers. All the helpers (like >>> device_create(), device_create_with_groups(), and so on) are just >>> convenience functions. The driver-core API explicitly allows drivers >>> to initialize devices manually. >>> >>> Nevertheless, this patch looks fine. >> >> Wait, no. Why don't you set dev->bus to cpu_subsys? Is this thing >> supposed to create child-devices of CPUs? Can you describe what your >> topology is supposed to look like? >> > > Yes, it's not done on purpose as mentioned in the commit log. > E.g.: cacheinfo topology will be as below > > /sys/devices/system/cpu/cpuX/cache/index0/<attributes> > /sys/devices/system/cpu/cpuX/cache/index1/<attributes> > .. > /sys/devices/system/cpu/cpuX/cache/index<Y/<attributes> > Does the above topology looks fine to you ? Since the parent is set properly, not setting bus will not cause any issue to the topology. > In this case 'cache' is cpuX's child and index<0..Y> are children of > cache in cpuX. The main problem with per-cpu device is that they have > same name for each cpu's instance and when the bus is set to this > devices, the driver model tries to create symlink to each of these > devices in /sys/bus/cpu/... which fails. > Here is the exact issue, probably kernel dump is easier to explain. It fails for second CPU as the sysfs path already exists. WARNING: CPU: 1 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x51/0x5c() sysfs: cannot create duplicate filename '/bus/cpu/devices/cache' Modules linked in: CPU: 1 PID: 1 Comm: swapper/0 Not tainted 3.17.0-rc2-00013-g7956a439b183-dirty #89 [<c0013c3d>] (unwind_backtrace) from [<c0010581>] (show_stack+0x11/0x14) [<c0010581>] (show_stack) from [<c04e9419>] (dump_stack+0x69/0x74) [<c04e9419>] (dump_stack) from [<c00204cb>] (warn_slowpath_common+0x5f/0x78) [<c00204cb>] (warn_slowpath_common) from [<c0020507>] (warn_slowpath_fmt+0x23/0x2c) [<c0020507>] (warn_slowpath_fmt) from [<c013b921>] (sysfs_warn_dup+0x51/0x5c) [<c013b921>] (sysfs_warn_dup) from [<c013bb6d>] (sysfs_do_create_link_sd.isra.2+0x91/0x94) [<c013bb6d>] (sysfs_do_create_link_sd.isra.2) from [<c031d4f3>] (bus_add_device+0xab/0x134) [<c031d4f3>] (bus_add_device) from [<c031c16d>] (device_add+0x2a1/0x3dc) [<c031c16d>] (device_add) from [<c031f905>] (cpu_device_create+0x85/0x94) Hi Greg, Any suggestions to proceed on this ? 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/
Hi On Tue, Aug 26, 2014 at 6:54 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> Yes, it's not done on purpose as mentioned in the commit log. >> E.g.: cacheinfo topology will be as below >> >> /sys/devices/system/cpu/cpuX/cache/index0/<attributes> >> /sys/devices/system/cpu/cpuX/cache/index1/<attributes> >> .. >> /sys/devices/system/cpu/cpuX/cache/index<Y/<attributes> >> > > Does the above topology looks fine to you ? Since the parent is set > properly, not setting bus will not cause any issue to the topology. Sure, looks fine. 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/
Hi Greg, On 22/08/14 12:29, Sudeep Holla wrote: > From: Sudeep Holla <sudeep.holla@arm.com> > > This patch adds a new function to create per-cpu devices. > This helps in: > 1. reusing the device infrastructure to create any cpu related > attributes and corresponding sysfs instead of creating and > dealing with raw kobjects directly > 2. retaining the legacy path(/sys/devices/system/cpu/..) to support > existing sysfs ABI > 3. avoiding to create links in the bus directory pointing to the > device as there would be per-cpu instance of these devices with > the same name since dev->bus is not populated to cpu_sysbus on > purpose > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/base/cpu.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/cpu.h | 4 ++++ > 2 files changed, 58 insertions(+) > > Hi Greg, > > Here is the alternate solution I could come up with instead of > creating cpu class. cpu_device_create is very similar to > device_create_groups_vargs w/o class support, but I could not > reuse anything else to avoid creating similar function. > > Let me know your thoughts/suggestions on this. > Any feedback on this ? If ok, I will respin cacheinfo series removing cpu class creation with this patch. 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 Tue, Sep 02, 2014 at 06:22:07PM +0100, Sudeep Holla wrote: > Hi Greg, > > On 22/08/14 12:29, Sudeep Holla wrote: > >From: Sudeep Holla <sudeep.holla@arm.com> > > > >This patch adds a new function to create per-cpu devices. > >This helps in: > >1. reusing the device infrastructure to create any cpu related > > attributes and corresponding sysfs instead of creating and > > dealing with raw kobjects directly > >2. retaining the legacy path(/sys/devices/system/cpu/..) to support > > existing sysfs ABI > >3. avoiding to create links in the bus directory pointing to the > > device as there would be per-cpu instance of these devices with > > the same name since dev->bus is not populated to cpu_sysbus on > > purpose > > > >Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > >Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >--- > > drivers/base/cpu.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/cpu.h | 4 ++++ > > 2 files changed, 58 insertions(+) > > > >Hi Greg, > > > >Here is the alternate solution I could come up with instead of > >creating cpu class. cpu_device_create is very similar to > >device_create_groups_vargs w/o class support, but I could not > >reuse anything else to avoid creating similar function. > > > >Let me know your thoughts/suggestions on this. > > > > Any feedback on this ? If ok, I will respin cacheinfo series removing > cpu class creation with this patch. Please address the issues others have already raised. -- 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 Greg, On 02/09/14 18:26, Greg Kroah-Hartman wrote: > On Tue, Sep 02, 2014 at 06:22:07PM +0100, Sudeep Holla wrote: >> Hi Greg, >> >> On 22/08/14 12:29, Sudeep Holla wrote: >>> From: Sudeep Holla <sudeep.holla@arm.com> >>> >>> This patch adds a new function to create per-cpu devices. >>> This helps in: >>> 1. reusing the device infrastructure to create any cpu related >>> attributes and corresponding sysfs instead of creating and >>> dealing with raw kobjects directly >>> 2. retaining the legacy path(/sys/devices/system/cpu/..) to support >>> existing sysfs ABI >>> 3. avoiding to create links in the bus directory pointing to the >>> device as there would be per-cpu instance of these devices with >>> the same name since dev->bus is not populated to cpu_sysbus on >>> purpose >>> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> --- >>> drivers/base/cpu.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/cpu.h | 4 ++++ >>> 2 files changed, 58 insertions(+) >>> >>> Hi Greg, >>> >>> Here is the alternate solution I could come up with instead of >>> creating cpu class. cpu_device_create is very similar to >>> device_create_groups_vargs w/o class support, but I could not >>> reuse anything else to avoid creating similar function. >>> >>> Let me know your thoughts/suggestions on this. >>> >> >> Any feedback on this ? If ok, I will respin cacheinfo series removing >> cpu class creation with this patch. > > Please address the issues others have already raised. > If you meant issues raised on cacheinfo series, I have addressed them already in v3(raised by you, Stephen Boyd, Mark and Russell). I have retained hotplug notifiers which Stephen wanted to remove as I don't know if that's acceptable on all the architectures. I posted this single patch as a replacement to the patch creating cpu class as Kay and David suggested that we can't have class and bus with same name. 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 Tue, Sep 02, 2014 at 06:40:21PM +0100, Sudeep Holla wrote: > Hi Greg, > > On 02/09/14 18:26, Greg Kroah-Hartman wrote: > >On Tue, Sep 02, 2014 at 06:22:07PM +0100, Sudeep Holla wrote: > >>Hi Greg, > >> > >>On 22/08/14 12:29, Sudeep Holla wrote: > >>>From: Sudeep Holla <sudeep.holla@arm.com> > >>> > >>>This patch adds a new function to create per-cpu devices. > >>>This helps in: > >>>1. reusing the device infrastructure to create any cpu related > >>> attributes and corresponding sysfs instead of creating and > >>> dealing with raw kobjects directly > >>>2. retaining the legacy path(/sys/devices/system/cpu/..) to support > >>> existing sysfs ABI > >>>3. avoiding to create links in the bus directory pointing to the > >>> device as there would be per-cpu instance of these devices with > >>> the same name since dev->bus is not populated to cpu_sysbus on > >>> purpose > >>> > >>>Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > >>>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >>>--- > >>> drivers/base/cpu.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > >>> include/linux/cpu.h | 4 ++++ > >>> 2 files changed, 58 insertions(+) > >>> > >>>Hi Greg, > >>> > >>>Here is the alternate solution I could come up with instead of > >>>creating cpu class. cpu_device_create is very similar to > >>>device_create_groups_vargs w/o class support, but I could not > >>>reuse anything else to avoid creating similar function. > >>> > >>>Let me know your thoughts/suggestions on this. > >>> > >> > >>Any feedback on this ? If ok, I will respin cacheinfo series removing > >>cpu class creation with this patch. > > > >Please address the issues others have already raised. > > > > If you meant issues raised on cacheinfo series, I have addressed them > already in v3(raised by you, Stephen Boyd, Mark and Russell). I have > retained hotplug notifiers which Stephen wanted to remove as I don't > know if that's acceptable on all the architectures. > > I posted this single patch as a replacement to the patch creating cpu > class as Kay and David suggested that we can't have class and bus with > same name. Then I have no idea what is going on, sorry, please respin as you see fit and resend... thanks, greg k-h -- 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/cpu.c b/drivers/base/cpu.c index 277a9cfa9040..53f0c4141d05 100644 --- a/drivers/base/cpu.c +++ b/drivers/base/cpu.c @@ -363,6 +363,60 @@ struct device *get_cpu_device(unsigned cpu) } EXPORT_SYMBOL_GPL(get_cpu_device); +static void device_create_release(struct device *dev) +{ + kfree(dev); +} + +static struct device * +__cpu_device_create(struct device *parent, void *drvdata, + const struct attribute_group **groups, + const char *fmt, va_list args) +{ + struct device *dev = NULL; + int retval = -ENODEV; + + dev = kzalloc(sizeof(*dev), GFP_KERNEL); + if (!dev) { + retval = -ENOMEM; + goto error; + } + + device_initialize(dev); + dev->parent = parent; + dev->groups = groups; + dev->release = device_create_release; + dev_set_drvdata(dev, drvdata); + + retval = kobject_set_name_vargs(&dev->kobj, fmt, args); + if (retval) + goto error; + + retval = device_add(dev); + if (retval) + goto error; + + return dev; + +error: + put_device(dev); + return ERR_PTR(retval); +} + +struct device *cpu_device_create(struct device *parent, void *drvdata, + const struct attribute_group **groups, + const char *fmt, ...) +{ + va_list vargs; + struct device *dev; + + va_start(vargs, fmt); + dev = __cpu_device_create(parent, drvdata, groups, fmt, vargs); + va_end(vargs); + return dev; +} +EXPORT_SYMBOL_GPL(cpu_device_create); + #ifdef CONFIG_GENERIC_CPU_AUTOPROBE static DEVICE_ATTR(modalias, 0444, print_cpu_modalias, NULL); #endif diff --git a/include/linux/cpu.h b/include/linux/cpu.h index 95978ad7fcdd..bb790a5621c0 100644 --- a/include/linux/cpu.h +++ b/include/linux/cpu.h @@ -19,6 +19,7 @@ struct device; struct device_node; +struct attribute_group; struct cpu { int node_id; /* The node which contains the CPU */ @@ -39,6 +40,9 @@ 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 device *cpu_device_create(struct device *parent, void *drvdata, + const struct attribute_group **groups, + const char *fmt, ...); #ifdef CONFIG_HOTPLUG_CPU extern void unregister_cpu(struct cpu *cpu); extern ssize_t arch_cpu_probe(const char *, size_t);