On Tue, 29 Nov 2016, Felipe Balbi wrote:
> Hi Greg,
>
> Greg KH <greg@xxxxxxxxx> writes:
> > On Tue, Nov 29, 2016 at 03:58:57PM +0200, Felipe Balbi wrote:
> >>
> >> Hi,
> >>
> >> Greg KH <greg@xxxxxxxxx> writes:
> >> > On Tue, Nov 29, 2016 at 01:48:59PM +0200, Felipe Balbi wrote:
> >> >>
> >> >> Hi folks,
> >> >>
> >> >> I've been staring at this part of the code for a while. To make things
> >> >> easier here's the part of XHCI I mean:
> >> >>
> >> >> > irqreturn_t xhci_irq(struct usb_hcd *hcd)
> >> >> > {
> >> >> > struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> >> >> > union xhci_trb *event_ring_deq;
> >> >> > irqreturn_t ret = IRQ_NONE;
> >> >> > dma_addr_t deq;
> >> >> > u64 temp_64;
> >> >> > u32 status;
> >> >> >
> >> >> > spin_lock(&xhci->lock);
> >> >>
> >> >> considering this is running in hardirq context, this is lock *really*
> >> >> necessary? I mean, could this race with khubd or something else?
> >> >
> >> > It could be a threaded irq, right?
> >>
> >> except that it isn't :-) Should we handle threaded_irq when we get to it?
> >
> > How do you know it isn't? Doesn't the -rt patchset turn all of these
> > irqs into threaded irqs automatically without the driver knowing that or
> > not?
>
> Yes, all IRQs are turned into threaded. You can also do the same with
> threadirqs CMDLINE option. The catch here is that both -RT and
> threadirqs option force IRQF_ONESHOT which will keep IRQs disabled until
> the thread runs, so there shouldn't be any possible races either :-)
>
> > The driver shouldn't really care either way.
>
> agreed :-)
>
> >> > Not that this really matters, it's hard to figure out what this lock
> >> > actually "protects", it seems like "everything for this hcd", which
> >> > seems like a lot, but was probably safest/easiest to get up and running,
> >> > and mirrors what ehci does from what I can tell.
> >>
> >> The problem is that now locking in xhci is rather convoluted and it
> >> became really difficult to track its usage. I don't mind having a "lock
> >> everything from this controller" lock, but currently the usage is
> >> difficult to track because the lock is apparently sprinkled all over the
> >> place willy-nilly.
> >>
> >> It lacks proper __releases()/__acquires() annotation where code
> >> *actually* releases the lock and reacquires later. We also have the
> >> inverse: annotation being added to functions which *don't* release and
> >> reacquire locks.
> >>
> >> IOW, the code is made hard to maintain because of one simple thing :-)
> >
> > Ok, no objection from me to clean it up :)
>
> alright, thanks
>
> >> >> My understanding is that this lock is completely unnecessary and it
> >> >> should be safe to remove it. In fact we would have the benefit of
> >> >> removing __releases()/__acquires() from a few places (some of which are
> >> >> wrong, btw!!) and removing the actual spin_unlock(); do_something();
> >> >> spin_lock(); which have been sprinkled in several places.
> >> >
> >> > Drop the lock entirely? Ok, but what then protects the fields that this
> >> > lock was protecting from being accessed at the same time? How about the
> >> > host controller removal path?
> >>
> >> well, for those cases we need locking. But from hardirq handler we don't
> >> need any, right? Unless there's some case where the hardirq handler
> >> itself could race with something else.
> >
> > irq being run on one processor and something else happening on another
> > one that touches the driver?
>
> and that's exactly my doubt. Is it even possible to have anything racing
> with the hardirq handler in this case? I mean, sure we have khubd. But
> that remains sleeping until an IRQ wakes it up :-)
>
> Also, from driver removal point of view, we will call free_irq() which
> will call synchronize_irq() and, again, it'll wait for hardirq handler
> to complete.
>
> So the question I have really is: can XHCI hardirq handler race with
> anything else happening anywhere in any other CPU?
Yes, of course. It can race with URB submission and cancellation. Not
to mention calls to the hub_control routine.
The question of how fine-grained locking should be is a difficult one.
uhci-hcd, ohci-hcd, and ehci-hcd all use a coarse-grained approach: A
single lock protects everything. (Before I took over its
maintainership, uhci-hcd was exactly the opposite -- separate spinlocks
for each endpoint and each URB, maybe others, I can't remember.
Locking order problems. It looked awful.)
Unless you have a good reason to split up the big lock into multiple
sub-locks, I say don't worry about it.
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
