Re: [PATCH v3 3/4] USB: EHCI: improve interrupt qh unlink

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jul 1, 2013 at 10:44 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, 1 Jul 2013, Ming Lei wrote:
>
>> >> Currently, URB might be probably submitted to HCD too even after
>> >> usb_hcd_flush_endpoint() completes since both accesses to  dev->ep_in[epnum]
>> >> and ep->enabled aren't protected by effective locks.
>> >
>> > The urb_list_lock in hcd.c serves to synchronize changes to
>> > ep->enabled.  An URB might be submitted after usb_hcd_flush_endpoint()
>> > completes, but the submission will fail in usb_hcd_link_urb_to_ep().
>>
>> But the lock isn't held when setting and clearing ep->enabled in
>> usb_enable_endpoint and usb_disable_endpoint, is it?
>
> No.  But it is held in usb_hcd_flush_endpoint() and
> usb_hcd_link_urb_to_ep().  Therefore, if a thread clears ep->enabled
> and then flushes the endpoint, further submissions will fail.

I am wondering if holding the lock in usb_hcd_flush_endpoint() can
avoid the race completely. Suppose usb_hcd_link_urb_to_ep() in submit
path runs on CPU1 just when usb_hcd_flush_endpoint()(called from
usb_disable_endpoint()) completes on CPU0, there is no any guarantee
that CPU1 can see the up-to-date value of ep->enabled, so the urb might
be submitted to HCD successfully.

>
> However, this does still leave one possible race: On the first
> submission to an isochronous endpoint, itd_submit() and sitd_submit()
> will allocate a new ehci_iso_stream before calling
> usb_hcd_link_urb_to_ep().  If the endpoint has been flushed and
> disabled, the enqueue will fail but the new iso_stream will not be
> destroyed -- it will leak.  But this is an old failure mode, not
> related to the new changes.
>
>> >> So how about not adding the WARN_ON(!list_empty(&qh->qtd_list)
>> >> now when qh->qh_state is QH_STATE_LINKED?
>> >
>> > I think we should add the WARN_ON.  Or jump to the default case in that
>> > switch statement -- maybe the error message there should include a
>> > WARN_ON.
>>
>> How about just adding WARN_ON(!list_empty(&qh->qtd_list) but
>> continue unlinking the qh like current code, which should be
>> harmless and avoid the QH leak?
>
> I think the end result would be the same.  It wouldn't hurt and it
> wouldn't help.  We'd still end up at the "default" case.

It may help to avoid one QH leak, even though the problem is caused
by usbcore, so I plan to do that if you have no objection.

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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux