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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPTae5L6Worr3WsuuV-SUNJh6SOVf+RnLWt2d1LNLDYvY6uRuA@mail.gmail.com>
Date: Wed, 29 Jan 2025 00:30:45 -0800
From: Badhri Jagan Sridharan <badhri@...gle.com>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>, 
	"felipe.balbi@...ux.intel.com" <felipe.balbi@...ux.intel.com>, 
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "jameswei@...gle.com" <jameswei@...gle.com>, 
	"stable@...nel.org" <stable@...nel.org>
Subject: Re: [PATCH v1] usb: dwc3: gadget: Prevent irq storm when TH re-executes

On Mon, Jan 27, 2025 at 6:44 PM Thinh Nguyen <Thinh.Nguyen@...opsys.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@...nel.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@...gle.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
> >