lkml.org 
[lkml]   [2015]   [Jan]   [30]   [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
At Thu, 29 Jan 2015 20:26:26 -0800,
Greg Kroah-Hartman wrote:
>
> 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.

The latter one (regulator/core.c) is actually a case where is_visible
callback would work better, I noticed after studying mode code.
Thanks for hints. I'm going to submit a patch later.

OTOH, the leds class looks not intuitive. Need more investigation.

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

If we export device_add_groups() and device_remove_groups(), is it
safe to call it before device_add()? If yes, some drivers/subsystems
can have a code flow like:

some_subsystem_init(struct device *dev)
{
device_initialize(dev);
devs->groups = subsystem_groups;
....
}

driver_init(struct device *dev)
{
some_subsystem_init(dev);
device_add_groups(dev, additional_groups);
....
device_add(dev);
....
}

The network device has a own multi dev_groups array so that the driver
can put an own group while the net core fills common groups
dynamically just before the device registration call. I though of
implementing similar for others (including the sound stuff), but if
the scheme above works, the rewrite will become smaller.

Of corse, the drawback of the explicit device_add_groups() call would
be that you'll have to call device_remove_groups() at removal or error
paths.


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

Right, there are a bunch of drivers doing it. I guess partly because
they don't need uevents for creation, but also partly because there is
no way to give attribute groups properly in some cases. For example,
misc_register() or register_framebuffer() calls device_create() so the
caller can't pass groups.

It'd be trivial to extend struct miscdevice to carry an optional group
field and change the call to device_create_with_groups(). But,
fb_info has also common sysfs entries, so it'd need also the solution
above with device_add_groups() in addition.

The similar pattern is found for many drivers with platform devices.
But I haven't figured out yet what would be a good way...


thanks,

Takashi


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