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

[Takashi Iwai <tiwai@suse.de>]

At Sat, 7 Feb 2015 18:10:48 +0800,
Greg Kroah-Hartman wrote:
> 
> On Fri, Jan 30, 2015 at 05:31:51PM +0100, Takashi Iwai wrote:
> > 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.
> 
> Right now, no, you can't call device_add_groups() until after
> device_add() happens, as it device_initialize() doesn't do enough sysfs
> work in order to be able to create the files.

OK, that's not so trivial as I hoped.
One can add some list to manage the additional attributes, but it
would put at least a pointer to each struct device, and it's certainly
a waste that doesn't pay enough for the gain.

BTW, I wonder whether we can drop many codes in the remove path.
IIRC, kobject_del() should remove the whole sysfs files recursively,
so we don't have to remove files individually.


> > > > > > 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.
> 
> Your patch to do that looks good, I'll queue them all up after 3.20-rc1
> is out as it's too close to 3.19 at the moment.

Yeah, 3.20 is good enough.  Thanks for picking it up.

I've submitted other cleanup patches to various subsystems, some have
been merged for 3.20 and some are pending to 3.21, as it seems.  There
are still a few easy lowhanging fruits left, but mostly arch stuff (or
arch-specific drivers).

However, there are also many device_create_file() calls that can't be
translated to static attribute groups.  Namely, lots of drivers add
the sysfs files onto the device that is being probed.  That is, in
xxx_probe() for a platform device or a pci device (or others), the
driver puts new files to the probed device itself.

I have no idea how this can be implemented in a better way.


Takashi