Re: [PATCH] usb: dwc3: Potential fix of possible dwc3 interrupt storm

[Selvarasu Ganesan <selvarasu.g@samsung.com>]

On 9/4/2024 6:33 AM, Thinh Nguyen wrote:
> On Mon, Sep 02, 2024, Selvarasu Ganesan wrote:
>> I would like to reconfirm from our end that in our failure scenario, we
>> observe that DWC3_EVENT_PENDING is set in evt->flags when the dwc3
>> resume sequence is executed, and the dwc->pending_events flag is not
>> being set.
>>
> If the controller is stopped, no event is generated until it's restarted
> again. (ie, you should not see GEVNTCOUNT updated after clearing
> DCTL.run_stop). If there's no event, no interrupt assertion should come
> from the controller.
>
> If the pending_events is not set and you still see this failure, then
> likely that the controller had started, and the interrupt is generated
> from the controller event. This occurs along with the interrupt
> generated from your connection notification from your setup.


I completely agree. My discussion revolves around the handling of the 
DWC3_EVENT_PENDING flag in all situations. The purpose of using this 
flag is to prevent the processing of new events if an existing event is 
still being processed. This flag is set in the top-half interrupt 
handler and cleared at the end of the bottom-half handler.

Now, let's consider scenarios where the bottom half is not scheduled, 
and a USB reconnect occurs. In this case, there is a possibility that 
the interrupt line is unmasked in dwc3_event_buffers_setup, and the USB 
controller begins posting new events. The top-half interrupt handler 
checks for the DWC3_EVENT_PENDING flag and returns IRQ_HANDLED without 
processing any new events. However, the USB controller continues to post 
interrupts until they are acknowledged.

Please review the complete sequence once with DWC3_EVENT_PENDING flag.

My proposal is to clear or reset the DWC3_EVENT_PENDING flag when 
unmasking the interrupt line dwc3_event_buffers_setup, apart from 
bottom-half handler. Clearing the DWC3_EVENT_PENDING flag in 
dwc3_event_buffers_setup does not cause any harm, as we have implemented 
a temporary workaround in our test setup to prevent IRQ storms.



Working scenarios:
==================
1. Top-half handler:
     a. if (evt->flags & DWC3_EVENT_PENDING)
         return IRQ_HANDLED;
     b. Set DWC3_EVENT_PENDING flag
     c. Masking interrupt line

2. Bottom-half handler:
     a. Un-masking interrupt line
     b. Clear DWC3_EVENT_PENDING flag

Failure scenarios:
==================
1. Top-half handler:
     a. if (evt->flags & DWC3_EVENT_PENDING)
                 return IRQ_HANDLED;
     b. Set DWC3_EVENT_PENDING flag
     c. Masking interrupt line

2. No Bottom-half scheduled:

3. USB reconnect: dwc3_event_buffers_setup
     a. Un-masking interrupt line

4. Continuous interrupts : Top-half handler:
     a. if (evt->flags & DWC3_EVENT_PENDING)
                 return IRQ_HANDLED;

     a. if (evt->flags & DWC3_EVENT_PENDING)
                 return IRQ_HANDLED;

     a. if (evt->flags & DWC3_EVENT_PENDING)
                 return IRQ_HANDLED;
.....

.....

.....

Thanks,
Selva

>
> Check your platform and internal team, what condition would cause the
> setup to generate the interrupt and what condition would stop this
> custom connection interrupt assertion? This is outside of what's defined
> by the flow of the controller.
>
> BR,
> Thinh