| Message ID | 20250208033129.3524423-1-badhri@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: dwc3: gadget: Prevent irq storm when TH re-executes | expand |
On Sat, Feb 08, 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 by clearing the flag before > event handler busy is cleared. Default enable interrupt moderation in all > versions which support them. > > 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 > Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Fixes: d325a1de49d6 ("usb: dwc3: gadget: Prevent losing events in event cache") > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > --- > drivers/usb/dwc3/core.c | 2 +- > drivers/usb/dwc3/gadget.c | 10 +++++++--- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index dfa1b5fe48dc..6df971ef7285 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1835,7 +1835,7 @@ static void dwc3_get_properties(struct dwc3 *dwc) > dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd; > dwc->tx_max_burst_prd = tx_max_burst_prd; > > - dwc->imod_interval = 0; > + dwc->imod_interval = 1; Use dwc3_has_imod() to determine whether to set this. Otherwise we get a warning on setups that don't support imod. > > dwc->tx_fifo_resize_max_num = tx_fifo_resize_max_num; > } > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index d27af65eb08a..fad115113d28 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -4467,14 +4467,18 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3_event_buffer *evt) > dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), > DWC3_GEVNTSIZ_SIZE(evt->length)); > > + evt->flags &= ~DWC3_EVENT_PENDING; > + /* > + * Add an explicit write memory barrier to make sure that the update of > + * clearing DWC3_EVENT_PENDING is observed in dwc3_check_event_buf() > + */ > + wmb(); > + > 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); > } > > - /* Keep the clearing of DWC3_EVENT_PENDING at the end */ > - evt->flags &= ~DWC3_EVENT_PENDING; > - > return ret; > } > > > base-commit: 9682c35ff6ecd76d9462d4749b8b413d3e8e605e > -- > 2.48.1.502.g6dc24dfdaf-goog > The rest looks fine. Thanks, Thinh
. On Mon, Feb 10, 2025 at 4:22 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > On Sat, Feb 08, 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 by clearing the flag before > > event handler busy is cleared. Default enable interrupt moderation in all > > versions which support them. > > > > 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 > > Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > Fixes: d325a1de49d6 ("usb: dwc3: gadget: Prevent losing events in event cache") > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > > --- > > drivers/usb/dwc3/core.c | 2 +- > > drivers/usb/dwc3/gadget.c | 10 +++++++--- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index dfa1b5fe48dc..6df971ef7285 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -1835,7 +1835,7 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd; > > dwc->tx_max_burst_prd = tx_max_burst_prd; > > > > - dwc->imod_interval = 0; > > + dwc->imod_interval = 1; > > Use dwc3_has_imod() to determine whether to set this. Otherwise we get > a warning on setups that don't support imod. Hi Thinh, dwc3_check_params() which gets invoked after dwc3_get_properties() at https://elixir.bootlin.com/linux/v6.14-rc1/source/drivers/usb/dwc3/core.c#L1851 seems to already call dwc3_has_imod(). Do you prefer me to add an explicit check here as well ? Thanks, Badhri > > > > > dwc->tx_fifo_resize_max_num = tx_fifo_resize_max_num; > > } > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index d27af65eb08a..fad115113d28 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -4467,14 +4467,18 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3_event_buffer *evt) > > dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), > > DWC3_GEVNTSIZ_SIZE(evt->length)); > > > > + evt->flags &= ~DWC3_EVENT_PENDING; > > + /* > > + * Add an explicit write memory barrier to make sure that the update of > > + * clearing DWC3_EVENT_PENDING is observed in dwc3_check_event_buf() > > + */ > > + wmb(); > > + > > 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); > > } > > > > - /* Keep the clearing of DWC3_EVENT_PENDING at the end */ > > - evt->flags &= ~DWC3_EVENT_PENDING; > > - > > return ret; > > } > > > > > > base-commit: 9682c35ff6ecd76d9462d4749b8b413d3e8e605e > > -- > > 2.48.1.502.g6dc24dfdaf-goog > > > > The rest looks fine. > > Thanks, > Thinh
On Mon, Feb 10, 2025, Badhri Jagan Sridharan wrote: > . > > On Mon, Feb 10, 2025 at 4:22 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > > On Sat, Feb 08, 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 by clearing the flag before > > > event handler busy is cleared. Default enable interrupt moderation in all > > > versions which support them. > > > > > > 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 > > > Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > > Fixes: d325a1de49d6 ("usb: dwc3: gadget: Prevent losing events in event cache") > > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > > > --- > > > drivers/usb/dwc3/core.c | 2 +- > > > drivers/usb/dwc3/gadget.c | 10 +++++++--- > > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index dfa1b5fe48dc..6df971ef7285 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -1835,7 +1835,7 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > > dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd; > > > dwc->tx_max_burst_prd = tx_max_burst_prd; > > > > > > - dwc->imod_interval = 0; > > > + dwc->imod_interval = 1; > > > > Use dwc3_has_imod() to determine whether to set this. Otherwise we get > > a warning on setups that don't support imod. > > Hi Thinh, > > dwc3_check_params() which gets invoked after dwc3_get_properties() at > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.14-rc1/source/drivers/usb/dwc3/core.c*L1851__;Iw!!A4F2R9G_pg!Zar83WUe4sM-EF4c2wR2-vWBJHgYOCWEc1ijhOsWQHiXtzCF0d_t2gckS0YJUv4lAZgGZl2C-oSp1QMIx28$ > seems to already call dwc3_has_imod(). Do you prefer me to add an > explicit check here as well ? > Yes. I don't want to see dev_warn print when there shouldn't be any for setup that don't support imod. BR, Thinh
On Mon, Feb 10, 2025 at 4:55 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > On Mon, Feb 10, 2025, Badhri Jagan Sridharan wrote: > > . > > > > On Mon, Feb 10, 2025 at 4:22 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > > > > On Sat, Feb 08, 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 by clearing the flag before > > > > event handler busy is cleared. Default enable interrupt moderation in all > > > > versions which support them. > > > > > > > > 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 > > > > Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > > > Fixes: d325a1de49d6 ("usb: dwc3: gadget: Prevent losing events in event cache") > > > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > > > > --- > > > > drivers/usb/dwc3/core.c | 2 +- > > > > drivers/usb/dwc3/gadget.c | 10 +++++++--- > > > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > index dfa1b5fe48dc..6df971ef7285 100644 > > > > --- a/drivers/usb/dwc3/core.c > > > > +++ b/drivers/usb/dwc3/core.c > > > > @@ -1835,7 +1835,7 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > > > dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd; > > > > dwc->tx_max_burst_prd = tx_max_burst_prd; > > > > > > > > - dwc->imod_interval = 0; > > > > + dwc->imod_interval = 1; > > > > > > Use dwc3_has_imod() to determine whether to set this. Otherwise we get > > > a warning on setups that don't support imod. > > > > Hi Thinh, > > > > dwc3_check_params() which gets invoked after dwc3_get_properties() at > > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.14-rc1/source/drivers/usb/dwc3/core.c*L1851__;Iw!!A4F2R9G_pg!Zar83WUe4sM-EF4c2wR2-vWBJHgYOCWEc1ijhOsWQHiXtzCF0d_t2gckS0YJUv4lAZgGZl2C-oSp1QMIx28$ > > seems to already call dwc3_has_imod(). Do you prefer me to add an > > explicit check here as well ? > > > > Yes. I don't want to see dev_warn print when there shouldn't be any for > setup that don't support imod. Hi Thinh, Looks like adding dwc3_has_imod() in dwc3_get_properties() would not be possible as the dwc->revision gets filled in much later at dwc3_core_is_valid(): https://elixir.bootlin.com/linux/v6.14-rc2/source/drivers/usb/dwc3/core.c#L2218, also, the core is still not brought out of reset yet. Would it be reasonable to initialize dwc->imod_interval to 1 in dwc3_check_params() like below ? +++ b/drivers/usb/dwc3/core.c @@ -1835,8 +1835,6 @@ static void dwc3_get_properties(struct dwc3 *dwc) dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd; dwc->tx_max_burst_prd = tx_max_burst_prd; - dwc->imod_interval = 1; - dwc->tx_fifo_resize_max_num = tx_fifo_resize_max_num; } @@ -1858,6 +1856,8 @@ static void dwc3_check_params(struct dwc3 *dwc) if (dwc->imod_interval && !dwc3_has_imod(dwc)) { dev_warn(dwc->dev, "Interrupt moderation not supported\n"); dwc->imod_interval = 0; + } else if (!dwc->imod_interval && dwc3_has_imod(dwc)) { + dwc->imod_interval = 1; } Thanks, Badhri > > BR, > Thinh
On Wed, Feb 12, 2025, Badhri Jagan Sridharan wrote: > On Mon, Feb 10, 2025 at 4:55 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > > On Mon, Feb 10, 2025, Badhri Jagan Sridharan wrote: > > > . > > > > > > On Mon, Feb 10, 2025 at 4:22 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > > > > > > On Sat, Feb 08, 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 by clearing the flag before > > > > > event handler busy is cleared. Default enable interrupt moderation in all > > > > > versions which support them. > > > > > > > > > > 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 > > > > > Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > > > > Fixes: d325a1de49d6 ("usb: dwc3: gadget: Prevent losing events in event cache") > > > > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > > > > > --- > > > > > drivers/usb/dwc3/core.c | 2 +- > > > > > drivers/usb/dwc3/gadget.c | 10 +++++++--- > > > > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > > index dfa1b5fe48dc..6df971ef7285 100644 > > > > > --- a/drivers/usb/dwc3/core.c > > > > > +++ b/drivers/usb/dwc3/core.c > > > > > @@ -1835,7 +1835,7 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > > > > dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd; > > > > > dwc->tx_max_burst_prd = tx_max_burst_prd; > > > > > > > > > > - dwc->imod_interval = 0; > > > > > + dwc->imod_interval = 1; > > > > > > > > Use dwc3_has_imod() to determine whether to set this. Otherwise we get > > > > a warning on setups that don't support imod. > > > > > > Hi Thinh, > > > > > > dwc3_check_params() which gets invoked after dwc3_get_properties() at > > > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.14-rc1/source/drivers/usb/dwc3/core.c*L1851__;Iw!!A4F2R9G_pg!Zar83WUe4sM-EF4c2wR2-vWBJHgYOCWEc1ijhOsWQHiXtzCF0d_t2gckS0YJUv4lAZgGZl2C-oSp1QMIx28$ > > > seems to already call dwc3_has_imod(). Do you prefer me to add an > > > explicit check here as well ? > > > > > > > Yes. I don't want to see dev_warn print when there shouldn't be any for > > setup that don't support imod. > > Hi Thinh, > > Looks like adding dwc3_has_imod() in dwc3_get_properties() would not > be possible as the dwc->revision gets filled in much later at > dwc3_core_is_valid(): > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.14-rc2/source/drivers/usb/dwc3/core.c*L2218__;Iw!!A4F2R9G_pg!d67RghVyoDYTtMqlnAcNgHywoW0ZfJnYX1NSjokyqaBnPrdiF4w0FlFgTGDEVcSZeUHfpBGIgQtx_UAa1t0$ , > also, the core is still not brought out of reset yet. Would it be > reasonable to initialize dwc->imod_interval to 1 in > dwc3_check_params() like below ? > > +++ b/drivers/usb/dwc3/core.c > @@ -1835,8 +1835,6 @@ static void dwc3_get_properties(struct dwc3 *dwc) > dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd; > dwc->tx_max_burst_prd = tx_max_burst_prd; > > - dwc->imod_interval = 1; > - > dwc->tx_fifo_resize_max_num = tx_fifo_resize_max_num; > } > > @@ -1858,6 +1856,8 @@ static void dwc3_check_params(struct dwc3 *dwc) > if (dwc->imod_interval && !dwc3_has_imod(dwc)) { > dev_warn(dwc->dev, "Interrupt moderation not supported\n"); > dwc->imod_interval = 0; > + } else if (!dwc->imod_interval && dwc3_has_imod(dwc)) { > + dwc->imod_interval = 1; > } > Can you consolidate all the settings of IMOD to the below: diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 423866b2ffaa..a485fef82301 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -2021,21 +2021,19 @@ static void dwc3_check_params(struct dwc3 *dwc) unsigned int hwparam_gen = DWC3_GHWPARAMS3_SSPHY_IFC(dwc->hwparams.hwparams3); - /* Check for proper value of imod_interval */ - if (dwc->imod_interval && !dwc3_has_imod(dwc)) { - dev_warn(dwc->dev, "Interrupt moderation not supported\n"); - dwc->imod_interval = 0; - } - /* + * Enable IMOD for all supporting controllers. + * + * Particularly, DWC_usb3 v3.00a must enable this feature for + * the following reason: + * * Workaround for STAR 9000961433 which affects only version * 3.00a of the DWC_usb3 core. This prevents the controller * interrupt from being masked while handling events. IMOD * allows us to work around this issue. Enable it for the * affected version. */ - if (!dwc->imod_interval && - DWC3_VER_IS(DWC3, 300A)) + if (dwc3_has_imod((dwc))) dwc->imod_interval = 1; Thanks, Thinh
On Thu, Feb 13, 2025 at 2:29 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > On Wed, Feb 12, 2025, Badhri Jagan Sridharan wrote: > > On Mon, Feb 10, 2025 at 4:55 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > > > > On Mon, Feb 10, 2025, Badhri Jagan Sridharan wrote: > > > > . > > > > > > > > On Mon, Feb 10, 2025 at 4:22 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > > > > > > > > On Sat, Feb 08, 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 by clearing the flag before > > > > > > event handler busy is cleared. Default enable interrupt moderation in all > > > > > > versions which support them. > > > > > > > > > > > > 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 > > > > > > Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > > > > > Fixes: d325a1de49d6 ("usb: dwc3: gadget: Prevent losing events in event cache") > > > > > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > > > > > > --- > > > > > > drivers/usb/dwc3/core.c | 2 +- > > > > > > drivers/usb/dwc3/gadget.c | 10 +++++++--- > > > > > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > > > index dfa1b5fe48dc..6df971ef7285 100644 > > > > > > --- a/drivers/usb/dwc3/core.c > > > > > > +++ b/drivers/usb/dwc3/core.c > > > > > > @@ -1835,7 +1835,7 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > > > > > dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd; > > > > > > dwc->tx_max_burst_prd = tx_max_burst_prd; > > > > > > > > > > > > - dwc->imod_interval = 0; > > > > > > + dwc->imod_interval = 1; > > > > > > > > > > Use dwc3_has_imod() to determine whether to set this. Otherwise we get > > > > > a warning on setups that don't support imod. > > > > > > > > Hi Thinh, > > > > > > > > dwc3_check_params() which gets invoked after dwc3_get_properties() at > > > > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.14-rc1/source/drivers/usb/dwc3/core.c*L1851__;Iw!!A4F2R9G_pg!Zar83WUe4sM-EF4c2wR2-vWBJHgYOCWEc1ijhOsWQHiXtzCF0d_t2gckS0YJUv4lAZgGZl2C-oSp1QMIx28$ > > > > seems to already call dwc3_has_imod(). Do you prefer me to add an > > > > explicit check here as well ? > > > > > > > > > > Yes. I don't want to see dev_warn print when there shouldn't be any for > > > setup that don't support imod. > > > > Hi Thinh, > > > > Looks like adding dwc3_has_imod() in dwc3_get_properties() would not > > be possible as the dwc->revision gets filled in much later at > > dwc3_core_is_valid(): > > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.14-rc2/source/drivers/usb/dwc3/core.c*L2218__;Iw!!A4F2R9G_pg!d67RghVyoDYTtMqlnAcNgHywoW0ZfJnYX1NSjokyqaBnPrdiF4w0FlFgTGDEVcSZeUHfpBGIgQtx_UAa1t0$ , > > also, the core is still not brought out of reset yet. Would it be > > reasonable to initialize dwc->imod_interval to 1 in > > dwc3_check_params() like below ? > > > > +++ b/drivers/usb/dwc3/core.c > > @@ -1835,8 +1835,6 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd; > > dwc->tx_max_burst_prd = tx_max_burst_prd; > > > > - dwc->imod_interval = 1; > > - > > dwc->tx_fifo_resize_max_num = tx_fifo_resize_max_num; > > } > > > > @@ -1858,6 +1856,8 @@ static void dwc3_check_params(struct dwc3 *dwc) > > if (dwc->imod_interval && !dwc3_has_imod(dwc)) { > > dev_warn(dwc->dev, "Interrupt moderation not supported\n"); > > dwc->imod_interval = 0; > > + } else if (!dwc->imod_interval && dwc3_has_imod(dwc)) { > > + dwc->imod_interval = 1; > > } > > > > Can you consolidate all the settings of IMOD to the below: Done Thinh ! Sent out V3 version of the patch: https://lore.kernel.org/all/20250216223003.3568039-1-badhri@google.com/ Thanks, Badhri > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 423866b2ffaa..a485fef82301 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -2021,21 +2021,19 @@ static void dwc3_check_params(struct dwc3 *dwc) > unsigned int hwparam_gen = > DWC3_GHWPARAMS3_SSPHY_IFC(dwc->hwparams.hwparams3); > > - /* Check for proper value of imod_interval */ > - if (dwc->imod_interval && !dwc3_has_imod(dwc)) { > - dev_warn(dwc->dev, "Interrupt moderation not supported\n"); > - dwc->imod_interval = 0; > - } > - > /* > + * Enable IMOD for all supporting controllers. > + * > + * Particularly, DWC_usb3 v3.00a must enable this feature for > + * the following reason: > + * > * Workaround for STAR 9000961433 which affects only version > * 3.00a of the DWC_usb3 core. This prevents the controller > * interrupt from being masked while handling events. IMOD > * allows us to work around this issue. Enable it for the > * affected version. > */ > - if (!dwc->imod_interval && > - DWC3_VER_IS(DWC3, 300A)) > + if (dwc3_has_imod((dwc))) > dwc->imod_interval = 1; > > > Thanks, > Thinh
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index dfa1b5fe48dc..6df971ef7285 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1835,7 +1835,7 @@ static void dwc3_get_properties(struct dwc3 *dwc) dwc->tx_thr_num_pkt_prd = tx_thr_num_pkt_prd; dwc->tx_max_burst_prd = tx_max_burst_prd; - dwc->imod_interval = 0; + dwc->imod_interval = 1; dwc->tx_fifo_resize_max_num = tx_fifo_resize_max_num; } diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index d27af65eb08a..fad115113d28 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -4467,14 +4467,18 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3_event_buffer *evt) dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), DWC3_GEVNTSIZ_SIZE(evt->length)); + evt->flags &= ~DWC3_EVENT_PENDING; + /* + * Add an explicit write memory barrier to make sure that the update of + * clearing DWC3_EVENT_PENDING is observed in dwc3_check_event_buf() + */ + wmb(); + 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); } - /* Keep the clearing of DWC3_EVENT_PENDING at the end */ - evt->flags &= ~DWC3_EVENT_PENDING; - return ret; }
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 by clearing the flag before event handler busy is cleared. Default enable interrupt moderation in all versions which support them. 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 Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Fixes: d325a1de49d6 ("usb: dwc3: gadget: Prevent losing events in event cache") Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> --- drivers/usb/dwc3/core.c | 2 +- drivers/usb/dwc3/gadget.c | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) base-commit: 9682c35ff6ecd76d9462d4749b8b413d3e8e605e