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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3883 bytes --]

On Fri 2016-12-02 09:48:18, Ralph Sennhauser wrote:
> On Thu, 1 Dec 2016 17:56:07 +0100
> Rafał Miłecki <rafal@milecki.pl> 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?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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@ucw.cz> 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@milecki.pl> 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ł

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

[Jacek Anaszewski]

Hi Rafał,

On 12/06/2016 06:29 PM, Rafał Miłecki wrote:
> On 6 December 2016 at 18:26, Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org> 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-g1n6cQUeyibVItvQsEIGlw@public.gmane.org> 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?
>

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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Greg Kroah-Hartman]

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 <pavel@ucw.cz> 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@milecki.pl> 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?
> > 
> 
> 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.

This trigger is for when your device is a USB host, not a gadget, so I
don't think it works on your hardware :)

thanks,

greg k-h

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

[Rafał Miłecki]

On 8 December 2016 at 09:40, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
> On 12/06/2016 06:29 PM, Rafał Miłecki wrote:
>>
>> On 6 December 2016 at 18:26, Pavel Machek <pavel@ucw.cz> 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@milecki.pl> 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?
>>
>
> 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

[Jacek Anaszewski]

On 12/08/2016 12:38 PM, Rafał Miłecki wrote:
> On 8 December 2016 at 09:40, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
>> On 12/06/2016 06:29 PM, Rafał Miłecki wrote:
>>>
>>> On 6 December 2016 at 18:26, Pavel Machek <pavel@ucw.cz> 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@milecki.pl> 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?
>>>
>>
>> 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