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

[Sudeep Holla]

On 22/08/14 12:37, David Herrmann wrote:
> Hi
>
> On Fri, Aug 22, 2014 at 1:29 PM, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> > From: Sudeep Holla <sudeep.holla@xxxxxxx>
> >
> > 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@xxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/