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

[PATCH V3] 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.

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

Felipe: I'd like to ask for your Ack before having this patch pushed.
---
 Documentation/leds/ledtrig-usbport.txt |  49 +++++++
 drivers/leds/trigger/Kconfig           |   8 ++
 drivers/leds/trigger/Makefile          |   1 +
 drivers/leds/trigger/ledtrig-usbport.c | 253 +++++++++++++++++++++++++++++++++
 4 files changed, 311 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-usbport.txt
 create mode 100644 drivers/leds/trigger/ledtrig-usbport.c

diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
new file mode 100644
index 0000000..fa42227
--- /dev/null
+++ b/Documentation/leds/ledtrig-usbport.txt
@@ -0,0 +1,49 @@
+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 the following sysfs attributes to the LED:
+
+  ports - Reading it lists all USB ports assigned to the trigger. Writing USB
+	  port number to it will make this driver start observing it. It's also
+	  possible to remove USB port from observable list by writing it with a
+	  "-" prefix.
+
+Example use-case:
+
+  echo usbport > trigger
+  echo 4-1 > ports
+  echo 2-1 > ports
+  echo -4-1 > ports
+  cat 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..7f5237c
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -0,0 +1,253 @@
+/*
+ * 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 list_head list;
+};
+
+struct usbport_trig_data {
+	struct led_classdev *led_cdev;
+	struct list_head ports;
+	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);
+}
+
+/* Device attr */
+
+static ssize_t ports_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
+	struct usbport_trig_port *port;
+	ssize_t ret = 0;
+	int len;
+
+	list_for_each_entry(port, &usbport_data->ports, list) {
+		len = sprintf(buf + ret, "%s\n", port->name);
+		if (len >= 0)
+			ret += len;
+	}
+
+	return ret;
+}
+
+static ssize_t ports_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;
+	bool add = true;
+	size_t len;
+
+	len = strlen(buf);
+	/* See if this is add or remove request */
+	if (len && buf[0] == '-') {
+		add = false;
+		buf++;
+		len--;
+	}
+	/* For user convenience trim line break */
+	if (len && buf[len - 1] == '\n')
+		len--;
+	if (!len)
+		return -EINVAL;
+
+	if (add) {
+		port = kzalloc(sizeof(*port), GFP_KERNEL);
+		if (!port)
+			return -ENOMEM;
+		port->name = kzalloc(strlen(buf), GFP_KERNEL);
+		if (!port->name) {
+			kfree(port);
+			return -ENOMEM;
+		}
+		strncpy(port->name, buf, len);
+
+		list_add_tail(&port->list, &usbport_data->ports);
+
+		usbport_trig_update_count(usbport_data);
+	} else {
+		struct usbport_trig_port *tmp;
+		bool found = false;
+
+		list_for_each_entry_safe(port, tmp, &usbport_data->ports,
+					 list) {
+			if (strlen(port->name) == len &&
+			    !strncmp(port->name, buf, len)) {
+				list_del(&port->list);
+				kfree(port->name);
+				kfree(port);
+
+				found = true;
+				break;
+			}
+		}
+
+		if (!found)
+			return -ENOENT;
+	}
+
+	usbport_trig_update_count(usbport_data);
+
+	return size;
+}
+
+static DEVICE_ATTR(ports, S_IRUSR | S_IWUSR, ports_show, ports_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;
+
+	switch (action) {
+	case USB_DEVICE_ADD:
+		if (usbport_trig_usb_dev_observed(usbport_data, data)) {
+			if (usbport_data->count++ == 0)
+				led_set_brightness_nosleep(led_cdev, LED_FULL);
+		}
+		break;
+	case USB_DEVICE_REMOVE:
+		if (usbport_trig_usb_dev_observed(usbport_data, data)) {
+			if (--usbport_data->count == 0)
+				led_set_brightness_nosleep(led_cdev, LED_OFF);
+		}
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+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;
+	INIT_LIST_HEAD(&usbport_data->ports);
+	usbport_data->nb.notifier_call = usbport_trig_notify,
+	led_cdev->trigger_data = usbport_data;
+
+	err = device_create_file(led_cdev->dev, &dev_attr_ports);
+	if (err)
+		goto err_free;
+
+	usb_register_notify(&usbport_data->nb);
+
+	led_cdev->activated = true;
+	return;
+
+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;
+
+	usb_unregister_notify(&usbport_data->nb);
+
+	list_for_each_entry_safe(port, tmp, &usbport_data->ports, list) {
+		list_del(&port->list);
+		kfree(port->name);
+		kfree(port);
+	}
+
+	device_remove_file(led_cdev->dev, &dev_attr_ports);
+	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 V3] 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.

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

Felipe: I'd like to ask for your Ack before having this patch pushed.
---
 Documentation/leds/ledtrig-usbport.txt |  49 +++++++
 drivers/leds/trigger/Kconfig           |   8 ++
 drivers/leds/trigger/Makefile          |   1 +
 drivers/leds/trigger/ledtrig-usbport.c | 253 +++++++++++++++++++++++++++++++++
 4 files changed, 311 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-usbport.txt
 create mode 100644 drivers/leds/trigger/ledtrig-usbport.c

diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
new file mode 100644
index 0000000..fa42227
--- /dev/null
+++ b/Documentation/leds/ledtrig-usbport.txt
@@ -0,0 +1,49 @@
+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 the following sysfs attributes to the LED:
+
+  ports - Reading it lists all USB ports assigned to the trigger. Writing USB
+	  port number to it will make this driver start observing it. It's also
+	  possible to remove USB port from observable list by writing it with a
+	  "-" prefix.
+
+Example use-case:
+
+  echo usbport > trigger
+  echo 4-1 > ports
+  echo 2-1 > ports
+  echo -4-1 > ports
+  cat 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..7f5237c
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -0,0 +1,253 @@
+/*
+ * 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 list_head list;
+};
+
+struct usbport_trig_data {
+	struct led_classdev *led_cdev;
+	struct list_head ports;
+	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);
+}
+
+/* Device attr */
+
+static ssize_t ports_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
+	struct usbport_trig_port *port;
+	ssize_t ret = 0;
+	int len;
+
+	list_for_each_entry(port, &usbport_data->ports, list) {
+		len = sprintf(buf + ret, "%s\n", port->name);
+		if (len >= 0)
+			ret += len;
+	}
+
+	return ret;
+}
+
+static ssize_t ports_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;
+	bool add = true;
+	size_t len;
+
+	len = strlen(buf);
+	/* See if this is add or remove request */
+	if (len && buf[0] == '-') {
+		add = false;
+		buf++;
+		len--;
+	}
+	/* For user convenience trim line break */
+	if (len && buf[len - 1] == '\n')
+		len--;
+	if (!len)
+		return -EINVAL;
+
+	if (add) {
+		port = kzalloc(sizeof(*port), GFP_KERNEL);
+		if (!port)
+			return -ENOMEM;
+		port->name = kzalloc(strlen(buf), GFP_KERNEL);
+		if (!port->name) {
+			kfree(port);
+			return -ENOMEM;
+		}
+		strncpy(port->name, buf, len);
+
+		list_add_tail(&port->list, &usbport_data->ports);
+
+		usbport_trig_update_count(usbport_data);
+	} else {
+		struct usbport_trig_port *tmp;
+		bool found = false;
+
+		list_for_each_entry_safe(port, tmp, &usbport_data->ports,
+					 list) {
+			if (strlen(port->name) == len &&
+			    !strncmp(port->name, buf, len)) {
+				list_del(&port->list);
+				kfree(port->name);
+				kfree(port);
+
+				found = true;
+				break;
+			}
+		}
+
+		if (!found)
+			return -ENOENT;
+	}
+
+	usbport_trig_update_count(usbport_data);
+
+	return size;
+}
+
+static DEVICE_ATTR(ports, S_IRUSR | S_IWUSR, ports_show, ports_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;
+
+	switch (action) {
+	case USB_DEVICE_ADD:
+		if (usbport_trig_usb_dev_observed(usbport_data, data)) {
+			if (usbport_data->count++ == 0)
+				led_set_brightness_nosleep(led_cdev, LED_FULL);
+		}
+		break;
+	case USB_DEVICE_REMOVE:
+		if (usbport_trig_usb_dev_observed(usbport_data, data)) {
+			if (--usbport_data->count == 0)
+				led_set_brightness_nosleep(led_cdev, LED_OFF);
+		}
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+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;
+	INIT_LIST_HEAD(&usbport_data->ports);
+	usbport_data->nb.notifier_call = usbport_trig_notify,
+	led_cdev->trigger_data = usbport_data;
+
+	err = device_create_file(led_cdev->dev, &dev_attr_ports);
+	if (err)
+		goto err_free;
+
+	usb_register_notify(&usbport_data->nb);
+
+	led_cdev->activated = true;
+	return;
+
+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;
+
+	usb_unregister_notify(&usbport_data->nb);
+
+	list_for_each_entry_safe(port, tmp, &usbport_data->ports, list) {
+		list_del(&port->list);
+		kfree(port->name);
+		kfree(port);
+	}
+
+	device_remove_file(led_cdev->dev, &dev_attr_ports);
+	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 V3] leds: trigger: Introduce an USB port trigger

[Greg KH]

On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote:
> 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.
> 
> 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
> 
> Felipe: I'd like to ask for your Ack before having this patch pushed.
> ---
>  Documentation/leds/ledtrig-usbport.txt |  49 +++++++
>  drivers/leds/trigger/Kconfig           |   8 ++
>  drivers/leds/trigger/Makefile          |   1 +
>  drivers/leds/trigger/ledtrig-usbport.c | 253 +++++++++++++++++++++++++++++++++
>  4 files changed, 311 insertions(+)
>  create mode 100644 Documentation/leds/ledtrig-usbport.txt
>  create mode 100644 drivers/leds/trigger/ledtrig-usbport.c

You are adding sysfs files without adding a Documentation/ABI/ entry,
please never do that.

NAK.

greg k-h

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

[Greg KH]

On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote:
> +static ssize_t ports_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
> +	struct usbport_trig_port *port;
> +	ssize_t ret = 0;
> +	int len;
> +
> +	list_for_each_entry(port, &usbport_data->ports, list) {
> +		len = sprintf(buf + ret, "%s\n", port->name);
> +		if (len >= 0)
> +			ret += len;
> +	}
> +
> +	return ret;
> +}

sysfs is "one value per file", here you are listing a bunch of things in
one sysfs file.  Please don't do that.

greg k-h

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

[Rafał Miłecki]

On 24 August 2016 at 11:21, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>
>> 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.
>>
>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>> ---
>> 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
>>
>> Felipe: I'd like to ask for your Ack before having this patch pushed.
>> ---
>>  Documentation/leds/ledtrig-usbport.txt |  49 +++++++
>>  drivers/leds/trigger/Kconfig           |   8 ++
>>  drivers/leds/trigger/Makefile          |   1 +
>>  drivers/leds/trigger/ledtrig-usbport.c | 253 +++++++++++++++++++++++++++++++++
>>  4 files changed, 311 insertions(+)
>>  create mode 100644 Documentation/leds/ledtrig-usbport.txt
>>  create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
>
> You are adding sysfs files without adding a Documentation/ABI/ entry,
> please never do that.

This is the way all LED triggers are documented, I just blindly
assumed it's OK. Could you be a bit more helpful and help me to
understand further steps? Should I drop
Documentation/leds/ledtrig-usbport.txt and add proper entry in
Documentation/ABI/testing? Or should I keep both files? What about
current sysfs of other triggers? Should they be moved/documented in
ABI?

-- 
Rafał
--
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 V3] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 24 August 2016 at 11:21, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote:
>> 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.
>>
>> 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
>>
>> Felipe: I'd like to ask for your Ack before having this patch pushed.
>> ---
>>  Documentation/leds/ledtrig-usbport.txt |  49 +++++++
>>  drivers/leds/trigger/Kconfig           |   8 ++
>>  drivers/leds/trigger/Makefile          |   1 +
>>  drivers/leds/trigger/ledtrig-usbport.c | 253 +++++++++++++++++++++++++++++++++
>>  4 files changed, 311 insertions(+)
>>  create mode 100644 Documentation/leds/ledtrig-usbport.txt
>>  create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
>
> You are adding sysfs files without adding a Documentation/ABI/ entry,
> please never do that.

This is the way all LED triggers are documented, I just blindly
assumed it's OK. Could you be a bit more helpful and help me to
understand further steps? Should I drop
Documentation/leds/ledtrig-usbport.txt and add proper entry in
Documentation/ABI/testing? Or should I keep both files? What about
current sysfs of other triggers? Should they be moved/documented in
ABI?

-- 
Rafał

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

[Rafał Miłecki]

On 24 August 2016 at 11:22, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote:
>> +static ssize_t ports_show(struct device *dev, struct device_attribute *attr,
>> +                       char *buf)
>> +{
>> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +     struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
>> +     struct usbport_trig_port *port;
>> +     ssize_t ret = 0;
>> +     int len;
>> +
>> +     list_for_each_entry(port, &usbport_data->ports, list) {
>> +             len = sprintf(buf + ret, "%s\n", port->name);
>> +             if (len >= 0)
>> +                     ret += len;
>> +     }
>> +
>> +     return ret;
>> +}
>
> sysfs is "one value per file", here you are listing a bunch of things in
> one sysfs file.  Please don't do that.

OK. What do you think about creating "ports" subdirectory and creating
file-per-port in it? Then I'd need to bring back something like
"new_port" and "remove_port". Does it sound OK?

-- 
Rafał

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

[Matthias Brugger]

On 24/08/16 00:03, Rafał Miłecki wrote:
> 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.
>
> 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
>
> Felipe: I'd like to ask for your Ack before having this patch pushed.
> ---
>  Documentation/leds/ledtrig-usbport.txt |  49 +++++++
>  drivers/leds/trigger/Kconfig           |   8 ++
>  drivers/leds/trigger/Makefile          |   1 +
>  drivers/leds/trigger/ledtrig-usbport.c | 253 +++++++++++++++++++++++++++++++++
>  4 files changed, 311 insertions(+)
>  create mode 100644 Documentation/leds/ledtrig-usbport.txt
>  create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
>
> diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
> new file mode 100644
> index 0000000..fa42227
> --- /dev/null
> +++ b/Documentation/leds/ledtrig-usbport.txt
> @@ -0,0 +1,49 @@
> +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 the following sysfs attributes to the LED:
> +
> +  ports - Reading it lists all USB ports assigned to the trigger. Writing USB
> +	  port number to it will make this driver start observing it. It's also
> +	  possible to remove USB port from observable list by writing it with a
> +	  "-" prefix.
> +
> +Example use-case:
> +
> +  echo usbport > trigger
> +  echo 4-1 > ports
> +  echo 2-1 > ports
> +  echo -4-1 > ports
> +  cat 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..7f5237c
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-usbport.c
> @@ -0,0 +1,253 @@
> +/*
> + * 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 list_head list;
> +};
> +
> +struct usbport_trig_data {
> +	struct led_classdev *led_cdev;
> +	struct list_head ports;
> +	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);
> +}
> +
> +/* Device attr */
> +
> +static ssize_t ports_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
> +	struct usbport_trig_port *port;
> +	ssize_t ret = 0;
> +	int len;
> +
> +	list_for_each_entry(port, &usbport_data->ports, list) {
> +		len = sprintf(buf + ret, "%s\n", port->name);
> +		if (len >= 0)
> +			ret += len;
> +	}
> +
> +	return ret;
> +}
> +
> +static ssize_t ports_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;
> +	bool add = true;
> +	size_t len;
> +
> +	len = strlen(buf);
> +	/* See if this is add or remove request */
> +	if (len && buf[0] == '-') {
> +		add = false;
> +		buf++;
> +		len--;
> +	}
> +	/* For user convenience trim line break */
> +	if (len && buf[len - 1] == '\n')
> +		len--;
> +	if (!len)
> +		return -EINVAL;
> +
> +	if (add) {
> +		port = kzalloc(sizeof(*port), GFP_KERNEL);
> +		if (!port)
> +			return -ENOMEM;
> +		port->name = kzalloc(strlen(buf), GFP_KERNEL);
> +		if (!port->name) {
> +			kfree(port);
> +			return -ENOMEM;
> +		}
> +		strncpy(port->name, buf, len);
> +
> +		list_add_tail(&port->list, &usbport_data->ports);
> +
> +		usbport_trig_update_count(usbport_data);
> +	} else {
> +		struct usbport_trig_port *tmp;
> +		bool found = false;
> +
> +		list_for_each_entry_safe(port, tmp, &usbport_data->ports,
> +					 list) {
> +			if (strlen(port->name) == len &&
> +			    !strncmp(port->name, buf, len)) {
> +				list_del(&port->list);
> +				kfree(port->name);
> +				kfree(port);
> +
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (!found)
> +			return -ENOENT;
> +	}
> +
> +	usbport_trig_update_count(usbport_data);
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR(ports, S_IRUSR | S_IWUSR, ports_show, ports_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;
> +
> +	switch (action) {
> +	case USB_DEVICE_ADD:
> +		if (usbport_trig_usb_dev_observed(usbport_data, data)) {

Maybe we should switch this and fist see if the usbport is observed 
before evaluating the action. Also cast data to "struct usb_device *" to 
make that clear.

> +			if (usbport_data->count++ == 0)

I'm a bit puzzled. I think:
if (++usbport_data->count > 0)
makes this more consistent with the remove case.

> +				led_set_brightness_nosleep(led_cdev, LED_FULL);
> +		}
> +		break;
> +	case USB_DEVICE_REMOVE:
> +		if (usbport_trig_usb_dev_observed(usbport_data, data)) {
> +			if (--usbport_data->count == 0)
> +				led_set_brightness_nosleep(led_cdev, LED_OFF);
> +		}
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +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;
> +	INIT_LIST_HEAD(&usbport_data->ports);
> +	usbport_data->nb.notifier_call = usbport_trig_notify,
> +	led_cdev->trigger_data = usbport_data;
> +
> +	err = device_create_file(led_cdev->dev, &dev_attr_ports);
> +	if (err)
> +		goto err_free;
> +
> +	usb_register_notify(&usbport_data->nb);
> +
> +	led_cdev->activated = true;
> +	return;
> +
> +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;
> +
> +	usb_unregister_notify(&usbport_data->nb);
> +
> +	list_for_each_entry_safe(port, tmp, &usbport_data->ports, list) {
> +		list_del(&port->list);
> +		kfree(port->name);
> +		kfree(port);
> +	}
> +
> +	device_remove_file(led_cdev->dev, &dev_attr_ports);
> +	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>");

Nit: rafal@milecki.pl

Regards,
Matthias

> +MODULE_DESCRIPTION("USB port trigger");
> +MODULE_LICENSE("GPL");
>

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

[Rafał Miłecki]

On 24 August 2016 at 12:49, Matthias Brugger <mbrugger@suse.com> wrote:
> On 24/08/16 00:03, Rafał Miłecki wrote:
>>
>> 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.
>>
>> 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
>>
>> Felipe: I'd like to ask for your Ack before having this patch pushed.
>> ---
>>  Documentation/leds/ledtrig-usbport.txt |  49 +++++++
>>  drivers/leds/trigger/Kconfig           |   8 ++
>>  drivers/leds/trigger/Makefile          |   1 +
>>  drivers/leds/trigger/ledtrig-usbport.c | 253
>> +++++++++++++++++++++++++++++++++
>>  4 files changed, 311 insertions(+)
>>  create mode 100644 Documentation/leds/ledtrig-usbport.txt
>>  create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
>>
>> diff --git a/Documentation/leds/ledtrig-usbport.txt
>> b/Documentation/leds/ledtrig-usbport.txt
>> new file mode 100644
>> index 0000000..fa42227
>> --- /dev/null
>> +++ b/Documentation/leds/ledtrig-usbport.txt
>> @@ -0,0 +1,49 @@
>> +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 the following sysfs attributes to the LED:
>> +
>> +  ports - Reading it lists all USB ports assigned to the trigger. Writing
>> USB
>> +         port number to it will make this driver start observing it. It's
>> also
>> +         possible to remove USB port from observable list by writing it
>> with a
>> +         "-" prefix.
>> +
>> +Example use-case:
>> +
>> +  echo usbport > trigger
>> +  echo 4-1 > ports
>> +  echo 2-1 > ports
>> +  echo -4-1 > ports
>> +  cat 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..7f5237c
>> --- /dev/null
>> +++ b/drivers/leds/trigger/ledtrig-usbport.c
>> @@ -0,0 +1,253 @@
>> +/*
>> + * 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 list_head list;
>> +};
>> +
>> +struct usbport_trig_data {
>> +       struct led_classdev *led_cdev;
>> +       struct list_head ports;
>> +       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);
>> +}
>> +
>> +/* Device attr */
>> +
>> +static ssize_t ports_show(struct device *dev, struct device_attribute
>> *attr,
>> +                         char *buf)
>> +{
>> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> +       struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
>> +       struct usbport_trig_port *port;
>> +       ssize_t ret = 0;
>> +       int len;
>> +
>> +       list_for_each_entry(port, &usbport_data->ports, list) {
>> +               len = sprintf(buf + ret, "%s\n", port->name);
>> +               if (len >= 0)
>> +                       ret += len;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static ssize_t ports_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;
>> +       bool add = true;
>> +       size_t len;
>> +
>> +       len = strlen(buf);
>> +       /* See if this is add or remove request */
>> +       if (len && buf[0] == '-') {
>> +               add = false;
>> +               buf++;
>> +               len--;
>> +       }
>> +       /* For user convenience trim line break */
>> +       if (len && buf[len - 1] == '\n')
>> +               len--;
>> +       if (!len)
>> +               return -EINVAL;
>> +
>> +       if (add) {
>> +               port = kzalloc(sizeof(*port), GFP_KERNEL);
>> +               if (!port)
>> +                       return -ENOMEM;
>> +               port->name = kzalloc(strlen(buf), GFP_KERNEL);
>> +               if (!port->name) {
>> +                       kfree(port);
>> +                       return -ENOMEM;
>> +               }
>> +               strncpy(port->name, buf, len);
>> +
>> +               list_add_tail(&port->list, &usbport_data->ports);
>> +
>> +               usbport_trig_update_count(usbport_data);
>> +       } else {
>> +               struct usbport_trig_port *tmp;
>> +               bool found = false;
>> +
>> +               list_for_each_entry_safe(port, tmp, &usbport_data->ports,
>> +                                        list) {
>> +                       if (strlen(port->name) == len &&
>> +                           !strncmp(port->name, buf, len)) {
>> +                               list_del(&port->list);
>> +                               kfree(port->name);
>> +                               kfree(port);
>> +
>> +                               found = true;
>> +                               break;
>> +                       }
>> +               }
>> +
>> +               if (!found)
>> +                       return -ENOENT;
>> +       }
>> +
>> +       usbport_trig_update_count(usbport_data);
>> +
>> +       return size;
>> +}
>> +
>> +static DEVICE_ATTR(ports, S_IRUSR | S_IWUSR, ports_show, ports_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;
>> +
>> +       switch (action) {
>> +       case USB_DEVICE_ADD:
>> +               if (usbport_trig_usb_dev_observed(usbport_data, data)) {
>
>
> Maybe we should switch this and fist see if the usbport is observed before
> evaluating the action. Also cast data to "struct usb_device *" to make that
> clear.

I'm aware there is one duplicated line of code, I did to first
evaluate very quick test (checking unsigned long value), then iterate
over ports & keep only 1 switch block. I could move
usbport_trig_usb_dev_observed call up, but it would be executed for
other actions as well (currently just USB_BUS_ADD and USB_BUS_REMOVE).


>> +                       if (usbport_data->count++ == 0)
>
>
> I'm a bit puzzled. I think:
> if (++usbport_data->count > 0)
> makes this more consistent with the remove case.

Your condition would be always true (as we don't use negative
numbers). The point is to enable LED only if it was disabled before.
So I need to increase counter unconditionally but enable LED only if
initial value (before increasing it) was 0.


>> +module_init(usbport_trig_init);
>> +module_exit(usbport_trig_exit);
>> +
>> +MODULE_AUTHOR("Rafał Miłecki <rafal@milecki@pl>");
>
>
> Nit: rafal@milecki.pl

Oops, thanks!

-- 
Rafał

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

[Matthias Brugger]

On 24/08/16 13:02, Rafał Miłecki wrote:
> On 24 August 2016 at 12:49, Matthias Brugger <mbrugger@suse.com> wrote:
>> On 24/08/16 00:03, Rafał Miłecki wrote:

[...]

>>> +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;
>>> +
>>> +       switch (action) {
>>> +       case USB_DEVICE_ADD:
>>> +               if (usbport_trig_usb_dev_observed(usbport_data, data)) {
>>
>>
>> Maybe we should switch this and fist see if the usbport is observed before
>> evaluating the action. Also cast data to "struct usb_device *" to make that
>> clear.
>
> I'm aware there is one duplicated line of code, I did to first
> evaluate very quick test (checking unsigned long value), then iterate
> over ports & keep only 1 switch block. I could move
> usbport_trig_usb_dev_observed call up, but it would be executed for
> other actions as well (currently just USB_BUS_ADD and USB_BUS_REMOVE).
>

Ok. I'm a USB noop but from my understanding the notifier is only called 
when a device or a hub gets added/removed. So this shouldn't happen that 
much. Therefor it has no impact if we check if the usb device is in the 
observer list for all actions.

>
>>> +                       if (usbport_data->count++ == 0)
>>
>>
>> I'm a bit puzzled. I think:
>> if (++usbport_data->count > 0)
>> makes this more consistent with the remove case.
>
> Your condition would be always true (as we don't use negative
> numbers). The point is to enable LED only if it was disabled before.
> So I need to increase counter unconditionally but enable LED only if
> initial value (before increasing it) was 0.
>

Got it. My personal opinion is, that adding one line for 
incrementing/decrementing the counter would help to make this 
crystal-clear to everyone (at least to me :)

Cheers,
Matthias

>
>>> +module_init(usbport_trig_init);
>>> +module_exit(usbport_trig_exit);
>>> +
>>> +MODULE_AUTHOR("Rafał Miłecki <rafal@milecki@pl>");
>>
>>
>> Nit: rafal@milecki.pl
>
> Oops, thanks!
>

[PATCH RFC V3.5] 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.

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.

The last big missing thing is Documentation update (this is why I'm
sending RFC). Greg pointed out we should have some entries in
Documentation/ABI, but it seems none of triggers have it. Any idea why
is that? Do we need to change it? Or is it required for new code only?
If so, should I care about Documentation/leds/ledtrig-usbport.txt at
all in this patch?

For now I didn't update Documentation/leds/ledtrig-usbport.txt with the
new new_port and remove_port API, until I get a clue how to proceed.
---
 Documentation/leds/ledtrig-usbport.txt |  49 ++++++
 drivers/leds/trigger/Kconfig           |   8 +
 drivers/leds/trigger/Makefile          |   1 +
 drivers/leds/trigger/ledtrig-usbport.c | 309 +++++++++++++++++++++++++++++++++
 4 files changed, 367 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-usbport.txt
 create mode 100644 drivers/leds/trigger/ledtrig-usbport.c

diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
new file mode 100644
index 0000000..fa42227
--- /dev/null
+++ b/Documentation/leds/ledtrig-usbport.txt
@@ -0,0 +1,49 @@
+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 the following sysfs attributes to the LED:
+
+  ports - Reading it lists all USB ports assigned to the trigger. Writing USB
+	  port number to it will make this driver start observing it. It's also
+	  possible to remove USB port from observable list by writing it with a
+	  "-" prefix.
+
+Example use-case:
+
+  echo usbport > trigger
+  echo 4-1 > ports
+  echo 2-1 > ports
+  echo -4-1 > ports
+  cat 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 RFC V3.5] 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.

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.

The last big missing thing is Documentation update (this is why I'm
sending RFC). Greg pointed out we should have some entries in
Documentation/ABI, but it seems none of triggers have it. Any idea why
is that? Do we need to change it? Or is it required for new code only?
If so, should I care about Documentation/leds/ledtrig-usbport.txt at
all in this patch?

For now I didn't update Documentation/leds/ledtrig-usbport.txt with the
new new_port and remove_port API, until I get a clue how to proceed.
---
 Documentation/leds/ledtrig-usbport.txt |  49 ++++++
 drivers/leds/trigger/Kconfig           |   8 +
 drivers/leds/trigger/Makefile          |   1 +
 drivers/leds/trigger/ledtrig-usbport.c | 309 +++++++++++++++++++++++++++++++++
 4 files changed, 367 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-usbport.txt
 create mode 100644 drivers/leds/trigger/ledtrig-usbport.c

diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
new file mode 100644
index 0000000..fa42227
--- /dev/null
+++ b/Documentation/leds/ledtrig-usbport.txt
@@ -0,0 +1,49 @@
+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 the following sysfs attributes to the LED:
+
+  ports - Reading it lists all USB ports assigned to the trigger. Writing USB
+	  port number to it will make this driver start observing it. It's also
+	  possible to remove USB port from observable list by writing it with a
+	  "-" prefix.
+
+Example use-case:
+
+  echo usbport > trigger
+  echo 4-1 > ports
+  echo 2-1 > ports
+  echo -4-1 > ports
+  cat 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 RFC V3.5] leds: trigger: Introduce an USB port trigger

[Bjørn Mork]

Rafał Miłecki <zajec5@gmail.com> writes:

> The last big missing thing is Documentation update (this is why I'm
> sending RFC). Greg pointed out we should have some entries in
> Documentation/ABI, but it seems none of triggers have it.

There's a lot missing, but there is at least one exception:
The "inverted" attribute of the  gpio and backlight triggers is
documented as part of Documentation/ABI/testing/sysfs-class-led

> Any idea why is that?

Manual enforcement fails from time to time? The requirement was less
strict in the early days of sysfs? Does it matter?

> Do we need to change it? Or is it required for new code only?

The lack of ABI docs is a bug. Don't add new code with known bugs. Old
code should be fixed, but there is no immediate *need* to fix it.


Bjørn

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Bjørn Mork]

Rafał Miłecki <zajec5@gmail.com> writes:

> The last big missing thing is Documentation update (this is why I'm
> sending RFC). Greg pointed out we should have some entries in
> Documentation/ABI, but it seems none of triggers have it.

There's a lot missing, but there is at least one exception:
The "inverted" attribute of the  gpio and backlight triggers is
documented as part of Documentation/ABI/testing/sysfs-class-led

> Any idea why is that?

Manual enforcement fails from time to time? The requirement was less
strict in the early days of sysfs? Does it matter?

> Do we need to change it? Or is it required for new code only?

The lack of ABI docs is a bug. Don't add new code with known bugs. Old
code should be fixed, but there is no immediate *need* to fix it.


Bjørn

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 24 August 2016 at 20:48, Bjørn Mork <bjorn@mork.no> wrote:
> Rafał Miłecki <zajec5@gmail.com> writes:
>
>> The last big missing thing is Documentation update (this is why I'm
>> sending RFC). Greg pointed out we should have some entries in
>> Documentation/ABI, but it seems none of triggers have it.
>
> There's a lot missing, but there is at least one exception:
> The "inverted" attribute of the  gpio and backlight triggers is
> documented as part of Documentation/ABI/testing/sysfs-class-led
>
>> Any idea why is that?
>
> Manual enforcement fails from time to time? The requirement was less
> strict in the early days of sysfs? Does it matter?
>
>> Do we need to change it? Or is it required for new code only?
>
> The lack of ABI docs is a bug. Don't add new code with known bugs. Old
> code should be fixed, but there is no immediate *need* to fix it.

Don't get me wrong. I care about code quality and documentation. I
mean to follow kernel rules. It's just some things may be not clear to
ppl and it would be nice to get any explanation/help. That said thanks
a lot of your e-mail, I'll try to expand pointed Documentation.

-- 
Rafał

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

[Greg KH]

On Wed, Aug 24, 2016 at 11:26:27AM +0200, Rafał Miłecki wrote:
> On 24 August 2016 at 11:21, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote:
> >> 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.
> >>
> >> 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
> >>
> >> Felipe: I'd like to ask for your Ack before having this patch pushed.
> >> ---
> >>  Documentation/leds/ledtrig-usbport.txt |  49 +++++++
> >>  drivers/leds/trigger/Kconfig           |   8 ++
> >>  drivers/leds/trigger/Makefile          |   1 +
> >>  drivers/leds/trigger/ledtrig-usbport.c | 253 +++++++++++++++++++++++++++++++++
> >>  4 files changed, 311 insertions(+)
> >>  create mode 100644 Documentation/leds/ledtrig-usbport.txt
> >>  create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
> >
> > You are adding sysfs files without adding a Documentation/ABI/ entry,
> > please never do that.
> 
> This is the way all LED triggers are documented, I just blindly
> assumed it's OK. Could you be a bit more helpful and help me to
> understand further steps? Should I drop
> Documentation/leds/ledtrig-usbport.txt and add proper entry in
> Documentation/ABI/testing? Or should I keep both files? What about
> current sysfs of other triggers? Should they be moved/documented in
> ABI?

In the end, all should be moved into Documentation/ABI/  But for you,
might as well start documenting them in there for any new ones so you
don't have to just move them later.

thanks,

greg k-h

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

[Greg KH]

On Wed, Aug 24, 2016 at 11:29:51AM +0200, Rafał Miłecki wrote:
> On 24 August 2016 at 11:22, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote:
> >> +static ssize_t ports_show(struct device *dev, struct device_attribute *attr,
> >> +                       char *buf)
> >> +{
> >> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >> +     struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
> >> +     struct usbport_trig_port *port;
> >> +     ssize_t ret = 0;
> >> +     int len;
> >> +
> >> +     list_for_each_entry(port, &usbport_data->ports, list) {
> >> +             len = sprintf(buf + ret, "%s\n", port->name);
> >> +             if (len >= 0)
> >> +                     ret += len;
> >> +     }
> >> +
> >> +     return ret;
> >> +}
> >
> > sysfs is "one value per file", here you are listing a bunch of things in
> > one sysfs file.  Please don't do that.
> 
> OK. What do you think about creating "ports" subdirectory and creating
> file-per-port in it? Then I'd need to bring back something like
> "new_port" and "remove_port". Does it sound OK?

Maybe, I don't know.  Why is "USB" somehow unique here?  Why isn't this
the same for PCI slots/ports?  pccard?  sdcard?  thunderbolt?

thanks,

greg k-h

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

[Rafał Miłecki]

On 24 August 2016 at 23:04, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Aug 24, 2016 at 11:29:51AM +0200, Rafał Miłecki wrote:
>> On 24 August 2016 at 11:22, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote:
>> >> +static ssize_t ports_show(struct device *dev, struct device_attribute *attr,
>> >> +                       char *buf)
>> >> +{
>> >> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> >> +     struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
>> >> +     struct usbport_trig_port *port;
>> >> +     ssize_t ret = 0;
>> >> +     int len;
>> >> +
>> >> +     list_for_each_entry(port, &usbport_data->ports, list) {
>> >> +             len = sprintf(buf + ret, "%s\n", port->name);
>> >> +             if (len >= 0)
>> >> +                     ret += len;
>> >> +     }
>> >> +
>> >> +     return ret;
>> >> +}
>> >
>> > sysfs is "one value per file", here you are listing a bunch of things in
>> > one sysfs file.  Please don't do that.
>>
>> OK. What do you think about creating "ports" subdirectory and creating
>> file-per-port in it? Then I'd need to bring back something like
>> "new_port" and "remove_port". Does it sound OK?
>
> Maybe, I don't know.  Why is "USB" somehow unique here?  Why isn't this
> the same for PCI slots/ports?  pccard?  sdcard?  thunderbolt?

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 RFC V3.5] leds: trigger: Introduce an USB port trigger

[Jacek Anaszewski]

zOn 08/24/2016 07:52 PM, Rafał Miłecki wrote:
> 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.
>
> 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.

Now writing new_port with "1-1" produces a file named "1-1" in the
ports directory with 000 permissions. I think that what Greg had
on mind by referring to "one value per file" rule was a set of
files representing ports, like "1-1 1-2 2-1", and each file should be
readable/writeable.

For instance "echo 1 > 1-1" would enable the trigger for the port 1-1
and "echo 0 > 1-1" would disable it. The problem is that we don't know
the number of required ports at compilation time and the sysfs
attributes would have to be dynamically created on driver instantiation.
What is more, as the USB ports can dynamically appear/disappear in the
system, the files would have to be created/removed accordingly during
LED class device lifetime, which is not the best design for the sysfs
interface I think.

Therefore, maybe it would be good to follow the "triggers" sysfs
attribute pattern, where it lists the available LED triggers?

The question is whether there is some mechanism available for
notifying addition/removal of a USB port?

Also a description of the device connected to the port would be a nice
feature, however I am not certain about the feasibility thereof.

> The last big missing thing is Documentation update (this is why I'm
> sending RFC). Greg pointed out we should have some entries in
> Documentation/ABI, but it seems none of triggers have it. Any idea why
> is that? Do we need to change it? Or is it required for new code only?
> If so, should I care about Documentation/leds/ledtrig-usbport.txt at
> all in this patch?
>
> For now I didn't update Documentation/leds/ledtrig-usbport.txt with the
> new new_port and remove_port API, until I get a clue how to proceed.
> ---
>  Documentation/leds/ledtrig-usbport.txt |  49 ++++++
>  drivers/leds/trigger/Kconfig           |   8 +
>  drivers/leds/trigger/Makefile          |   1 +
>  drivers/leds/trigger/ledtrig-usbport.c | 309 +++++++++++++++++++++++++++++++++
>  4 files changed, 367 insertions(+)
>  create mode 100644 Documentation/leds/ledtrig-usbport.txt
>  create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
>
> diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
> new file mode 100644
> index 0000000..fa42227
> --- /dev/null
> +++ b/Documentation/leds/ledtrig-usbport.txt
> @@ -0,0 +1,49 @@
> +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 the following sysfs attributes to the LED:
> +
> +  ports - Reading it lists all USB ports assigned to the trigger. Writing USB
> +	  port number to it will make this driver start observing it. It's also
> +	  possible to remove USB port from observable list by writing it with a
> +	  "-" prefix.
> +
> +Example use-case:
> +
> +  echo usbport > trigger
> +  echo 4-1 > ports
> +  echo 2-1 > ports
> +  echo -4-1 > ports
> +  cat 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");
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Matthias Brugger]

On 25/08/16 10:03, Jacek Anaszewski wrote:
> zOn 08/24/2016 07:52 PM, Rafał Miłecki wrote:
>> 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.
>>
>> 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.
>
> Now writing new_port with "1-1" produces a file named "1-1" in the
> ports directory with 000 permissions. I think that what Greg had
> on mind by referring to "one value per file" rule was a set of
> files representing ports, like "1-1 1-2 2-1", and each file should be
> readable/writeable.
>
> For instance "echo 1 > 1-1" would enable the trigger for the port 1-1
> and "echo 0 > 1-1" would disable it. The problem is that we don't know
> the number of required ports at compilation time and the sysfs
> attributes would have to be dynamically created on driver instantiation.
> What is more, as the USB ports can dynamically appear/disappear in the
> system, the files would have to be created/removed accordingly during
> LED class device lifetime, which is not the best design for the sysfs
> interface I think.
>
> Therefore, maybe it would be good to follow the "triggers" sysfs
> attribute pattern, where it lists the available LED triggers?
>
> The question is whether there is some mechanism available for
> notifying addition/removal of a USB port?
>

I think this should be easily doable through the notifier catching 
USB_BUS_[ADD,REMOVE].

Regards,
Matthias

> Also a description of the device connected to the port would be a nice
> feature, however I am not certain about the feasibility thereof.
>
>> The last big missing thing is Documentation update (this is why I'm
>> sending RFC). Greg pointed out we should have some entries in
>> Documentation/ABI, but it seems none of triggers have it. Any idea why
>> is that? Do we need to change it? Or is it required for new code only?
>> If so, should I care about Documentation/leds/ledtrig-usbport.txt at
>> all in this patch?
>>
>> For now I didn't update Documentation/leds/ledtrig-usbport.txt with the
>> new new_port and remove_port API, until I get a clue how to proceed.
>> ---
>>  Documentation/leds/ledtrig-usbport.txt |  49 ++++++
>>  drivers/leds/trigger/Kconfig           |   8 +
>>  drivers/leds/trigger/Makefile          |   1 +
>>  drivers/leds/trigger/ledtrig-usbport.c | 309
>> +++++++++++++++++++++++++++++++++
>>  4 files changed, 367 insertions(+)
>>  create mode 100644 Documentation/leds/ledtrig-usbport.txt
>>  create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
>>
>> diff --git a/Documentation/leds/ledtrig-usbport.txt
>> b/Documentation/leds/ledtrig-usbport.txt
>> new file mode 100644
>> index 0000000..fa42227
>> --- /dev/null
>> +++ b/Documentation/leds/ledtrig-usbport.txt
>> @@ -0,0 +1,49 @@
>> +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 the following sysfs attributes to the LED:
>> +
>> +  ports - Reading it lists all USB ports assigned to the trigger.
>> Writing USB
>> +      port number to it will make this driver start observing it.
>> It's also
>> +      possible to remove USB port from observable list by writing it
>> with a
>> +      "-" prefix.
>> +
>> +Example use-case:
>> +
>> +  echo usbport > trigger
>> +  echo 4-1 > ports
>> +  echo 2-1 > ports
>> +  echo -4-1 > ports
>> +  cat 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");
>>
>
>

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 25 August 2016 at 10:03, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
> On 08/24/2016 07:52 PM, Rafał Miłecki wrote:
>>
>> 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.
>>
>> 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.
>
>
> Now writing new_port with "1-1" produces a file named "1-1" in the
> ports directory with 000 permissions. I think that what Greg had
> on mind by referring to "one value per file" rule was a set of
> files representing ports, like "1-1 1-2 2-1", and each file should be
> readable/writeable.
>
> For instance "echo 1 > 1-1" would enable the trigger for the port 1-1
> and "echo 0 > 1-1" would disable it. The problem is that we don't know
> the number of required ports at compilation time and the sysfs
> attributes would have to be dynamically created on driver instantiation.
> What is more, as the USB ports can dynamically appear/disappear in the
> system, the files would have to be created/removed accordingly during
> LED class device lifetime, which is not the best design for the sysfs
> interface I think.
>
> Therefore, maybe it would be good to follow the "triggers" sysfs
> attribute pattern, where it lists the available LED triggers?
>
> The question is whether there is some mechanism available for
> notifying addition/removal of a USB port?

Every port is part of some hub and every hub (even root one) is a USB
device. So I could just get struct usb_device and check its "maxchild"
property. If e.g. I get USB device "1-1" with maxchild 4, I know there
are:
1-1.1
1-1.2
1-1.3
1-1.4

So the sysfs structure you suggested would be possible, I just don't
know if it's preferred one or not.

Confirmation: yes, right now I create simple files with chmod 000 for
every port added to the usbport observable list.


> Also a description of the device connected to the port would be a nice
> feature, however I am not certain about the feasibility thereof.

What kind of description do you mean? Where should it be used / where
should it appear?

-- 
Rafał

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 25 August 2016 at 10:03, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
> On 08/24/2016 07:52 PM, Rafał Miłecki wrote:
>>
>> 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.
>>
>> 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.
>
>
> Now writing new_port with "1-1" produces a file named "1-1" in the
> ports directory with 000 permissions. I think that what Greg had
> on mind by referring to "one value per file" rule was a set of
> files representing ports, like "1-1 1-2 2-1", and each file should be
> readable/writeable.

On the other hand, when I described my idea with "new_port" and
"remove_port", Greg replied with "Maybe", so I'm not sure if he really
got a design you described in his mind.

Greg: could you comment on this sysfs interface, please? :)

-- 
Rafał

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Jacek Anaszewski]

On 08/25/2016 10:29 AM, Rafał Miłecki wrote:
> On 25 August 2016 at 10:03, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
>> On 08/24/2016 07:52 PM, Rafał Miłecki wrote:
>>>
>>> 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.
>>>
>>> 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.
>>
>>
>> Now writing new_port with "1-1" produces a file named "1-1" in the
>> ports directory with 000 permissions. I think that what Greg had
>> on mind by referring to "one value per file" rule was a set of
>> files representing ports, like "1-1 1-2 2-1", and each file should be
>> readable/writeable.
>>
>> For instance "echo 1 > 1-1" would enable the trigger for the port 1-1
>> and "echo 0 > 1-1" would disable it. The problem is that we don't know
>> the number of required ports at compilation time and the sysfs
>> attributes would have to be dynamically created on driver instantiation.
>> What is more, as the USB ports can dynamically appear/disappear in the
>> system, the files would have to be created/removed accordingly during
>> LED class device lifetime, which is not the best design for the sysfs
>> interface I think.
>>
>> Therefore, maybe it would be good to follow the "triggers" sysfs
>> attribute pattern, where it lists the available LED triggers?
>>
>> The question is whether there is some mechanism available for
>> notifying addition/removal of a USB port?
>
> Every port is part of some hub and every hub (even root one) is a USB
> device. So I could just get struct usb_device and check its "maxchild"
> property. If e.g. I get USB device "1-1" with maxchild 4, I know there
> are:
> 1-1.1
> 1-1.2
> 1-1.3
> 1-1.4
>
> So the sysfs structure you suggested would be possible, I just don't
> know if it's preferred one or not.

It would require an ack from Greg.

I'd see it as follows:

#cat available_ports
#1-1 1-2 2-1

#echo "1-1" > new_port

#cat observed_ports
#1-1

#echo "2-1" > new_port

#cat observed_ports
#1-1 2-1

We've already had few discussions about the sysfs designs trying
to break the one-value-per-file rule for LED class device, and
there was always strong resistance against.

Cc Pavel.

> Confirmation: yes, right now I create simple files with chmod 000 for
> every port added to the usbport observable list.
>
>
>> Also a description of the device connected to the port would be a nice
>> feature, however I am not certain about the feasibility thereof.
>
> What kind of description do you mean? Where should it be used / where
> should it appear?
>

Product name/symbol. Actually it should be USB subsystem responsibility
to provide the means for querying the product name by port id, if it
is possible at all.

-- 
Best regards,
Jacek Anaszewski

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

[Greg KH]

On Thu, Aug 25, 2016 at 07:14:52AM +0200, Rafał Miłecki wrote:
> On 24 August 2016 at 23:04, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Aug 24, 2016 at 11:29:51AM +0200, Rafał Miłecki wrote:
> >> On 24 August 2016 at 11:22, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> > On Wed, Aug 24, 2016 at 12:03:29AM +0200, Rafał Miłecki wrote:
> >> >> +static ssize_t ports_show(struct device *dev, struct device_attribute *attr,
> >> >> +                       char *buf)
> >> >> +{
> >> >> +     struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >> >> +     struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
> >> >> +     struct usbport_trig_port *port;
> >> >> +     ssize_t ret = 0;
> >> >> +     int len;
> >> >> +
> >> >> +     list_for_each_entry(port, &usbport_data->ports, list) {
> >> >> +             len = sprintf(buf + ret, "%s\n", port->name);
> >> >> +             if (len >= 0)
> >> >> +                     ret += len;
> >> >> +     }
> >> >> +
> >> >> +     return ret;
> >> >> +}
> >> >
> >> > sysfs is "one value per file", here you are listing a bunch of things in
> >> > one sysfs file.  Please don't do that.
> >>
> >> OK. What do you think about creating "ports" subdirectory and creating
> >> file-per-port in it? Then I'd need to bring back something like
> >> "new_port" and "remove_port". Does it sound OK?
> >
> > Maybe, I don't know.  Why is "USB" somehow unique here?  Why isn't this
> > the same for PCI slots/ports?  pccard?  sdcard?  thunderbolt?
> 
> 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/.

Oh that's never going to work, sorry.  USB "activity" happens deep in
USB host controller hardware, not up at the kernel level at all.  It's
just too fast, and would take up too much CPU to do it otherwise.  Heck,
even old UHCI 1.1 USB controllers did this.

USB "activity" happens all the time, look at a USB bus analyzer if you
want to see what goes on.  Teasing out what is "real data" and what is
just "normal bus activity" is impossible at the kernel level, and really
makes no sense at all (your led would just be "on" all the time.)

Are you sure you know how USB works?  Writing things like this makes me
really worry...

greg k-h

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Alan Stern]

On Thu, 25 Aug 2016, Jacek Anaszewski wrote:

> I'd see it as follows:
> 
> #cat available_ports
> #1-1 1-2 2-1
> 
> #echo "1-1" > new_port
> 
> #cat observed_ports
> #1-1
> 
> #echo "2-1" > new_port
> 
> #cat observed_ports
> #1-1 2-1
> 
> We've already had few discussions about the sysfs designs trying
> to break the one-value-per-file rule for LED class device, and
> there was always strong resistance against.

This scheme has multiple values in both the available_ports and 
observed_ports files.  :-(  Not that I have any better suggestions...

> >> Also a description of the device connected to the port would be a nice
> >> feature, however I am not certain about the feasibility thereof.
> >
> > What kind of description do you mean? Where should it be used / where
> > should it appear?
> >
> 
> Product name/symbol. Actually it should be USB subsystem responsibility
> to provide the means for querying the product name by port id, if it
> is possible at all.

	cat /sys/bus/usb/devices/PORT/product
	cat /sys/bus/usb/devices/PORT/manufacturer

These will work if there is a device registered under PORT.

Alan Stern

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

[Bjørn Mork]

Greg KH <gregkh@linuxfoundation.org> writes:
> On Thu, Aug 25, 2016 at 07:14:52AM +0200, Rafał Miłecki 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/.
>
> Oh that's never going to work, sorry.  USB "activity" happens deep in
> USB host controller hardware, not up at the kernel level at all.  It's
> just too fast, and would take up too much CPU to do it otherwise.  Heck,
> even old UHCI 1.1 USB controllers did this.
>
> USB "activity" happens all the time, look at a USB bus analyzer if you
> want to see what goes on.  Teasing out what is "real data" and what is
> just "normal bus activity" is impossible at the kernel level,

OK, I am going to play stupid again: Does this mean that usbmon is
impossible?

> and really
> makes no sense at all (your led would just be "on" all the time.)

I don't see how this is any different from e.g. the network activity
triggers.

FWIW, on my system "cat /sys/kernel/debug/usb/usbmon/0u" can be silent
for long periods, and show a rush of lines whenever there is USB
activity.  I imagine that a LED triggered by a hook in a similar place
would be useful on systems with LEDs for that sort of stuff.


Bjørn

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Jacek Anaszewski]

On 08/25/2016 04:30 PM, Alan Stern wrote:
> On Thu, 25 Aug 2016, Jacek Anaszewski wrote:
>
>> I'd see it as follows:
>>
>> #cat available_ports
>> #1-1 1-2 2-1
>>
>> #echo "1-1" > new_port
>>
>> #cat observed_ports
>> #1-1
>>
>> #echo "2-1" > new_port
>>
>> #cat observed_ports
>> #1-1 2-1
>>
>> We've already had few discussions about the sysfs designs trying
>> to break the one-value-per-file rule for LED class device, and
>> there was always strong resistance against.
>
> This scheme has multiple values in both the available_ports and
> observed_ports files.  :-(  Not that I have any better suggestions...

Right, I forgot to add a note here, that this follows space
separated list pattern similarly as in case of triggers attribute.
Of course other suggestions are welcome.

>>>> Also a description of the device connected to the port would be a nice
>>>> feature, however I am not certain about the feasibility thereof.
>>>
>>> What kind of description do you mean? Where should it be used / where
>>> should it appear?
>>>
>>
>> Product name/symbol. Actually it should be USB subsystem responsibility
>> to provide the means for querying the product name by port id, if it
>> is possible at all.
>
> 	cat /sys/bus/usb/devices/PORT/product
> 	cat /sys/bus/usb/devices/PORT/manufacturer
>
> These will work if there is a device registered under PORT.

I've found only idProduct and idVendor files. They indeed uniquely
identify the device, but the numbers are not human readable.
Is there a way to retrieve the corresponding names in kernel?
Does the lsusb command do the mapping in the user space or maybe
it takes the names from kernel?

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Alan Stern]

On Thu, 25 Aug 2016, Jacek Anaszewski wrote:

> >>> What kind of description do you mean? Where should it be used / where
> >>> should it appear?
> >>>
> >>
> >> Product name/symbol. Actually it should be USB subsystem responsibility
> >> to provide the means for querying the product name by port id, if it
> >> is possible at all.
> >
> > 	cat /sys/bus/usb/devices/PORT/product
> > 	cat /sys/bus/usb/devices/PORT/manufacturer
> >
> > These will work if there is a device registered under PORT.
> 
> I've found only idProduct and idVendor files. They indeed uniquely
> identify the device, but the numbers are not human readable.
> Is there a way to retrieve the corresponding names in kernel?

If the device provides the string descriptors then the textual product
and manufacturer files are available in sysfs, otherwise they aren't.

> Does the lsusb command do the mapping in the user space or maybe
> it takes the names from kernel?

lsusb does the mapping in userspace, based on an ID database.  On my 
system (Fedora), the database is /etc/udev/hwdb.bin.

Alan Stern

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Alan Stern]

On Thu, 25 Aug 2016, Jacek Anaszewski wrote:

> >>> What kind of description do you mean? Where should it be used / where
> >>> should it appear?
> >>>
> >>
> >> Product name/symbol. Actually it should be USB subsystem responsibility
> >> to provide the means for querying the product name by port id, if it
> >> is possible at all.
> >
> > 	cat /sys/bus/usb/devices/PORT/product
> > 	cat /sys/bus/usb/devices/PORT/manufacturer
> >
> > These will work if there is a device registered under PORT.
> 
> I've found only idProduct and idVendor files. They indeed uniquely
> identify the device, but the numbers are not human readable.
> Is there a way to retrieve the corresponding names in kernel?

If the device provides the string descriptors then the textual product
and manufacturer files are available in sysfs, otherwise they aren't.

> Does the lsusb command do the mapping in the user space or maybe
> it takes the names from kernel?

lsusb does the mapping in userspace, based on an ID database.  On my 
system (Fedora), the database is /etc/udev/hwdb.bin.

Alan Stern

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Alan Stern]

On Thu, 25 Aug 2016, Alan Stern wrote:

> > Does the lsusb command do the mapping in the user space or maybe
> > it takes the names from kernel?
> 
> lsusb does the mapping in userspace, based on an ID database.  On my 
> system (Fedora), the database is /etc/udev/hwdb.bin.

There's also /usr/share/hwdata/usb.ids -- I don't know if or how the
two databases are related.  And there's /usr/lib/udev/hwdb.d/20-usb*.

Alan Stern

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Alan Stern]

On Thu, 25 Aug 2016, Alan Stern wrote:

> > Does the lsusb command do the mapping in the user space or maybe
> > it takes the names from kernel?
> 
> lsusb does the mapping in userspace, based on an ID database.  On my 
> system (Fedora), the database is /etc/udev/hwdb.bin.

There's also /usr/share/hwdata/usb.ids -- I don't know if or how the
two databases are related.  And there's /usr/lib/udev/hwdb.d/20-usb*.

Alan Stern

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

[Greg KH]

On Thu, Aug 25, 2016 at 08:39:24PM +0200, Bjørn Mork wrote:
> Greg KH <gregkh@linuxfoundation.org> writes:
> > On Thu, Aug 25, 2016 at 07:14:52AM +0200, Rafał Miłecki 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/.
> >
> > Oh that's never going to work, sorry.  USB "activity" happens deep in
> > USB host controller hardware, not up at the kernel level at all.  It's
> > just too fast, and would take up too much CPU to do it otherwise.  Heck,
> > even old UHCI 1.1 USB controllers did this.
> >
> > USB "activity" happens all the time, look at a USB bus analyzer if you
> > want to see what goes on.  Teasing out what is "real data" and what is
> > just "normal bus activity" is impossible at the kernel level,
> 
> OK, I am going to play stupid again: Does this mean that usbmon is
> impossible?

Hah, nice try :)

I guess when I think "USB activity" I think about the lower layers of
frames and acks and naks on the bus.  But maybe because I have just
recently been doing a lot of work down at the EHCI frame timing layer
for some strange hardware...

But you are right, if all you care about is the data that the host sees,
then yes, you could do something like usbmon to "tickle" a led if you
want to.

Odds are, someone's already done this for a raspberry pi :)

thanks,

greg k-h

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 25 August 2016 at 20:48, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> On 08/25/2016 04:30 PM, Alan Stern wrote:
>>
>> On Thu, 25 Aug 2016, Jacek Anaszewski wrote:
>>
>>> I'd see it as follows:
>>>
>>> #cat available_ports
>>> #1-1 1-2 2-1
>>>
>>> #echo "1-1" > new_port
>>>
>>> #cat observed_ports
>>> #1-1
>>>
>>> #echo "2-1" > new_port
>>>
>>> #cat observed_ports
>>> #1-1 2-1
>>>
>>> We've already had few discussions about the sysfs designs trying
>>> to break the one-value-per-file rule for LED class device, and
>>> there was always strong resistance against.
>>
>>
>> This scheme has multiple values in both the available_ports and
>> observed_ports files.  :-(  Not that I have any better suggestions...
>
>
> Right, I forgot to add a note here, that this follows space
> separated list pattern similarly as in case of triggers attribute.
> Of course other suggestions are welcome.

So ppl have doubts about multiple values in a single sysfs file
(whatever we call it: "ports" or "observed_ports"). Greg clearly said:
> sysfs is "one value per file", here you are listing a bunch of things in
> one sysfs file.  Please don't do that.

What about my idea of using "ports" subdirectory and having each port
as separated file inside that subdir? I think there are two ways of
doing this:

1) Having "ports" subdir with 0x0000 chmod files, one per each port
specified as observable
In this solution we need "new_port" and "remove_port" that can be used
for management of observable ports.
I think Jacek wasn't happy with this chmod and he believes Greg meant R/W files.

2) Having "ports" subdir with RW files, one per each existing physical port
In this situation we don't need "new_port" or "remove_port". If we
want port to be observable we just do:
echo 1 > 1-1
Implementing this solution needs reading more details from USB subsystem.

Do you find any of solutions with "ports" subdir better than dealing
with new-line/space separated values in a single sysfs file?

-- 
Rafał

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 25 August 2016 at 20:48, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> On 08/25/2016 04:30 PM, Alan Stern wrote:
>>
>> On Thu, 25 Aug 2016, Jacek Anaszewski wrote:
>>
>>> I'd see it as follows:
>>>
>>> #cat available_ports
>>> #1-1 1-2 2-1
>>>
>>> #echo "1-1" > new_port
>>>
>>> #cat observed_ports
>>> #1-1
>>>
>>> #echo "2-1" > new_port
>>>
>>> #cat observed_ports
>>> #1-1 2-1
>>>
>>> We've already had few discussions about the sysfs designs trying
>>> to break the one-value-per-file rule for LED class device, and
>>> there was always strong resistance against.
>>
>>
>> This scheme has multiple values in both the available_ports and
>> observed_ports files.  :-(  Not that I have any better suggestions...
>
>
> Right, I forgot to add a note here, that this follows space
> separated list pattern similarly as in case of triggers attribute.
> Of course other suggestions are welcome.

So ppl have doubts about multiple values in a single sysfs file
(whatever we call it: "ports" or "observed_ports"). Greg clearly said:
> sysfs is "one value per file", here you are listing a bunch of things in
> one sysfs file.  Please don't do that.

What about my idea of using "ports" subdirectory and having each port
as separated file inside that subdir? I think there are two ways of
doing this:

1) Having "ports" subdir with 0x0000 chmod files, one per each port
specified as observable
In this solution we need "new_port" and "remove_port" that can be used
for management of observable ports.
I think Jacek wasn't happy with this chmod and he believes Greg meant R/W files.

2) Having "ports" subdir with RW files, one per each existing physical port
In this situation we don't need "new_port" or "remove_port". If we
want port to be observable we just do:
echo 1 > 1-1
Implementing this solution needs reading more details from USB subsystem.

Do you find any of solutions with "ports" subdir better than dealing
with new-line/space separated values in a single sysfs file?

-- 
Rafał

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Pavel Machek]

On Thu 2016-08-25 11:04:38, Jacek Anaszewski wrote:
> On 08/25/2016 10:29 AM, Rafał Miłecki wrote:
> >On 25 August 2016 at 10:03, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
> >>On 08/24/2016 07:52 PM, Rafał Miłecki wrote:
> >>>
> >>>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.
> >>>
> >>>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.
> >>
> >>
> >>Now writing new_port with "1-1" produces a file named "1-1" in the
> >>ports directory with 000 permissions. I think that what Greg had
> >>on mind by referring to "one value per file" rule was a set of
> >>files representing ports, like "1-1 1-2 2-1", and each file should be
> >>readable/writeable.
> >>
> >>For instance "echo 1 > 1-1" would enable the trigger for the port 1-1
> >>and "echo 0 > 1-1" would disable it. The problem is that we don't know
> >>the number of required ports at compilation time and the sysfs
> >>attributes would have to be dynamically created on driver instantiation.
> >>What is more, as the USB ports can dynamically appear/disappear in the
> >>system, the files would have to be created/removed accordingly during
> >>LED class device lifetime, which is not the best design for the sysfs
> >>interface I think.

Could we add a flag to the USB port, instead? That way, USB code would
take care of creating the enable file in its own directory.

Is per-port control needed? Would just global control be sufficient?

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

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Pavel Machek]

On Thu 2016-08-25 20:48:04, Jacek Anaszewski wrote:
> On 08/25/2016 04:30 PM, Alan Stern wrote:
> >On Thu, 25 Aug 2016, Jacek Anaszewski wrote:
> >
> >>I'd see it as follows:
> >>
> >>#cat available_ports
> >>#1-1 1-2 2-1
> >>
> >>#echo "1-1" > new_port
> >>
> >>#cat observed_ports
> >>#1-1
> >>
> >>#echo "2-1" > new_port
> >>
> >>#cat observed_ports
> >>#1-1 2-1
> >>
> >>We've already had few discussions about the sysfs designs trying
> >>to break the one-value-per-file rule for LED class device, and
> >>there was always strong resistance against.
> >
> >This scheme has multiple values in both the available_ports and
> >observed_ports files.  :-(  Not that I have any better suggestions...
> 
> Right, I forgot to add a note here, that this follows space
> separated list pattern similarly as in case of triggers attribute.
> Of course other suggestions are welcome.
> 
> >>>>Also a description of the device connected to the port would be a nice
> >>>>feature, however I am not certain about the feasibility thereof.
> >>>
> >>>What kind of description do you mean? Where should it be used / where
> >>>should it appear?
> >>>
> >>
> >>Product name/symbol. Actually it should be USB subsystem responsibility
> >>to provide the means for querying the product name by port id, if it
> >>is possible at all.
> >
> >	cat /sys/bus/usb/devices/PORT/product
> >	cat /sys/bus/usb/devices/PORT/manufacturer
> >
> >These will work if there is a device registered under PORT.
> 
> I've found only idProduct and idVendor files. They indeed uniquely
> identify the device, but the numbers are not human readable.

Actually, they don't. They identify device _type_. If you have two
mice of the same type connected, they'll have same idProduct /
idVendor values.

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

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Pavel Machek]

On Thu 2016-08-25 20:48:04, Jacek Anaszewski wrote:
> On 08/25/2016 04:30 PM, Alan Stern wrote:
> >On Thu, 25 Aug 2016, Jacek Anaszewski wrote:
> >
> >>I'd see it as follows:
> >>
> >>#cat available_ports
> >>#1-1 1-2 2-1
> >>
> >>#echo "1-1" > new_port
> >>
> >>#cat observed_ports
> >>#1-1
> >>
> >>#echo "2-1" > new_port
> >>
> >>#cat observed_ports
> >>#1-1 2-1
> >>
> >>We've already had few discussions about the sysfs designs trying
> >>to break the one-value-per-file rule for LED class device, and
> >>there was always strong resistance against.
> >
> >This scheme has multiple values in both the available_ports and
> >observed_ports files.  :-(  Not that I have any better suggestions...
> 
> Right, I forgot to add a note here, that this follows space
> separated list pattern similarly as in case of triggers attribute.
> Of course other suggestions are welcome.
> 
> >>>>Also a description of the device connected to the port would be a nice
> >>>>feature, however I am not certain about the feasibility thereof.
> >>>
> >>>What kind of description do you mean? Where should it be used / where
> >>>should it appear?
> >>>
> >>
> >>Product name/symbol. Actually it should be USB subsystem responsibility
> >>to provide the means for querying the product name by port id, if it
> >>is possible at all.
> >
> >	cat /sys/bus/usb/devices/PORT/product
> >	cat /sys/bus/usb/devices/PORT/manufacturer
> >
> >These will work if there is a device registered under PORT.
> 
> I've found only idProduct and idVendor files. They indeed uniquely
> identify the device, but the numbers are not human readable.

Actually, they don't. They identify device _type_. If you have two
mice of the same type connected, they'll have same idProduct /
idVendor values.

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

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Jacek Anaszewski]

On 08/26/2016 05:58 PM, Rafał Miłecki wrote:
> On 25 August 2016 at 20:48, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>> On 08/25/2016 04:30 PM, Alan Stern wrote:
>>>
>>> On Thu, 25 Aug 2016, Jacek Anaszewski wrote:
>>>
>>>> I'd see it as follows:
>>>>
>>>> #cat available_ports
>>>> #1-1 1-2 2-1
>>>>
>>>> #echo "1-1" > new_port
>>>>
>>>> #cat observed_ports
>>>> #1-1
>>>>
>>>> #echo "2-1" > new_port
>>>>
>>>> #cat observed_ports
>>>> #1-1 2-1
>>>>
>>>> We've already had few discussions about the sysfs designs trying
>>>> to break the one-value-per-file rule for LED class device, and
>>>> there was always strong resistance against.
>>>
>>>
>>> This scheme has multiple values in both the available_ports and
>>> observed_ports files.  :-(  Not that I have any better suggestions...
>>
>>
>> Right, I forgot to add a note here, that this follows space
>> separated list pattern similarly as in case of triggers attribute.
>> Of course other suggestions are welcome.
>
> So ppl have doubts about multiple values in a single sysfs file
> (whatever we call it: "ports" or "observed_ports"). Greg clearly said:
>> sysfs is "one value per file", here you are listing a bunch of things in
>> one sysfs file.  Please don't do that.
>
> What about my idea of using "ports" subdirectory and having each port
> as separated file inside that subdir? I think there are two ways of
> doing this:
>
> 1) Having "ports" subdir with 0x0000 chmod files, one per each port
> specified as observable
> In this solution we need "new_port" and "remove_port" that can be used
> for management of observable ports.
> I think Jacek wasn't happy with this chmod and he believes Greg meant R/W files.

It looks odd to me. In this case it would also abuse "one value per
file" rule - the files would have no value, and only their names would
carry an information.

> 2) Having "ports" subdir with RW files, one per each existing physical port
> In this situation we don't need "new_port" or "remove_port". If we
> want port to be observable we just do:
> echo 1 > 1-1
> Implementing this solution needs reading more details from USB subsystem.

The situation here is clear IMO - the number of USB ports in the system
can change dynamically. I'm not sure if this can be handled easily with
sysfs, where we usually expose an interface for known set of settings.
struct attribute arrays are usually defined statically at the compile
time and filled with the variables, that are created with DEVICE_ATTR
macro.

> Do you find any of solutions with "ports" subdir better than dealing
> with new-line/space separated values in a single sysfs file?
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Pavel Machek]

Hi!

> >2) Having "ports" subdir with RW files, one per each existing physical port
> >In this situation we don't need "new_port" or "remove_port". If we
> >want port to be observable we just do:
> >echo 1 > 1-1
> >Implementing this solution needs reading more details from USB subsystem.
> 
> The situation here is clear IMO - the number of USB ports in the system
> can change dynamically. I'm not sure if this can be handled easily with
> sysfs, where we usually expose an interface for known set of settings.
> struct attribute arrays are usually defined statically at the compile
> time and filled with the variables, that are created with DEVICE_ATTR
> macro.

sysfs already exposes current view of all usb devices. Just use it.

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

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Pavel Machek]

Hi!

> >2) Having "ports" subdir with RW files, one per each existing physical port
> >In this situation we don't need "new_port" or "remove_port". If we
> >want port to be observable we just do:
> >echo 1 > 1-1
> >Implementing this solution needs reading more details from USB subsystem.
> 
> The situation here is clear IMO - the number of USB ports in the system
> can change dynamically. I'm not sure if this can be handled easily with
> sysfs, where we usually expose an interface for known set of settings.
> struct attribute arrays are usually defined statically at the compile
> time and filled with the variables, that are created with DEVICE_ATTR
> macro.

sysfs already exposes current view of all usb devices. Just use it.

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

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 29 August 2016 at 10:05, Pavel Machek <pavel@ucw.cz> wrote:
>> >2) Having "ports" subdir with RW files, one per each existing physical port
>> >In this situation we don't need "new_port" or "remove_port". If we
>> >want port to be observable we just do:
>> >echo 1 > 1-1
>> >Implementing this solution needs reading more details from USB subsystem.
>>
>> The situation here is clear IMO - the number of USB ports in the system
>> can change dynamically. I'm not sure if this can be handled easily with
>> sysfs, where we usually expose an interface for known set of settings.
>> struct attribute arrays are usually defined statically at the compile
>> time and filled with the variables, that are created with DEVICE_ATTR
>> macro.
>
> sysfs already exposes current view of all usb devices. Just use it.

We're talking about USB ports not devices, but this is still true. You
can find them in
/sys/bus/usb/devices/*/*-port*

I can't see how we could use them. How could I develop sysfs interface
in /sys/class/leds/*/ to allow userspace assigning USB ports to the
LED trigger?

-- 
Rafał

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 29 August 2016 at 10:05, Pavel Machek <pavel@ucw.cz> wrote:
>> >2) Having "ports" subdir with RW files, one per each existing physical port
>> >In this situation we don't need "new_port" or "remove_port". If we
>> >want port to be observable we just do:
>> >echo 1 > 1-1
>> >Implementing this solution needs reading more details from USB subsystem.
>>
>> The situation here is clear IMO - the number of USB ports in the system
>> can change dynamically. I'm not sure if this can be handled easily with
>> sysfs, where we usually expose an interface for known set of settings.
>> struct attribute arrays are usually defined statically at the compile
>> time and filled with the variables, that are created with DEVICE_ATTR
>> macro.
>
> sysfs already exposes current view of all usb devices. Just use it.

We're talking about USB ports not devices, but this is still true. You
can find them in
/sys/bus/usb/devices/*/*-port*

I can't see how we could use them. How could I develop sysfs interface
in /sys/class/leds/*/ to allow userspace assigning USB ports to the
LED trigger?

-- 
Rafał

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Jacek Anaszewski]

On 08/26/2016 09:50 PM, Pavel Machek wrote:
> On Thu 2016-08-25 20:48:04, Jacek Anaszewski wrote:
>> On 08/25/2016 04:30 PM, Alan Stern wrote:
>>> On Thu, 25 Aug 2016, Jacek Anaszewski wrote:
>>>
>>>> I'd see it as follows:
>>>>
>>>> #cat available_ports
>>>> #1-1 1-2 2-1
>>>>
>>>> #echo "1-1" > new_port
>>>>
>>>> #cat observed_ports
>>>> #1-1
>>>>
>>>> #echo "2-1" > new_port
>>>>
>>>> #cat observed_ports
>>>> #1-1 2-1
>>>>
>>>> We've already had few discussions about the sysfs designs trying
>>>> to break the one-value-per-file rule for LED class device, and
>>>> there was always strong resistance against.
>>>
>>> This scheme has multiple values in both the available_ports and
>>> observed_ports files.  :-(  Not that I have any better suggestions...
>>
>> Right, I forgot to add a note here, that this follows space
>> separated list pattern similarly as in case of triggers attribute.
>> Of course other suggestions are welcome.
>>
>>>>>> Also a description of the device connected to the port would be a nice
>>>>>> feature, however I am not certain about the feasibility thereof.
>>>>>
>>>>> What kind of description do you mean? Where should it be used / where
>>>>> should it appear?
>>>>>
>>>>
>>>> Product name/symbol. Actually it should be USB subsystem responsibility
>>>> to provide the means for querying the product name by port id, if it
>>>> is possible at all.
>>>
>>> 	cat /sys/bus/usb/devices/PORT/product
>>> 	cat /sys/bus/usb/devices/PORT/manufacturer
>>>
>>> These will work if there is a device registered under PORT.
>>
>> I've found only idProduct and idVendor files. They indeed uniquely
>> identify the device, but the numbers are not human readable.
>
> Actually, they don't. They identify device _type_. If you have two
> mice of the same type connected, they'll have same idProduct /
> idVendor values.

That's true. We'd have to be able to distinguish between the devices
of the same type. The only way seems to be the port id, whereas
the initial intention was to give a hint on what device is represented
by given port id :-)

Regardless of that, having a device name instead of port id would allow
for unique identification of a device in most of cases.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Pavel Machek]

On Mon 2016-08-29 10:21:48, Rafał Miłecki wrote:
> On 29 August 2016 at 10:05, Pavel Machek <pavel@ucw.cz> wrote:
> >> >2) Having "ports" subdir with RW files, one per each existing physical port
> >> >In this situation we don't need "new_port" or "remove_port". If we
> >> >want port to be observable we just do:
> >> >echo 1 > 1-1
> >> >Implementing this solution needs reading more details from USB subsystem.
> >>
> >> The situation here is clear IMO - the number of USB ports in the system
> >> can change dynamically. I'm not sure if this can be handled easily with
> >> sysfs, where we usually expose an interface for known set of settings.
> >> struct attribute arrays are usually defined statically at the compile
> >> time and filled with the variables, that are created with DEVICE_ATTR
> >> macro.
> >
> > sysfs already exposes current view of all usb devices. Just use it.
> 
> We're talking about USB ports not devices, but this is still true. You
> can find them in
> /sys/bus/usb/devices/*/*-port*
> 
> I can't see how we could use them. How could I develop sysfs interface
> in /sys/class/leds/*/ to allow userspace assigning USB ports to the
> LED trigger?

Create /sys/bus/usb/devices/*/*-port*/led_trigger file?

(Do you plan one USB trigger, or multiple ones?)
									Pavel

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

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Pavel Machek]

On Mon 2016-08-29 10:21:48, Rafał Miłecki wrote:
> On 29 August 2016 at 10:05, Pavel Machek <pavel@ucw.cz> wrote:
> >> >2) Having "ports" subdir with RW files, one per each existing physical port
> >> >In this situation we don't need "new_port" or "remove_port". If we
> >> >want port to be observable we just do:
> >> >echo 1 > 1-1
> >> >Implementing this solution needs reading more details from USB subsystem.
> >>
> >> The situation here is clear IMO - the number of USB ports in the system
> >> can change dynamically. I'm not sure if this can be handled easily with
> >> sysfs, where we usually expose an interface for known set of settings.
> >> struct attribute arrays are usually defined statically at the compile
> >> time and filled with the variables, that are created with DEVICE_ATTR
> >> macro.
> >
> > sysfs already exposes current view of all usb devices. Just use it.
> 
> We're talking about USB ports not devices, but this is still true. You
> can find them in
> /sys/bus/usb/devices/*/*-port*
> 
> I can't see how we could use them. How could I develop sysfs interface
> in /sys/class/leds/*/ to allow userspace assigning USB ports to the
> LED trigger?

Create /sys/bus/usb/devices/*/*-port*/led_trigger file?

(Do you plan one USB trigger, or multiple ones?)
									Pavel

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

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 29 August 2016 at 10:41, Pavel Machek <pavel@ucw.cz> wrote:
> On Mon 2016-08-29 10:21:48, Rafał Miłecki wrote:
>> On 29 August 2016 at 10:05, Pavel Machek <pavel@ucw.cz> wrote:
>> >> >2) Having "ports" subdir with RW files, one per each existing physical port
>> >> >In this situation we don't need "new_port" or "remove_port". If we
>> >> >want port to be observable we just do:
>> >> >echo 1 > 1-1
>> >> >Implementing this solution needs reading more details from USB subsystem.
>> >>
>> >> The situation here is clear IMO - the number of USB ports in the system
>> >> can change dynamically. I'm not sure if this can be handled easily with
>> >> sysfs, where we usually expose an interface for known set of settings.
>> >> struct attribute arrays are usually defined statically at the compile
>> >> time and filled with the variables, that are created with DEVICE_ATTR
>> >> macro.
>> >
>> > sysfs already exposes current view of all usb devices. Just use it.
>>
>> We're talking about USB ports not devices, but this is still true. You
>> can find them in
>> /sys/bus/usb/devices/*/*-port*
>>
>> I can't see how we could use them. How could I develop sysfs interface
>> in /sys/class/leds/*/ to allow userspace assigning USB ports to the
>> LED trigger?
>
> Create /sys/bus/usb/devices/*/*-port*/led_trigger file?
>
> (Do you plan one USB trigger, or multiple ones?)

I guess it means slightly more complex cross-subsystem interaction I
may need help to understand.

First of all: I will need multiple USB port triggers. It's because
there are many devices with multiple USB LEDs. Some complex (but
existing!) case:
USB 2.0 white LED - activated by USB 2.0 dev in USB 2.0 port
USB 3.0 white LED - activated by USB 2.0 dev in USB 3.0 port
USB 3.0 green LED - activated by USB 3.0 dev in USB 3.0 port
(This devices has separated EHCI and XHCI controllers, so we have 2
logical ports for 1 physical one)

So I guess I'll need something more complex like
/sys/bus/usb/devices/*/*-port*/bcm53xx:white:usb2_led_trigger
/sys/bus/usb/devices/*/*-port*/bcm53xx:white:usb3_led_trigger
/sys/bus/usb/devices/*/*-port*/bcm53xx:green:usb3_led_trigger
in case every USB led has "usbport" trigger assigned.

How can I create above sysfs files? Should I write code for this in
ledtrig-usbport.c? That would require accessing struct usb_hub and
then struct usb_port, but both of them are internal structs defined in
drivers/usb/core/hub.h. How could I work with that?

-- 
Rafał

Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Rafał Miłecki]

On 29 August 2016 at 10:41, Pavel Machek <pavel@ucw.cz> wrote:
> On Mon 2016-08-29 10:21:48, Rafał Miłecki wrote:
>> On 29 August 2016 at 10:05, Pavel Machek <pavel@ucw.cz> wrote:
>> >> >2) Having "ports" subdir with RW files, one per each existing physical port
>> >> >In this situation we don't need "new_port" or "remove_port". If we
>> >> >want port to be observable we just do:
>> >> >echo 1 > 1-1
>> >> >Implementing this solution needs reading more details from USB subsystem.
>> >>
>> >> The situation here is clear IMO - the number of USB ports in the system
>> >> can change dynamically. I'm not sure if this can be handled easily with
>> >> sysfs, where we usually expose an interface for known set of settings.
>> >> struct attribute arrays are usually defined statically at the compile
>> >> time and filled with the variables, that are created with DEVICE_ATTR
>> >> macro.
>> >
>> > sysfs already exposes current view of all usb devices. Just use it.
>>
>> We're talking about USB ports not devices, but this is still true. You
>> can find them in
>> /sys/bus/usb/devices/*/*-port*
>>
>> I can't see how we could use them. How could I develop sysfs interface
>> in /sys/class/leds/*/ to allow userspace assigning USB ports to the
>> LED trigger?
>
> Create /sys/bus/usb/devices/*/*-port*/led_trigger file?
>
> (Do you plan one USB trigger, or multiple ones?)

I guess it means slightly more complex cross-subsystem interaction I
may need help to understand.

First of all: I will need multiple USB port triggers. It's because
there are many devices with multiple USB LEDs. Some complex (but
existing!) case:
USB 2.0 white LED - activated by USB 2.0 dev in USB 2.0 port
USB 3.0 white LED - activated by USB 2.0 dev in USB 3.0 port
USB 3.0 green LED - activated by USB 3.0 dev in USB 3.0 port
(This devices has separated EHCI and XHCI controllers, so we have 2
logical ports for 1 physical one)

So I guess I'll need something more complex like
/sys/bus/usb/devices/*/*-port*/bcm53xx:white:usb2_led_trigger
/sys/bus/usb/devices/*/*-port*/bcm53xx:white:usb3_led_trigger
/sys/bus/usb/devices/*/*-port*/bcm53xx:green:usb3_led_trigger
in case every USB led has "usbport" trigger assigned.

How can I create above sysfs files? Should I write code for this in
ledtrig-usbport.c? That would require accessing struct usb_hub and
then struct usb_port, but both of them are internal structs defined in
drivers/usb/core/hub.h. How could I work with that?

-- 
Rafał