On Fri, Apr 20, 2012 at 12:11 AM, Oliver Neukum <[email protected]> wrote: > Am Donnerstag, 19. April 2012, 15:51:04 schrieb Ming Lei: >> The URB complete handler may be called by usb_unlink_urb directly, >> so deadlock will be triggered in __usbhid_submit_report since >> usbhid->lock is to be acquired in ctrl/out URB complete handler >> but it is hold before calling usb_unlink_urb. >> >> This patch avoids the deadlock by releasing the lock before >> calling usb_unlink_urb. >> >> CC: <[email protected]> >> Signed-off-by: Ming Lei <[email protected]> >> --- >> drivers/hid/usbhid/hid-core.c | 16 ++++++++++------ >> 1 file changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c >> index aa1c503..b5d07da 100644 >> --- a/drivers/hid/usbhid/hid-core.c >> +++ b/drivers/hid/usbhid/hid-core.c >> @@ -543,11 +543,13 @@ static void __usbhid_submit_report(struct hid_device >> *hid, struct hid_report *re >> * the queue is known to run >> * but an earlier request may be stuck >> * we may need to time out >> - * no race because this is called under >> - * spinlock >> + * release spinlock to avoid deadlock. >> */ >> - if (time_after(jiffies, usbhid->last_out + HZ * 5)) >> + if (time_after(jiffies, usbhid->last_out + HZ * 5)) { >> + spin_unlock(&usbhid->lock); >> usb_unlink_urb(usbhid->urbout); >> + spin_lock(&usbhid->lock); > > The problem indeed exists on some HCDs. > I am afraid if you drop the lock there you introduce a race whereby > you might unlink the wrong request.
The complete handler is called just one time per one submit in either irq path or unlink path. Secondly, usb_unlink_urb itself is race free. Finally, usb_unlink_urb was always the last function called inside __usbhid_submit_report. So I don't see any races can be introduced by the patch. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
