Re: [PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Daniel Kurtz]

On Wed, Dec 14, 2011 at 7:22 PM, Oliver Neukum <oneukum@xxxxxxx> wrote:
> Am Mittwoch, 14. Dezember 2011, 11:33:03 schrieb Daniel Kurtz:
>> On Wed, Dec 14, 2011 at 4:01 PM, Oliver Neukum <oneukum@xxxxxxx> wrote:
>> > Am Donnerstag, 17. November 2011, 12:23:50 schrieb Daniel Kurtz:
>> >> Defer LED setting action to a workqueue.
>> >> This is more likely to send all LED change events in a single URB.
>> >
>> > Hi,
>> >
>> > I hope I am looking at the correct version of this patch.
>> > But as far as I can see the work for handling LEDs is not delayed
>> > while a reset is going on. That is wrong.
>>
>> Actually, I'm not sure why this is wrong.  I find the reset handling
>> quite confusing in this driver.
>> Can you point out what will fail, and recommend how to fix it?
>
> That is part of the workings of a USB driver. After you return from
> pre_reset() you don't do IO to the device. After post_reset() the
> device should be in its default state whose LED states won't match
> what they ought to be.

Hi Oliver,

I'm sorry, I'm not seeing how using a workqueue to set the LEDs
changes anything with respect to how the driver deals with USB reset.

With or without a workqueue, LED control urbs are being queued and/or
submitted asynchronously to reset.  AFAICT, the only thing that
changes here is exactly when __usbhid_submit_report() is called.  In
the original case, this happens directly in the input event handler.
In the workqueue case, it happens in a system worker thread some short
time after the input handler finishes.

Could you please be more specific about what this patch breaks and
perhaps give some guidance on how to fix it?

Thanks and Best Regards,
-Daniel

>
>        Regards
>                Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/