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?
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.
> 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?
> Please clarify
>
> > /* Check if the xHC generated the interrupt, or the irq is shared */
> > status = readl(&xhci->op_regs->status);
> > if (status == 0xffffffff) {
device is gone from the system, you have to check this, right? What's
wrong with that? Hm, the comment seems odd...
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
