Re: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers

[Takashi Iwai <tiwai@suse.de>]

At Wed, 28 Jan 2015 14:28:51 -0800,
Greg Kroah-Hartman wrote:
> 
> On Wed, Jan 28, 2015 at 11:18:57PM +0100, Takashi Iwai wrote:
> > At Wed, 28 Jan 2015 13:34:21 -0800,
> > Greg Kroah-Hartman wrote:
> > > 
> > > On Wed, Jan 28, 2015 at 10:26:28PM +0100, Takashi Iwai wrote:
> > > > At Wed, 28 Jan 2015 13:05:47 -0800,
> > > > Greg Kroah-Hartman wrote:
> > > > > 
> > > > > On Wed, Jan 28, 2015 at 09:46:12PM +0100, Takashi Iwai wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > this is a simple patch to add device_create_files() and
> > > > > > device_remove_files() to replace multiple device_create_file() or
> > > > > > _remove() calls with a single shot with the device_attr list.
> > > > > > 
> > > > > > It's basically just a clean up, but also helps to simplify the error
> > > > > > handling a lot in many existing codes since the function itself does
> > > > > > rollback at error.
> > > > > > 
> > > > > > The series contains a patch to apply these to drivers/base/node.c.
> > > > > > I have lots of patches (up to 30) to use these in the whole tree, but
> > > > > > maybe it'd be easier too apply once after this stuff is merged at
> > > > > > first.  It's just a cleanup so no urgent task, after all.
> > > > > 
> > > > > I'd like to some day be able to drop device_create_file entirely, as it
> > > > > is almost always used in a racy way (but not always, so we can't get rid
> > > > > of it today.)
> > > > > 
> > > > > A driver should be using an attribute group and be created/registered
> > > > > with it if they want any files associated with it, so giving people the
> > > > > ability to add large numbers of files all at once seems like the wrong
> > > > > thing to do :)
> > > > 
> > > > Well, through the glance over many codes using device_create_file(),
> > > > I think the problem of the attribute group is that there is little
> > > > help for generating the entries dynamically.  For example, if you have
> > > > two groups you want to enable conditionally, what would be the best
> > > > way to implement?
> > > 
> > > Use the is_visable() function callback, that's what it is there for.
> > 
> > But if the entries are determined dynamically?  Selecting the enabled
> > elements from the static list is one way, it'd work in many cases, but
> > it's not always the most straightforward way.  It often would be
> > easier to build up the list dynamically.
> 
> Do you have an example of this?  Wouldn't it be the same thing to list
> them all in an attribute group, but only say "this is valid" in the
> is_visable() callback for those that would have been built up
> dynamically?

One common scene is the case where a device has already the static
group defined in the core helper module while a driver wants to put
additional sysfs entries on it.

A complex case is something in drivers/leds/trigger/ledtrig-*.c.

Another interesting example is drivers/regulator/core.c.  It creates a
bunch of various sysfs files depending on the client's ops presence.
It might be implemented via is_visible, but then it'd become more
lengthy (too many small callback functions).

Also, multiple drivers seem calling device_create_file() from the
array of attributes in a loop.  One reason might be that it's easier
to write for a bunch of entries, without defining many piece of
structs.  An example is found in drivers/gpu/drm/drm_sysfs.c.


> > What if having a link to the chained group for appending entries
> > dynamically?  Just a wild idea, but it might make things easier.
> 
> We have the ability to pass a group list pointer to device_create
> already, and the attribute pointer is a list of groups as well, how can
> we change this to be "easier"?

I guess the order is the problem.  In many cases, you know the
additional entries only after the device creation.  The device
creation is often done by a helper code.  So the driver has no control
to it, just gets the resultant device.


thanks,

Takashi