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?
The driver shouldn't really care either way.
> > 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 :)
> >> 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?
thanks,
greg k-h
--
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
