Re: [PATCH v2 4/7] driver core: add devm_device_add_group() and friends

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On July 20, 2017 1:20:09 AM PDT, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>On Thu, Jul 20, 2017 at 01:12:56AM -0700, Dmitry Torokhov wrote:
>> On July 19, 2017 10:10:18 PM PDT, Greg Kroah-Hartman
><gregkh@linuxfoundation.org> wrote:
>> >On Wed, Jul 19, 2017 at 05:24:33PM -0700, Dmitry Torokhov wrote:
>> >> Many drivers create additional driver-specific device attributes
>when
>> >> binding to the device, and providing managed version of
>> >> device_create_group() will simplify unbinding and error handling
>in
>> >probe
>> >> path for such drivers.
>> >> 
>> >> Without managed version driver writers either have to mix manual
>and
>> >> managed resources, which is prone to errors, or open-code this
>> >function by
>> >> providing a wrapper to device_add_group() and use it with
>> >devm_add_action()
>> >> or devm_add_action_or_reset().
>> >> 
>> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> >> ---
>> >>  drivers/base/core.c    | 130
>> >+++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  include/linux/device.h |   9 ++++
>> >>  2 files changed, 139 insertions(+)
>> >> 
>> >> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> >> index 14f8cf5c8b05..09723532725d 100644
>> >> --- a/drivers/base/core.c
>> >> +++ b/drivers/base/core.c
>> >> @@ -1035,6 +1035,136 @@ void device_remove_groups(struct device
>*dev,
>> >>  }
>> >>  EXPORT_SYMBOL_GPL(device_remove_groups);
>> >>  
>> >> +union device_attr_group_devres {
>> >> +	const struct attribute_group *group;
>> >> +	const struct attribute_group **groups;
>> >> +};
>> >> +
>> >> +static int devm_attr_group_match(struct device *dev, void *res,
>void
>> >*data)
>> >> +{
>> >> +	return ((union device_attr_group_devres *)res)->group == data;
>> >> +}
>> >> +
>> >> +static void devm_attr_group_remove(struct device *dev, void *res)
>> >> +{
>> >> +	union device_attr_group_devres *devres = res;
>> >> +	const struct attribute_group *group = devres->group;
>> >> +
>> >> +	dev_dbg(dev, "%s: removing group %p\n", __func__, group);
>> >> +	sysfs_remove_group(&dev->kobj, group);
>> >> +}
>> >> +
>> >> +static void devm_attr_groups_remove(struct device *dev, void
>*res)
>> >> +{
>> >> +	union device_attr_group_devres *devres = res;
>> >> +	const struct attribute_group **groups = devres->groups;
>> >> +
>> >> +	dev_dbg(dev, "%s: removing groups %p\n", __func__, groups);
>> >> +	sysfs_remove_groups(&dev->kobj, groups);
>> >> +}
>> >> +
>> >> +/**
>> >> + * devm_device_add_group - given a device, create a managed
>> >attribute group
>> >> + * @dev:	The device to create the group for
>> >> + * @grp:	The attribute group to create
>> >> + *
>> >> + * This function creates a group for the first time.  It will
>> >explicitly
>> >> + * warn and error if any of the attribute files being created
>> >already exist.
>> >> + *
>> >> + * Returns 0 on success or error code on failure.
>> >> + */
>> >> +int devm_device_add_group(struct device *dev, const struct
>> >attribute_group *grp)
>> >> +{
>> >> +	union device_attr_group_devres *devres;
>> >> +	int error;
>> >> +
>> >> +	devres = devres_alloc(devm_attr_group_remove,
>> >> +			      sizeof(*devres), GFP_KERNEL);
>> >> +	if (!devres)
>> >> +		return -ENOMEM;
>> >> +
>> >> +	error = sysfs_create_group(&dev->kobj, grp);
>> >
>> >Minor nit, this can now call device_create_group(), right?
>> >
>> >Same with below I think as well.
>> 
>> Right.
>> 
>> >
>> >It's fine, these look great, I'll queue them up this afternoon...
>> >
>> >Thanks for persisting with these, and sorry it took so long to
>convince
>> >me I was wrong :)
>> 
>> :)
>> 
>> Any chance you could create an unmutable branch off 4.12 so I can
>start using it in input drivers?
>
>I'll be glad to, can it be off of 4.13-rc1?  Or do you need it off of
>4.12?

It's just my preference for topic branches to be off a stable[r] releases, unless patches do not apply cleanly or will lead to merge conflicts down the road.


Thanks!

-- 
Dmitry