On Fri, Apr 20, 2012 at 6:45 PM, Oliver Neukum <oneukum@xxxxxxx> wrote:
> Am Freitag, 20. April 2012, 12:17:51 schrieb Ming Lei:
>> On Fri, Apr 20, 2012 at 3:57 PM, Oliver Neukum <oneukum@xxxxxxx> wrote:
>> >
>> > 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.
>>
>> Yes, it is the race I missed, :-(
>>
>> > 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)
>>
>> Looks it is not easy to avoid the race if the lock is to be dropped.
>>
>> So how about not acquiring the lock during unlinking as below?
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index aa1c503..b437223 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -429,7 +429,8 @@ static void hid_irq_out(struct urb *urb)
>> urb->status);
>> }
>>
>> - spin_lock_irqsave(&usbhid->lock, flags);
>> + if (!test_cpu_bit(HID_PCPU_TIMEOUT, usbhid->cpuiofl))
>> + spin_lock_irqsave(&usbhid->lock, flags);
>>
>> if (unplug)
>> usbhid->outtail = usbhid->outhead;
>> @@ -438,12 +439,14 @@ static void hid_irq_out(struct urb *urb)
>>
>> if (usbhid->outhead != usbhid->outtail && !hid_submit_out(hid)) {
>> /* Successfully submitted next urb in queue */
>> - spin_unlock_irqrestore(&usbhid->lock, flags);
>> + if (!test_cpu_bit(HID_PCPU_TIMEOUT, usbhid->cpuiofl))
>> + spin_unlock_irqrestore(&usbhid->lock, flags);
>> return;
>> }
>
> No. This introduces a race where you can drop a lock you've not taken
> and vice versa.
The flag is defined as per cpu variable, so it can't changed during the two
test_cpu_bit in complete handler and can't cause the races you mentioned.
> And why are you using per cpu structures here?
If we want to introduce flag to address the issue, I think it has to be defined
per CPU because we can't acquire the lock in complete handler triggered
by unlink path, but at the same time we must spin on the lock in complete
handler from other CPUs(non unlinking, irq context) to avoid races.
IMO, the patch can fix the deadlock problem.
>
> To be blunt, I'd prefer a guarantee that allows usb_unlink_urb() to be
> called with spinlocks held.
I'd prefer it to, but looks it is not practical because quite a few
host controller
drivers need to be modified.
>
> Well, if we can't get a good usb_unlink_urb() then the lock must be dropped.
> Your idea of setting a flag is good.
>
> You'd do in hid_irq_out():
>
> if (usbhid->outhead != usbhid->outtail && !usbhid->timeout_detected && !hid_submit_out(hid))
>
> and you set usbhid->timeout_detected under lock. That way we can never
> accidentally kill a new urb. This however means that then we need to kill the
> urb and restart the queue preferably from a workqueue and this must be handled
> in suspend/resume/close/pre-/postreset/disconnect
It is a solution, but looks far more complicated than the per cpu flag patch.
Thanks,
--
Ming Lei
--
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
