On Tue, 22 May 2018, Sebastian Andrzej Siewior wrote:
> On 2018-05-07 11:37:29 [-0400], Alan Stern wrote:
> > > As far as I understand it there should be no deadlock. Without the
> > > local_irq_save() things should not deadlock because the HCD invokes USB
> > > driver's (usb-storage for instance) ->complete callback always in the
> > > same way. If you mix the usb-driver with two different HCDs (say ehci
> > > and xhci) then lockdep would complain. However, the locks which were
> > > allocated by usb-storage for the ehci driver would never be used in the
> > > context of the xhci driver. So lockdep would report a deacklock but
> > > there wouldn't be one (again, if I got the piece right).
> >
> > That argument would be correct if the completion routines were the only
> > places where the higher-level drivers (such as usb-storage or usbhid)
> > acquired their locks. But we can't depend on this being true; you
> > would have to audit each USB driver to make sure.
>
> an entry point for IRQ usage outside of the driver would be the usage of
> hrtimer.
Sorry, I don't understand that sentence at all. And I don't see how it
could be relevant to the point I was trying to make.
Consider, for example, drivers/hid/usbhid/hid-core.c. In that file,
hid_io_error() is called by hid_irq_in(), which is an URB completion
handler. hid_io_error() acquires usbhid_lock. Therefore it would be
necessary to audit the usbhid driver to see whether interrupts are
enabled or disabled any place where usbhid_lock is acquired. And in
fact, hid_ctrl() (another completion handler) calls
spin_lock(&usbhid->lock) -- this could cause problems for you. And
usbhid->lock is acquired in other places, ones that are not inside
completion handlers.
None of this has anything to do with IRQ usage or hrtimers.
> We have a flag to let the hrtimer run in softirq but yes, we
> need to audit them.
>
> > > And I was thinking about converting all drivers to one model and then we
> > > could get rid of the block I quoted above.
> > >
> > > If nobody rejects the approach as such I would go and start hacking…
> > >
> > > > And even for those two, the conversion will not be easy. Simply
> > > > changing the giveback routines would violate the documented guarantees
> > > > for isochronous transfers.
> > >
> > > The requirement was that the ISO urb is completed before the BULK urb,
> > > right?
> >
> > No, that's not what I meant. For one thing, isochronous URBs don't
> > need to complete before bulk URBs in general (although they do have
> > higher priority).
> >
> > However, I was really referring to the kerneldoc for usb_submit_urb().
> > The part that talks about scheduling of isochronous and interrupt URBs
> > lists a bunch of requirements. In order to meet these requirements
> > some of the host controller drivers may rely on the fact that when they
> > give back an URB, the URB's completion routine will return before the
> > giveback call finishes.
>
> You mean the "Reserved Bandwidth Transfers:" paragraph?
Paragraphs (plural). Three paragraphs, to be precise.
> > It's possible to rewrite the HCDs not to rely on this (I did exactly
> > that for ehci-hcd), but it is a nontrivial job.
>
> are you referring to commit 9118f9eb4f1e ("USB: EHCI: improve interrupt
> qh unlink")?
That one, plus:
46c73d1d3ebc ("USB: EHCI: handle isochronous underruns with tasklets")
e4e18cbd52c8 ("USB: EHCI: code rearrangement in iso_stream_schedule()")
24f531371de1 ("USB: EHCI: accept very late isochronous URBs")
35371e4fbc3e ("USB: EHCI: improve ehci_endpoint_disable")
Not all parts of all those commits were relevant, but as far as I
recall, they each contributed something. And I may have omitted
one or two commits by mistake.
Alan Stern
--
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
