On Wed, Apr 25, 2012 at 4:06 PM, Oliver Neukum <oneukum@xxxxxxx> wrote:
>
> Indeed. Upon further thought it seems to me that we need to prevent
> resubmission and all is well. And we almost have all the infrastructure.
>
> So how about this, making it more reusable:
Yes, this one is good, better than the adding lock.
>
> From ce77d793252d1abf3a0ec6c67e4a7a05ac6d846a Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oliver@xxxxxxxxxx>
> Date: Wed, 25 Apr 2012 09:54:00 +0200
> Subject: [PATCH] usbhid: prevent deadlock during timeout
>
> On some HCDs usb_unlink_urb() can directly call the
> completion handler. That limits the spinlocks that can
> be taken in the handler to locks not held while calling
> usb_unlink_urb()
> To prevent a race with resubmission, this patch exposes
> usbcore's infrastructure for blocking submission, uses it
> and so drops the lock without causing a race in usbhid.
>
> Signed-off-by: Oliver Neukum <oneukum@xxxxxxx>
> ---
> drivers/hid/usbhid/hid-core.c | 38 ++++++++++++++++++++++++++++++++++----
> drivers/usb/core/urb.c | 30 ++++++++++++++++++++++++++++++
> include/linux/usb.h | 2 ++
> 3 files changed, 66 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 5bf91db..763257d 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -535,11 +535,27 @@ 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
> + * no race because the URB is blocked under
> * spinlock
> */
> - if (time_after(jiffies, usbhid->last_out + HZ * 5))
> + if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
> + usb_block_urb(usbhid->urbout);
> + /* drop lock to not deadlock if the callback is called */
> + spin_unlock(&usbhid->lock);
> usb_unlink_urb(usbhid->urbout);
> + spin_lock(&usbhid->lock);
> + usb_unblock_urb(usbhid->urbout);
> + /*
> + * if the unlinking has already completed
> + * the pump will have been stopped
> + * it must be restarted now
> + */
> + if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
> + if (!hid_submit_out(hid))
You need to add check of "usbhid->outtail != usbhid->outhead" above.
> + set_bit(HID_OUT_RUNNING, &usbhid->iofl);
> +
> +
> + }
> }
> return;
> }
> @@ -583,11 +599,25 @@ 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
> + * no race because the URB is blocked under
> * spinlock
> */
> - if (time_after(jiffies, usbhid->last_ctrl + HZ * 5))
> + if (time_after(jiffies, usbhid->last_ctrl + HZ * 5)) {
> + usb_block_urb(usbhid->urbctrl);
> + /* drop lock to not deadlock if the callback is called */
> + spin_unlock(&usbhid->lock);
> usb_unlink_urb(usbhid->urbctrl);
> + spin_lock(&usbhid->lock);
> + usb_unblock_urb(usbhid->urbctrl);
> + /*
> + * if the unlinking has already completed
> + * the pump will have been stopped
> + * it must be restarted now
> + */
> + if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl))
> + if (!hid_submit_ctrl(hid))
Similar as above.
> + set_bit(HID_CTRL_RUNNING, &usbhid->iofl);
> + }
> }
> }
>
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index cd9b3a2..1d1b010 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -681,6 +681,36 @@ void usb_unpoison_urb(struct urb *urb)
> EXPORT_SYMBOL_GPL(usb_unpoison_urb);
>
> /**
> + * usb_block_urb - reliably prevent further use of an URB
> + * @urb: pointer to URB to be blocked, may be NULL
> + *
> + * After the routine has run, attempts to resubmit the URB will fail
> + * with error -EPERM. Thus even if the URB's completion handler always
> + * tries to resubmit, it will not succeed and the URB will become idle.
> + *
> + * The URB must not be deallocated while this routine is running. In
> + * particular, when a driver calls this routine, it must insure that the
> + * completion handler cannot deallocate the URB.
> + */
> +void usb_block_urb(struct urb *urb)
> +{
> + if (!urb)
> + return;
> +
> + atomic_inc(&urb->reject);
> +}
> +EXPORT_SYMBOL_GPL(usb_block_urb);
> +
> +void usb_unblock_urb(struct urb *urb)
> +{
> + if (!urb)
> + return;
> +
> + atomic_dec(&urb->reject);
> +}
> +EXPORT_SYMBOL_GPL(usb_unblock_urb);
> +
> +/**
> * usb_kill_anchored_urbs - cancel transfer requests en masse
> * @anchor: anchor the requests are bound to
> *
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 73b68d1..23df8ae 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1379,6 +1379,8 @@ extern int usb_unlink_urb(struct urb *urb);
> extern void usb_kill_urb(struct urb *urb);
> extern void usb_poison_urb(struct urb *urb);
> extern void usb_unpoison_urb(struct urb *urb);
> +extern void usb_block_urb(struct urb *urb);
> +extern void usb_unblock_urb(struct urb *urb);
> extern void usb_kill_anchored_urbs(struct usb_anchor *anchor);
> extern void usb_poison_anchored_urbs(struct usb_anchor *anchor);
> extern void usb_unpoison_anchored_urbs(struct usb_anchor *anchor);
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
