[PATCH V4] leds: trigger: Introduce an USB port trigger

[PATCH V4] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This commit adds a new trigger responsible for turning on LED when USB
device gets connected to the specified USB port. This can can useful for
various home routers that have USB port(s) and a proper LED telling user
a device is connected.

The trigger gets its documentation file but basically it just requires
specifying USB port in a Linux format (e.g. echo 1-1 > new_port).

During work on this trigger there was a plan to add DT bindings for it,
but there wasn't an agreement on the format yet. This can be worked on
later, a sysfs interface is needed anyway for platforms not using DT.

Another planned feature is support for LED reacting to the USB activity.
This can be implemented with another sysfs file for setting mode. The
default mode wouldn't change so there won't be ABI breakage and such
feature can be safely implemented later.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Trying to add DT support, idea postponed as it will take more time
    to discuss the bindings.
V3: Fix typos in commit and Documentation (thanks Jacek!)
    Use "ports" sysfs file for adding and removing USB ports (thx Jacek)
    Check if there is USB device connected after adding new USB port
    Fix memory leak or two
V3.5: Fix e-mail address (thanks Matthias)
      Simplify conditions in usbport_trig_notify (thx Matthias)
      Make "ports" a subdirectory with file per port, to match one value
      per file sysfs rule (thanks Greg)
      As "ports" couldn't be used for adding and removing ports anymore,
      there are now "new_port" and "remove_port". Having them makes this
      API also common with e.g. pci and usb buses.
V4: Add Documentation/ABI/testing/sysfs-class-led-trigger-usbport and
    reference it in Documentation/leds/ledtrig-usbport.txt to avoid doc
    duplication.
---
 .../ABI/testing/sysfs-class-led-trigger-usbport    |  25 ++
 Documentation/leds/ledtrig-usbport.txt             |  46 +++
 drivers/leds/trigger/Kconfig                       |   8 +
 drivers/leds/trigger/Makefile                      |   1 +
 drivers/leds/trigger/ledtrig-usbport.c             | 309 +++++++++++++++++++++
 5 files changed, 389 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-usbport
 create mode 100644 Documentation/leds/ledtrig-usbport.txt
 create mode 100644 drivers/leds/trigger/ledtrig-usbport.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-usbport b/Documentation/ABI/testing/sysfs-class-led-trigger-usbport
new file mode 100644
index 0000000..653bde1
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-usbport
@@ -0,0 +1,25 @@
+What:		/sys/class/leds/<led>/new_port
+Date:		August 2016
+KernelVersion:	4.9
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Adds USB port (specified by a name using Linux format, e.g. 1-1)
+		to the list of observable ones. This will have immediate effect
+		if there is device already connected. This will also make
+		trigger checking for a match on every USB (dis)connection event.
+
+What:		/sys/class/leds/<led>/remove_port
+Date:		August 2016
+KernelVersion:	4.9
+Contact:	linux-leds@vger.kernel.org
+Description:
+		This allows removing USB port from the list of observable ones.
+
+What:		/sys/class/leds/<led>/ports/<port>
+Date:		August 2016
+KernelVersion:	4.9
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Every port entry is a name (in Linux format) of USB port that is
+		being observed by the usbport trigger. It's useful for checking
+		the current state of trigger setup.
diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
new file mode 100644
index 0000000..8e36e2d
--- /dev/null
+++ b/Documentation/leds/ledtrig-usbport.txt
@@ -0,0 +1,46 @@
+USB port LED trigger
+====================
+
+This LED trigger can be used for signalling to the user a presence of USB device
+in a given port. It simply turns on LED when device appears and turns it off
+when it disappears.
+
+It requires specifying a list of USB ports that should be observed. Used format
+matches Linux kernel format and consists of a root hub number and a hub port
+separated by a dash (e.g. 3-1).
+
+It is also possible to handle devices with internal hubs (that are always
+connected to the root hub). User can simply specify internal hub ports then
+(e.g. 1-1.1, 1-1.2, etc.).
+
+Please note that this trigger allows assigning multiple USB ports to a single
+LED. This can be useful in two cases:
+
+1) Device with single USB LED and few physical ports
+
+In such a case LED will be turned on as long as there is at least one connected
+USB device.
+
+2) Device with a physical port handled by few controllers
+
+Some devices have e.g. one controller per PHY standard. E.g. USB 3.0 physical
+port may be handled by ohci-platform, ehci-platform and xhci-hcd. If there is
+only one LED user will most likely want to assign ports from all 3 hubs.
+
+
+This trigger can be activated from user space on led class devices as shown
+below:
+
+  echo usbport > trigger
+
+This adds sysfs attributes to the LED that are documented in:
+Documentation/ABI/testing/sysfs-class-led-trigger-usbport
+
+Example use-case:
+
+  echo usbport > trigger
+  echo 4-1 > new_port
+  echo 2-1 > new_port
+  ls ports
+  echo 4-1 > remove_port
+  ls ports
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 3f9ddb9..bdd6fd2 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -126,4 +126,12 @@ config LEDS_TRIGGER_PANIC
 	  a different trigger.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_USBPORT
+	tristate "USB port LED trigger"
+	depends on LEDS_TRIGGERS && USB
+	help
+	  This allows LEDs to be controlled by USB events. Enabling this option
+	  allows specifying list of USB ports that should turn on LED when some
+	  USB device gets connected.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index a72c43c..56e1741 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
 obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
 obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
+obj-$(CONFIG_LEDS_TRIGGER_USBPORT)	+= ledtrig-usbport.o
diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
new file mode 100644
index 0000000..1532b60
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -0,0 +1,309 @@
+/*
+ * USB port LED trigger
+ *
+ * Copyright (C) 2016 Rafał Miłecki <rafal@milecki.pl>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include "../leds.h"
+
+struct usbport_trig_port {
+	char *name;
+	struct device_attribute attr;
+	struct list_head list;
+};
+
+struct usbport_trig_data {
+	struct led_classdev *led_cdev;
+	struct list_head ports;
+	struct kobject *ports_dir;
+	struct notifier_block nb;
+	int count; /* Amount of connected matching devices */
+};
+
+/*
+ * Helpers
+ */
+
+/**
+ * usbport_trig_usb_dev_observed - Check if dev is connected to observerd port
+ */
+static bool usbport_trig_usb_dev_observed(struct usbport_trig_data *usbport_data,
+					  struct usb_device *usb_dev)
+{
+	struct usbport_trig_port *port;
+	const char *name = dev_name(&usb_dev->dev);
+
+	list_for_each_entry(port, &usbport_data->ports, list) {
+		if (!strcmp(port->name, name))
+			return true;
+	}
+
+	return false;
+}
+
+static int usbport_trig_usb_dev_check(struct usb_device *usb_dev, void *data)
+{
+	struct usbport_trig_data *usbport_data = data;
+
+	if (usbport_trig_usb_dev_observed(usbport_data, usb_dev))
+		usbport_data->count++;
+
+	return 0;
+}
+
+/**
+ * usbport_trig_update_count - Recalculate amount of connected matching devices
+ */
+static void usbport_trig_update_count(struct usbport_trig_data *usbport_data)
+{
+	struct led_classdev *led_cdev = usbport_data->led_cdev;
+
+	usbport_data->count = 0;
+	usb_for_each_dev(usbport_data, usbport_trig_usb_dev_check);
+	led_set_brightness_nosleep(led_cdev,
+				   usbport_data->count ? LED_FULL : LED_OFF);
+}
+
+static int usbport_trig_add_port(struct usbport_trig_data *usbport_data,
+				 const char *name)
+{
+	struct usbport_trig_port *port;
+	int err;
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port) {
+		err = -ENOMEM;
+		goto err_out;
+	}
+
+	port->name = kzalloc(strlen(name) + 1, GFP_KERNEL);
+	if (!port->name) {
+		err = -ENOMEM;
+		goto err_free_port;
+	}
+	strcpy(port->name, name);
+
+	port->attr.attr.name = port->name;
+
+	err = sysfs_create_file(usbport_data->ports_dir, &port->attr.attr);
+	if (err)
+		goto err_free_name;
+
+	list_add_tail(&port->list, &usbport_data->ports);
+
+	return 0;
+
+err_free_name:
+	kfree(port->name);
+err_free_port:
+	kfree(port);
+err_out:
+	return err;
+}
+
+static void usbport_trig_remove_port(struct usbport_trig_data *usbport_data,
+				     struct usbport_trig_port *port)
+{
+	list_del(&port->list);
+	sysfs_remove_file(usbport_data->ports_dir, &port->attr.attr);
+	kfree(port->name);
+	kfree(port);
+}
+
+/*
+ * Device attr
+ */
+
+static ssize_t new_port_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
+	struct usbport_trig_port *port;
+	char *name;
+	size_t len;
+
+	len = strlen(buf);
+	/* For user convenience trim line break */
+	if (len && buf[len - 1] == '\n')
+		len--;
+	if (!len)
+		return -EINVAL;
+
+	name = kzalloc(len + 1, GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
+	strncpy(name, buf, len);
+
+	list_for_each_entry(port, &usbport_data->ports, list) {
+		if (!strcmp(port->name, name))
+			return -EEXIST;
+	}
+
+	usbport_trig_add_port(usbport_data, name);
+
+	kfree(name);
+
+	usbport_trig_update_count(usbport_data);
+
+	return size;
+}
+
+static DEVICE_ATTR(new_port, S_IWUSR, NULL, new_port_store);
+
+static ssize_t remove_port_store(struct device *dev,
+				 struct device_attribute *attr, const char *buf,
+				 size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
+	struct usbport_trig_port *port, *tmp;
+	size_t len;
+
+	len = strlen(buf);
+	/* For user convenience trim line break */
+	if (len && buf[len - 1] == '\n')
+		len--;
+	if (!len)
+		return -EINVAL;
+
+	list_for_each_entry_safe(port, tmp, &usbport_data->ports,
+					list) {
+		if (strlen(port->name) == len &&
+		    !strncmp(port->name, buf, len)) {
+			usbport_trig_remove_port(usbport_data, port);
+			usbport_trig_update_count(usbport_data);
+			return size;
+		}
+	}
+
+	return -ENOENT;
+}
+
+static DEVICE_ATTR(remove_port, S_IWUSR, NULL, remove_port_store);
+
+/*
+ * Init, exit, etc.
+ */
+
+static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
+			       void *data)
+{
+	struct usbport_trig_data *usbport_data =
+		container_of(nb, struct usbport_trig_data, nb);
+	struct led_classdev *led_cdev = usbport_data->led_cdev;
+
+	if (!usbport_trig_usb_dev_observed(usbport_data, data))
+		return NOTIFY_DONE;
+
+	switch (action) {
+	case USB_DEVICE_ADD:
+		if (usbport_data->count++ == 0)
+			led_set_brightness_nosleep(led_cdev, LED_FULL);
+		return NOTIFY_OK;
+	case USB_DEVICE_REMOVE:
+		if (--usbport_data->count == 0)
+			led_set_brightness_nosleep(led_cdev, LED_OFF);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static void usbport_trig_activate(struct led_classdev *led_cdev)
+{
+	struct usbport_trig_data *usbport_data;
+	int err;
+
+	usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
+	if (!usbport_data)
+		return;
+	usbport_data->led_cdev = led_cdev;
+
+	/* Storing ports */
+	INIT_LIST_HEAD(&usbport_data->ports);
+	usbport_data->ports_dir = kobject_create_and_add("ports",
+							 &led_cdev->dev->kobj);
+	if (!usbport_data->ports_dir)
+		goto err_free;
+
+	/* API for ports management */
+	err = device_create_file(led_cdev->dev, &dev_attr_new_port);
+	if (err)
+		goto err_put_ports;
+	err = device_create_file(led_cdev->dev, &dev_attr_remove_port);
+	if (err)
+		goto err_remove_new_port;
+
+	/* Notifications */
+	usbport_data->nb.notifier_call = usbport_trig_notify,
+	led_cdev->trigger_data = usbport_data;
+	usb_register_notify(&usbport_data->nb);
+
+	led_cdev->activated = true;
+	return;
+
+err_remove_new_port:
+	device_remove_file(led_cdev->dev, &dev_attr_new_port);
+err_put_ports:
+	kobject_put(usbport_data->ports_dir);
+err_free:
+	kfree(usbport_data);
+}
+
+static void usbport_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
+	struct usbport_trig_port *port, *tmp;
+
+	if (!led_cdev->activated)
+		return;
+
+	list_for_each_entry_safe(port, tmp, &usbport_data->ports, list) {
+		usbport_trig_remove_port(usbport_data, port);
+	}
+
+	usb_unregister_notify(&usbport_data->nb);
+
+	device_remove_file(led_cdev->dev, &dev_attr_remove_port);
+	device_remove_file(led_cdev->dev, &dev_attr_new_port);
+
+	kobject_put(usbport_data->ports_dir);
+
+	kfree(usbport_data);
+
+	led_cdev->activated = false;
+}
+
+static struct led_trigger usbport_led_trigger = {
+	.name     = "usbport",
+	.activate = usbport_trig_activate,
+	.deactivate = usbport_trig_deactivate,
+};
+
+static int __init usbport_trig_init(void)
+{
+	return led_trigger_register(&usbport_led_trigger);
+}
+
+static void __exit usbport_trig_exit(void)
+{
+	led_trigger_unregister(&usbport_led_trigger);
+}
+
+module_init(usbport_trig_init);
+module_exit(usbport_trig_exit);
+
+MODULE_AUTHOR("Rafał Miłecki <rafal@milecki.pl>");
+MODULE_DESCRIPTION("USB port trigger");
+MODULE_LICENSE("GPL");
-- 
2.9.3

[PATCH V4] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This commit adds a new trigger responsible for turning on LED when USB
device gets connected to the specified USB port. This can can useful for
various home routers that have USB port(s) and a proper LED telling user
a device is connected.

The trigger gets its documentation file but basically it just requires
specifying USB port in a Linux format (e.g. echo 1-1 > new_port).

During work on this trigger there was a plan to add DT bindings for it,
but there wasn't an agreement on the format yet. This can be worked on
later, a sysfs interface is needed anyway for platforms not using DT.

Another planned feature is support for LED reacting to the USB activity.
This can be implemented with another sysfs file for setting mode. The
default mode wouldn't change so there won't be ABI breakage and such
feature can be safely implemented later.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Trying to add DT support, idea postponed as it will take more time
    to discuss the bindings.
V3: Fix typos in commit and Documentation (thanks Jacek!)
    Use "ports" sysfs file for adding and removing USB ports (thx Jacek)
    Check if there is USB device connected after adding new USB port
    Fix memory leak or two
V3.5: Fix e-mail address (thanks Matthias)
      Simplify conditions in usbport_trig_notify (thx Matthias)
      Make "ports" a subdirectory with file per port, to match one value
      per file sysfs rule (thanks Greg)
      As "ports" couldn't be used for adding and removing ports anymore,
      there are now "new_port" and "remove_port". Having them makes this
      API also common with e.g. pci and usb buses.
V4: Add Documentation/ABI/testing/sysfs-class-led-trigger-usbport and
    reference it in Documentation/leds/ledtrig-usbport.txt to avoid doc
    duplication.
---
 .../ABI/testing/sysfs-class-led-trigger-usbport    |  25 ++
 Documentation/leds/ledtrig-usbport.txt             |  46 +++
 drivers/leds/trigger/Kconfig                       |   8 +
 drivers/leds/trigger/Makefile                      |   1 +
 drivers/leds/trigger/ledtrig-usbport.c             | 309 +++++++++++++++++++++
 5 files changed, 389 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-usbport
 create mode 100644 Documentation/leds/ledtrig-usbport.txt
 create mode 100644 drivers/leds/trigger/ledtrig-usbport.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-usbport b/Documentation/ABI/testing/sysfs-class-led-trigger-usbport
new file mode 100644
index 0000000..653bde1
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-usbport
@@ -0,0 +1,25 @@
+What:		/sys/class/leds/<led>/new_port
+Date:		August 2016
+KernelVersion:	4.9
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Adds USB port (specified by a name using Linux format, e.g. 1-1)
+		to the list of observable ones. This will have immediate effect
+		if there is device already connected. This will also make
+		trigger checking for a match on every USB (dis)connection event.
+
+What:		/sys/class/leds/<led>/remove_port
+Date:		August 2016
+KernelVersion:	4.9
+Contact:	linux-leds@vger.kernel.org
+Description:
+		This allows removing USB port from the list of observable ones.
+
+What:		/sys/class/leds/<led>/ports/<port>
+Date:		August 2016
+KernelVersion:	4.9
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Every port entry is a name (in Linux format) of USB port that is
+		being observed by the usbport trigger. It's useful for checking
+		the current state of trigger setup.
diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
new file mode 100644
index 0000000..8e36e2d
--- /dev/null
+++ b/Documentation/leds/ledtrig-usbport.txt
@@ -0,0 +1,46 @@
+USB port LED trigger
+====================
+
+This LED trigger can be used for signalling to the user a presence of USB device
+in a given port. It simply turns on LED when device appears and turns it off
+when it disappears.
+
+It requires specifying a list of USB ports that should be observed. Used format
+matches Linux kernel format and consists of a root hub number and a hub port
+separated by a dash (e.g. 3-1).
+
+It is also possible to handle devices with internal hubs (that are always
+connected to the root hub). User can simply specify internal hub ports then
+(e.g. 1-1.1, 1-1.2, etc.).
+
+Please note that this trigger allows assigning multiple USB ports to a single
+LED. This can be useful in two cases:
+
+1) Device with single USB LED and few physical ports
+
+In such a case LED will be turned on as long as there is at least one connected
+USB device.
+
+2) Device with a physical port handled by few controllers
+
+Some devices have e.g. one controller per PHY standard. E.g. USB 3.0 physical
+port may be handled by ohci-platform, ehci-platform and xhci-hcd. If there is
+only one LED user will most likely want to assign ports from all 3 hubs.
+
+
+This trigger can be activated from user space on led class devices as shown
+below:
+
+  echo usbport > trigger
+
+This adds sysfs attributes to the LED that are documented in:
+Documentation/ABI/testing/sysfs-class-led-trigger-usbport
+
+Example use-case:
+
+  echo usbport > trigger
+  echo 4-1 > new_port
+  echo 2-1 > new_port
+  ls ports
+  echo 4-1 > remove_port
+  ls ports
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 3f9ddb9..bdd6fd2 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -126,4 +126,12 @@ config LEDS_TRIGGER_PANIC
 	  a different trigger.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_USBPORT
+	tristate "USB port LED trigger"
+	depends on LEDS_TRIGGERS && USB
+	help
+	  This allows LEDs to be controlled by USB events. Enabling this option
+	  allows specifying list of USB ports that should turn on LED when some
+	  USB device gets connected.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index a72c43c..56e1741 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
 obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
 obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
+obj-$(CONFIG_LEDS_TRIGGER_USBPORT)	+= ledtrig-usbport.o
diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
new file mode 100644
index 0000000..1532b60
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -0,0 +1,309 @@
+/*
+ * USB port LED trigger
+ *
+ * Copyright (C) 2016 Rafał Miłecki <rafal@milecki.pl>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include "../leds.h"
+
+struct usbport_trig_port {
+	char *name;
+	struct device_attribute attr;
+	struct list_head list;
+};
+
+struct usbport_trig_data {
+	struct led_classdev *led_cdev;
+	struct list_head ports;
+	struct kobject *ports_dir;
+	struct notifier_block nb;
+	int count; /* Amount of connected matching devices */
+};
+
+/*
+ * Helpers
+ */
+
+/**
+ * usbport_trig_usb_dev_observed - Check if dev is connected to observerd port
+ */
+static bool usbport_trig_usb_dev_observed(struct usbport_trig_data *usbport_data,
+					  struct usb_device *usb_dev)
+{
+	struct usbport_trig_port *port;
+	const char *name = dev_name(&usb_dev->dev);
+
+	list_for_each_entry(port, &usbport_data->ports, list) {
+		if (!strcmp(port->name, name))
+			return true;
+	}
+
+	return false;
+}
+
+static int usbport_trig_usb_dev_check(struct usb_device *usb_dev, void *data)
+{
+	struct usbport_trig_data *usbport_data = data;
+
+	if (usbport_trig_usb_dev_observed(usbport_data, usb_dev))
+		usbport_data->count++;
+
+	return 0;
+}
+
+/**
+ * usbport_trig_update_count - Recalculate amount of connected matching devices
+ */
+static void usbport_trig_update_count(struct usbport_trig_data *usbport_data)
+{
+	struct led_classdev *led_cdev = usbport_data->led_cdev;
+
+	usbport_data->count = 0;
+	usb_for_each_dev(usbport_data, usbport_trig_usb_dev_check);
+	led_set_brightness_nosleep(led_cdev,
+				   usbport_data->count ? LED_FULL : LED_OFF);
+}
+
+static int usbport_trig_add_port(struct usbport_trig_data *usbport_data,
+				 const char *name)
+{
+	struct usbport_trig_port *port;
+	int err;
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port) {
+		err = -ENOMEM;
+		goto err_out;
+	}
+
+	port->name = kzalloc(strlen(name) + 1, GFP_KERNEL);
+	if (!port->name) {
+		err = -ENOMEM;
+		goto err_free_port;
+	}
+	strcpy(port->name, name);
+
+	port->attr.attr.name = port->name;
+
+	err = sysfs_create_file(usbport_data->ports_dir, &port->attr.attr);
+	if (err)
+		goto err_free_name;
+
+	list_add_tail(&port->list, &usbport_data->ports);
+
+	return 0;
+
+err_free_name:
+	kfree(port->name);
+err_free_port:
+	kfree(port);
+err_out:
+	return err;
+}
+
+static void usbport_trig_remove_port(struct usbport_trig_data *usbport_data,
+				     struct usbport_trig_port *port)
+{
+	list_del(&port->list);
+	sysfs_remove_file(usbport_data->ports_dir, &port->attr.attr);
+	kfree(port->name);
+	kfree(port);
+}
+
+/*
+ * Device attr
+ */
+
+static ssize_t new_port_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
+	struct usbport_trig_port *port;
+	char *name;
+	size_t len;
+
+	len = strlen(buf);
+	/* For user convenience trim line break */
+	if (len && buf[len - 1] == '\n')
+		len--;
+	if (!len)
+		return -EINVAL;
+
+	name = kzalloc(len + 1, GFP_KERNEL);
+	if (!name)
+		return -ENOMEM;
+	strncpy(name, buf, len);
+
+	list_for_each_entry(port, &usbport_data->ports, list) {
+		if (!strcmp(port->name, name))
+			return -EEXIST;
+	}
+
+	usbport_trig_add_port(usbport_data, name);
+
+	kfree(name);
+
+	usbport_trig_update_count(usbport_data);
+
+	return size;
+}
+
+static DEVICE_ATTR(new_port, S_IWUSR, NULL, new_port_store);
+
+static ssize_t remove_port_store(struct device *dev,
+				 struct device_attribute *attr, const char *buf,
+				 size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
+	struct usbport_trig_port *port, *tmp;
+	size_t len;
+
+	len = strlen(buf);
+	/* For user convenience trim line break */
+	if (len && buf[len - 1] == '\n')
+		len--;
+	if (!len)
+		return -EINVAL;
+
+	list_for_each_entry_safe(port, tmp, &usbport_data->ports,
+					list) {
+		if (strlen(port->name) == len &&
+		    !strncmp(port->name, buf, len)) {
+			usbport_trig_remove_port(usbport_data, port);
+			usbport_trig_update_count(usbport_data);
+			return size;
+		}
+	}
+
+	return -ENOENT;
+}
+
+static DEVICE_ATTR(remove_port, S_IWUSR, NULL, remove_port_store);
+
+/*
+ * Init, exit, etc.
+ */
+
+static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
+			       void *data)
+{
+	struct usbport_trig_data *usbport_data =
+		container_of(nb, struct usbport_trig_data, nb);
+	struct led_classdev *led_cdev = usbport_data->led_cdev;
+
+	if (!usbport_trig_usb_dev_observed(usbport_data, data))
+		return NOTIFY_DONE;
+
+	switch (action) {
+	case USB_DEVICE_ADD:
+		if (usbport_data->count++ == 0)
+			led_set_brightness_nosleep(led_cdev, LED_FULL);
+		return NOTIFY_OK;
+	case USB_DEVICE_REMOVE:
+		if (--usbport_data->count == 0)
+			led_set_brightness_nosleep(led_cdev, LED_OFF);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static void usbport_trig_activate(struct led_classdev *led_cdev)
+{
+	struct usbport_trig_data *usbport_data;
+	int err;
+
+	usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
+	if (!usbport_data)
+		return;
+	usbport_data->led_cdev = led_cdev;
+
+	/* Storing ports */
+	INIT_LIST_HEAD(&usbport_data->ports);
+	usbport_data->ports_dir = kobject_create_and_add("ports",
+							 &led_cdev->dev->kobj);
+	if (!usbport_data->ports_dir)
+		goto err_free;
+
+	/* API for ports management */
+	err = device_create_file(led_cdev->dev, &dev_attr_new_port);
+	if (err)
+		goto err_put_ports;
+	err = device_create_file(led_cdev->dev, &dev_attr_remove_port);
+	if (err)
+		goto err_remove_new_port;
+
+	/* Notifications */
+	usbport_data->nb.notifier_call = usbport_trig_notify,
+	led_cdev->trigger_data = usbport_data;
+	usb_register_notify(&usbport_data->nb);
+
+	led_cdev->activated = true;
+	return;
+
+err_remove_new_port:
+	device_remove_file(led_cdev->dev, &dev_attr_new_port);
+err_put_ports:
+	kobject_put(usbport_data->ports_dir);
+err_free:
+	kfree(usbport_data);
+}
+
+static void usbport_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
+	struct usbport_trig_port *port, *tmp;
+
+	if (!led_cdev->activated)
+		return;
+
+	list_for_each_entry_safe(port, tmp, &usbport_data->ports, list) {
+		usbport_trig_remove_port(usbport_data, port);
+	}
+
+	usb_unregister_notify(&usbport_data->nb);
+
+	device_remove_file(led_cdev->dev, &dev_attr_remove_port);
+	device_remove_file(led_cdev->dev, &dev_attr_new_port);
+
+	kobject_put(usbport_data->ports_dir);
+
+	kfree(usbport_data);
+
+	led_cdev->activated = false;
+}
+
+static struct led_trigger usbport_led_trigger = {
+	.name     = "usbport",
+	.activate = usbport_trig_activate,
+	.deactivate = usbport_trig_deactivate,
+};
+
+static int __init usbport_trig_init(void)
+{
+	return led_trigger_register(&usbport_led_trigger);
+}
+
+static void __exit usbport_trig_exit(void)
+{
+	led_trigger_unregister(&usbport_led_trigger);
+}
+
+module_init(usbport_trig_init);
+module_exit(usbport_trig_exit);
+
+MODULE_AUTHOR("Rafał Miłecki <rafal@milecki.pl>");
+MODULE_DESCRIPTION("USB port trigger");
+MODULE_LICENSE("GPL");
-- 
2.9.3

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Greg KH]

On Thu, Aug 25, 2016 at 10:03:52AM +0200, Rafał Miłecki wrote:
> +static void usbport_trig_activate(struct led_classdev *led_cdev)
> +{
> +	struct usbport_trig_data *usbport_data;
> +	int err;
> +
> +	usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
> +	if (!usbport_data)
> +		return;
> +	usbport_data->led_cdev = led_cdev;
> +
> +	/* Storing ports */
> +	INIT_LIST_HEAD(&usbport_data->ports);
> +	usbport_data->ports_dir = kobject_create_and_add("ports",
> +							 &led_cdev->dev->kobj);

If you _ever_ find yourself in a driver having to use kobject calls,
it's a HUGE hint that you are doing something wrong.

Hint, you are doing this wrong :)

Use an attribute group if you need a subdirectory in sysfs, using a
"raw" kobject like this hides it from all userspace tools and so no
userspace program can see it (hint, try using libudev to access these
files attached to the device...)

> +	if (!usbport_data->ports_dir)
> +		goto err_free;
> +
> +	/* API for ports management */
> +	err = device_create_file(led_cdev->dev, &dev_attr_new_port);
> +	if (err)
> +		goto err_put_ports;
> +	err = device_create_file(led_cdev->dev, &dev_attr_remove_port);
> +	if (err)
> +		goto err_remove_new_port;

Doesn't this race with userspace and fail?  Shouldn't the led core be
creating your leds for you?

> +
> +	/* Notifications */
> +	usbport_data->nb.notifier_call = usbport_trig_notify,
> +	led_cdev->trigger_data = usbport_data;
> +	usb_register_notify(&usbport_data->nb);

Don't abuse the USB notifier chain with stuff like this please, is that
really necessary?  Why can't your hardware just tell you what USB ports
are in use "out of band"?


> +
> +	led_cdev->activated = true;
> +	return;
> +
> +err_remove_new_port:
> +	device_remove_file(led_cdev->dev, &dev_attr_new_port);
> +err_put_ports:
> +	kobject_put(usbport_data->ports_dir);
> +err_free:
> +	kfree(usbport_data);
> +}

And again, why is this USB specific?  Why can't you use this same
userspace api and codebase for PCI ports?  For a sdcard port?  For a
thunderbolt port?

thanks,

greg k-h

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 25 August 2016 at 14:49, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Aug 25, 2016 at 10:03:52AM +0200, Rafał Miłecki wrote:
>> +static void usbport_trig_activate(struct led_classdev *led_cdev)
>> +{
>> +     struct usbport_trig_data *usbport_data;
>> +     int err;
>> +
>> +     usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
>> +     if (!usbport_data)
>> +             return;
>> +     usbport_data->led_cdev = led_cdev;
>> +
>> +     /* Storing ports */
>> +     INIT_LIST_HEAD(&usbport_data->ports);
>> +     usbport_data->ports_dir = kobject_create_and_add("ports",
>> +                                                      &led_cdev->dev->kobj);
>
> If you _ever_ find yourself in a driver having to use kobject calls,
> it's a HUGE hint that you are doing something wrong.
>
> Hint, you are doing this wrong :)
>
> Use an attribute group if you need a subdirectory in sysfs, using a
> "raw" kobject like this hides it from all userspace tools and so no
> userspace program can see it (hint, try using libudev to access these
> files attached to the device...)

Oops. Thanks for pointing groups to me. I was looking at sysfs.h
earlier but I didn't realize group can be a subdirectory. I can see
these sysfs_create_group(s) and friends now, thanks.


>> +     if (!usbport_data->ports_dir)
>> +             goto err_free;
>> +
>> +     /* API for ports management */
>> +     err = device_create_file(led_cdev->dev, &dev_attr_new_port);
>> +     if (err)
>> +             goto err_put_ports;
>> +     err = device_create_file(led_cdev->dev, &dev_attr_remove_port);
>> +     if (err)
>> +             goto err_remove_new_port;
>
> Doesn't this race with userspace and fail?  Shouldn't the led core be
> creating your leds for you?

These questions aren't clear to me. What kind of race? Doing
echo usbport > trigger
results in trigger core calling usbport_trig_activate. We create new
attributes and then we return.

I'm also not creating any leds there. This already has to be LED
available if you want to assign some trigger to it.


>> +
>> +     /* Notifications */
>> +     usbport_data->nb.notifier_call = usbport_trig_notify,
>> +     led_cdev->trigger_data = usbport_data;
>> +     usb_register_notify(&usbport_data->nb);
>
> Don't abuse the USB notifier chain with stuff like this please, is that
> really necessary?  Why can't your hardware just tell you what USB ports
> are in use "out of band"?

I totally don't understand this part. This LED trigger is supposed to
react to USB devices appearing at specified ports. How else can I know
there is a new USB device if not by notifications?
I'm wondering if we are on the same page. Could it be my idea of this
LED trigger is not clear at all? Could you take a look at commit
message again, please? Mostly this part:
> This commit adds a new trigger responsible for turning on LED when USB
> device gets connected to the specified USB port. This can can useful for
> various home routers that have USB port(s) and a proper LED telling user
> a device is connected.

Can I add something more to make it clear what this trigger is responsible for?


>> +
>> +     led_cdev->activated = true;
>> +     return;
>> +
>> +err_remove_new_port:
>> +     device_remove_file(led_cdev->dev, &dev_attr_new_port);
>> +err_put_ports:
>> +     kobject_put(usbport_data->ports_dir);
>> +err_free:
>> +     kfree(usbport_data);
>> +}
>
> And again, why is this USB specific?  Why can't you use this same
> userspace api and codebase for PCI ports?  For a sdcard port?  For a
> thunderbolt port?

I'm leaving this one unanswered as discussion on this continued in
V3.5 thread below my reply:
On 25 August 2016 at 07:14, Rafał Miłecki <zajec5@gmail.com> wrote:
> Good question. I would like to extend this USB port trigger in the
> future by reacting to USB activity. This involves playing with URBs
> and I believe that at that point it'd be getting too much USB specific
> to /rule them all/.

-- 
Rafał

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Greg KH]

On Fri, Aug 26, 2016 at 05:38:05PM +0200, Rafał Miłecki wrote:
> On 25 August 2016 at 14:49, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Thu, Aug 25, 2016 at 10:03:52AM +0200, Rafał Miłecki wrote:
> >> +static void usbport_trig_activate(struct led_classdev *led_cdev)
> >> +{
> >> +     struct usbport_trig_data *usbport_data;
> >> +     int err;
> >> +
> >> +     usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
> >> +     if (!usbport_data)
> >> +             return;
> >> +     usbport_data->led_cdev = led_cdev;
> >> +
> >> +     /* Storing ports */
> >> +     INIT_LIST_HEAD(&usbport_data->ports);
> >> +     usbport_data->ports_dir = kobject_create_and_add("ports",
> >> +                                                      &led_cdev->dev->kobj);
> >
> > If you _ever_ find yourself in a driver having to use kobject calls,
> > it's a HUGE hint that you are doing something wrong.
> >
> > Hint, you are doing this wrong :)
> >
> > Use an attribute group if you need a subdirectory in sysfs, using a
> > "raw" kobject like this hides it from all userspace tools and so no
> > userspace program can see it (hint, try using libudev to access these
> > files attached to the device...)
> 
> Oops. Thanks for pointing groups to me. I was looking at sysfs.h
> earlier but I didn't realize group can be a subdirectory. I can see
> these sysfs_create_group(s) and friends now, thanks.

No, use an attribute group, and name it, and the subdir will be created
automagically for you.

> >> +     if (!usbport_data->ports_dir)
> >> +             goto err_free;
> >> +
> >> +     /* API for ports management */
> >> +     err = device_create_file(led_cdev->dev, &dev_attr_new_port);
> >> +     if (err)
> >> +             goto err_put_ports;
> >> +     err = device_create_file(led_cdev->dev, &dev_attr_remove_port);
> >> +     if (err)
> >> +             goto err_remove_new_port;
> >
> > Doesn't this race with userspace and fail?  Shouldn't the led core be
> > creating your leds for you?
> 
> These questions aren't clear to me. What kind of race? Doing
> echo usbport > trigger
> results in trigger core calling usbport_trig_activate. We create new
> attributes and then we return.

Why aren't the attributes present when the device is found?  What is
"trigger" for?

> I'm also not creating any leds there. This already has to be LED
> available if you want to assign some trigger to it.

That sounds really odd...

> >> +
> >> +     /* Notifications */
> >> +     usbport_data->nb.notifier_call = usbport_trig_notify,
> >> +     led_cdev->trigger_data = usbport_data;
> >> +     usb_register_notify(&usbport_data->nb);
> >
> > Don't abuse the USB notifier chain with stuff like this please, is that
> > really necessary?  Why can't your hardware just tell you what USB ports
> > are in use "out of band"?
> 
> I totally don't understand this part. This LED trigger is supposed to
> react to USB devices appearing at specified ports. How else can I know
> there is a new USB device if not by notifications?
> I'm wondering if we are on the same page. Could it be my idea of this
> LED trigger is not clear at all? Could you take a look at commit
> message again, please? Mostly this part:
> > This commit adds a new trigger responsible for turning on LED when USB
> > device gets connected to the specified USB port. This can can useful for
> > various home routers that have USB port(s) and a proper LED telling user
> > a device is connected.
> 
> Can I add something more to make it clear what this trigger is responsible for?

Yes please, as I totally missed that.

And the USB notifier was created for some "special" parts of the USB
core to use, this feels like an abuse of that in some way.  But it's
hard to define, I know...

> >> +
> >> +     led_cdev->activated = true;
> >> +     return;
> >> +
> >> +err_remove_new_port:
> >> +     device_remove_file(led_cdev->dev, &dev_attr_new_port);
> >> +err_put_ports:
> >> +     kobject_put(usbport_data->ports_dir);
> >> +err_free:
> >> +     kfree(usbport_data);
> >> +}
> >
> > And again, why is this USB specific?  Why can't you use this same
> > userspace api and codebase for PCI ports?  For a sdcard port?  For a
> > thunderbolt port?
> 
> I'm leaving this one unanswered as discussion on this continued in
> V3.5 thread below my reply:
> On 25 August 2016 at 07:14, Rafał Miłecki <zajec5@gmail.com> wrote:
> > Good question. I would like to extend this USB port trigger in the
> > future by reacting to USB activity. This involves playing with URBs
> > and I believe that at that point it'd be getting too much USB specific
> > to /rule them all/.

No, I mean why not have this specific activity LED work for all types of
devices, not just USB.

Not that this specific LED works for other types of USB activity.

thanks,

greg k-h

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 30 August 2016 at 14:05, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Aug 26, 2016 at 05:38:05PM +0200, Rafał Miłecki wrote:
>> On 25 August 2016 at 14:49, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Thu, Aug 25, 2016 at 10:03:52AM +0200, Rafał Miłecki wrote:
>> >> +static void usbport_trig_activate(struct led_classdev *led_cdev)
>> >> +{
>> >> +     struct usbport_trig_data *usbport_data;
>> >> +     int err;
>> >> +
>> >> +     usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
>> >> +     if (!usbport_data)
>> >> +             return;
>> >> +     usbport_data->led_cdev = led_cdev;
>> >> +
>> >> +     /* Storing ports */
>> >> +     INIT_LIST_HEAD(&usbport_data->ports);
>> >> +     usbport_data->ports_dir = kobject_create_and_add("ports",
>> >> +                                                      &led_cdev->dev->kobj);
>> >
>> > If you _ever_ find yourself in a driver having to use kobject calls,
>> > it's a HUGE hint that you are doing something wrong.
>> >
>> > Hint, you are doing this wrong :)
>> >
>> > Use an attribute group if you need a subdirectory in sysfs, using a
>> > "raw" kobject like this hides it from all userspace tools and so no
>> > userspace program can see it (hint, try using libudev to access these
>> > files attached to the device...)
>>
>> Oops. Thanks for pointing groups to me. I was looking at sysfs.h
>> earlier but I didn't realize group can be a subdirectory. I can see
>> these sysfs_create_group(s) and friends now, thanks.
>
> No, use an attribute group, and name it, and the subdir will be created
> automagically for you.

Oh, so I got it wrong again. Thanks for explaining.


>> >> +     if (!usbport_data->ports_dir)
>> >> +             goto err_free;
>> >> +
>> >> +     /* API for ports management */
>> >> +     err = device_create_file(led_cdev->dev, &dev_attr_new_port);
>> >> +     if (err)
>> >> +             goto err_put_ports;
>> >> +     err = device_create_file(led_cdev->dev, &dev_attr_remove_port);
>> >> +     if (err)
>> >> +             goto err_remove_new_port;
>> >
>> > Doesn't this race with userspace and fail?  Shouldn't the led core be
>> > creating your leds for you?
>>
>> These questions aren't clear to me. What kind of race? Doing
>> echo usbport > trigger
>> results in trigger core calling usbport_trig_activate. We create new
>> attributes and then we return.
>
> Why aren't the attributes present when the device is found?  What is
> "trigger" for?
>
>> I'm also not creating any leds there. This already has to be LED
>> available if you want to assign some trigger to it.
>
> That sounds really odd...

Please take a look at Documentation to get some idea of LED triggers:
Documentation/leds/leds-class.txt

Basically a LED (/sys/class/leds/foo/) can be controller with
"brightness" sysfs file like this:
echo 0 > brightness
echo 5 > brightness
echo 255 > brightness
(most LEDs react only 0 and non-0 values).

As you quite often need more complex LED management, there are
triggers that were introduced in 2006 by c3bc9956ec52f ("[PATCH] LED:
add LED trigger tupport"). Some triggers are trivial and could be
implemented in userspace as well (e.g. "timer"). Some had to be
implemented in kernelspace (CPU activity, MTD activity, etc.). Having
few triggers compiled, you can assign them to LEDs at it pleases you.
Your hardware may have generic LED (not labeled) and you can
dynamically assign various triggers to it, depending e.g. on user
actions. E.g. if user (using GUI or whatever) wants to see flash
activity, your userspace script should do:
echo mtd > /sys/class/leds/foo/trigger

I hope it explains things a big and you can see know why trigger code
is independent of creating LEDs. It's about layers and making things
generic I believe.

There is a place for LED driver that is responsible for creating
"struct led_classdev" with proper callback setting brightness. It
provides LED name, but is unaware of the way it should be
lighted/turned off.
There is LED subsystem.
There are triggers that are unaware of LED hardware details and only
set LED brightness using generic API.

I'm obviously not author of all of that and I can't explain some
design decisions, but I hope I gave you a clue how it works.


>> >> +
>> >> +     /* Notifications */
>> >> +     usbport_data->nb.notifier_call = usbport_trig_notify,
>> >> +     led_cdev->trigger_data = usbport_data;
>> >> +     usb_register_notify(&usbport_data->nb);
>> >
>> > Don't abuse the USB notifier chain with stuff like this please, is that
>> > really necessary?  Why can't your hardware just tell you what USB ports
>> > are in use "out of band"?
>>
>> I totally don't understand this part. This LED trigger is supposed to
>> react to USB devices appearing at specified ports. How else can I know
>> there is a new USB device if not by notifications?
>> I'm wondering if we are on the same page. Could it be my idea of this
>> LED trigger is not clear at all? Could you take a look at commit
>> message again, please? Mostly this part:
>> > This commit adds a new trigger responsible for turning on LED when USB
>> > device gets connected to the specified USB port. This can can useful for
>> > various home routers that have USB port(s) and a proper LED telling user
>> > a device is connected.
>>
>> Can I add something more to make it clear what this trigger is responsible for?
>
> Yes please, as I totally missed that.
>
> And the USB notifier was created for some "special" parts of the USB
> core to use, this feels like an abuse of that in some way.  But it's
> hard to define, I know...

I didn't mean to abuse this USB notifier, can you think of any other
way to achieve the same result? We could think about modifying USB
core to call some symbol from the trigger driver (see usage of
ledtrig_mtd_activity symbol) but is it any better than using
notification block?


>> >> +
>> >> +     led_cdev->activated = true;
>> >> +     return;
>> >> +
>> >> +err_remove_new_port:
>> >> +     device_remove_file(led_cdev->dev, &dev_attr_new_port);
>> >> +err_put_ports:
>> >> +     kobject_put(usbport_data->ports_dir);
>> >> +err_free:
>> >> +     kfree(usbport_data);
>> >> +}
>> >
>> > And again, why is this USB specific?  Why can't you use this same
>> > userspace api and codebase for PCI ports?  For a sdcard port?  For a
>> > thunderbolt port?
>>
>> I'm leaving this one unanswered as discussion on this continued in
>> V3.5 thread below my reply:
>> On 25 August 2016 at 07:14, Rafał Miłecki <zajec5@gmail.com> wrote:
>> > Good question. I would like to extend this USB port trigger in the
>> > future by reacting to USB activity. This involves playing with URBs
>> > and I believe that at that point it'd be getting too much USB specific
>> > to /rule them all/.
>
> No, I mean why not have this specific activity LED work for all types of
> devices, not just USB.
>
> Not that this specific LED works for other types of USB activity.

Sorry, I'm really trying my best, but I may be missing some point. I'm
not sure if I'll answer your question as you expected.

This patch adds only 1 trigger some users may be interested in, user
is still free to assign any trigger he wants to any LED it has on his
hardware. One day I plan to work on more triggers, so user can pick
the one he wants (e.g. when using generic not-labeled LED):
echo none > trigger
echo usbport > trigger
echo foo > trigger

-- 
Rafał

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Alan Stern]

On Tue, 30 Aug 2016, Rafał Miłecki wrote:

> Please take a look at Documentation to get some idea of LED triggers:
> Documentation/leds/leds-class.txt
> 
> Basically a LED (/sys/class/leds/foo/) can be controller with
> "brightness" sysfs file like this:
> echo 0 > brightness
> echo 5 > brightness
> echo 255 > brightness
> (most LEDs react only 0 and non-0 values).
> 
> As you quite often need more complex LED management, there are
> triggers that were introduced in 2006 by c3bc9956ec52f ("[PATCH] LED:
> add LED trigger tupport"). Some triggers are trivial and could be
> implemented in userspace as well (e.g. "timer"). Some had to be
> implemented in kernelspace (CPU activity, MTD activity, etc.). Having
> few triggers compiled, you can assign them to LEDs at it pleases you.
> Your hardware may have generic LED (not labeled) and you can
> dynamically assign various triggers to it, depending e.g. on user
> actions. E.g. if user (using GUI or whatever) wants to see flash
> activity, your userspace script should do:
> echo mtd > /sys/class/leds/foo/trigger

So for example, you might want to do:

	echo usb1-4 >/sys/class/leds/foo/trigger

and then have the "foo" LED toggle whenever an URB was submitted or 
completed for a device attached to the 1-4 port.  Right?

> I hope it explains things a big and you can see know why trigger code
> is independent of creating LEDs. It's about layers and making things
> generic I believe.
> 
> There is a place for LED driver that is responsible for creating
> "struct led_classdev" with proper callback setting brightness. It
> provides LED name, but is unaware of the way it should be
> lighted/turned off.
> There is LED subsystem.
> There are triggers that are unaware of LED hardware details and only
> set LED brightness using generic API.
> 
> I'm obviously not author of all of that and I can't explain some
> design decisions, but I hope I gave you a clue how it works.

> >> >> +     /* Notifications */
> >> >> +     usbport_data->nb.notifier_call = usbport_trig_notify,
> >> >> +     led_cdev->trigger_data = usbport_data;
> >> >> +     usb_register_notify(&usbport_data->nb);
> >> >
> >> > Don't abuse the USB notifier chain with stuff like this please, is that
> >> > really necessary?  Why can't your hardware just tell you what USB ports
> >> > are in use "out of band"?
> >>
> >> I totally don't understand this part. This LED trigger is supposed to
> >> react to USB devices appearing at specified ports. How else can I know
> >> there is a new USB device if not by notifications?
> >> I'm wondering if we are on the same page. Could it be my idea of this
> >> LED trigger is not clear at all? Could you take a look at commit
> >> message again, please? Mostly this part:
> >> > This commit adds a new trigger responsible for turning on LED when USB
> >> > device gets connected to the specified USB port. This can can useful for
> >> > various home routers that have USB port(s) and a proper LED telling user
> >> > a device is connected.
> >>
> >> Can I add something more to make it clear what this trigger is responsible for?
> >
> > Yes please, as I totally missed that.
> >
> > And the USB notifier was created for some "special" parts of the USB
> > core to use, this feels like an abuse of that in some way.  But it's
> > hard to define, I know...
> 
> I didn't mean to abuse this USB notifier, can you think of any other
> way to achieve the same result? We could think about modifying USB
> core to call some symbol from the trigger driver (see usage of
> ledtrig_mtd_activity symbol) but is it any better than using
> notification block?

I don't think this is such a bad use of the USB notifier.  All you need 
to know is when a device is attached to or unplugged from an LED's 
port.  Neither of these is a very frequent event.

However, there is still the question of how to know when an URB is 
submitted or completed for the device in question.  Obviously the USB 
core would have to call an LED routine.  But how would you determine 
which LED(s) to toggle?  Go through the entire list, looking for ones 
that are bound to the USB device in question?  This seems inefficient.  
Use a hash table?

Alan Stern

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 30 August 2016 at 22:54, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 30 Aug 2016, Rafał Miłecki wrote:
>
>> Please take a look at Documentation to get some idea of LED triggers:
>> Documentation/leds/leds-class.txt
>>
>> Basically a LED (/sys/class/leds/foo/) can be controller with
>> "brightness" sysfs file like this:
>> echo 0 > brightness
>> echo 5 > brightness
>> echo 255 > brightness
>> (most LEDs react only 0 and non-0 values).
>>
>> As you quite often need more complex LED management, there are
>> triggers that were introduced in 2006 by c3bc9956ec52f ("[PATCH] LED:
>> add LED trigger tupport"). Some triggers are trivial and could be
>> implemented in userspace as well (e.g. "timer"). Some had to be
>> implemented in kernelspace (CPU activity, MTD activity, etc.). Having
>> few triggers compiled, you can assign them to LEDs at it pleases you.
>> Your hardware may have generic LED (not labeled) and you can
>> dynamically assign various triggers to it, depending e.g. on user
>> actions. E.g. if user (using GUI or whatever) wants to see flash
>> activity, your userspace script should do:
>> echo mtd > /sys/class/leds/foo/trigger
>
> So for example, you might want to do:
>
>         echo usb1-4 >/sys/class/leds/foo/trigger
>
> and then have the "foo" LED toggle whenever an URB was submitted or
> completed for a device attached to the 1-4 port.  Right?

Not really as it won't cover some pretty common use cases. Many home
routers have few USB ports (2-5) and only 1 USB LED. It has to be
possible to assign few USB ports to a single LED (trigger). That way
LED should be turned on (and kept on) if there is at least 1 USB
device connected. You obviously can't do:
echo "usb1-1 usb1-2 usb2-1" > /sys/class/leds/foo/trigger

This was already brought up by Rob (who mentioned CPU trigger) and I
replied him pretty much the same way in:
https://lkml.org/lkml/2016/7/29/38
(reply starts with "Anyway, the serious limitation I see").


>> I hope it explains things a big and you can see know why trigger code
>> is independent of creating LEDs. It's about layers and making things
>> generic I believe.
>>
>> There is a place for LED driver that is responsible for creating
>> "struct led_classdev" with proper callback setting brightness. It
>> provides LED name, but is unaware of the way it should be
>> lighted/turned off.
>> There is LED subsystem.
>> There are triggers that are unaware of LED hardware details and only
>> set LED brightness using generic API.
>>
>> I'm obviously not author of all of that and I can't explain some
>> design decisions, but I hope I gave you a clue how it works.
>
>> >> >> +     /* Notifications */
>> >> >> +     usbport_data->nb.notifier_call = usbport_trig_notify,
>> >> >> +     led_cdev->trigger_data = usbport_data;
>> >> >> +     usb_register_notify(&usbport_data->nb);
>> >> >
>> >> > Don't abuse the USB notifier chain with stuff like this please, is that
>> >> > really necessary?  Why can't your hardware just tell you what USB ports
>> >> > are in use "out of band"?
>> >>
>> >> I totally don't understand this part. This LED trigger is supposed to
>> >> react to USB devices appearing at specified ports. How else can I know
>> >> there is a new USB device if not by notifications?
>> >> I'm wondering if we are on the same page. Could it be my idea of this
>> >> LED trigger is not clear at all? Could you take a look at commit
>> >> message again, please? Mostly this part:
>> >> > This commit adds a new trigger responsible for turning on LED when USB
>> >> > device gets connected to the specified USB port. This can can useful for
>> >> > various home routers that have USB port(s) and a proper LED telling user
>> >> > a device is connected.
>> >>
>> >> Can I add something more to make it clear what this trigger is responsible for?
>> >
>> > Yes please, as I totally missed that.
>> >
>> > And the USB notifier was created for some "special" parts of the USB
>> > core to use, this feels like an abuse of that in some way.  But it's
>> > hard to define, I know...
>>
>> I didn't mean to abuse this USB notifier, can you think of any other
>> way to achieve the same result? We could think about modifying USB
>> core to call some symbol from the trigger driver (see usage of
>> ledtrig_mtd_activity symbol) but is it any better than using
>> notification block?
>
> I don't think this is such a bad use of the USB notifier.  All you need
> to know is when a device is attached to or unplugged from an LED's
> port.  Neither of these is a very frequent event.
>
> However, there is still the question of how to know when an URB is
> submitted or completed for the device in question.  Obviously the USB
> core would have to call an LED routine.  But how would you determine
> which LED(s) to toggle?  Go through the entire list, looking for ones
> that are bound to the USB device in question?  This seems inefficient.
> Use a hash table?

I was hoping to bring it to discussion later, as it seems even the
basic version of this trigger causes many design problems.

-- 
Rafał

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Alan Stern]

On Tue, 30 Aug 2016, Rafał Miłecki wrote:

> >> As you quite often need more complex LED management, there are
> >> triggers that were introduced in 2006 by c3bc9956ec52f ("[PATCH] LED:
> >> add LED trigger tupport"). Some triggers are trivial and could be
> >> implemented in userspace as well (e.g. "timer"). Some had to be
> >> implemented in kernelspace (CPU activity, MTD activity, etc.). Having
> >> few triggers compiled, you can assign them to LEDs at it pleases you.
> >> Your hardware may have generic LED (not labeled) and you can
> >> dynamically assign various triggers to it, depending e.g. on user
> >> actions. E.g. if user (using GUI or whatever) wants to see flash
> >> activity, your userspace script should do:
> >> echo mtd > /sys/class/leds/foo/trigger
> >
> > So for example, you might want to do:
> >
> >         echo usb1-4 >/sys/class/leds/foo/trigger
> >
> > and then have the "foo" LED toggle whenever an URB was submitted or
> > completed for a device attached to the 1-4 port.  Right?
> 
> Not really as it won't cover some pretty common use cases. Many home
> routers have few USB ports (2-5) and only 1 USB LED. It has to be
> possible to assign few USB ports to a single LED (trigger). That way
> LED should be turned on (and kept on) if there is at least 1 USB
> device connected. You obviously can't do:
> echo "usb1-1 usb1-2 usb2-1" > /sys/class/leds/foo/trigger
> 
> This was already brought up by Rob (who mentioned CPU trigger) and I
> replied him pretty much the same way in:
> https://lkml.org/lkml/2016/7/29/38
> (reply starts with "Anyway, the serious limitation I see").

The code for a bunch of triggers must already be written.  What would 
the user do if he wanted to flash a single LED in response to both
CPU activity and MTD activity?  If not

	echo "cpu mtd" >/sys/class/leds/foo/trigger

then what?

Alan Stern

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 31 August 2016 at 20:23, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 30 Aug 2016, Rafał Miłecki wrote:
>
>> >> As you quite often need more complex LED management, there are
>> >> triggers that were introduced in 2006 by c3bc9956ec52f ("[PATCH] LED:
>> >> add LED trigger tupport"). Some triggers are trivial and could be
>> >> implemented in userspace as well (e.g. "timer"). Some had to be
>> >> implemented in kernelspace (CPU activity, MTD activity, etc.). Having
>> >> few triggers compiled, you can assign them to LEDs at it pleases you.
>> >> Your hardware may have generic LED (not labeled) and you can
>> >> dynamically assign various triggers to it, depending e.g. on user
>> >> actions. E.g. if user (using GUI or whatever) wants to see flash
>> >> activity, your userspace script should do:
>> >> echo mtd > /sys/class/leds/foo/trigger
>> >
>> > So for example, you might want to do:
>> >
>> >         echo usb1-4 >/sys/class/leds/foo/trigger
>> >
>> > and then have the "foo" LED toggle whenever an URB was submitted or
>> > completed for a device attached to the 1-4 port.  Right?
>>
>> Not really as it won't cover some pretty common use cases. Many home
>> routers have few USB ports (2-5) and only 1 USB LED. It has to be
>> possible to assign few USB ports to a single LED (trigger). That way
>> LED should be turned on (and kept on) if there is at least 1 USB
>> device connected. You obviously can't do:
>> echo "usb1-1 usb1-2 usb2-1" > /sys/class/leds/foo/trigger
>>
>> This was already brought up by Rob (who mentioned CPU trigger) and I
>> replied him pretty much the same way in:
>> https://lkml.org/lkml/2016/7/29/38
>> (reply starts with "Anyway, the serious limitation I see").
>
> The code for a bunch of triggers must already be written.  What would
> the user do if he wanted to flash a single LED in response to both
> CPU activity and MTD activity?  If not
>
>         echo "cpu mtd" >/sys/class/leds/foo/trigger
>
> then what?

Well, it sounds like a new feature then. Shall we add an extra API
with a request function for turning LED on? It could internally count
how many requests were raised and keep LED on as long as there is at
least 1 left. I guess we should implement it in trigger "subsystem"
(if I can call it so). Does it sound like a good plan?

-- 
Rafał

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 31 August 2016 at 21:00, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 31 August 2016 at 20:23, Alan Stern <stern@rowland.harvard.edu> wrote:
>> On Tue, 30 Aug 2016, Rafał Miłecki wrote:
>>> Not really as it won't cover some pretty common use cases. Many home
>>> routers have few USB ports (2-5) and only 1 USB LED. It has to be
>>> possible to assign few USB ports to a single LED (trigger). That way
>>> LED should be turned on (and kept on) if there is at least 1 USB
>>> device connected. You obviously can't do:
>>> echo "usb1-1 usb1-2 usb2-1" > /sys/class/leds/foo/trigger
>>>
>>> This was already brought up by Rob (who mentioned CPU trigger) and I
>>> replied him pretty much the same way in:
>>> https://lkml.org/lkml/2016/7/29/38
>>> (reply starts with "Anyway, the serious limitation I see").
>>
>> The code for a bunch of triggers must already be written.  What would
>> the user do if he wanted to flash a single LED in response to both
>> CPU activity and MTD activity?  If not
>>
>>         echo "cpu mtd" >/sys/class/leds/foo/trigger
>>
>> then what?
>
> Well, it sounds like a new feature then. Shall we add an extra API
> with a request function for turning LED on? It could internally count
> how many requests were raised and keep LED on as long as there is at
> least 1 left. I guess we should implement it in trigger "subsystem"
> (if I can call it so). Does it sound like a good plan?

I'm pretty sure noone ever planned to have more than 1 trigger
assigned to a single LED. I just realized there will be a problem with
proposed solution: sysfs files conflict.

Consider 2 existing triggers for a moment:
1) oneshot: it creates following sysfs files:
/sys/class/leds/foo/delay_on
/sys/class/leds/foo/delay_off
/sys/class/leds/foo/invert
/sys/class/leds/foo/shot
2) timer: it creates following sysfs files:
/sys/class/leds/foo/delay_on
/sys/class/leds/foo/delay_off

Activating both of them will probably cause a WARNING in sysfs. They
can't coexist :(

We should probably have per-trigger subdirs, e.g.:
/sys/class/leds/foo/trigger-oneshot/delay_on
/sys/class/leds/foo/trigger-oneshot/delay_off
/sys/class/leds/foo/trigger-oneshot/invert
/sys/class/leds/foo/trigger-oneshot/shot
/sys/class/leds/foo/trigger-timer/delay_on
/sys/class/leds/foo/trigger-timer/delay_off
but implementing it now would break the ABI.

One workaround I can see is doing triggers V2, they:
1) Would put sysfs files in /sys/class/leds/foo/trigger-bar/
2) Use new API for *requesting* LED to be on/off
3) There would be a counter of requests in V2 API
4) Multiple triggers V2 would be allowed to be used (assigned) at the same time

-- 
Rafał

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Jacek Anaszewski]

On 09/01/2016 07:25 AM, Rafał Miłecki wrote:
> On 31 August 2016 at 21:00, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On 31 August 2016 at 20:23, Alan Stern <stern@rowland.harvard.edu> wrote:
>>> On Tue, 30 Aug 2016, Rafał Miłecki wrote:
>>>> Not really as it won't cover some pretty common use cases. Many home
>>>> routers have few USB ports (2-5) and only 1 USB LED. It has to be
>>>> possible to assign few USB ports to a single LED (trigger). That way
>>>> LED should be turned on (and kept on) if there is at least 1 USB
>>>> device connected. You obviously can't do:
>>>> echo "usb1-1 usb1-2 usb2-1" > /sys/class/leds/foo/trigger
>>>>
>>>> This was already brought up by Rob (who mentioned CPU trigger) and I
>>>> replied him pretty much the same way in:
>>>> https://lkml.org/lkml/2016/7/29/38
>>>> (reply starts with "Anyway, the serious limitation I see").
>>>
>>> The code for a bunch of triggers must already be written.  What would
>>> the user do if he wanted to flash a single LED in response to both
>>> CPU activity and MTD activity?  If not
>>>
>>>         echo "cpu mtd" >/sys/class/leds/foo/trigger
>>>
>>> then what?
>>
>> Well, it sounds like a new feature then. Shall we add an extra API
>> with a request function for turning LED on? It could internally count
>> how many requests were raised and keep LED on as long as there is at
>> least 1 left. I guess we should implement it in trigger "subsystem"
>> (if I can call it so). Does it sound like a good plan?
>
> I'm pretty sure noone ever planned to have more than 1 trigger
> assigned to a single LED. I just realized there will be a problem with
> proposed solution: sysfs files conflict.
>
> Consider 2 existing triggers for a moment:
> 1) oneshot: it creates following sysfs files:
> /sys/class/leds/foo/delay_on
> /sys/class/leds/foo/delay_off
> /sys/class/leds/foo/invert
> /sys/class/leds/foo/shot
> 2) timer: it creates following sysfs files:
> /sys/class/leds/foo/delay_on
> /sys/class/leds/foo/delay_off
>
> Activating both of them will probably cause a WARNING in sysfs. They
> can't coexist :(
>
> We should probably have per-trigger subdirs, e.g.:
> /sys/class/leds/foo/trigger-oneshot/delay_on
> /sys/class/leds/foo/trigger-oneshot/delay_off
> /sys/class/leds/foo/trigger-oneshot/invert
> /sys/class/leds/foo/trigger-oneshot/shot
> /sys/class/leds/foo/trigger-timer/delay_on
> /sys/class/leds/foo/trigger-timer/delay_off
> but implementing it now would break the ABI.
>
> One workaround I can see is doing triggers V2, they:
> 1) Would put sysfs files in /sys/class/leds/foo/trigger-bar/
> 2) Use new API for *requesting* LED to be on/off
> 3) There would be a counter of requests in V2 API
> 4) Multiple triggers V2 would be allowed to be used (assigned) at the same time

Currently we support only triggers dedicated to specific type of
devices. Even in case of triggers that don't expose custom sysfs
attributes, registered with led_trigger_register_simple(), device
drivers have to generate trigger event with dedicated function, e.g:

- ledtrig-cpu: void ledtrig_cpu(enum cpu_led_event ledevt)
- ledtrig-disk: void ledtrig_disk_activity(void)
- ledtrig-mtd: void ledtrig_mtd_activity(void)

If one wanted to have the LED notified by different type of devices,
then they would have to implement a trigger that would exposed all
required types of API.

Unfortunately, there are many possible combinations of
triggers and that doesn't sound sane to add a new one when someone
will find it useful. Of course it would entail adding a call to the
new trigger API in the drivers, which doesn't seem like something
acceptable in the mainline.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Alan Stern]

On Thu, 1 Sep 2016, Jacek Anaszewski wrote:

> On 09/01/2016 07:25 AM, Rafał Miłecki wrote:
> > On 31 August 2016 at 21:00, Rafał Miłecki <zajec5@gmail.com> wrote:
> >> On 31 August 2016 at 20:23, Alan Stern <stern@rowland.harvard.edu> wrote:
> >>> On Tue, 30 Aug 2016, Rafał Miłecki wrote:
> >>>> Not really as it won't cover some pretty common use cases. Many home
> >>>> routers have few USB ports (2-5) and only 1 USB LED. It has to be
> >>>> possible to assign few USB ports to a single LED (trigger). That way
> >>>> LED should be turned on (and kept on) if there is at least 1 USB
> >>>> device connected. You obviously can't do:
> >>>> echo "usb1-1 usb1-2 usb2-1" > /sys/class/leds/foo/trigger
> >>>>
> >>>> This was already brought up by Rob (who mentioned CPU trigger) and I
> >>>> replied him pretty much the same way in:
> >>>> https://lkml.org/lkml/2016/7/29/38
> >>>> (reply starts with "Anyway, the serious limitation I see").
> >>>
> >>> The code for a bunch of triggers must already be written.  What would
> >>> the user do if he wanted to flash a single LED in response to both
> >>> CPU activity and MTD activity?  If not
> >>>
> >>>         echo "cpu mtd" >/sys/class/leds/foo/trigger
> >>>
> >>> then what?
> >>
> >> Well, it sounds like a new feature then. Shall we add an extra API
> >> with a request function for turning LED on? It could internally count
> >> how many requests were raised and keep LED on as long as there is at
> >> least 1 left. I guess we should implement it in trigger "subsystem"
> >> (if I can call it so). Does it sound like a good plan?
> >
> > I'm pretty sure noone ever planned to have more than 1 trigger
> > assigned to a single LED. I just realized there will be a problem with
> > proposed solution: sysfs files conflict.
> >
> > Consider 2 existing triggers for a moment:
> > 1) oneshot: it creates following sysfs files:
> > /sys/class/leds/foo/delay_on
> > /sys/class/leds/foo/delay_off
> > /sys/class/leds/foo/invert
> > /sys/class/leds/foo/shot
> > 2) timer: it creates following sysfs files:
> > /sys/class/leds/foo/delay_on
> > /sys/class/leds/foo/delay_off
> >
> > Activating both of them will probably cause a WARNING in sysfs. They
> > can't coexist :(
> >
> > We should probably have per-trigger subdirs, e.g.:
> > /sys/class/leds/foo/trigger-oneshot/delay_on
> > /sys/class/leds/foo/trigger-oneshot/delay_off
> > /sys/class/leds/foo/trigger-oneshot/invert
> > /sys/class/leds/foo/trigger-oneshot/shot
> > /sys/class/leds/foo/trigger-timer/delay_on
> > /sys/class/leds/foo/trigger-timer/delay_off
> > but implementing it now would break the ABI.
> >
> > One workaround I can see is doing triggers V2, they:
> > 1) Would put sysfs files in /sys/class/leds/foo/trigger-bar/
> > 2) Use new API for *requesting* LED to be on/off
> > 3) There would be a counter of requests in V2 API
> > 4) Multiple triggers V2 would be allowed to be used (assigned) at the same time
> 
> Currently we support only triggers dedicated to specific type of
> devices. Even in case of triggers that don't expose custom sysfs
> attributes, registered with led_trigger_register_simple(), device
> drivers have to generate trigger event with dedicated function, e.g:
> 
> - ledtrig-cpu: void ledtrig_cpu(enum cpu_led_event ledevt)
> - ledtrig-disk: void ledtrig_disk_activity(void)
> - ledtrig-mtd: void ledtrig_mtd_activity(void)
> 
> If one wanted to have the LED notified by different type of devices,
> then they would have to implement a trigger that would exposed all
> required types of API.
> 
> Unfortunately, there are many possible combinations of
> triggers and that doesn't sound sane to add a new one when someone
> will find it useful. Of course it would entail adding a call to the
> new trigger API in the drivers, which doesn't seem like something
> acceptable in the mainline.

Maybe it would make more sense, in this case, to allow only three 
possibilities for a USB port activity trigger.  Toggle the LED 
whenever:

	There is activity on the specified port, or

	There is any activity on any port on the specified hub, or

	There is any USB activity on any port.

That ought to cover most of the normal use cases, and it would be
simple enough to implement.

Alan Stern

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Jacek Anaszewski]

On 09/01/2016 04:36 PM, Alan Stern wrote:
> On Thu, 1 Sep 2016, Jacek Anaszewski wrote:
>
>> On 09/01/2016 07:25 AM, Rafał Miłecki wrote:
>>> On 31 August 2016 at 21:00, Rafał Miłecki <zajec5@gmail.com> wrote:
>>>> On 31 August 2016 at 20:23, Alan Stern <stern@rowland.harvard.edu> wrote:
>>>>> On Tue, 30 Aug 2016, Rafał Miłecki wrote:
>>>>>> Not really as it won't cover some pretty common use cases. Many home
>>>>>> routers have few USB ports (2-5) and only 1 USB LED. It has to be
>>>>>> possible to assign few USB ports to a single LED (trigger). That way
>>>>>> LED should be turned on (and kept on) if there is at least 1 USB
>>>>>> device connected. You obviously can't do:
>>>>>> echo "usb1-1 usb1-2 usb2-1" > /sys/class/leds/foo/trigger
>>>>>>
>>>>>> This was already brought up by Rob (who mentioned CPU trigger) and I
>>>>>> replied him pretty much the same way in:
>>>>>> https://lkml.org/lkml/2016/7/29/38
>>>>>> (reply starts with "Anyway, the serious limitation I see").
>>>>>
>>>>> The code for a bunch of triggers must already be written.  What would
>>>>> the user do if he wanted to flash a single LED in response to both
>>>>> CPU activity and MTD activity?  If not
>>>>>
>>>>>         echo "cpu mtd" >/sys/class/leds/foo/trigger
>>>>>
>>>>> then what?
>>>>
>>>> Well, it sounds like a new feature then. Shall we add an extra API
>>>> with a request function for turning LED on? It could internally count
>>>> how many requests were raised and keep LED on as long as there is at
>>>> least 1 left. I guess we should implement it in trigger "subsystem"
>>>> (if I can call it so). Does it sound like a good plan?
>>>
>>> I'm pretty sure noone ever planned to have more than 1 trigger
>>> assigned to a single LED. I just realized there will be a problem with
>>> proposed solution: sysfs files conflict.
>>>
>>> Consider 2 existing triggers for a moment:
>>> 1) oneshot: it creates following sysfs files:
>>> /sys/class/leds/foo/delay_on
>>> /sys/class/leds/foo/delay_off
>>> /sys/class/leds/foo/invert
>>> /sys/class/leds/foo/shot
>>> 2) timer: it creates following sysfs files:
>>> /sys/class/leds/foo/delay_on
>>> /sys/class/leds/foo/delay_off
>>>
>>> Activating both of them will probably cause a WARNING in sysfs. They
>>> can't coexist :(
>>>
>>> We should probably have per-trigger subdirs, e.g.:
>>> /sys/class/leds/foo/trigger-oneshot/delay_on
>>> /sys/class/leds/foo/trigger-oneshot/delay_off
>>> /sys/class/leds/foo/trigger-oneshot/invert
>>> /sys/class/leds/foo/trigger-oneshot/shot
>>> /sys/class/leds/foo/trigger-timer/delay_on
>>> /sys/class/leds/foo/trigger-timer/delay_off
>>> but implementing it now would break the ABI.
>>>
>>> One workaround I can see is doing triggers V2, they:
>>> 1) Would put sysfs files in /sys/class/leds/foo/trigger-bar/
>>> 2) Use new API for *requesting* LED to be on/off
>>> 3) There would be a counter of requests in V2 API
>>> 4) Multiple triggers V2 would be allowed to be used (assigned) at the same time
>>
>> Currently we support only triggers dedicated to specific type of
>> devices. Even in case of triggers that don't expose custom sysfs
>> attributes, registered with led_trigger_register_simple(), device
>> drivers have to generate trigger event with dedicated function, e.g:
>>
>> - ledtrig-cpu: void ledtrig_cpu(enum cpu_led_event ledevt)
>> - ledtrig-disk: void ledtrig_disk_activity(void)
>> - ledtrig-mtd: void ledtrig_mtd_activity(void)
>>
>> If one wanted to have the LED notified by different type of devices,
>> then they would have to implement a trigger that would exposed all
>> required types of API.
>>
>> Unfortunately, there are many possible combinations of
>> triggers and that doesn't sound sane to add a new one when someone
>> will find it useful. Of course it would entail adding a call to the
>> new trigger API in the drivers, which doesn't seem like something
>> acceptable in the mainline.
>
> Maybe it would make more sense, in this case, to allow only three
> possibilities for a USB port activity trigger.  Toggle the LED
> whenever:
>
> 	There is activity on the specified port, or
>
> 	There is any activity on any port on the specified hub, or
>
> 	There is any USB activity on any port.
>
> That ought to cover most of the normal use cases, and it would be
> simple enough to implement.

What would be the benefit of having a USB port activity trigger,
for which we would be specifying the port to observe, but in the same
time we would react on any activity on any port (cases 1 and 3)?

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Alan Stern]

On Fri, 2 Sep 2016, Jacek Anaszewski wrote:

> >>> I'm pretty sure noone ever planned to have more than 1 trigger
> >>> assigned to a single LED. I just realized there will be a problem with
> >>> proposed solution: sysfs files conflict.

...

> >> Currently we support only triggers dedicated to specific type of
> >> devices. Even in case of triggers that don't expose custom sysfs
> >> attributes, registered with led_trigger_register_simple(), device
> >> drivers have to generate trigger event with dedicated function, e.g:
> >>
> >> - ledtrig-cpu: void ledtrig_cpu(enum cpu_led_event ledevt)
> >> - ledtrig-disk: void ledtrig_disk_activity(void)
> >> - ledtrig-mtd: void ledtrig_mtd_activity(void)
> >>
> >> If one wanted to have the LED notified by different type of devices,
> >> then they would have to implement a trigger that would exposed all
> >> required types of API.
> >>
> >> Unfortunately, there are many possible combinations of
> >> triggers and that doesn't sound sane to add a new one when someone
> >> will find it useful. Of course it would entail adding a call to the
> >> new trigger API in the drivers, which doesn't seem like something
> >> acceptable in the mainline.
> >
> > Maybe it would make more sense, in this case, to allow only three
> > possibilities for a USB port activity trigger.  Toggle the LED
> > whenever:
> >
> > 	There is activity on the specified port, or
> >
> > 	There is any activity on any port on the specified hub, or
> >
> > 	There is any USB activity on any port.
> >
> > That ought to cover most of the normal use cases, and it would be
> > simple enough to implement.
> 
> What would be the benefit of having a USB port activity trigger,
> for which we would be specifying the port to observe, but in the same
> time we would react on any activity on any port (cases 1 and 3)?

I meant these three cases to be mutually exclusive.  For a given LED,
you could have only one of those trigger types (like mentioned above,
only one trigger per LED).  For example, you might accept any one of:

	echo usb1-4.2 >/sys/class/led/foo/trigger

	echo hub1-4 >/sys/class/led/foo/trigger

	echo usb >/sys/class/led/foo/trigger

Yes, it would be possible to have a port-specific trigger for one LED
and an overall USB activity trigger for another LED.  I don't know how
useful this would be -- you could probably imagine some unlikely
scenario.

The point is that doing things this way wouldn't require any API
violations, and it would allow users to do almost all of the things
they are likely to want.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Alan Stern]

On Fri, 2 Sep 2016, Jacek Anaszewski wrote:

> >>> I'm pretty sure noone ever planned to have more than 1 trigger
> >>> assigned to a single LED. I just realized there will be a problem with
> >>> proposed solution: sysfs files conflict.

...

> >> Currently we support only triggers dedicated to specific type of
> >> devices. Even in case of triggers that don't expose custom sysfs
> >> attributes, registered with led_trigger_register_simple(), device
> >> drivers have to generate trigger event with dedicated function, e.g:
> >>
> >> - ledtrig-cpu: void ledtrig_cpu(enum cpu_led_event ledevt)
> >> - ledtrig-disk: void ledtrig_disk_activity(void)
> >> - ledtrig-mtd: void ledtrig_mtd_activity(void)
> >>
> >> If one wanted to have the LED notified by different type of devices,
> >> then they would have to implement a trigger that would exposed all
> >> required types of API.
> >>
> >> Unfortunately, there are many possible combinations of
> >> triggers and that doesn't sound sane to add a new one when someone
> >> will find it useful. Of course it would entail adding a call to the
> >> new trigger API in the drivers, which doesn't seem like something
> >> acceptable in the mainline.
> >
> > Maybe it would make more sense, in this case, to allow only three
> > possibilities for a USB port activity trigger.  Toggle the LED
> > whenever:
> >
> > 	There is activity on the specified port, or
> >
> > 	There is any activity on any port on the specified hub, or
> >
> > 	There is any USB activity on any port.
> >
> > That ought to cover most of the normal use cases, and it would be
> > simple enough to implement.
> 
> What would be the benefit of having a USB port activity trigger,
> for which we would be specifying the port to observe, but in the same
> time we would react on any activity on any port (cases 1 and 3)?

I meant these three cases to be mutually exclusive.  For a given LED,
you could have only one of those trigger types (like mentioned above,
only one trigger per LED).  For example, you might accept any one of:

	echo usb1-4.2 >/sys/class/led/foo/trigger

	echo hub1-4 >/sys/class/led/foo/trigger

	echo usb >/sys/class/led/foo/trigger

Yes, it would be possible to have a port-specific trigger for one LED
and an overall USB activity trigger for another LED.  I don't know how
useful this would be -- you could probably imagine some unlikely
scenario.

The point is that doing things this way wouldn't require any API
violations, and it would allow users to do almost all of the things
they are likely to want.

Alan Stern

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Jacek Anaszewski]

On 09/02/2016 04:33 PM, Alan Stern wrote:
> On Fri, 2 Sep 2016, Jacek Anaszewski wrote:
>
>>>>> I'm pretty sure noone ever planned to have more than 1 trigger
>>>>> assigned to a single LED. I just realized there will be a problem with
>>>>> proposed solution: sysfs files conflict.
>
> ...
>
>>>> Currently we support only triggers dedicated to specific type of
>>>> devices. Even in case of triggers that don't expose custom sysfs
>>>> attributes, registered with led_trigger_register_simple(), device
>>>> drivers have to generate trigger event with dedicated function, e.g:
>>>>
>>>> - ledtrig-cpu: void ledtrig_cpu(enum cpu_led_event ledevt)
>>>> - ledtrig-disk: void ledtrig_disk_activity(void)
>>>> - ledtrig-mtd: void ledtrig_mtd_activity(void)
>>>>
>>>> If one wanted to have the LED notified by different type of devices,
>>>> then they would have to implement a trigger that would exposed all
>>>> required types of API.
>>>>
>>>> Unfortunately, there are many possible combinations of
>>>> triggers and that doesn't sound sane to add a new one when someone
>>>> will find it useful. Of course it would entail adding a call to the
>>>> new trigger API in the drivers, which doesn't seem like something
>>>> acceptable in the mainline.
>>>
>>> Maybe it would make more sense, in this case, to allow only three
>>> possibilities for a USB port activity trigger.  Toggle the LED
>>> whenever:
>>>
>>> 	There is activity on the specified port, or
>>>
>>> 	There is any activity on any port on the specified hub, or
>>>
>>> 	There is any USB activity on any port.
>>>
>>> That ought to cover most of the normal use cases, and it would be
>>> simple enough to implement.
>>
>> What would be the benefit of having a USB port activity trigger,
>> for which we would be specifying the port to observe, but in the same
>> time we would react on any activity on any port (cases 1 and 3)?
>
> I meant these three cases to be mutually exclusive.  For a given LED,
> you could have only one of those trigger types (like mentioned above,
> only one trigger per LED).  For example, you might accept any one of:
>
> 	echo usb1-4.2 >/sys/class/led/foo/trigger
>
> 	echo hub1-4 >/sys/class/led/foo/trigger
>
> 	echo usb >/sys/class/led/foo/trigger
>
> Yes, it would be possible to have a port-specific trigger for one LED
> and an overall USB activity trigger for another LED.  I don't know how
> useful this would be -- you could probably imagine some unlikely
> scenario.
>
> The point is that doing things this way wouldn't require any API
> violations, and it would allow users to do almost all of the things
> they are likely to want.

We'd have to define single API for generating USB trigger event,
so as not enforce addition of three different API calls to the USB
drivers. The type of USB events that the LED should react upon could be
defined by parsing the value written to the sysfs file.
This of course implies, that we should have single LED USB port trigger.

The remaining issue is the sysfs interface design for defining and
presenting multiple USB ports. I'm still in favour of a single
attribute with space separated list. This scheme is commonly used
in existing interfaces.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Alan Stern]

On Sat, 3 Sep 2016, Jacek Anaszewski wrote:

> >>> Maybe it would make more sense, in this case, to allow only three
> >>> possibilities for a USB port activity trigger.  Toggle the LED
> >>> whenever:
> >>>
> >>> 	There is activity on the specified port, or
> >>>
> >>> 	There is any activity on any port on the specified hub, or
> >>>
> >>> 	There is any USB activity on any port.
> >>>
> >>> That ought to cover most of the normal use cases, and it would be
> >>> simple enough to implement.
> >>
> >> What would be the benefit of having a USB port activity trigger,
> >> for which we would be specifying the port to observe, but in the same
> >> time we would react on any activity on any port (cases 1 and 3)?
> >
> > I meant these three cases to be mutually exclusive.  For a given LED,
> > you could have only one of those trigger types (like mentioned above,
> > only one trigger per LED).  For example, you might accept any one of:
> >
> > 	echo usb1-4.2 >/sys/class/led/foo/trigger
> >
> > 	echo hub1-4 >/sys/class/led/foo/trigger
> >
> > 	echo usb >/sys/class/led/foo/trigger
> >
> > Yes, it would be possible to have a port-specific trigger for one LED
> > and an overall USB activity trigger for another LED.  I don't know how
> > useful this would be -- you could probably imagine some unlikely
> > scenario.
> >
> > The point is that doing things this way wouldn't require any API
> > violations, and it would allow users to do almost all of the things
> > they are likely to want.
> 
> We'd have to define single API for generating USB trigger event,
> so as not enforce addition of three different API calls to the USB
> drivers.

The USB core would need only one LED-API call, which it would make upon
the completion of an URB.  The trigger code should be able to handle
all the rest (i.e., see which LEDs should be triggered by that URB).

>  The type of USB events that the LED should react upon could be
> defined by parsing the value written to the sysfs file.

There is only one type of event: completion of an URB.  Triggers would
differ depending only on the device/port that the URB was aimed at.  
_That_ information could be defined by parsing the value written to the 
sysfs file.

> This of course implies, that we should have single LED USB port trigger.
> 
> The remaining issue is the sysfs interface design for defining and
> presenting multiple USB ports. I'm still in favour of a single
> attribute with space separated list. This scheme is commonly used
> in existing interfaces.

No such interface is needed if you do things the way I outlined above.  
Each trigger would require the user to specify either one port, one 
hub, or nothing at all.  Multiple ports would not be used.

Alan Stern

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Jacek Anaszewski]

On 09/03/2016 05:17 PM, Alan Stern wrote:
> On Sat, 3 Sep 2016, Jacek Anaszewski wrote:
>
>>>>> Maybe it would make more sense, in this case, to allow only three
>>>>> possibilities for a USB port activity trigger.  Toggle the LED
>>>>> whenever:
>>>>>
>>>>> 	There is activity on the specified port, or
>>>>>
>>>>> 	There is any activity on any port on the specified hub, or
>>>>>
>>>>> 	There is any USB activity on any port.
>>>>>
>>>>> That ought to cover most of the normal use cases, and it would be
>>>>> simple enough to implement.
>>>>
>>>> What would be the benefit of having a USB port activity trigger,
>>>> for which we would be specifying the port to observe, but in the same
>>>> time we would react on any activity on any port (cases 1 and 3)?
>>>
>>> I meant these three cases to be mutually exclusive.  For a given LED,
>>> you could have only one of those trigger types (like mentioned above,
>>> only one trigger per LED).  For example, you might accept any one of:
>>>
>>> 	echo usb1-4.2 >/sys/class/led/foo/trigger
>>>
>>> 	echo hub1-4 >/sys/class/led/foo/trigger
>>>
>>> 	echo usb >/sys/class/led/foo/trigger
>>>
>>> Yes, it would be possible to have a port-specific trigger for one LED
>>> and an overall USB activity trigger for another LED.  I don't know how
>>> useful this would be -- you could probably imagine some unlikely
>>> scenario.
>>>
>>> The point is that doing things this way wouldn't require any API
>>> violations, and it would allow users to do almost all of the things
>>> they are likely to want.
>>
>> We'd have to define single API for generating USB trigger event,
>> so as not enforce addition of three different API calls to the USB
>> drivers.

Of course this trigger represents yet another type of triggers, that
don't require exposing an API for generating events, but instead
register as listeners to some notifiers. I missed that initially.

> The USB core would need only one LED-API call, which it would make upon
> the completion of an URB.  The trigger code should be able to handle
> all the rest (i.e., see which LEDs should be triggered by that URB).

This is assured by the LED trigger core.

>
>>  The type of USB events that the LED should react upon could be
>> defined by parsing the value written to the sysfs file.
>
> There is only one type of event: completion of an URB.  Triggers would
> differ depending only on the device/port that the URB was aimed at.
> _That_ information could be defined by parsing the value written to the
> sysfs file.
>
>> This of course implies, that we should have single LED USB port trigger.
>>
>> The remaining issue is the sysfs interface design for defining and
>> presenting multiple USB ports. I'm still in favour of a single
>> attribute with space separated list. This scheme is commonly used
>> in existing interfaces.
>
> No such interface is needed if you do things the way I outlined above.
> Each trigger would require the user to specify either one port, one
> hub, or nothing at all.  Multiple ports would not be used.

The patch assumes that it is possible to register trigger for many
ports.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Alan Stern]

On Sat, 3 Sep 2016, Jacek Anaszewski wrote:

> >> The remaining issue is the sysfs interface design for defining and
> >> presenting multiple USB ports. I'm still in favour of a single
> >> attribute with space separated list. This scheme is commonly used
> >> in existing interfaces.
> >
> > No such interface is needed if you do things the way I outlined above.
> > Each trigger would require the user to specify either one port, one
> > hub, or nothing at all.  Multiple ports would not be used.
> 
> The patch assumes that it is possible to register trigger for many
> ports.

The patch could be changed to remove that assumption.

Alan Stern

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 4 September 2016 at 02:24, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 3 Sep 2016, Jacek Anaszewski wrote:
>
>> >> The remaining issue is the sysfs interface design for defining and
>> >> presenting multiple USB ports. I'm still in favour of a single
>> >> attribute with space separated list. This scheme is commonly used
>> >> in existing interfaces.
>> >
>> > No such interface is needed if you do things the way I outlined above.
>> > Each trigger would require the user to specify either one port, one
>> > hub, or nothing at all.  Multiple ports would not be used.
>>
>> The patch assumes that it is possible to register trigger for many
>> ports.
>
> The patch could be changed to remove that assumption.

I did this assumption for a reason. There are many devices with more
than 1 USB port and these ports are handled by the same controllers,
just different ports. Let me share USB details of my SmartRG SR400ac
device with 2 physical USB ports:

Physical USB 3.0 port is handled by following controllers and their
root hub ports:
1) OHCI's port 1
2) EHCI's port 1
3) XHCI's port 1

Physical USB 2.0 port is handled by following controllers and their
root hub ports:
1) OHCI's port 2
2) EHCI's port 2


This devices has 3 separated LEDs. I'll list them and tell which ports
should be assigned to them:

bcm53xx:green:usb3 should have assigned:
1) OHCI's port 1
2) EHCI's port 1

bcm53xx:white:usb3 should have assigned:
1) XHCI's port 1

bcm53xx:white:usb2 should have assigned:
1) OHCI's port 2
2) EHCI's port 2


As you can see, I need to specify ports precisely. Assigning a single
port is not enough. Assigning whole hub is too much. So none of:
>         echo usb1-4.2 >/sys/class/led/foo/trigger
>         echo hub1-4 >/sys/class/led/foo/trigger
>         echo usb >/sys/class/led/foo/trigger
would be sufficient here :(

-- 
Rafał

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

Hi!

> This commit adds a new trigger responsible for turning on LED when USB
> device gets connected to the specified USB port. This can can useful for
> various home routers that have USB port(s) and a proper LED telling user
> a device is connected.
> 
> The trigger gets its documentation file but basically it just requires
> specifying USB port in a Linux format (e.g. echo 1-1 > new_port).
> 
> During work on this trigger there was a plan to add DT bindings for it,
> but there wasn't an agreement on the format yet. This can be worked on
> later, a sysfs interface is needed anyway for platforms not using DT.
> 
> Another planned feature is support for LED reacting to the USB activity.
> This can be implemented with another sysfs file for setting mode. The
> default mode wouldn't change so there won't be ABI breakage and such
> feature can be safely implemented later.

Actually... USB device plugs/unplugs are pretty rare events... Can we
just do this in userspace using udevd?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2433 bytes --]

On Wed 2016-08-31 14:23:13, Alan Stern wrote:
> On Tue, 30 Aug 2016, Rafał Miłecki wrote:
> 
> > >> As you quite often need more complex LED management, there are
> > >> triggers that were introduced in 2006 by c3bc9956ec52f ("[PATCH] LED:
> > >> add LED trigger tupport"). Some triggers are trivial and could be
> > >> implemented in userspace as well (e.g. "timer"). Some had to be
> > >> implemented in kernelspace (CPU activity, MTD activity, etc.). Having
> > >> few triggers compiled, you can assign them to LEDs at it pleases you.
> > >> Your hardware may have generic LED (not labeled) and you can
> > >> dynamically assign various triggers to it, depending e.g. on user
> > >> actions. E.g. if user (using GUI or whatever) wants to see flash
> > >> activity, your userspace script should do:
> > >> echo mtd > /sys/class/leds/foo/trigger
> > >
> > > So for example, you might want to do:
> > >
> > >         echo usb1-4 >/sys/class/leds/foo/trigger
> > >
> > > and then have the "foo" LED toggle whenever an URB was submitted or
> > > completed for a device attached to the 1-4 port.  Right?
> > 
> > Not really as it won't cover some pretty common use cases. Many home
> > routers have few USB ports (2-5) and only 1 USB LED. It has to be
> > possible to assign few USB ports to a single LED (trigger). That way
> > LED should be turned on (and kept on) if there is at least 1 USB
> > device connected. You obviously can't do:
> > echo "usb1-1 usb1-2 usb2-1" > /sys/class/leds/foo/trigger
> > 
> > This was already brought up by Rob (who mentioned CPU trigger) and I
> > replied him pretty much the same way in:
> > https://lkml.org/lkml/2016/7/29/38
> > (reply starts with "Anyway, the serious limitation I see").
> 
> The code for a bunch of triggers must already be written.  What would 
> the user do if he wanted to flash a single LED in response to both
> CPU activity and MTD activity?  If not
> 
> 	echo "cpu mtd" >/sys/class/leds/foo/trigger

Lets not overcomplicate this... What if user wanted to blink only when
there's both cpu and mtd activity?

I mean, there are way too many possible combinations, but we should
not implement everything. "Heartbeat" for example is nice demo and
nice test case, but ...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]