1. Patchew
  2. linux
  3. View series

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

Badhri Jagan Sridharan posted 1 patch 1 year, 3 months ago
There is a newer version of this series
drivers/usb/dwc3/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH v1] usb: dwc3: gadget: Prevent irq storm when TH re-executes
Posted by Badhri Jagan Sridharan 1 year, 3 months ago 
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")
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;
 
 	count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
 	count &= DWC3_GEVNTCOUNT_MASK;

base-commit: 70cd0576aa39c55aabd227851cba0c601e811fb6
-- 
2.48.1.262.g85cc9f2d1e-goog
Re: [PATCH v1] usb: dwc3: gadget: Prevent irq storm when TH re-executes
Posted by Thinh Nguyen 1 year, 3 months ago 
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.

Thanks,
Thinh

>  
>  	count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>  	count &= DWC3_GEVNTCOUNT_MASK;
> 
> base-commit: 70cd0576aa39c55aabd227851cba0c601e811fb6
> -- 
> 2.48.1.262.g85cc9f2d1e-goog
>
Re: [PATCH v1] usb: dwc3: gadget: Prevent irq storm when TH re-executes
Posted by Badhri Jagan Sridharan 1 year, 3 months ago 
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 ?

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));

Regards,
Badhri




>
> Thanks,
> Thinh
>
> >
> >       count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> >       count &= DWC3_GEVNTCOUNT_MASK;
> >
> > base-commit: 70cd0576aa39c55aabd227851cba0c601e811fb6
> > --
> > 2.48.1.262.g85cc9f2d1e-goog
> >

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.