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

[RafaÅ MiÅecki]

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/
>> > > root@wrt1900acs:/sys/devices/platform/soc/soc:internal-regs/f1011000.i2c/i2c-0/0-0068/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
>> > > port->data:dd708140
>> > > root@wrt1900acs:/sys/devices/platform/soc/soc:internal-regs/f1011000.i2c/i2c-0/0-0068/leds/pca963x:shelby:white:usb2#
>> > > 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
>> > > 00000000
>> >
>> > 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
>> > with
>> > led_set_brightness_nosleep
>> > 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:
https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=89778ba335e302a450932ce5b703c1ee6216e949

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

-- 
RafaÅ