Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754076AbcLFR3T (ORCPT ); Tue, 6 Dec 2016 12:29:19 -0500 Received: from mail-oi0-f67.google.com ([209.85.218.67]:35134 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753982AbcLFR3P (ORCPT ); Tue, 6 Dec 2016 12:29:15 -0500 MIME-Version: 1.0 In-Reply-To: <20161206172619.GB4406@amd> References: <20161121094022.30478a37@gmail.com> <43ea90fb-780a-3612-77dd-265e877ac18e@milecki.pl> <20161201152825.3b2686db@gmail.com> <20161202094818.6b9c1ed2@gmail.com> <20161206172619.GB4406@amd> From: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= Date: Tue, 6 Dec 2016 18:29:13 +0100 Message-ID: Subject: Re: [BUG 4.9] New led trigger usbport gets the kernel to panic To: Pavel Machek Cc: Ralph Sennhauser , "open list:LED SUBSYSTEM" , Jacek Anaszewski , Richard Purdie , =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Greg Kroah-Hartman , "linux-usb@vger.kernel.org" , Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uB6HTUYx013776 Content-Length: 4092 Lines: 90 On 6 December 2016 at 18:26, Pavel Machek wrote: > On Fri 2016-12-02 09:48:18, Ralph Sennhauser wrote: >> On Thu, 1 Dec 2016 17:56:07 +0100 >> Rafał Miłecki 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ł