linux-kernel - Re: [PATCH V3] 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]

Message-ID: <6beb372b-cc66-0d7b-0111-19a1217f7833@suse.com>
Date:   Wed, 24 Aug 2016 13:27:50 +0200
From:   Matthias Brugger <mbrugger@...e.com>
To:     Rafał Miłecki <zajec5@...il.com>
Cc:     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:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:LED SUBSYSTEM" <linux-leds@...r.kernel.org>
Subject: Re: [PATCH V3] leds: trigger: Introduce an USB port trigger



On 24/08/16 13:02, Rafał Miłecki wrote:
> On 24 August 2016 at 12:49, Matthias Brugger <mbrugger@...e.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@...ecki@pl>");
>>
>>
>> Nit: rafal@...ecki.pl
>
> Oops, thanks!
>