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

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

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