Re: [PATCH v4 3/3] usb: Add LED triggers for USB activity
On Wed, Aug 27 2014, Greg Kroah-Hartman wrote:
> On Wed, Aug 27, 2014 at 10:58:00PM +0200, Michal Sojka wrote:
>> With this patch, USB activity can be signaled by blinking a LED. There
>> are two triggers, one for activity on USB host and one for USB gadget.
>>
>> Both trigger should work with all host/device controllers. Tested only
>> with musb.
>>
>> Signed-off-by: Michal Sojka
>> ---
>> drivers/usb/Kconfig | 11
>> drivers/usb/common/Makefile | 1 +
>> drivers/usb/common/led.c | 56
>> +++
>> drivers/usb/core/hcd.c| 2 ++
>> drivers/usb/gadget/udc/udc-core.c | 4 +++
>> include/linux/usb.h | 12 +
>> 6 files changed, 86 insertions(+)
>> create mode 100644 drivers/usb/common/led.c
>>
>> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
>> index e0cad441..fc90cda 100644
>> --- a/drivers/usb/Kconfig
>> +++ b/drivers/usb/Kconfig
>> @@ -147,4 +147,15 @@ source "drivers/usb/phy/Kconfig"
>>
>> source "drivers/usb/gadget/Kconfig"
>>
>> +config USB_LED_TRIG
>> +bool "USB LED Triggers"
>> +depends on LEDS_CLASS && USB_COMMON
>> +select LEDS_TRIGGERS
>
> I hate select lines, can't we just depend on LEDS_TRIGGERS as well as
> LEDS_CLASS?
>
>
>> +help
>> + This option adds LED triggers for USB host and/or gadget activity.
>> +
>> + Say Y here if you are working on a system with led-class supported
>> + LEDs and you want to use them as activity indicators for USB host or
>> + gadget.
>> +
>> endif # USB_SUPPORT
>> diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
>> index 052c120..ca2f8bd 100644
>> --- a/drivers/usb/common/Makefile
>> +++ b/drivers/usb/common/Makefile
>> @@ -4,5 +4,6 @@
>>
>> obj-$(CONFIG_USB_COMMON) += usb-common.o
>> usb-common-y += common.o
>> +usb-common-$(CONFIG_USB_LED_TRIG) += led.o
>>
>> obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
>> diff --git a/drivers/usb/common/led.c b/drivers/usb/common/led.c
>> new file mode 100644
>> index 000..af3ce2c
>> --- /dev/null
>> +++ b/drivers/usb/common/led.c
>> @@ -0,0 +1,56 @@
>> +/*
>> + * LED Triggers for USB Activity
>> + *
>> + * Copyright 2014 Michal Sojka
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#define BLINK_DELAY 30
>> +
>> +static unsigned long usb_blink_delay = BLINK_DELAY;
>> +
>> +DEFINE_LED_TRIGGER(ledtrig_usb_gadget);
>> +DEFINE_LED_TRIGGER(ledtrig_usb_host);
>> +
>> +void usb_led_activity(enum usb_led_event ev)
>> +{
>> +struct led_trigger *trig = NULL;
>> +
>> +switch (ev) {
>> +case USB_LED_EVENT_GADGET: trig = ledtrig_usb_gadget; break;
>> +case USB_LED_EVENT_HOST: trig = ledtrig_usb_host;break;
>> +}
>
> Very odd formatting, please use the correct one and don't try to put
> case expressions all on one line.
>
>> +led_trigger_blink_oneshot(trig, &usb_blink_delay, &usb_blink_delay, 0);
>
> What happens if trig is NULL?
This will work correctly - I added a comment about it in v5.
>> +}
>> +EXPORT_SYMBOL(usb_led_activity);
>
> EXPORT_SYMBOL_GPL() please.
>
>> +static int __init ledtrig_usb_init(void)
>> +{
>> +#ifdef CONFIG_USB_GADGET
>> +led_trigger_register_simple("usb-gadget", &ledtrig_usb_gadget);
>> +#endif
>> +#ifdef CONFIG_USB
>> +led_trigger_register_simple("usb-host", &ledtrig_usb_host);
>> +#endif
>
> Why not just always register both?
I didn't want to offer users a trigger that doesn't do anything useful
on their system. Since you are the second person suggesting registering
both, I did just that in v5.
>> +return 0;
>> +}
>> +
>> +static void __exit ledtrig_usb_exit(void)
>> +{
>> +led_trigger_unregister_simple(ledtrig_usb_gadget);
>> +led_trigger_unregister_simple(ledtrig_usb_host);
>
> So you can unregister things that you never registered with no issues?
Yes, but it doesn't matter anymore in v5.
>> +}
>> +
>> +module_init(ledtrig_usb_init);
>> +module_exit(ledtrig_usb_exit);
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 487abcf..409cb95 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -1664,6 +1664,8 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>> usbmon_urb_complete(&hcd->self, urb, status);
>> usb_anchor_suspend_wakeups(anchor);
>> usb_unanchor_urb(urb);
>> +if (likely(status == 0))
>> +usb_led_activity(USB_LED_EVENT_HOST);
>
> That's a _really_ hot code path, how much is this going to cause in
> overhead?
I measured the overheads on ARM Cortex-A8 (TI AM335x) running on 600 MHz.
Duration of usb_led_activity():
- with no LED attached to the trigger:2 ± 1 µs
- with one GPIO LED attached
Re: [PATCH v4 3/3] usb: Add LED triggers for USB activity
On Wed, Aug 27, 2014 at 10:58:00PM +0200, Michal Sojka wrote:
> With this patch, USB activity can be signaled by blinking a LED. There
> are two triggers, one for activity on USB host and one for USB gadget.
>
> Both trigger should work with all host/device controllers. Tested only
> with musb.
>
> Signed-off-by: Michal Sojka
> ---
> drivers/usb/Kconfig | 11
> drivers/usb/common/Makefile | 1 +
> drivers/usb/common/led.c | 56
> +++
> drivers/usb/core/hcd.c| 2 ++
> drivers/usb/gadget/udc/udc-core.c | 4 +++
> include/linux/usb.h | 12 +
> 6 files changed, 86 insertions(+)
> create mode 100644 drivers/usb/common/led.c
>
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index e0cad441..fc90cda 100644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> @@ -147,4 +147,15 @@ source "drivers/usb/phy/Kconfig"
>
> source "drivers/usb/gadget/Kconfig"
>
> +config USB_LED_TRIG
> + bool "USB LED Triggers"
> + depends on LEDS_CLASS && USB_COMMON
> + select LEDS_TRIGGERS
I hate select lines, can't we just depend on LEDS_TRIGGERS as well as
LEDS_CLASS?
> + help
> + This option adds LED triggers for USB host and/or gadget activity.
> +
> + Say Y here if you are working on a system with led-class supported
> + LEDs and you want to use them as activity indicators for USB host or
> + gadget.
> +
> endif # USB_SUPPORT
> diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
> index 052c120..ca2f8bd 100644
> --- a/drivers/usb/common/Makefile
> +++ b/drivers/usb/common/Makefile
> @@ -4,5 +4,6 @@
>
> obj-$(CONFIG_USB_COMMON) += usb-common.o
> usb-common-y += common.o
> +usb-common-$(CONFIG_USB_LED_TRIG) += led.o
>
> obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
> diff --git a/drivers/usb/common/led.c b/drivers/usb/common/led.c
> new file mode 100644
> index 000..af3ce2c
> --- /dev/null
> +++ b/drivers/usb/common/led.c
> @@ -0,0 +1,56 @@
> +/*
> + * LED Triggers for USB Activity
> + *
> + * Copyright 2014 Michal Sojka
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define BLINK_DELAY 30
> +
> +static unsigned long usb_blink_delay = BLINK_DELAY;
> +
> +DEFINE_LED_TRIGGER(ledtrig_usb_gadget);
> +DEFINE_LED_TRIGGER(ledtrig_usb_host);
> +
> +void usb_led_activity(enum usb_led_event ev)
> +{
> + struct led_trigger *trig = NULL;
> +
> + switch (ev) {
> + case USB_LED_EVENT_GADGET: trig = ledtrig_usb_gadget; break;
> + case USB_LED_EVENT_HOST: trig = ledtrig_usb_host;break;
> + }
Very odd formatting, please use the correct one and don't try to put
case expressions all on one line.
> + led_trigger_blink_oneshot(trig, &usb_blink_delay, &usb_blink_delay, 0);
What happens if trig is NULL?
> +}
> +EXPORT_SYMBOL(usb_led_activity);
EXPORT_SYMBOL_GPL() please.
> +static int __init ledtrig_usb_init(void)
> +{
> +#ifdef CONFIG_USB_GADGET
> + led_trigger_register_simple("usb-gadget", &ledtrig_usb_gadget);
> +#endif
> +#ifdef CONFIG_USB
> + led_trigger_register_simple("usb-host", &ledtrig_usb_host);
> +#endif
Why not just always register both?
> + return 0;
> +}
> +
> +static void __exit ledtrig_usb_exit(void)
> +{
> + led_trigger_unregister_simple(ledtrig_usb_gadget);
> + led_trigger_unregister_simple(ledtrig_usb_host);
So you can unregister things that you never registered with no issues?
> +}
> +
> +module_init(ledtrig_usb_init);
> +module_exit(ledtrig_usb_exit);
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 487abcf..409cb95 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1664,6 +1664,8 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
> usbmon_urb_complete(&hcd->self, urb, status);
> usb_anchor_suspend_wakeups(anchor);
> usb_unanchor_urb(urb);
> + if (likely(status == 0))
> + usb_led_activity(USB_LED_EVENT_HOST);
That's a _really_ hot code path, how much is this going to cause in
overhead?
> /* pass ownership to the completion handler */
> urb->status = status;
> diff --git a/drivers/usb/gadget/udc/udc-core.c
> b/drivers/usb/gadget/udc/udc-core.c
> index c1df19b..9fc4a36 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -27,6 +27,7 @@
>
> #include
> #include
> +#include
>
> /**
> * struct usb_udc - describes one usb device controller
> @@ -116,6 +117,9 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request);
> void usb_gadget_giveback_request(struct usb_ep *ep,
> struct usb_request *req)
> {
> + if (likely(req->status == 0))
> + usb_
[PATCH v4 3/3] usb: Add LED triggers for USB activity
With this patch, USB activity can be signaled by blinking a LED. There
are two triggers, one for activity on USB host and one for USB gadget.
Both trigger should work with all host/device controllers. Tested only
with musb.
Signed-off-by: Michal Sojka
---
drivers/usb/Kconfig | 11
drivers/usb/common/Makefile | 1 +
drivers/usb/common/led.c | 56 +++
drivers/usb/core/hcd.c| 2 ++
drivers/usb/gadget/udc/udc-core.c | 4 +++
include/linux/usb.h | 12 +
6 files changed, 86 insertions(+)
create mode 100644 drivers/usb/common/led.c
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index e0cad441..fc90cda 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -147,4 +147,15 @@ source "drivers/usb/phy/Kconfig"
source "drivers/usb/gadget/Kconfig"
+config USB_LED_TRIG
+ bool "USB LED Triggers"
+ depends on LEDS_CLASS && USB_COMMON
+ select LEDS_TRIGGERS
+ help
+ This option adds LED triggers for USB host and/or gadget activity.
+
+ Say Y here if you are working on a system with led-class supported
+ LEDs and you want to use them as activity indicators for USB host or
+ gadget.
+
endif # USB_SUPPORT
diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
index 052c120..ca2f8bd 100644
--- a/drivers/usb/common/Makefile
+++ b/drivers/usb/common/Makefile
@@ -4,5 +4,6 @@
obj-$(CONFIG_USB_COMMON) += usb-common.o
usb-common-y += common.o
+usb-common-$(CONFIG_USB_LED_TRIG) += led.o
obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
diff --git a/drivers/usb/common/led.c b/drivers/usb/common/led.c
new file mode 100644
index 000..af3ce2c
--- /dev/null
+++ b/drivers/usb/common/led.c
@@ -0,0 +1,56 @@
+/*
+ * LED Triggers for USB Activity
+ *
+ * Copyright 2014 Michal Sojka
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#define BLINK_DELAY 30
+
+static unsigned long usb_blink_delay = BLINK_DELAY;
+
+DEFINE_LED_TRIGGER(ledtrig_usb_gadget);
+DEFINE_LED_TRIGGER(ledtrig_usb_host);
+
+void usb_led_activity(enum usb_led_event ev)
+{
+ struct led_trigger *trig = NULL;
+
+ switch (ev) {
+ case USB_LED_EVENT_GADGET: trig = ledtrig_usb_gadget; break;
+ case USB_LED_EVENT_HOST: trig = ledtrig_usb_host;break;
+ }
+ led_trigger_blink_oneshot(trig, &usb_blink_delay, &usb_blink_delay, 0);
+}
+EXPORT_SYMBOL(usb_led_activity);
+
+
+static int __init ledtrig_usb_init(void)
+{
+#ifdef CONFIG_USB_GADGET
+ led_trigger_register_simple("usb-gadget", &ledtrig_usb_gadget);
+#endif
+#ifdef CONFIG_USB
+ led_trigger_register_simple("usb-host", &ledtrig_usb_host);
+#endif
+ return 0;
+}
+
+static void __exit ledtrig_usb_exit(void)
+{
+ led_trigger_unregister_simple(ledtrig_usb_gadget);
+ led_trigger_unregister_simple(ledtrig_usb_host);
+}
+
+module_init(ledtrig_usb_init);
+module_exit(ledtrig_usb_exit);
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 487abcf..409cb95 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1664,6 +1664,8 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
usbmon_urb_complete(&hcd->self, urb, status);
usb_anchor_suspend_wakeups(anchor);
usb_unanchor_urb(urb);
+ if (likely(status == 0))
+ usb_led_activity(USB_LED_EVENT_HOST);
/* pass ownership to the completion handler */
urb->status = status;
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index c1df19b..9fc4a36 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -27,6 +27,7 @@
#include
#include
+#include
/**
* struct usb_udc - describes one usb device controller
@@ -116,6 +117,9 @@ EXPORT_SYMBOL_GPL(usb_gadget_unmap_request);
void usb_gadget_giveback_request(struct usb_ep *ep,
struct usb_request *req)
{
+ if (likely(req->status == 0))
+ usb_led_activity(USB_LED_EVENT_GADGET);
+
BUG_ON(req->complete == NULL);
req->complete(ep, req);
}
diff --git a/include/linux/usb.h b/include/linux/usb.h
index d2465bc..447a7e2 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1862,6 +1862,18 @@ extern void usb_unregister_notify(struct notifier_block
*nb);
/* debugfs stuff */
extern struct dentry *usb_debug_root;
+/* LED triggers */
+enum usb_led_event {
+ USB_LED_EVENT_HOST = 0,
+ USB_LED_EVENT_GADGET = 1,
+};
+
+#ifdef CONFIG_USB_LED_TRIG
+extern void usb_led_activity(enum usb_led_event ev);
+#else
+static inline void usb_led_activity(enum usb_led_event ev) {}
+#endif
+
#endif /* __KERNEL__ */
#endif
--
2.1.0
