On Mon, 7 May 2018, Sebastian Andrzej Siewior wrote:
> On 2018-05-04 16:07:22 [-0400], Alan Stern wrote:
> > On Fri, 4 May 2018, Sebastian Andrzej Siewior wrote:
> >
> > > Hi Alan,
> > >
> > > I posted a RFC [0] to let the HCD complete the URB in threaded-IRQ
> > > context to avoid the softirq/tasklet. Mauro reported that this does not
> > > help him and never came back after that.
> > > You did not oppose the approach as long as there is no performance issue
> > > which I did not see. Would you mind if I revisit the approach and
> > > convert all HCDs to threaded IRQs while at it?
> >
> > I don't mind revisiting the approach, although it certainly would be
> > good to find out why it doesn't help Mauro's video problem.
> >
> > However, converting _all_ the HCDs to threaded IRQs is a different
> > story. It should be okay to convert the ones that currently use
> > tasklets, but I can't approve changes to all the others. Only the
> > drivers that I maintain (ohci-hcd and uhci-hcd).
>
> Sure. The plan was to convert all drivers to one model so we could get
> rid of:
>
> |static void __usb_hcd_giveback_urb(struct urb *urb)
> |{
> |…
> | /*
> | * We disable local IRQs here avoid possible deadlock because
> | * drivers may call spin_lock() to hold lock which might be
> | * acquired in one hard interrupt handler.
> | *
> | * The local_irq_save()/local_irq_restore() around complete()
> | * will be removed if current USB drivers have been cleaned up
> | * and no one may trigger the above deadlock situation when
> | * running complete() in tasklet.
> | */
> | local_irq_save(flags);
> | urb->complete(urb);
> | local_irq_restore(flags);
> |…
> |}
>
> 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.
> 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.
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.
Alan Stern
> If so, the last patch would enqueue the ISO urb and wait until the BULK
> urb was completed before it completed the ISO urb.
>
> > Alan Stern
>
> Sebastian
--
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
