Re: [BUG 4.9] New led trigger usbport gets the kernel to panic

From: Jacek Anaszewski
Date: Thu Dec 08 2016 - 08:47:10 EST

On 12/08/2016 12:38 PM, RafaÅ MiÅecki wrote:
On 8 December 2016 at 09:40, Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> wrote:
On 12/06/2016 06:29 PM, RafaÅ MiÅecki wrote:

On 6 December 2016 at 18:26, Pavel Machek <pavel@xxxxxx> wrote:

On Fri 2016-12-02 09:48:18, Ralph Sennhauser wrote:

On Thu, 1 Dec 2016 17:56:07 +0100
RafaÅ MiÅecki <rafal@xxxxxxxxxx> wrote:

On 12/01/2016 03:28 PM, Ralph Sennhauser wrote:

Below the oops with your debug patch applied.


root@wrt1900acs:/# cd sys/class/leds/pca963x\:shelby\:white\:usb2/

echo usbport > trigger [ 124.727665] leds
pca963x:shelby:white:usb2: [usbport_trig_add_port] port:dd708240
[ 124.735266] leds pca963x:shelby:white:usb2:
[usbport_trig_add_port] port->port_name:usb1-port1
port->data:dd708140 [ 124.745671] leds pca963x:shelby:white:usb2:
[usbport_trig_add_port] port:dd7083c0 [ 124.753194] leds
pca963x:shelby:white:usb2: [usbport_trig_add_port]
port->port_name:usb2-port1 port->data:dd708140 [ 124.763594] leds
pca963x:shelby:white:usb2: [usbport_trig_add_port] port:dd708300
[ 124.771114] leds pca963x:shelby:white:usb2:
[usbport_trig_add_port] port->port_name:usb3-port1

echo 1 > ports/usb2-port1[ 171.649751] leds
pca963x:shelby:white:usb2: [usbport_trig_port_store] buf:1
[ 171.649751] size:2

[ 171.660160] leds pca963x:shelby:white:usb2:
[usbport_trig_port_store] port:dd7083c0 [ 171.668103] leds
pca963x:shelby:white:usb2: [usbport_trig_port_store]
port->port_name:usb2-port1 port->data:dd708140 [ 171.678678]
[usbport_trig_update_count] usbport_data->count:0 [ 171.684457]
[usbport_trig_update_count] usbport_data->count:0 [ 171.690253]
Unable to handle kernel NULL pointer dereference at virtual address

Oh, so this happens a bit later than I expected or I could read from
the backtrace. Anyway this debugging was still helpful, I think I can
see a possible problem.

So most likely the crash happens at the:
led_cdev->brightness_set(led_cdev, ...);
call. I'm not sure what I was thinking when writing that code. It
looks wrong.

The thing is some LEDs (drivers) don't provide brightness_set op.
It's fine, we should just use brightness_set_blocking op when that
happens. Of course kernel has proper helpers for that, we don't have
to worry about the choice of op or scheduling the work. I have no
idea why I didn't use a proper helper in the first place.

So we should simply replace above call with one of following ones:
1) led_set_brightness(led_cdev, ...);
2) led_set_brightness_nosleep(led_cdev, ...);
3) led_set_brightness_sync(led_cdev, ...);

I still have to check which one is correct. In theory we don't deal
blinking at this point so we shouldn't need to use led_set_brightness.

led_set_brightness_nosleep looks like the most likely correct choice.

led_set_brightness_sync requires brightness_set_blocking which is not
always present so most likely we don't want this one.

If you have some free time and you want to play with this, please
replace led_cdev->brightness_set
and give it a try. This should fix crashes for you.

I'll look at this again during next days.

Hi RafaÅ

I just gave 2) led_set_brightness_nosleep a try. The kernel no longer
crashes and the led does work as expected.

Do you have any patch that needs to be applied?

Yes, it has been pushed:

When sending it I didn't Cc linux-leds as didn't
pick it. Should we add MAINTAINERS entry for ledtrig-usbport.c maybe?

I haven't tested this trigger earlier, probably due to the fact that
it was targeted for usb subsystem, sorry about that. Nonetheless,
I've just tried it on exynos4412-trats2 board and it seems not to
work properly.

I have the board connected through USB (I have an opened ssh session),
I'm doing "echo usbport > trigger", then I can see the "ports"
directory, but it is empty.

Any ideas on the possible reasons? Exynos4412-trats2 uses
Ethernet Gadget (with CDC Ethernet support) USB Gadget Driver
for USB networking.

If board has any USB ports you shouldn't have empty directory. What about:
ls /sys/bus/usb/devices/*/*-port* | grep ":"

As Greg noticed my device in this configuration is not a USB host, but
gadget, and its /sys/bus/usb/devices/ dir is empty, so it was false

Best regards,
Jacek Anaszewski