Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1579334 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] driver core: emit uevents when device is bound to a driver Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-13 01:40 +0100
    [PATCH 2/2] sysfs: add devm_sysfs_create_group() and friends Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-13 01:40 +0100
    Re: [PATCH 1/2] driver core: emit uevents when device is bound to a  driver Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-02-13 13:10 +0100
      Re: [PATCH 1/2] driver core: emit uevents when device is bound to a  driver Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-13 19:50 +0100

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web