Re: [PATCH v1] usb: dwc3: gadget: Prevent irq storm when TH re-executes

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Wed, Jan 29, 2025, Badhri Jagan Sridharan wrote:
> On Mon, Jan 27, 2025 at 6:44 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >
> > On Fri, Jan 24, 2025, Badhri Jagan Sridharan wrote:
> > > While commit d325a1de49d6 ("usb: dwc3: gadget: Prevent losing events
> > > in event cache") makes sure that top half(TH) does not end up overwriting
> > > the cached events before processing them when the TH gets invoked more
> > > than one time, returning IRQ_HANDLED results in occasional irq storm
> > > where the TH hogs the CPU. The irq storm can be prevented if
> > > IRQ_WAKE_THREAD is returned.
> > >
> > > ftrace event stub during dwc3 irq storm:
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000866: irq_handler_exit: irq=14 ret=handled
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000872: irq_handler_entry: irq=504 name=dwc3
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000874: irq_handler_exit: irq=504 ret=handled
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000881: irq_handler_entry: irq=504 name=dwc3
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000883: irq_handler_exit: irq=504 ret=handled
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000889: irq_handler_entry: irq=504 name=dwc3
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000892: irq_handler_exit: irq=504 ret=handled
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000898: irq_handler_entry: irq=504 name=dwc3
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000901: irq_handler_exit: irq=504 ret=handled
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000907: irq_handler_entry: irq=504 name=dwc3
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000909: irq_handler_exit: irq=504 ret=handled
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000915: irq_handler_entry: irq=504 name=dwc3
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000918: irq_handler_exit: irq=504 ret=handled
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000924: irq_handler_entry: irq=504 name=dwc3
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000927: irq_handler_exit: irq=504 ret=handled
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000933: irq_handler_entry: irq=504 name=dwc3
> > >     irq/504_dwc3-1111  ( 1111) [000] .... 70.000935: irq_handler_exit: irq=504 ret=handled
> > >     ....
> > >
> > > Cc: stable@kernel.org
> > > Fixes: d325a1de49d6 ("usb: dwc3: gadget: Prevent losing events in event cache")
> >
> > I don't think this should be Cc to stable, at least not the way it is
> > right now.
> >
> > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > > ---
> > >  drivers/usb/dwc3/gadget.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > index d27af65eb08a..376ab75adc4e 100644
> > > --- a/drivers/usb/dwc3/gadget.c
> > > +++ b/drivers/usb/dwc3/gadget.c
> > > @@ -4519,7 +4519,7 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
> > >        * losing events.
> > >        */
> > >       if (evt->flags & DWC3_EVENT_PENDING)
> > > -             return IRQ_HANDLED;
> > > +             return IRQ_WAKE_THREAD;
> >
> > This looks like a workaround to the issue we have. Have you tried to
> > enable imod instead? It's the feature to help avoid these kind of issue.
> 
> Hi Thinh,
> 
> Thanks for the feedback ! Yes, we have been experimenting with the
> interrupt moderation interval as well.
> Have follow up questions though, please bear with me !
> 
> 1. Given that when DWC3_EVENT_PENDING  is still set,
> dwc3_check_event_buf() is still waiting for the previous cached events
> to be processed by the dwc3_thread_interrupt(), what's the reasoning
> behind returning IRQ_HANDLED here ? Shouldn't we be returning
> IRQ_WAKE_THREAD anyways ?

Currently dwc3 is implemented such that it will not process new events
until the BH is done with its work. The DWC3_EVENT_PENDING flag
indicates when the events are processed. With this expectation, we
should not schedule the BH as the events are still being handled.

In your case, there's a small window where the TH may be scheduled but
the DWC3_EVENT_PENDING flag is not cleared yet. Returning
IRQ_WAKE_THREAD may workaround your issue because the BH may already be
running when DWC3_EVENT_PENDING is set. I'm not sure what other side
effect this may have since we're breaking this expectation.

We may enhance the dwc3 handling of event flow in the future to improve
this. But at the moment, we should not return IRQ_WAKE_THREAD here.

> 
> 2. When interrupt moderation is enabled, does DEVICE_IMODC start to
> decrement as soon as the interrupt is masked (where I expect that the
> interrupt line gets de-asserted by the controller) in
> dwc3_check_event_buf()  ?
> 
> /* Mask interrupt */
> dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
>    DWC3_GEVNTSIZ_INTMASK | DWC3_GEVNTSIZ_SIZE(evt->length));
> 

The DEVICE_IMODC is loaded with DEVICE_IMODI and starts to decrement as
soon as the interrupt is de-asserted from the asserted state, which
includes when the interrupt is masked. You brought up a good question
here. The IMOD count may already be 0 when we exit the BH. Can you try
this experiment to update the count and let me know if it helps:

Note: not tested.

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 0fe92c0fb520..62aaac31ca68 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -5739,7 +5739,8 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3_event_buffer *evt)
 
        if (dwc->imod_interval) {
                dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), DWC3_GEVNTCOUNT_EHB);
-               dwc3_writel(dwc->regs, DWC3_DEV_IMOD(0), dwc->imod_interval);
+               dwc3_writel(dwc->regs, DWC3_DEV_IMOD(0),
+                           (dwc->imod_interval << 16) | dwc->imod_interval);
        }
 
        /* Keep the clearing of DWC3_EVENT_PENDING at the end */


Thanks,
Thinh