Subject: Re: [BUG 4.9] New led trigger usbport gets the kernel to panic
To: Ralph Sennhauser <ralph.sennhauser@gmail.com>
References: <20161121094022.30478a37@gmail.com>
 <43ea90fb-780a-3612-77dd-265e877ac18e@milecki.pl>
 <20161201152825.3b2686db@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>
From: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>
Message-ID: <b7fb9a03-a7aa-97e9-0923-de11b71c1951@milecki.pl>
Date: Thu, 1 Dec 2016 17:56:07 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <20161201152825.3b2686db@gmail.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3043
Lines: 57

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.