From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758972AbcHYIPC (ORCPT ); Thu, 25 Aug 2016 04:15:02 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:62867 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757031AbcHYIOr (ORCPT ); Thu, 25 Aug 2016 04:14:47 -0400 MIME-version: 1.0 Content-type: text/plain; charset=utf-8; format=flowed X-AuditID: cbfec7f4-f79cb6d000001359-8d-57bea6595696 Subject: Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger To: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Richard Purdie , Felipe Balbi , Greg KH References: <20160823220404.9887-1-zajec5@gmail.com> <20160824175345.7033-1-zajec5@gmail.com> Cc: Peter Chen , linux-usb@vger.kernel.org, =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Jonathan Corbet , Ezequiel Garcia , Stephan Linz , Matthias Brugger , Boris Brezillon , Geert Uytterhoeven , "open list:DOCUMENTATION" , open list , "open list:LED SUBSYSTEM" From: Jacek Anaszewski Message-id: Date: Thu, 25 Aug 2016 10:03:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 In-reply-to: <20160824175345.7033-1-zajec5@gmail.com> Content-transfer-encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrJIsWRmVeSWpSXmKPExsVy+t/xq7qRy/aFG7Tt5LQ41vaE3eLAi4Us Fk8OtDNabHzxmcXi2a29TBbNi9ezWXRd3cRmsbBtCYvF5V1z2Cy2vlnHaLFoWSuzxdXXi1gs Fkx+wmqxe+0iJovdu56yWqw5meog4PFk00VGj52z7rJ7bFrVyeax+22qx6HDHYwe++euYfdY 3DeZ1eP8VE+PPfN/sHqs33KVxaPryHU2j8+b5AJ4orhsUlJzMstSi/TtErgydt78xFawqari 9sFDLA2MU5O6GDk5JARMJJa8esUGYYtJXLi3Hsjm4hASWMoocWDXdhaQBK+AoMSPyffAbGYB M4kvLw+zQhQ9Y5RouvwOrFtYwEvi5L1FTCAJEYFljBKnV5xn72LkAKqKl3iyrhCieTaLxLfZ ciA2m4ChxM8Xr5kgFthJnL+0mR3EZhFQlfg85wAjiC0qECFxa9VHMJtTwFSi8c03qCPkJQ5e ec4ygVFgFpL7ZiG5bxaSsgWMzKsYRVNLkwuKk9JzDfWKE3OLS/PS9ZLzczcxQiLwyw7Gxces DjEKcDAq8fDuWLU3XIg1say4MvcQowQHs5IIb8WMfeFCvCmJlVWpRfnxRaU5qcWHGKU5WJTE eefueh8iJJCeWJKanZpakFoEk2Xi4JRqYNQ9KSv7au9Zxuwp+2YHP3WN5zdJkd7xJvtV/1sp hY23xBKC50y7O697wnvpvkUKTkZ2Xxo9utK2s/jpr54s4tp5vbBRhbfj5gnVqXbr5d66ZCWr iYSFvyxw8TheGtS1lVPpyFSfX4+eiOYEeba9rJm2biGHorv9mwYhTssbtgKm3xW2WSQ4KbEU ZyQaajEXFScCAEi+DNu8AgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org zOn 08/24/2016 07:52 PM, Rafał Miłecki wrote: > From: Rafał Miłecki > > 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 > --- > 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 > + * > + * 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 > +#include > +#include > +#include > +#include > +#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 "); > +MODULE_DESCRIPTION("USB port trigger"); > +MODULE_LICENSE("GPL"); > -- Best regards, Jacek Anaszewski