Re: [PATCH v2 1/1] usb: dwc3: gadget: fix IRQ storm on invalid event buffer count

Hi @[email protected] , Krishna,

Thanks for the detailed review.
For the gadget ops protection, I propose adding the check only in
dwc3_gadget_ep_queue.
This covers two scenarios:
1. The window between error detection and soft_disconnect completing.
2. After soft_disconnect, existing checks on
connected/pullups_connected/endpoint.desc naturally block all ops.
This is also consistent with how other gadget handles it.

Brs
Jiazi


Thinh Nguyen <[email protected]> 于2026年8月28日周五 10:36写道：
>
> Hi,
>
> Sorry for the delay response.
>
> On Wed, Aug 12, 2026, Jiazi Liu wrote:
> > From: Jiazi Liu <[email protected]>
> >
> > When dwc3_check_event_buf() reads a GEVNTCOUNT value exceeding the
> > event buffer length, commit 63ccd26cd1f6 ("usb: dwc3: gadget: check
> > that event count does not exceed event buffer length") returns IRQ_NONE
> > without writing back GEVNTCOUNT. Since the DWC3 interrupt is
> > level-triggered, the uncleared IRQ source keeps the line asserted,
> > causing a tight IRQ storm that accumulates 99,900 unhandled interrupts
> > and triggers spurious.c:184 BUG -> kernel panic.
> >
> > The resulting call stack:
> >   __report_bad_irq+0xac/0xc8
> >   note_interrupt+0x340/0x468
> >   handle_irq_event+0xac/0xc0
> >   handle_fasteoi_irq+0x120/0x228
> >   gic_handle_irq+0x68/0x108
> >   ...
> >   kernel BUG at kernel/irq/spurious.c:184
> >
> > To reproduce, write a bogus value exceeding the event buffer length
> > directly to the GEVNTCOUNT register:
> >
> >   devmem <DWC3_BASE + 0xc40c> 4 0x1004
> >
> > Write the bogus count back to GEVNTCOUNT to clear the IRQ source,
> > consistent with the stale event clearing pattern in
> > dwc3_event_buffers_setup(), and schedule error recovery to
> > reinitialize the controller.
> >
> > Fixes: 63ccd26cd1f6 ("usb: dwc3: gadget: check that event count does not
> > exceed event buffer length")
>
> Fixes tag should be single line.
>
> > Cc: [email protected]
> > Signed-off-by: Jiazi Liu <[email protected]>
>
> Your email from Signed-off-by is mismatching your From: line above.
>
> > ---
> > Changes in v2:
> >   - Rename softcon_work to err_recovery_work
> >   - Use dev_err instead of dev_err_ratelimited
> >   - Update comment to document driver/controller out-of-sync fatal error
> >     condition rather than describing IRQ storm behavior
> >   - Remove dev_err from work handler; error message belongs at the call
> >     site where the specific failure is known
> >   - Follow dwc3_gadget_suspend/resume logic in recovery work handler
> >   - Add err_dying state to reject gadget driver requests during recovery
> >   - Add err_recovery_count counter and stop scheduling recovery after
> >     DWC3_ERR_RECOVERY_MAX attempts to prevent infinite retry loops
> >   - Add synchronize_irq() before soft_disconnect to ensure IRQ handler
> >     has completed before reinitializing the controller
> >   - Define DWC3_ERR_RECOVERY_MAX in core.h alongside other global constants
> >   - Fix sparse warning: use dwc as first argument to dwc3_writel, not dwc->regs
> > ---
> >  drivers/usb/dwc3/core.h   |  7 +++++++
> >  drivers/usb/dwc3/gadget.c | 41 +++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index 608daeb7ef10..b1cd888eca77 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -49,6 +49,7 @@
> >  #define DWC3_ENDPOINTS_NUM   32
> >  #define DWC3_XHCI_RESOURCES_NUM      2
> >  #define DWC3_ISOC_MAX_RETRIES        5
> > +#define DWC3_ERR_RECOVERY_MAX        3
> >
> >  #define DWC3_SCRATCHBUF_SIZE 4096    /* each buffer is assumed to be 4KiB */
> >  #define DWC3_EVENT_BUFFERS_SIZE      4096
> > @@ -1004,6 +1005,7 @@ struct dwc3_glue_ops {
> >  /**
> >   * struct dwc3 - representation of our controller
> >   * @drd_work: workqueue used for role swapping
> > + * @err_recovery_work: workqueue used for controller error recovery
> >   * @ep0_trb: trb which is used for the ctrl_req
> >   * @bounce: address of bounce buffer
> >   * @setup_buf: used while precessing STD USB requests
> > @@ -1171,6 +1173,7 @@ struct dwc3_glue_ops {
> >   * @wakeup_configured: set if the device is configured for remote wakeup.
> >   * @suspended: set to track suspend event due to U3/L2.
> >   * @susphy_state: state of DWC3_GUSB2PHYCFG_SUSPHY + DWC3_GUSB3PIPECTL_SUSPHY
> > + * @err_dying: true when controller is in error recovery, reject all requests
> >   *             before PM suspend.
> >   * @imod_interval: set the interrupt moderation interval in 250ns
> >   *                   increments or 0 to disable.
> > @@ -1186,9 +1189,11 @@ struct dwc3_glue_ops {
> >   * @wakeup_pending_funcs: Indicates whether any interface has requested for
> >   *                    function wakeup in bitmap format where bit position
> >   *                    represents interface_id.
> > + * @err_recovery_count: number of consecutive error recovery attempts
> >   */
> >  struct dwc3 {
> >       struct work_struct      drd_work;
> > +     struct work_struct      err_recovery_work;
> >       struct dwc3_trb         *ep0_trb;
> >       void                    *bounce;
> >       u8                      *setup_buf;
> > @@ -1420,6 +1425,7 @@ struct dwc3 {
> >       unsigned                wakeup_configured:1;
> >       unsigned                suspended:1;
> >       unsigned                susphy_state:1;
> > +     unsigned                err_dying:1;
>
> Can we change this to enum dwc3_err_state err_state where
>
> enum dwc3_err_state {
>         DWC3_ERR_NONE = 0,
>         DWC3_ERR_RECOVERY,
>         DWC3_ERR_UNRECOVERABLE,
> };
>
> Then we can set err_state to DWC3_ERR_RECOVERY before scheduling the
> recovery work, and set it to DWC3_ERR_NONE when the recovery work is
> done. If we hit the max number of retries, we can set it to
> DWC3_ERR_UNRECOVERABLE. This will make it clearer what state the
> controller is in.
>
> >
> >       u16                     imod_interval;
> >
> > @@ -1429,6 +1435,7 @@ struct dwc3 {
> >       struct dentry           *debug_root;
> >       u32                     gsbuscfg0_reqinfo;
> >       u32                     wakeup_pending_funcs;
> > +     u32                     err_recovery_count;
> >  };
> >
> >  #define INCRX_BURST_MODE 0
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index fa0f16ffafef..05f3ffab6e47 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -2054,6 +2054,9 @@ static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,
> >
> >       int                             ret;
> >
> > +     if (dwc->err_dying)
> > +             return -ESHUTDOWN;
> > +
>
> As Krishna brought up, may need to double check, but I think all the
> gadget and ep ops need to be guarded.
>
> >       spin_lock_irqsave(&dwc->lock, flags);
> >       ret = __dwc3_gadget_ep_queue(dep, req);
> >       spin_unlock_irqrestore(&dwc->lock, flags);
> > @@ -4667,9 +4670,22 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
> >               return IRQ_NONE;
> >
> >       if (count > evt->length) {
> > -             dev_err_ratelimited(dwc->dev, "invalid count(%u) > evt->length(%u)\n",
> > +             dev_err(dwc->dev, "invalid count(%u) > evt->length(%u)\n",
> >                       count, evt->length);
> > -             return IRQ_NONE;
> > +             /*
> > +              * This is a fatal error - the driver and controller are out of
> > +              * sync on which event has been consumed. Reinitializing the
> > +              * controller is required to recover. Write the bogus count back
> > +              * to GEVNTCOUNT to clear the IRQ source, consistent with the
> > +              * stale event clearing in dwc3_event_buffers_setup(), then
> > +              * schedule error recovery.
> > +              */
> > +             dwc3_writel(dwc, DWC3_GEVNTCOUNT(0), count);
> > +             dwc->err_dying = true;
> > +             dwc->err_recovery_count++;
>
> This increment should be done in dwc3_err_recovery_work(). Reset the
> count to 0 after err_state transition to DWC3_ERR_NONE and on reset
> event interrupt.
>
> > +             if (dwc->err_recovery_count <= DWC3_ERR_RECOVERY_MAX)
>
> The dwc->err_recovery_count check here should be checked in the
> dwc3_err_recovery_work, to check whether to perform soft connect or not.
> Set the err_state to unrecoverable and print a dev_err() if soft_connect
> or soft_disconnect fails or if recovery count exceeds the max.
>
> > +                     schedule_work(&dwc->err_recovery_work);
> > +             return IRQ_HANDLED;
> >       }
> >
> >       evt->count = count;
> > @@ -4729,6 +4745,25 @@ static void dwc_gadget_release(struct device *dev)
> >       kfree(gadget);
> >  }
> >
> > +static void dwc3_err_recovery_work(struct work_struct *work)
> > +{
> > +     struct dwc3 *dwc = container_of(work, struct dwc3, err_recovery_work);
> > +     int ret;
> > +
> > +     synchronize_irq(dwc->irq_gadget);
>
> I think we should use disable_irq_nosync() before soft_disconnect and
> re-enabling it after soft_connect succeeds. Leave it disabled if
> DWC3_ERR_RECOVERY_MAX is reached or soft_connect fails or is skipped.
>
> > +
> > +     ret = dwc3_gadget_soft_disconnect(dwc);
> > +     if (ret)
> > +             return;
> > +
> > +     dwc3_disconnect_gadget_sleepable(dwc);
> > +
> > +     dwc->err_dying = false;
> > +
> > +     if (dwc->softconnect)
> > +             dwc3_gadget_soft_connect(dwc);
> > +}
> > +
> >  /**
> >   * dwc3_gadget_init - initializes gadget related registers
> >   * @dwc: pointer to our controller context structure
> > @@ -4772,6 +4807,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
> >       }
> >
> >       init_completion(&dwc->ep0_in_setup);
> > +     INIT_WORK(&dwc->err_recovery_work, dwc3_err_recovery_work);
> >       dwc->gadget = kzalloc_obj(struct usb_gadget);
> >       if (!dwc->gadget) {
> >               ret = -ENOMEM;
> > @@ -4868,6 +4904,7 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
> >       if (!dwc->gadget)
> >               return;
> >
> > +     cancel_work_sync(&dwc->err_recovery_work);
> >       dwc3_enable_susphy(dwc, true);
> >       usb_del_gadget(dwc->gadget);
> >       dwc3_gadget_free_endpoints(dwc);
> > --
> > 2.50.1 (Apple Git-155)
> >
>
> Thanks,
> Thinh