Skip to contentSkip to conversation

New bind/unbingd uevents

PATCH Superseded Dmitry Torokhov 2 participants 7 messages Jul 18, 2017 – Jul 19, 2017

Subsystems (2)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover New bind/unbingd uevents

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

Quoted 1 line

From 6d10e621578dffcca0ad785e4a73196aa25350f6 Mon Sep 17 00:00:00 2001

From: Dmitry Torokhov <[email protected]>
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(+)
src/libsystemd/sd-device/device-internal.h +2 -0
index f4783deef..0505a2730 100644
@@ -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;
src/libsystemd/sd-device/device-private.c +2 -0
index b4cd676c1..8839c3266 100644
@@ -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);
Signature · 1 line
2.13.2.932.g7449e964c-goog
#1 lore raw

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

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.

---
 drivers/base/dd.c       | 4 ++++
 include/linux/kobject.h | 2 ++
 lib/kobject_uevent.c    | 2 ++
 3 files changed, 8 insertions(+)
drivers/base/dd.c +4 -0
index 4882f06d12df..c17fefc77345 100644
@@ -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);
 	}
 }
 
include/linux/kobject.h +2 -0
index ca85cb80e99a..12f5ddccb97c 100644
@@ -57,6 +57,8 @@ enum kobject_action {
 	KOBJ_MOVE,
 	KOBJ_ONLINE,
 	KOBJ_OFFLINE,
+	KOBJ_BIND,
+	KOBJ_UNBIND,
 	KOBJ_MAX
 };
 
lib/kobject_uevent.c +2 -0
index 9a2b811966eb..4682e8545b5c 100644
@@ -50,6 +50,8 @@ static const char *kobject_actions[] = {
 	[KOBJ_MOVE] =		"move",
 	[KOBJ_ONLINE] =		"online",
 	[KOBJ_OFFLINE] =	"offline",
+	[KOBJ_BIND] =		"bind",
+	[KOBJ_UNBIND] =		"unbind",
 };
 
 /**
Signature · 1 line
2.13.2.932.g7449e964c-goog
#2 lore raw

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

In reply to 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().

---
 fs/sysfs/group.c      | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  10 ++++
 2 files changed, 134 insertions(+)
fs/sysfs/group.c +124 -0
index ac2de0ed69ad..24fc205fcb9b 100644
@@ -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);
include/linux/sysfs.h +10 -0
index c6f0f0d0e17e..d9a5b8aab0e2 100644
@@ -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);
Signature · 1 line
2.13.2.932.g7449e964c-goog
#3 lore raw

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

include/linux/sysfs.h int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, · line 285

Quoted 7 lines · Dmitry Torokhov
include/linux/sysfs.h +1 -0
@@ -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?

include/linux/sysfs.h int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, · line 293

Quoted 8 lines
include/linux/sysfs.h +8 -0
+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

#4 lore raw

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

Quoted 10 lines · Greg Kroah-Hartman

include/linux/sysfs.h int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, · line 285

Quoted 7 lines · Dmitry Torokhov
include/linux/sysfs.h +1 -0
@@ -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.

Quoted 13 lines

include/linux/sysfs.h int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, · line 293

Quoted 9 lines
include/linux/sysfs.h +8 -0
+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.

Quoted 5 lines

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

Quoted 3 lines

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!

Signature · 1 line
Dmitry
#5 lore raw

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

Quoted 12 lines · Dmitry Torokhov
Quoted 10 lines · Greg Kroah-Hartman

include/linux/sysfs.h int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, · line 285

Quoted 7 lines · Dmitry Torokhov
include/linux/sysfs.h +1 -0
@@ -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?

Quoted 6 lines

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 :)

Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
include/linux/sysfs.h +4 -0
+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);
Quoted 8 lines

include/linux/sysfs.h int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj, · line 293

Quoted 4 lines
include/linux/sysfs.h +4 -0
+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?

Quoted 3 lines

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.

Quoted 7 lines
Quoted 4 lines

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 :)

Quoted 9 lines
Quoted 2 lines

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

#6 lore raw
In reply to Dmitry Torokhov
Quoted 10 lines · 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).

Looks good, only minor comments on patch 2.

thanks,

greg k-h