lkml.org 
[lkml]   [2015]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers
    On Thu, Jan 29, 2015 at 12:11:21AM +0100, Takashi Iwai wrote:
    > 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).

    Yeah, I'm not saying it's easy, or simple, it's just the only way I know
    how to do this in a race-free way. We have to create the files before
    the uevent happens, not after, like these drivers are doing.

    If you can think of a way that we can do this in a simpler way, that
    would be great.

    > 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.

    That one should just be adding the whole attribute group, using
    device_add_groups, which we have in the driver core, but I didn't export
    publicly. That is if those are being added in a race-free way, I
    couldn't unwind the drm mess to see if the uevent is happening after the
    files are added or before.

    > > > 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.

    Yeah, that's the problem. And another problem is drivers adding
    attributes to devices after they are bound to a device, which is kind of
    pointless, as the uevent is long past at that point in time. I've
    cleaned up a bunch of those, but odds are there are still more to fix.

    thanks,

    greg k-h


    \
     
     \ /
      Last update: 2015-01-30 05:41    [W:8.830 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site