Am Freitag, 20. April 2012, 04:10:09 schrieb Ming Lei:
> On Fri, Apr 20, 2012 at 12:11 AM, Oliver Neukum <oneukum@xxxxxxx> 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: <stable@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxxxxx>
> >> ---
> >> 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
Indeed.
> 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.
But under spinlock.
> So I don't see any races can be introduced by the patch.
You are racing with hid_irq_out(). It calls hid_submit_out()
under lock. So if hid_irq_out() is running between dropping
the lock and usb_unlink_urb() you may kill the newly submitted
urb, not the old urb that has timed out.
You must make sure that between the times you check usbhid->last_out
and calling unlink hid_submit_out() cannot be called.
You can't just drop the lock (at least on SMP)
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
