Re: [PATCH] drivers: base: add cpu_device_create to support per-cpu devices

[Sudeep Holla <sudeep.holla@arm.com>]

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