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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 30 Aug 2016 16:54:14 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Rafał Miłecki <zajec5@...il.com>
cc:     Greg KH <gregkh@...uxfoundation.org>,
        Richard Purdie <rpurdie@...ys.net>,
        Jacek Anaszewski <j.anaszewski@...sung.com>,
        Felipe Balbi <balbi@...nel.org>,
        Peter Chen <hzpeterchen@...il.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        Rafał Miłecki <rafal@...ecki.pl>,
        Jonathan Corbet <corbet@....net>,
        Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Stephan Linz <linz@...pro.net>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        "open list:LED SUBSYSTEM" <linux-leds@...r.kernel.org>
Subject: Re: [PATCH V4] leds: trigger: Introduce an USB port trigger

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

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

So for example, you might want to do:

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

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

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

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

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

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

Alan Stern