[PATCH 1/2] driver core: emit uevents when device is bound to a driver

[PATCH 1/2] driver core: emit uevents when device is bound to a driver

[Dmitry Torokhov]

Majority of standard for a subsystem device attributes are created at the
same time devices are created, before KOBJECT_ADD uevent is emitted by the
driver core. This means that attributes are there when userspace is
notified about new device appearance.

However many drivers create additional driver-specific device attributes
when binding to the device, to provide userspace with additional controls,
and such attributes may not be there yet when userpsace receives
KOBJECT_ADD event. Changing the drivers to introduce intermediate "dummy"
device as a container for such attributes would be wasteful, and in many
cases, braking our sysfs ABI. Let's add a new event, KOBJECT_BIND (and its
counterpart, KOBJECT_UNBIND) that is emitted after a driver is bound to a
device. It can be used by userspace wishing to use driver-specific
attributes of a device.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/dd.c       | 4 ++++
 include/linux/kobject.h | 2 ++
 lib/kobject_uevent.c    | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index a1fbf55c4d3a..a9a5cc0560e5 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -258,6 +258,8 @@ static void driver_bound(struct device *dev)
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_BOUND_DRIVER, dev);
+
+	kobject_uevent(&dev->kobj, KOBJ_BIND);
 }
 
 static int driver_sysfs_add(struct device *dev)
@@ -839,6 +841,8 @@ static void __device_release_driver(struct device *dev, struct device *parent)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBOUND_DRIVER,
 						     dev);
+
+		kobject_uevent(&dev->kobj, KOBJ_UNBIND);
 	}
 }
 
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index e6284591599e..07292df4776e 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -57,6 +57,8 @@ enum kobject_action {
 	KOBJ_MOVE,
 	KOBJ_ONLINE,
 	KOBJ_OFFLINE,
+	KOBJ_BIND,
+	KOBJ_UNBIND,
 	KOBJ_MAX
 };
 
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 9a2b811966eb..4682e8545b5c 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -50,6 +50,8 @@ static const char *kobject_actions[] = {
 	[KOBJ_MOVE] =		"move",
 	[KOBJ_ONLINE] =		"online",
 	[KOBJ_OFFLINE] =	"offline",
+	[KOBJ_BIND] =		"bind",
+	[KOBJ_UNBIND] =		"unbind",
 };
 
 /**
-- 
2.11.0.483.g087da7b7c-goog

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

[Dmitry Torokhov]

Many drivers create additional driver-specific device attributes when
binding to the device and providing managed version of sysfs_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 sysfs_create_group() and use it with
devm_add_action() or devm_add_action_or_reset().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 fs/sysfs/group.c      | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  10 ++++
 2 files changed, 134 insertions(+)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index ac2de0ed69ad..24fc205fcb9b 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -13,6 +13,7 @@
 #include <linux/kobject.h>
 #include <linux/module.h>
 #include <linux/dcache.h>
+#include <linux/device.h>
 #include <linux/namei.h>
 #include <linux/err.h>
 #include "sysfs.h"
@@ -409,3 +410,126 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
 	return IS_ERR(link) ? PTR_ERR(link) : 0;
 }
 EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
+
+struct sysfs_group_devres {
+	const struct attribute_group *group;
+};
+
+static int devm_sysfs_group_match(struct device *dev, void *res, void *data)
+{
+	return ((struct sysfs_group_devres *)res)->group == data;
+}
+
+static void devm_sysfs_group_remove_group(struct device *dev, void *res)
+{
+	struct sysfs_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);
+}
+
+/**
+ * devm_sysfs_create_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_sysfs_create_group(struct device *dev,
+			    const struct attribute_group *grp)
+{
+	struct sysfs_group_devres *devres;
+	int error;
+
+	devres = devres_alloc(devm_sysfs_group_remove_group,
+			      sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
+
+	error = sysfs_create_group(&dev->kobj, grp);
+	if (error) {
+		devres_free(devres);
+		return error;
+	}
+
+	devres_add(dev, devres);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_sysfs_create_group);
+
+/**
+ * devm_sysfs_create_groups - create a bunch of managed attribute groups
+ * @dev:	The device to create the group for
+ * @groups:	The attribute groups to create, NULL terminated
+ *
+ * This function creates a bunch of managed attribute groups.  If an error
+ * occurs when creating a group, all previously created groups will be
+ * removed, unwinding everything back to the original state when this
+ * function was called.  It will explicitly warn and error if any of the
+ * attribute files being created already exist.
+ *
+ * Returns 0 on success or error code from sysfs_create_group on failure.
+ */
+int devm_sysfs_create_groups(struct device *dev,
+			     const struct attribute_group **groups)
+{
+	int error;
+	int i;
+
+	if (!groups)
+		return 0;
+
+	for (i = 0; groups[i]; i++) {
+		error = devm_sysfs_create_group(dev, groups[i]);
+		if (error) {
+			while (--i >= 0)
+				devm_sysfs_remove_group(dev, groups[i]);
+			return error;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_sysfs_create_groups);
+
+/**
+ * devm_sysfs_remove_group: remove a managed group from a device
+ * @dev:	device to remove the group from
+ * @grp:	group to remove
+ *
+ * This function removes a group of attributes from a device.  The attributes
+ * previously have to have been created for this group, otherwise it will fail.
+ */
+void devm_sysfs_remove_group(struct device *dev,
+			     const struct attribute_group *grp)
+{
+	WARN_ON(devres_release(dev, devm_sysfs_group_remove_group,
+			       devm_sysfs_group_match,
+			       /* cast away const */ (void *)grp));
+}
+EXPORT_SYMBOL_GPL(devm_sysfs_remove_group);
+
+/**
+ * devm_sysfs_remove_groups - remove a list of managed groups
+ *
+ * @dev:	The device for the groups to be removed from
+ * @groups:	NULL terminated list of groups to be removed
+ *
+ * If groups is not NULL, remove the specified groups from the device.
+ */
+void devm_sysfs_remove_groups(struct device *dev,
+			      const struct attribute_group **groups)
+{
+	int i;
+
+	if (!groups)
+		return;
+
+	for (i = 0; groups[i]; i++)
+		devm_sysfs_remove_group(dev, groups[i]);
+}
+EXPORT_SYMBOL_GPL(devm_sysfs_remove_groups);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index c6f0f0d0e17e..d9a5b8aab0e2 100644
--- 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;
+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);
+
 void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr);
 
 int __must_check sysfs_init(void);
-- 
2.11.0.483.g087da7b7c-goog

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

[Dmitry Torokhov]

On Sun, Feb 12, 2017 at 04:36:19PM -0800, Dmitry Torokhov wrote:
> Many drivers create additional driver-specific device attributes when
> binding to the device and providing managed version of sysfs_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 sysfs_create_group() and use it with
> devm_add_action() or devm_add_action_or_reset().
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---

Greg, gentle ping...


>  fs/sysfs/group.c      | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sysfs.h |  10 ++++
>  2 files changed, 134 insertions(+)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index ac2de0ed69ad..24fc205fcb9b 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -13,6 +13,7 @@
>  #include <linux/kobject.h>
>  #include <linux/module.h>
>  #include <linux/dcache.h>
> +#include <linux/device.h>
>  #include <linux/namei.h>
>  #include <linux/err.h>
>  #include "sysfs.h"
> @@ -409,3 +410,126 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>  	return IS_ERR(link) ? PTR_ERR(link) : 0;
>  }
>  EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
> +
> +struct sysfs_group_devres {
> +	const struct attribute_group *group;
> +};
> +
> +static int devm_sysfs_group_match(struct device *dev, void *res, void *data)
> +{
> +	return ((struct sysfs_group_devres *)res)->group == data;
> +}
> +
> +static void devm_sysfs_group_remove_group(struct device *dev, void *res)
> +{
> +	struct sysfs_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);
> +}
> +
> +/**
> + * devm_sysfs_create_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_sysfs_create_group(struct device *dev,
> +			    const struct attribute_group *grp)
> +{
> +	struct sysfs_group_devres *devres;
> +	int error;
> +
> +	devres = devres_alloc(devm_sysfs_group_remove_group,
> +			      sizeof(*devres), GFP_KERNEL);
> +	if (!devres)
> +		return -ENOMEM;
> +
> +	error = sysfs_create_group(&dev->kobj, grp);
> +	if (error) {
> +		devres_free(devres);
> +		return error;
> +	}
> +
> +	devres_add(dev, devres);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_sysfs_create_group);
> +
> +/**
> + * devm_sysfs_create_groups - create a bunch of managed attribute groups
> + * @dev:	The device to create the group for
> + * @groups:	The attribute groups to create, NULL terminated
> + *
> + * This function creates a bunch of managed attribute groups.  If an error
> + * occurs when creating a group, all previously created groups will be
> + * removed, unwinding everything back to the original state when this
> + * function was called.  It will explicitly warn and error if any of the
> + * attribute files being created already exist.
> + *
> + * Returns 0 on success or error code from sysfs_create_group on failure.
> + */
> +int devm_sysfs_create_groups(struct device *dev,
> +			     const struct attribute_group **groups)
> +{
> +	int error;
> +	int i;
> +
> +	if (!groups)
> +		return 0;
> +
> +	for (i = 0; groups[i]; i++) {
> +		error = devm_sysfs_create_group(dev, groups[i]);
> +		if (error) {
> +			while (--i >= 0)
> +				devm_sysfs_remove_group(dev, groups[i]);
> +			return error;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_sysfs_create_groups);
> +
> +/**
> + * devm_sysfs_remove_group: remove a managed group from a device
> + * @dev:	device to remove the group from
> + * @grp:	group to remove
> + *
> + * This function removes a group of attributes from a device.  The attributes
> + * previously have to have been created for this group, otherwise it will fail.
> + */
> +void devm_sysfs_remove_group(struct device *dev,
> +			     const struct attribute_group *grp)
> +{
> +	WARN_ON(devres_release(dev, devm_sysfs_group_remove_group,
> +			       devm_sysfs_group_match,
> +			       /* cast away const */ (void *)grp));
> +}
> +EXPORT_SYMBOL_GPL(devm_sysfs_remove_group);
> +
> +/**
> + * devm_sysfs_remove_groups - remove a list of managed groups
> + *
> + * @dev:	The device for the groups to be removed from
> + * @groups:	NULL terminated list of groups to be removed
> + *
> + * If groups is not NULL, remove the specified groups from the device.
> + */
> +void devm_sysfs_remove_groups(struct device *dev,
> +			      const struct attribute_group **groups)
> +{
> +	int i;
> +
> +	if (!groups)
> +		return;
> +
> +	for (i = 0; groups[i]; i++)
> +		devm_sysfs_remove_group(dev, groups[i]);
> +}
> +EXPORT_SYMBOL_GPL(devm_sysfs_remove_groups);
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index c6f0f0d0e17e..d9a5b8aab0e2 100644
> --- 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;
> +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);
> +
>  void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr);
>  
>  int __must_check sysfs_init(void);
> -- 
> 2.11.0.483.g087da7b7c-goog
> 

-- 
Dmitry

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

[Greg Kroah-Hartman]

On Mon, Mar 27, 2017 at 10:53:59AM -0700, Dmitry Torokhov wrote:
> On Sun, Feb 12, 2017 at 04:36:19PM -0800, Dmitry Torokhov wrote:
> > Many drivers create additional driver-specific device attributes when
> > binding to the device and providing managed version of sysfs_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 sysfs_create_group() and use it with
> > devm_add_action() or devm_add_action_or_reset().
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> 
> Greg, gentle ping...

It's not lost, it's in my queue...

thanks,

greg k-h

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

[Dmitry Torokhov]

On Mon, Mar 27, 2017 at 09:35:45PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Mar 27, 2017 at 10:53:59AM -0700, Dmitry Torokhov wrote:
> > On Sun, Feb 12, 2017 at 04:36:19PM -0800, Dmitry Torokhov wrote:
> > > Many drivers create additional driver-specific device attributes when
> > > binding to the device and providing managed version of sysfs_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 sysfs_create_group() and use it with
> > > devm_add_action() or devm_add_action_or_reset().
> > > 
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > 
> > Greg, gentle ping...
> 
> It's not lost, it's in my queue...

Another gentle ping...

-- 
Dmitry

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

[Greg Kroah-Hartman]

On Wed, Jun 07, 2017 at 10:04:45AM -0700, Dmitry Torokhov wrote:
> On Mon, Mar 27, 2017 at 09:35:45PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Mar 27, 2017 at 10:53:59AM -0700, Dmitry Torokhov wrote:
> > > On Sun, Feb 12, 2017 at 04:36:19PM -0800, Dmitry Torokhov wrote:
> > > > Many drivers create additional driver-specific device attributes when
> > > > binding to the device and providing managed version of sysfs_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 sysfs_create_group() and use it with
> > > > devm_add_action() or devm_add_action_or_reset().
> > > > 
> > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > ---
> > > 
> > > Greg, gentle ping...
> > 
> > It's not lost, it's in my queue...
> 
> Another gentle ping...

Ugh, I've been hoping you would forget about this :)

But, in thinking a lot about this recently, I have now come to the same
conclusion as you, I think we really need this as there isn't a way to
handle it any other way.

So, can you rebase and resend these patches?  I don't have them in my
queue anymore and I'd like to play with them and see how they work.

thanks,

greg k-h

Re: [PATCH 1/2] driver core: emit uevents when device is bound to a driver

[Greg Kroah-Hartman]

On Sun, Feb 12, 2017 at 04:36:18PM -0800, Dmitry Torokhov wrote:
> Majority of standard for a subsystem device attributes are created at the
> same time devices are created, before KOBJECT_ADD uevent is emitted by the
> driver core. This means that attributes are there when userspace is
> notified about new device appearance.
> 
> However many drivers create additional driver-specific device attributes
> when binding to the device, to provide userspace with additional controls,
> and such attributes may not be there yet when userpsace receives
> KOBJECT_ADD event.

How about we fix those drivers instead?

Are you going to change userspace (i.e. libudev) to refresh with this
new kobject uevent type?

The 'groups' field for drivers should handle this, but yes, there are
some subsystems that don't really do it, and there are drivers that like
to add random sysfs files to their device's directories which I would
argue is the correct solution here, but you don't like this, because you
say:

> Changing the drivers to introduce intermediate "dummy" device as a
> container for such attributes would be wasteful, and in many cases,
> braking our sysfs ABI.

I'd argue that you are adding random sysfs files to random device types
(i.e. a PCI device gets a random set of sysfs files just depending on
what driver bound to it.)  And that's wrong, and is why classes were
created.

Is there a specific type of devices that you have that you wish to fix
up using this new uevent type?

thanks,

greg k-h

Re: [PATCH 1/2] driver core: emit uevents when device is bound to a driver

[Dmitry Torokhov]

On Mon, Feb 13, 2017 at 04:07:01AM -0800, Greg Kroah-Hartman wrote:
> On Sun, Feb 12, 2017 at 04:36:18PM -0800, Dmitry Torokhov wrote:
> > Majority of standard for a subsystem device attributes are created at the
> > same time devices are created, before KOBJECT_ADD uevent is emitted by the
> > driver core. This means that attributes are there when userspace is
> > notified about new device appearance.
> > 
> > However many drivers create additional driver-specific device attributes
> > when binding to the device, to provide userspace with additional controls,
> > and such attributes may not be there yet when userpsace receives
> > KOBJECT_ADD event.
> 
> How about we fix those drivers instead?

They haven't been fixed so far (for many years), and there are ABI
concerns that would prevent us from "fixing" them.

> 
> Are you going to change userspace (i.e. libudev) to refresh with this
> new kobject uevent type?

Refresh? I do not think we need to refresh anything, as there attributes
are very devise specific so there would be rules listening for these
"bind" events and adjust the attributes as needed.

But if we do agree on these 2 new actions I'll prepare a patch for
systemd so that it recognizes them (although why systemd believes that
it needs to "verify" actions reported by the kernel is beyond me).

> 
> The 'groups' field for drivers should handle this, but yes, there are
> some subsystems that don't really do it, and there are drivers that like
> to add random sysfs files to their device's directories which I would
> argue is the correct solution here, but you don't like this, because you
> say:
> 
> > Changing the drivers to introduce intermediate "dummy" device as a
> > container for such attributes would be wasteful, and in many cases,
> > braking our sysfs ABI.
> 
> I'd argue that you are adding random sysfs files to random device types
> (i.e. a PCI device gets a random set of sysfs files just depending on
> what driver bound to it.)  And that's wrong, and is why classes were
> created.

Classes are good when you have several devices with common
characteristics and purpose, they do not fit in the cases when we use
sysfs to create a device-specific knob. I.e. there s only one device
implementing IBM Trackpoint protocol. It has the following attributes
controlling hardware behavior:

TRACKPOINT_INT_ATTR(sensitivity, TP_SENS, TP_DEF_SENS);
TRACKPOINT_INT_ATTR(speed, TP_SPEED, TP_DEF_SPEED);
TRACKPOINT_INT_ATTR(inertia, TP_INERTIA, TP_DEF_INERTIA);
TRACKPOINT_INT_ATTR(reach, TP_REACH, TP_DEF_REACH);
TRACKPOINT_INT_ATTR(draghys, TP_DRAGHYS, TP_DEF_DRAGHYS);
TRACKPOINT_INT_ATTR(mindrag, TP_MINDRAG, TP_DEF_MINDRAG);
TRACKPOINT_INT_ATTR(thresh, TP_THRESH, TP_DEF_THRESH);
TRACKPOINT_INT_ATTR(upthresh, TP_UP_THRESH, TP_DEF_UP_THRESH);
TRACKPOINT_INT_ATTR(ztime, TP_Z_TIME, TP_DEF_Z_TIME);
TRACKPOINT_INT_ATTR(jenks, TP_JENKS_CURV, TP_DEF_JENKS_CURV);
TRACKPOINT_INT_ATTR(drift_time, TP_DRIFT_TIME, TP_DEF_DRIFT_TIME);

TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, 0,
                    TP_DEF_PTSON);
TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, 0,
                    TP_DEF_SKIPBACK);
TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV, TP_MASK_EXT_DEV, 1,
                    TP_DEF_EXT_DEV);

They are not applicable to a generic input device, and thus they do not
belong to 'inptut' class (I keep only attributes that are common to all
input devices in input class). Since they control hardware properties of
device on given port they are attached to serio port of that device.

We also have other PS/2 mice protocol having their very own quirks; same
goes for touchscreens, miscellaneous button devices, etc. If you look
outside of input, you will see a ton of drivers using
device_create_file() and sysfs_create_group(). Some of them can be
changed to use attribute groups attached to a device (although it will
take some time), and for some they are not in charge of creating the
device instance, and so they can't:

drivers/usb/misc/cypress_cy7c63.c
drivers/usb/misc/cytherm.c
drivers/usb/misc/trancevibrator.c
drivers/usb/class/cdc-acm.c
drivers/usb/atm/cxacru.c
<...more usb stuff...>
drivers/pcmcia/yenta_socket.c
drivers/pcmcia/soc_common.c
drivers/rtc/<quite a few>
drivers/power/supply/<a few>
drivers/spi/spi-tle62x0.c
drivers/spi/spi-tle62x0.c
drivers/ssb/pci.c
drivers/ssb/pcmcia.c

... network drivers, OF and ACPI-instantiated platform devices, media
devices...

Some driver create links (for compatibility I guess). These also not
going to be created with the devices.

> 
> Is there a specific type of devices that you have that you wish to fix
> up using this new uevent type?

As I shown above, there is not a particular class but rather quite a few
drivers across the kernel.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] driver core: emit uevents when device is bound to a driver

[Greg Kroah-Hartman]

On Mon, Feb 13, 2017 at 10:46:16AM -0800, Dmitry Torokhov wrote:
> On Mon, Feb 13, 2017 at 04:07:01AM -0800, Greg Kroah-Hartman wrote:
> > On Sun, Feb 12, 2017 at 04:36:18PM -0800, Dmitry Torokhov wrote:
> > > Majority of standard for a subsystem device attributes are created at the
> > > same time devices are created, before KOBJECT_ADD uevent is emitted by the
> > > driver core. This means that attributes are there when userspace is
> > > notified about new device appearance.
> > > 
> > > However many drivers create additional driver-specific device attributes
> > > when binding to the device, to provide userspace with additional controls,
> > > and such attributes may not be there yet when userpsace receives
> > > KOBJECT_ADD event.
> > 
> > How about we fix those drivers instead?
> 
> They haven't been fixed so far (for many years), and there are ABI
> concerns that would prevent us from "fixing" them.

I agree, I have a bunch listed that should be fixed up (with groups),
but I don't have the time to do so :(

> > Are you going to change userspace (i.e. libudev) to refresh with this
> > new kobject uevent type?
> 
> Refresh? I do not think we need to refresh anything, as there attributes
> are very devise specific so there would be rules listening for these
> "bind" events and adjust the attributes as needed.

What rules?  Who needs/wants this?  I think libudev caches attributes,
but I could be wrong, when it gets a uevent about a new device being
present.  That would need to be updated with this new uevent.

> But if we do agree on these 2 new actions I'll prepare a patch for
> systemd so that it recognizes them (although why systemd believes that
> it needs to "verify" actions reported by the kernel is beyond me).

It doesn't "verify" anything, it just makes it easier for programs using
libudev to access attributes.  But it's been years since I looked at
that code, it might not be caching anything.  But it needs to be
verified.

Who is going to be relying on this new uevent?

> > The 'groups' field for drivers should handle this, but yes, there are
> > some subsystems that don't really do it, and there are drivers that like
> > to add random sysfs files to their device's directories which I would
> > argue is the correct solution here, but you don't like this, because you
> > say:
> > 
> > > Changing the drivers to introduce intermediate "dummy" device as a
> > > container for such attributes would be wasteful, and in many cases,
> > > braking our sysfs ABI.
> > 
> > I'd argue that you are adding random sysfs files to random device types
> > (i.e. a PCI device gets a random set of sysfs files just depending on
> > what driver bound to it.)  And that's wrong, and is why classes were
> > created.
> 
> Classes are good when you have several devices with common
> characteristics and purpose, they do not fit in the cases when we use
> sysfs to create a device-specific knob. I.e. there s only one device
> implementing IBM Trackpoint protocol. It has the following attributes
> controlling hardware behavior:
> 
> TRACKPOINT_INT_ATTR(sensitivity, TP_SENS, TP_DEF_SENS);
> TRACKPOINT_INT_ATTR(speed, TP_SPEED, TP_DEF_SPEED);
> TRACKPOINT_INT_ATTR(inertia, TP_INERTIA, TP_DEF_INERTIA);
> TRACKPOINT_INT_ATTR(reach, TP_REACH, TP_DEF_REACH);
> TRACKPOINT_INT_ATTR(draghys, TP_DRAGHYS, TP_DEF_DRAGHYS);
> TRACKPOINT_INT_ATTR(mindrag, TP_MINDRAG, TP_DEF_MINDRAG);
> TRACKPOINT_INT_ATTR(thresh, TP_THRESH, TP_DEF_THRESH);
> TRACKPOINT_INT_ATTR(upthresh, TP_UP_THRESH, TP_DEF_UP_THRESH);
> TRACKPOINT_INT_ATTR(ztime, TP_Z_TIME, TP_DEF_Z_TIME);
> TRACKPOINT_INT_ATTR(jenks, TP_JENKS_CURV, TP_DEF_JENKS_CURV);
> TRACKPOINT_INT_ATTR(drift_time, TP_DRIFT_TIME, TP_DEF_DRIFT_TIME);
> 
> TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, 0,
>                     TP_DEF_PTSON);
> TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, 0,
>                     TP_DEF_SKIPBACK);
> TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV, TP_MASK_EXT_DEV, 1,
>                     TP_DEF_EXT_DEV);
> 
> They are not applicable to a generic input device, and thus they do not
> belong to 'inptut' class (I keep only attributes that are common to all
> input devices in input class). Since they control hardware properties of
> device on given port they are attached to serio port of that device.

Ick, that sucks, why can't it be part of a 'misc' input device?  This
makes userspace programs have to do more work by having to look at
non-input devices to find the trackpoint.

> We also have other PS/2 mice protocol having their very own quirks; same
> goes for touchscreens, miscellaneous button devices, etc. If you look
> outside of input, you will see a ton of drivers using
> device_create_file() and sysfs_create_group(). Some of them can be
> changed to use attribute groups attached to a device (although it will
> take some time), and for some they are not in charge of creating the
> device instance, and so they can't:
> 
> drivers/usb/misc/cypress_cy7c63.c
> drivers/usb/misc/cytherm.c
> drivers/usb/misc/trancevibrator.c
> drivers/usb/class/cdc-acm.c
> drivers/usb/atm/cxacru.c
> <...more usb stuff...>

Ick, those USB drivers should be fixed up, if only the maintainer of
that subsystem wasn't so lazy... :)

> drivers/pcmcia/yenta_socket.c
> drivers/pcmcia/soc_common.c
> drivers/rtc/<quite a few>
> drivers/power/supply/<a few>
> drivers/spi/spi-tle62x0.c
> drivers/spi/spi-tle62x0.c
> drivers/ssb/pci.c
> drivers/ssb/pcmcia.c
> 
> ... network drivers, OF and ACPI-instantiated platform devices, media
> devices...
> 
> Some driver create links (for compatibility I guess). These also not
> going to be created with the devices.

Yeah, lots of things get it wrong.

But now you just created 2 different times when the files get created,
so userspace has to monitor two different uevents?  How does it know
what to wait for, a ADD or a BIND?  What about just using the existing
"CHANGE" uevent?

But really, I don't like this, what tools are currently broken that you
are wanting to fix up?

thanks,

greg k-h

Re: [PATCH 1/2] driver core: emit uevents when device is bound to a driver

[Dmitry Torokhov]

On Mon, Feb 13, 2017 at 04:52:07PM -0800, Greg Kroah-Hartman wrote:
> On Mon, Feb 13, 2017 at 10:46:16AM -0800, Dmitry Torokhov wrote:
> > On Mon, Feb 13, 2017 at 04:07:01AM -0800, Greg Kroah-Hartman wrote:
> > > On Sun, Feb 12, 2017 at 04:36:18PM -0800, Dmitry Torokhov wrote:
> > > > Majority of standard for a subsystem device attributes are created at the
> > > > same time devices are created, before KOBJECT_ADD uevent is emitted by the
> > > > driver core. This means that attributes are there when userspace is
> > > > notified about new device appearance.
> > > > 
> > > > However many drivers create additional driver-specific device attributes
> > > > when binding to the device, to provide userspace with additional controls,
> > > > and such attributes may not be there yet when userpsace receives
> > > > KOBJECT_ADD event.
> > > 
> > > How about we fix those drivers instead?
> > 
> > They haven't been fixed so far (for many years), and there are ABI
> > concerns that would prevent us from "fixing" them.
> 
> I agree, I have a bunch listed that should be fixed up (with groups),
> but I don't have the time to do so :(
> 
> > > Are you going to change userspace (i.e. libudev) to refresh with this
> > > new kobject uevent type?
> > 
> > Refresh? I do not think we need to refresh anything, as there attributes
> > are very devise specific so there would be rules listening for these
> > "bind" events and adjust the attributes as needed.
> 
> What rules?  Who needs/wants this?

Anyone who needs to act on driver-specific attribute. For example, you
could tie firmware updates for some devices to them.

> I think libudev caches attributes,
> but I could be wrong, when it gets a uevent about a new device being
> present.  That would need to be updated with this new uevent.
> 
> > But if we do agree on these 2 new actions I'll prepare a patch for
> > systemd so that it recognizes them (although why systemd believes that
> > it needs to "verify" actions reported by the kernel is beyond me).
> 
> It doesn't "verify" anything, it just makes it easier for programs using
> libudev to access attributes.  But it's been years since I looked at
> that code, it might not be caching anything.  But it needs to be
> verified.

No, I mean that current systemd drops events that it does not know about
on the floor. I.e. if it is not in the add, remove, change, move,
offline, or online list, it will get dropped on the floor instead of
being passed on to rules engine. Why? Not sure. Older udev versions did
not need any changes as far as I can see.

Anyway, if we come to an agreement on this I will look into getting
systemd handle this nicely.

> 
> Who is going to be relying on this new uevent?
> 
> > > The 'groups' field for drivers should handle this, but yes, there are
> > > some subsystems that don't really do it, and there are drivers that like
> > > to add random sysfs files to their device's directories which I would
> > > argue is the correct solution here, but you don't like this, because you
> > > say:
> > > 
> > > > Changing the drivers to introduce intermediate "dummy" device as a
> > > > container for such attributes would be wasteful, and in many cases,
> > > > braking our sysfs ABI.
> > > 
> > > I'd argue that you are adding random sysfs files to random device types
> > > (i.e. a PCI device gets a random set of sysfs files just depending on
> > > what driver bound to it.)  And that's wrong, and is why classes were
> > > created.
> > 
> > Classes are good when you have several devices with common
> > characteristics and purpose, they do not fit in the cases when we use
> > sysfs to create a device-specific knob. I.e. there s only one device
> > implementing IBM Trackpoint protocol. It has the following attributes
> > controlling hardware behavior:
> > 
> > TRACKPOINT_INT_ATTR(sensitivity, TP_SENS, TP_DEF_SENS);
> > TRACKPOINT_INT_ATTR(speed, TP_SPEED, TP_DEF_SPEED);
> > TRACKPOINT_INT_ATTR(inertia, TP_INERTIA, TP_DEF_INERTIA);
> > TRACKPOINT_INT_ATTR(reach, TP_REACH, TP_DEF_REACH);
> > TRACKPOINT_INT_ATTR(draghys, TP_DRAGHYS, TP_DEF_DRAGHYS);
> > TRACKPOINT_INT_ATTR(mindrag, TP_MINDRAG, TP_DEF_MINDRAG);
> > TRACKPOINT_INT_ATTR(thresh, TP_THRESH, TP_DEF_THRESH);
> > TRACKPOINT_INT_ATTR(upthresh, TP_UP_THRESH, TP_DEF_UP_THRESH);
> > TRACKPOINT_INT_ATTR(ztime, TP_Z_TIME, TP_DEF_Z_TIME);
> > TRACKPOINT_INT_ATTR(jenks, TP_JENKS_CURV, TP_DEF_JENKS_CURV);
> > TRACKPOINT_INT_ATTR(drift_time, TP_DRIFT_TIME, TP_DEF_DRIFT_TIME);
> > 
> > TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, 0,
> >                     TP_DEF_PTSON);
> > TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, 0,
> >                     TP_DEF_SKIPBACK);
> > TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV, TP_MASK_EXT_DEV, 1,
> >                     TP_DEF_EXT_DEV);
> > 
> > They are not applicable to a generic input device, and thus they do not
> > belong to 'inptut' class (I keep only attributes that are common to all
> > input devices in input class). Since they control hardware properties of
> > device on given port they are attached to serio port of that device.
> 
> Ick, 

"Yeah, well, you know, that’s just, like, your opinion, man." ;)

> that sucks, why can't it be part of a 'misc' input device?  This
> makes userspace programs have to do more work by having to look at
> non-input devices to find the trackpoint.

I think we just have fundamental disagreement here. I believe these
attributes belong to the device that is closest to the actual hardware.
This way, even if we come up with brand new input subsystem, inputNG,
and switch over to it, the attributes affecting hardware behavior,
rather than class abstractions, would stay the same. And if we keep
input v1.0 alongside of inputNG for a while we would not need to
duplicate this in both places.

Also, consider attributes such as firmware checksum or configuration
checksum and firmware update triggers that are used to flash persistent
firmware on some devices (Elan touchscreens and touchpads, Atmel touch
controllers, Weida touch controllers, and so on). Quite often, if device
comes up in so-called bootloader mode (because nvram is messed up or
something) there might not even be input device you want to attach
attributes to. The only thing that is there is i2c_client or spi device
or USB interface.

> 
> > We also have other PS/2 mice protocol having their very own quirks; same
> > goes for touchscreens, miscellaneous button devices, etc. If you look
> > outside of input, you will see a ton of drivers using
> > device_create_file() and sysfs_create_group(). Some of them can be
> > changed to use attribute groups attached to a device (although it will
> > take some time), and for some they are not in charge of creating the
> > device instance, and so they can't:
> > 
> > drivers/usb/misc/cypress_cy7c63.c
> > drivers/usb/misc/cytherm.c
> > drivers/usb/misc/trancevibrator.c
> > drivers/usb/class/cdc-acm.c
> > drivers/usb/atm/cxacru.c
> > <...more usb stuff...>
> 
> Ick, those USB drivers should be fixed up, if only the maintainer of
> that subsystem wasn't so lazy... :)

Some of them can be fixed, some can't (becaue ABI).

> 
> > drivers/pcmcia/yenta_socket.c
> > drivers/pcmcia/soc_common.c
> > drivers/rtc/<quite a few>
> > drivers/power/supply/<a few>
> > drivers/spi/spi-tle62x0.c
> > drivers/spi/spi-tle62x0.c
> > drivers/ssb/pci.c
> > drivers/ssb/pcmcia.c
> > 
> > ... network drivers, OF and ACPI-instantiated platform devices, media
> > devices...
> > 
> > Some driver create links (for compatibility I guess). These also not
> > going to be created with the devices.
> 
> Yeah, lots of things get it wrong.
> 
> But now you just created 2 different times when the files get created,
> so userspace has to monitor two different uevents?  How does it know
> what to wait for, a ADD or a BIND?

I do not think there would be much confusion between ADD and BIND,
because the attributes are really driver specific (otherwise they'd be
pushed into subsystem) so whoever is creating the rules would know quite
a bit about the device and driver.

> What about just using the existing
> "CHANGE" uevent?

I thought BIND would convey the event more clearly than overloading
CHANGE.

> 
> But really, I don't like this, what tools are currently broken that you
> are wanting to fix up?

We might want to look into switching our firmware updaters to trigger
off udev events (ChromeOS). Also, here is an example of people trying to
automatically configure trackpoint and it being unreliable:

https://wiki.archlinux.org/index.php/TrackPoint#Configuration_at_boot

BTW, what I really want now is to get the patch 2/2 in, as it will allow
cleaning up bunch of drivers. And it does not depend on this patch, I
just sent them together.

Thanks.

-- 
Dmitry

[PATCH 0/2] New bind/unbingd uevents

[Dmitry Torokhov]

Hi Greg,

I am resending the bind/unbind and devm_sysfs_create_group() patches,
as you requested. The new bind/unbind will allow triggering firmware
update through udev, and the new sysfs API will cut down on some
boilerplate code in drivers.

Below is also a patch to systemd to stop dropping the new attributes
(why they think they need to inspect and discard the data they do not
understand is beyond me).

Thanks,
Dmitry

Dmitry Torokhov (2):
  driver core: emit uevents when device is bound to a driver
  sysfs: add devm_sysfs_create_group() and friends

 drivers/base/dd.c       |   4 ++
 fs/sysfs/group.c        | 124 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/kobject.h |   2 +
 include/linux/sysfs.h   |  10 ++++
 lib/kobject_uevent.c    |   2 +
 5 files changed, 142 insertions(+)

-- >8 --

>From 6d10e621578dffcca0ad785e4a73196aa25350f6 Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Mon, 17 Jul 2017 20:10:17 -0700
Subject: [PATCH] Add handling for bind/unbind actions

Newer kernels will emit uevents with "bind" and "unbind" actions. These
uevents will be issued when driver is bound to or unbound from a device.
"Bind" events are helpful when device requires a firmware to operate
properly, and driver is unable to create a child device before firmware
is properly loaded.

For some reason systemd validates actions and drops the ones it does not
know, instead of passing them on through as old udev did, so we need to
explicitly teach it about them.
---
 src/libsystemd/sd-device/device-internal.h | 2 ++
 src/libsystemd/sd-device/device-private.c  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/src/libsystemd/sd-device/device-internal.h b/src/libsystemd/sd-device/device-internal.h
index f4783deef..0505a2730 100644
--- a/src/libsystemd/sd-device/device-internal.h
+++ b/src/libsystemd/sd-device/device-internal.h
@@ -104,6 +104,8 @@ typedef enum DeviceAction {
         DEVICE_ACTION_MOVE,
         DEVICE_ACTION_ONLINE,
         DEVICE_ACTION_OFFLINE,
+        DEVICE_ACTION_BIND,
+        DEVICE_ACTION_UNBIND,
         _DEVICE_ACTION_MAX,
         _DEVICE_ACTION_INVALID = -1,
 } DeviceAction;
diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c
index b4cd676c1..8839c3266 100644
--- a/src/libsystemd/sd-device/device-private.c
+++ b/src/libsystemd/sd-device/device-private.c
@@ -466,6 +466,8 @@ static const char* const device_action_table[_DEVICE_ACTION_MAX] = {
         [DEVICE_ACTION_MOVE] = "move",
         [DEVICE_ACTION_ONLINE] = "online",
         [DEVICE_ACTION_OFFLINE] = "offline",
+        [DEVICE_ACTION_BIND] = "bind",
+        [DEVICE_ACTION_UNBIND] = "unbind",
 };
 
 DEFINE_STRING_TABLE_LOOKUP(device_action, DeviceAction);
-- 
2.13.2.932.g7449e964c-goog

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

[Dmitry Torokhov]

Many drivers create additional driver-specific device attributes when
binding to the device and providing managed version of sysfs_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 sysfs_create_group() and use it with
devm_add_action() or devm_add_action_or_reset().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 fs/sysfs/group.c      | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  10 ++++
 2 files changed, 134 insertions(+)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index ac2de0ed69ad..24fc205fcb9b 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -13,6 +13,7 @@
 #include <linux/kobject.h>
 #include <linux/module.h>
 #include <linux/dcache.h>
+#include <linux/device.h>
 #include <linux/namei.h>
 #include <linux/err.h>
 #include "sysfs.h"
@@ -409,3 +410,126 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
 	return IS_ERR(link) ? PTR_ERR(link) : 0;
 }
 EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
+
+struct sysfs_group_devres {
+	const struct attribute_group *group;
+};
+
+static int devm_sysfs_group_match(struct device *dev, void *res, void *data)
+{
+	return ((struct sysfs_group_devres *)res)->group == data;
+}
+
+static void devm_sysfs_group_remove_group(struct device *dev, void *res)
+{
+	struct sysfs_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);
+}
+
+/**
+ * devm_sysfs_create_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_sysfs_create_group(struct device *dev,
+			    const struct attribute_group *grp)
+{
+	struct sysfs_group_devres *devres;
+	int error;
+
+	devres = devres_alloc(devm_sysfs_group_remove_group,
+			      sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
+
+	error = sysfs_create_group(&dev->kobj, grp);
+	if (error) {
+		devres_free(devres);
+		return error;
+	}
+
+	devres_add(dev, devres);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_sysfs_create_group);
+
+/**
+ * devm_sysfs_create_groups - create a bunch of managed attribute groups
+ * @dev:	The device to create the group for
+ * @groups:	The attribute groups to create, NULL terminated
+ *
+ * This function creates a bunch of managed attribute groups.  If an error
+ * occurs when creating a group, all previously created groups will be
+ * removed, unwinding everything back to the original state when this
+ * function was called.  It will explicitly warn and error if any of the
+ * attribute files being created already exist.
+ *
+ * Returns 0 on success or error code from sysfs_create_group on failure.
+ */
+int devm_sysfs_create_groups(struct device *dev,
+			     const struct attribute_group **groups)
+{
+	int error;
+	int i;
+
+	if (!groups)
+		return 0;
+
+	for (i = 0; groups[i]; i++) {
+		error = devm_sysfs_create_group(dev, groups[i]);
+		if (error) {
+			while (--i >= 0)
+				devm_sysfs_remove_group(dev, groups[i]);
+			return error;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_sysfs_create_groups);
+
+/**
+ * devm_sysfs_remove_group: remove a managed group from a device
+ * @dev:	device to remove the group from
+ * @grp:	group to remove
+ *
+ * This function removes a group of attributes from a device.  The attributes
+ * previously have to have been created for this group, otherwise it will fail.
+ */
+void devm_sysfs_remove_group(struct device *dev,
+			     const struct attribute_group *grp)
+{
+	WARN_ON(devres_release(dev, devm_sysfs_group_remove_group,
+			       devm_sysfs_group_match,
+			       /* cast away const */ (void *)grp));
+}
+EXPORT_SYMBOL_GPL(devm_sysfs_remove_group);
+
+/**
+ * devm_sysfs_remove_groups - remove a list of managed groups
+ *
+ * @dev:	The device for the groups to be removed from
+ * @groups:	NULL terminated list of groups to be removed
+ *
+ * If groups is not NULL, remove the specified groups from the device.
+ */
+void devm_sysfs_remove_groups(struct device *dev,
+			      const struct attribute_group **groups)
+{
+	int i;
+
+	if (!groups)
+		return;
+
+	for (i = 0; groups[i]; i++)
+		devm_sysfs_remove_group(dev, groups[i]);
+}
+EXPORT_SYMBOL_GPL(devm_sysfs_remove_groups);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index c6f0f0d0e17e..d9a5b8aab0e2 100644
--- 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;
+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);
+
 void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr);
 
 int __must_check sysfs_init(void);
-- 
2.13.2.932.g7449e964c-goog

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

[Greg Kroah-Hartman]

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?

> +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?

And do we need/want the non-devm versions:
	device_create_groups()
	device_remove_groups()
?

And you can probably drop the 'sysfs' from the function name if you
want.

</bikeshedding> :)

thanks,

greg k-h

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

[Dmitry Torokhov]

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.

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

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

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

Thanks!

-- 
Dmitry

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