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?
> 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 :-)
>> 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.
>> 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...
nothing wrong with that, it's just part of the context ;-)
--
balbi
Attachment:
signature.asc
Description: PGP signature
