On Tue, Apr 24, 2012 at 10:22 PM, Oliver Neukum <oneukum@xxxxxxx> wrote:
> Am Dienstag, 24. April 2012, 06:19:00 schrieb Ming Lei:
>
>> @@ -486,11 +494,15 @@ static void hid_ctrl(struct urb *urb)
>>
>> if (usbhid->ctrlhead != usbhid->ctrltail && !hid_submit_ctrl(hid)) {
>> /* Successfully submitted next urb in queue */
>> + if (status != -ECONNRESET)
>> + spin_unlock(&usbhid->unlink_lock);
>> spin_unlock(&usbhid->lock);
>> return;
>> }
>>
>> clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
>> + if (status != -ECONNRESET)
>> + spin_unlock(&usbhid->unlink_lock);
>> spin_unlock(&usbhid->lock);
>> usb_autopm_put_interface_async(usbhid->intf);
>> wake_up(&usbhid->wait);
>
> Now you race against a double time out
>
> CPU A CPU B
>
> __usbhid_submit_report()
> time_after()
> usb_unlink_urb()
> -- this has to go to the hardware -->
> hid_irq_out()
> if (status != -ECONNRESET)
> --> no lock
> hid_submit_out()
> __usbhid_submit_report()
> time_after()
> usb_submit_urb()
This submit won't happen because HID_OUT_RUNNING is not cleared.
> usb_unlink_urb()
>
>
>> @@ -546,8 +558,13 @@ static void __usbhid_submit_report(struct
>> hid_device *hid, struct hid_report *re
>> * no race because this is called under
>> * spinlock
>> */
>> - if (time_after(jiffies, usbhid->last_out + HZ * 5))
>> + spin_lock(&usbhid->unlink_lock);
>> + if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
>> + spin_unlock(&usbhid->lock);
>> usb_unlink_urb(usbhid->urbout);
>> + spin_lock(&usbhid->lock);
>> + }
>> + spin_unlock(&usbhid->unlink_lock);
>
> AB-BA deadlock
OK, if we always acquire unlink_lock before lock in usbhid_submit_report,
hid_led, hid_ctrl, and hid_irq_out, the AB-BA deadlock can be removed.
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
