Re: [PATCH 2/2] sysfs: add devm_sysfs_create_group() and friends

[Greg Kroah-Hartman]

On Tue, Jul 18, 2017 at 01:26:06PM -0700, Dmitry Torokhov wrote:
> On Tue, Jul 18, 2017 at 10:03:36PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jul 18, 2017 at 12:30:58PM -0700, Dmitry Torokhov wrote:
> > > --- a/include/linux/sysfs.h
> > > +++ b/include/linux/sysfs.h
> > > @@ -282,6 +282,16 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
> > >  				      struct kobject *target_kobj,
> > >  				      const char *target_name);
> > >  
> > > +struct device;
> > 
> > Put this in device.h instead?
> 
> I think we normally keep devm* and non-managed APIs together, like both
> regulator*() and devm_regulator*() are in include/linux/regulator/consumer.h,
> gpiod*() and devm_gpiod*() are in include/gpio/linux/consumer.h,
> clk*() and devm_clk() are in include/linux/clk.h, and so forth.
> 
> I think there is benefit of having these together as well.

That's fine, I just ment to put all of these changes in device.h, not
sysfs.h as they are dealing with devices.  sysfs.h does not define
'struct device' for that reason :)

> > > +int __must_check devm_sysfs_create_group(struct device *dev,
> > > +				const struct attribute_group *grp);
> > > +int __must_check devm_sysfs_create_groups(struct device *dev,
> > > +				const struct attribute_group **groups);
> > > +void devm_sysfs_remove_group(struct device *dev,
> > > +			     const struct attribute_group *grp);
> > > +void devm_sysfs_remove_groups(struct device *dev,
> > > +			      const struct attribute_group **groups);
> > 
> > I have finally moved the driver core to only accept/need "groups" not a
> > single "group", so we should only need devm_sysfs_create_groups and
> > devm_sysfs_remove_groups, right?
> 
> This makes total sense for the driver core, but individual drivers
> usually have a single group. Requiring all of them to have array of
> groups just adds unneeded boilerplate that I was trying to cut down.

ATTRIBUTE_GROUPS()?

Anyway, ok, I can live with all of these, but, do you actually have a
user for them already?  That would be nice to see in this series.

> > And do we need/want the non-devm versions:
> > 	device_create_groups()
> > 	device_remove_groups()
> > ?
> 
> We already have non-managed sysfs_create_groups() and
> sysfs_remove_groups().

Yeah, and bug driver should ever be calling a "raw" sysfs_* function :)

> > And you can probably drop the 'sysfs' from the function name if you
> > want.
> 
> I think there is benefit of having devm version having name matching the
> non-managed one:
> 
> sysfs_create_groups() and devm_sysfs_create_groups().
> 
> I hope you will reconsider.

Ok, I'm not totally sold, but I'm not going to argue anymore, except
please move the .h changes to device.h and it would be great to have a
user of these apis as well so we can at least test to see if it all
works properly.

thanks,

greg k-h