[PATCH 0/2] New bind/unbingd uevents

[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 1/2] driver core: emit uevents when device is bound to a driver

[Dmitry Torokhov]

There are certain touch controllers that may come up in either normal
(application) or boot mode, depending on whether firmware/configuration is
corrupted when they are powered on. In boot mode the kernel does not create
input device instance (because it does not necessarily know the
characteristics of the input device in question).

Another number of controllers does not store firmware in a non-volatile
memory, and they similarly need to have firmware loaded before input device
instance is created. There are also other types of devices with similar
behavior.

There is a desire to be able to trigger firmware loading via udev, but it
has to happen only when driver is bound to a physical device (i2c or spi).
These udev actions can not use ADD events, as those happen too early, so we
are introducing BIND and UNBIND events that are emitted at the right
moment.

Also, many drivers create additional driver-specific device attributes
when binding to the device, to provide userspace with additional controls.
The new events allow userspace to adjust these driver-specific attributes
without worrying that they are not there yet.

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 4882f06d12df..c17fefc77345 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -259,6 +259,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)
@@ -848,6 +850,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 ca85cb80e99a..12f5ddccb97c 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.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 0/2] New bind/unbingd uevents

[Greg Kroah-Hartman]

On Tue, Jul 18, 2017 at 12:30:56PM -0700, Dmitry Torokhov wrote:
> 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).

Looks good, only minor comments on patch 2.

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