On Thu, Sep 24, 2020 at 10:39:24AM +0300, Felipe Balbi wrote:
> >>> + /*
> >>> + * Synchronize and disable any further event handling while controller
> >>> + * is being enabled/disabled.
> >>> + */
> >>> + disable_irq(dwc->irq_gadget);
> >>
> >> why isn't dwc3_gadget_disable_irq() enough?
> >>
> >>> spin_lock_irqsave(&dwc->lock, flags);
> >>
> >> spin_lock_irqsave() will disable interrupts, why disable_irq() above?
> >>
> >
> > In the discussion I had with Thinh, the concern was that with the newly
> > added code to override the lpos here, if the interrupt routine
> > (dwc3_check_event_buf()) runs, then it will reference the lpos for
>
> that's running in hardirq context. All interrupts are disabled while
> that runs, there's no risk of race, right?
>
> > copying the event buffer contents to the event cache, and potentially
> > process events. There is no locking in place, so it could be possible
> > to have both run in parallel.
>
> Is this academic or have you actually found a situation where this
> could, indeed, happen? The spin_lock_irqsave() should be enough to
> synchronize dwc3_gadget_pullup() and the interrupt handler.
>
> > Hence, the reason if there was already a pending IRQ triggered, the
> > dwc3_gadget_disable_irq() won't ensure the IRQ is handled. We can do
> > something like:
> > if (!is_on)
> > dwc3_gadget_disable_irq()
> > synchronize_irq()
> > spin_lock_irqsave()
> > if(!is_on) {
> > ...
> >
> > But the logic to only apply this on the pullup removal case is a little
> > messy. Also, from my understanding, the spin_lock_irqsave() will only
> > disable the local CPU IRQs, but not the interrupt line on the GIC, which
> > means other CPUs can handle it, unless we explicitly set the IRQ
> > affinity to CPUX.
>
> Yeah, the way I understand this can't really happen. But I'm open to
> being educated. Maybe Alan can explain if this is really possibility?
It depends on the details of the hardware, but yes, it is possible in
general for an interrupt handler to run after you have turned off the
device's interrupt-request line. For example:
CPU A CPU B
--------------------------- ----------------------
Gets an IRQ from the device
Calls handler routine spin_lock_irq
spin_lock_irq Turns off the IRQ line
...spins... spin_unlock_irq
Rest of handler runs
spin_unlock_irq
That's why we have synchronize_irq(). The usual pattern is something
like this:
spin_lock_irq(&priv->lock);
priv->disconnected = true;
my_disable_irq(priv);
spin_unlock_irq(&priv->lock);
synchronize_irq(priv->irq);
And of course this has to be done in a context that can sleep.
Does this answer your question?
Alan Stern
