Re: [BUG 4.9] New led trigger usbport gets the kernel to panic
On 12/08/2016 12:38 PM, Rafał Miłecki wrote: On 8 December 2016 at 09:40, Jacek Anaszewski wrote: On 12/06/2016 06:29 PM, Rafał Miłecki wrote: 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 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? 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 alarm. -- Best regards, Jacek Anaszewski
Re: [BUG 4.9] New led trigger usbport gets the kernel to panic
On 8 December 2016 at 09:40, Jacek Anaszewski wrote: > On 12/06/2016 06:29 PM, Rafał Miłecki wrote: >> >> 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 >> > > > 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? >> > > 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 ":" -- Rafał
Re: [BUG 4.9] New led trigger usbport gets the kernel to panic
On Thu, Dec 08, 2016 at 09:40:43AM +0100, Jacek Anaszewski wrote: > Hi Rafał, > > On 12/06/2016 06:29 PM, Rafał Miłecki wrote: > > 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 > > > > > > > > > > > > > > > > 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? > > > > 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
Re: [BUG 4.9] New led trigger usbport gets the kernel to panic
Hi Rafał, On 12/06/2016 06:29 PM, Rafał Miłecki wrote: 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 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? 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. -- Best regards, Jacek Anaszewski
Re: [BUG 4.9] New led trigger usbport gets the kernel to panic
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 >> > > >> > >> > 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ł
Re: [BUG 4.9] New led trigger usbport gets the kernel to panic
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 > > > > > > > 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? -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [BUG 4.9] New led trigger usbport gets the kernel to panic
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 > > > > 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. Thanks Ralph
Re: [BUG 4.9] New led trigger usbport gets the kernel to panic
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 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.
Re: [BUG 4.9] New led trigger usbport gets the kernel to panic
On Mon, 28 Nov 2016 23:20:13 +0100 Rafal Milecki wrote: > Hi Ralph, > > On 11/21/2016 09:40 AM, Ralph Sennhauser wrote: > > I tried your new usbport trigger in Linux 4.9 with little luck as > > can be seen in the following output of the serial console. > > I'm really happy my trigger got some attention. I'm sorry it crashes > for you, I never experienced any crash so far. I also got few ppl > using it as part of LEDE project but no reports from them neither. > > Also sorry for the late reply, my private life took all my time > previous week. > > Hi Rafal no worries, I know well how it is not to have enough time. Below the oops with your debug patch applied. Thanks Ralph 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 [ 171.698382] pgd = de71c000 [ 171.701102] [] *pgd=1e362831, *pte=, *ppte= [ 171.707427] Internal error: Oops: 8007 [#1] SMP ARM [ 171.712671] Modules linked in: iptable_nat nft_chain_nat_ipv4 nf_tables_inet nf_nat_ipv4 nf_conntrack_ipv6 nf_conntrack_ipv4 ipt_REJECT ipt_MASQUERADE xt_time xt_tcpudp xt_state xt_nat xt_multiport xt_mark xt_mac xt_limit xt_conntrack xt_comment xt_TCP MSS xt_REDIRECT xt_LOG xt_CT nft_set_rbtree nft_set_hash nft_reject_inet nft_reject nft_redir_ipv4 nft_redir nft_nat nft_meta nft_masq_ipv4 nft_masq nft_log nft_limit nft_hash nft_exthdr nft_ct nft_counter nft_chain_route_ipv6 nft_chain_route_ipv4 nf_tabl es_ipv6 nf_tables_ipv4 nf_tables nf_reject_ipv4 nf_nat_redirect nf_nat_masquerade_ipv4 nf_nat nf_log_ipv4 nf_defrag_ipv6 nf_defrag_ipv4 nf_conntrack_netlink nf_conntrack macvlan libcrc32c iptable_raw iptable_mangle iptable_filter ip_tables act_skbedit act _mirred em_u32 cls_u32 cls_tcindex cls_flow cls_route cls_fw sch_hfsc sch_ingress mwlwifi(O) mac80211 cfg80211 ledtrig_netdev(O) ip_set_list_set ip_set_hash_netiface ip_set_hash_netport ip_set_hash_netnet ip_set_hash_net ip_set_hash_netportnet ip_set_hash _mac ip_set_hash_ipportnet ip_set_hash_ipportip ip_set_hash_ipport ip_set_hash_ipmark ip_set_hash_ip ip_set_bitmap_port ip_set_bitmap_ipmac ip_set_bitmap_ip ip_set nfnetlink ip6t_REJECT nf_reject_ipv6 nf_log_ipv6 nf_log_common ip6table_raw ip6table_mangle ip6table_filter ip6_tables x_tables ifb sit tunnel4 ip_tunnel vfat fat nls_utf8 nls_iso8859_1 nls_cp850 nls_cp437 rfkill input_core sha256_generic seqiv jitterentropy_rng drbg hmac ghash_generic gf128mul gcm ctr ccm ledtrig_usbport ext4 jbd2 mbcache btrf s xor raid6_pq crc32c_generic ubifs ubi [ 171.851028] CPU: 1 PID: 724 Comm: ash Tainted: G O4.9.0-rc7 #2 [ 171.858103] Hardware name: Marvell Armada 380/385 (Device Tree) [ 171.864046] task: df71bc80 task.stack: df7d [ 171.868594] PC is at 0x0 [ 171.871139] LR is at usbport_trig_port_store+0x10c/0x17c [ledtrig_usbport] [ 171.878042] pc : [<>]lr : []psr: 6013 [ 171.878042] sp : df7d1e48 ip : 0007 fp : df7d1e6c [ 171.889566] r10: r9 : r8 : [ 171.894811] r7 : df63c4ec r6 : deeec700 r5 : dd708140 r4 : 0002 [ 171.901363] r3 : r2 : r1 : r0 : df63c4ec [ 171.907916] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none [ 171.915079] Control: 10c5387d Table: 1e71c04a DAC: 0051 [ 171.920847] Process ash (pid: 724, stack limit = 0xdf7d0210) [ 171.926528] Stack: (0xdf7d1e48 to 0xdf7d2000) [ 171.930902] 1e40: dd708140 ddc7a840 bf228164 0002 df7d1f80 dd708500 [ 171.939114] 1e60: df7d1e84 df7d1e70 c0237e00 bf228170 c0237de0 0002 df7d1
Re: [BUG 4.9] New led trigger usbport gets the kernel to panic
Hi Ralph, On 11/21/2016 09:40 AM, Ralph Sennhauser wrote: I tried your new usbport trigger in Linux 4.9 with little luck as can be seen in the following output of the serial console. I'm really happy my trigger got some attention. I'm sorry it crashes for you, I never experienced any crash so far. I also got few ppl using it as part of LEDE project but no reports from them neither. Also sorry for the late reply, my private life took all my time previous week. 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 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 [ 1461.761528] Unable to handle kernel NULL pointer dereference at virtual address [ 1461.769734] pgd = de33c000 [ 1461.772454] [] *pgd=1cb2a831, *pte=, *ppte= [ 1461.778791] Internal error: Oops: 8007 [#1] SMP ARM [ 1461.784036] Modules linked in: iptable_nat nft_chain_nat_ipv4 nf_tables_inet nf_nat_ipv4 nf_conntrack_ipv6 nf_conntrack_ipv4 ipt_REJECT ipt_MASQUERADE xt_time xt_tcpudp xt_state xt_nat xt_multiport xt_mark xt_mac xt_limit xt_conntrack xt_comment xt_TCP MSS xt_REDIRECT xt_LOG xt_CT nft_set_rbtree nft_set_hash nft_reject_inet nft_reject nft_redir_ipv4 nft_redir nft_nat nft_meta nft_masq_ipv4 nft_masq nft_log nft_limit nft_hash nft_exthdr nft_ct nft_counter nft_chain_route_ipv6 nft_chain_route_ipv4 nf_tabl es_ipv6 nf_tables_ipv4 nf_tables nf_reject_ipv4 nf_nat_redirect nf_nat_masquerade_ipv4 nf_nat nf_log_ipv4 nf_defrag_ipv6 nf_defrag_ipv4 nf_conntrack_netlink nf_conntrack macvlan libcrc32c iptable_raw iptable_mangle iptable_filter ip_tables act_skbedit act _mirred em_u32 cls_u32 cls_tcindex cls_flow cls_route cls_fw sch_hfsc sch_ingress mwlwifi(O) mac80211 cfg80211 ledtrig_netdev(O) ip_set_list_set ip_set_hash_netiface ip_set_hash_netport ip_set_hash_netnet ip_set_hash_net ip_set_hash_netportnet ip_set_hash _mac ip_set_hash_ipportnet ip_set_hash_ipportip ip_set_hash_ipport ip_set_hash_ipmark ip_set_hash_ip ip_set_bitmap_port ip_set_bitmap_ipmac ip_set_bitmap_ip ip_set nfnetlink ip6t_REJECT nf_reject_ipv6 nf_log_ipv6 nf_log_common ip6table_raw ip6table_mangle ip6table_filter ip6_tables x_tables ifb sit tunnel4 ip_tunnel vfat fat nls_utf8 nls_iso8859_1 nls_cp850 nls_cp437 rfkill input_core sha256_generic seqiv jitterentropy_rng drbg hmac ghash_generic gf128mul gcm ctr ccm ledtrig_usbport ext4 jbd2 mbcache btrf s xor raid6_pq crc32c_generic ubifs ubi [ 1461.922401] CPU: 0 PID: 759 Comm: ash Tainted: G O4.9.0-rc6 #2 [ 1461.929476] Hardware name: Marvell Armada 380/385 (Device Tree) [ 1461.935418] task: df621080 task.stack: dee0e000 [ 1461.939966] PC is at 0x0 [ 1461.942510] LR is at usbport_trig_port_store+0x8c/0xd8 [ledtrig_usbport] [ 1461.949238] pc : [<>]lr : []psr: 6013 [ 1461.949238] sp : dee0fe50 ip : dee0fe10 fp : dee0fe6c [ 1461.960761] r10: r9 : r8 : [ 1461.966006] r7 : dcabc7c0 r6 : df4164ec r5 : 0002 r4 : dcabc040 [ 1461.972558] r3 : r2 : 001a r1 : r0 : df4164ec [ 1461.979112] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none [ 1461.986275] Control: 10c5387d Table: 1e33c04a DAC: 0051 [ 1461.992043] Process ash (pid: 759, stack limit = 0xdee0e210) [ 1461.997724] Stack: (0xdee0fe50 to 0xdee1) [ 1462.002097] fe40: bf228164 0002 dee0ff80 dcabc7c0 [ 1462.010309] fe60: dee0fe84 dee0fe70 c023f758 bf228170 c023f738 0002 dee0fe9c dee0fe88 [ 1462.018520] fe80: c0169310 c023f744 df4a1000 0002 dee0fedc dee0fea0 c0168ab8 c01692d4 [ 1462.026731] fea0: df747c10 df4a100c d4aca1c9 df727a80 c01689c0 dee0ff80 [ 1462.034943] fec0: 0002 c000fc24 dee0e000 dee0ff4c dee0fee0 c0100c78 c01689cc [ 1462.043153] fee0: dee0fee8 dee0ff14 dee0fef8 dee0ff14 dee0ff00 c0060668 c04640a0 [ 1462.051365] ff00: dee0ff50 df621350 dee0ff4c dee0ff18 c00298f8 c0060630 fff6 dee0ff68 [ 1462.059576] ff20: c00ff23c 0007 0002 df727a80 b6f092d0 dee0ff80 c000fc24 dee0e000 [ 1462.067786] ff40: dee0ff7c dee0ff50 c0101ae4 c0100c50 0003 0007 df727a80 df727a80 [ 1462.075997] ff60: b6f092d0 0002 c000fc24 dee0e000 dee0ffa4 dee0ff80 c01028dc c0101a44 [ 1462.084208] ff80: 0004 dee0ffa8 [ 1462.092419] ffa0: c000fa60 c010289c 0001 b6f092d0 0002 [ 1462.100630] ffc0: 0004 b6f092d0 0020 00059ec0 00059ea0 [ 1462.108841] ffe0: beb744f8 beb744e4 b6ee1904 b6ee0dc8 6010 0001 [ 1462.117049] B
