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

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

On 9/5/2024 5:56 AM, Thinh Nguyen wrote:
> On Wed, Sep 04, 2024, Selvarasu Ganesan wrote:
>> 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
> For DWC3_EVENT_PENDING flag to be set at this point (before we start the
> controller), that means that the GEVNTCOUNT was not 0 after
> soft-disconnect and that the pm_runtime_suspended() must be false.

In the top-half code where we set the DWC3_EVENT_PENDING flag, we 
acknowledge GEVNTCOUNT. Therefore, I think it is not necessary for 
GEVNTCOUNT to have a non-zero value until a new event occurs. In fact, 
when we tried to print GEVNTCOUNT in a non-interrupt context, we found 
that it was zero, where we received DWC3_EVENT_PENDING being set in 
non-interrupt context.

>
>> 2. No Bottom-half scheduled:
> Why is the bottom-half not scheduled? Or do you mean it hasn't woken up
> yet before the next top-half coming?

In very rare cases, it is possible in our platform that the CPU may not 
be able to schedule the bottom half of the dwc3 interrupt because a work 
queue lockup has occurred on the same CPU that is attempting to schedule 
the dwc3 thread interrupt. In this case Yes, the bottom-half handler 
hasn't woken up, then initiate an IRQ storm for new events after the 
controller restarts, resulting in no more bottom-half scheduling due to 
the CPU being stuck in processing continuous interrupts and return 
IRQ_HANDLED by checking if (evt->flags & DWC3_EVENT_PENDING).

>
>> 3. USB reconnect: dwc3_event_buffers_setup
>>       a. Un-masking interrupt line
> Do we know that the GEVNTCOUNT is non-zero before starting the
> controller again?

The GEVNTCOUNT value showing as zero that we confirmed by adding debug 
message here.
>
>> 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;
>> .....
>>
>> .....
>>
>> .....
>>

Sure, I can try implementing the proposed code modifications in our 
testing environment.

But, I am uncertain about how these changes will effectively prevent an 
IRQ storm when the USB controller sequence restarts with the 
DWC3_EVENT_PENDING. The following code will only execute until the 
DWC3_EVENT_PENDING is cleared, at which point the previous bottom-half 
will not be scheduled.

Please correct me if i am wrong in my above understanding.


static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) { 
struct dwc3 *dwc = evt->dwc; u32 amount; u32 count; .....

.....

.....

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


Thanks,
Selva
> I don't want the driver to process any stale event after a
> soft-disconnect. Can we try this instead:
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index ccc3895dbd7f..a1e6ba92e808 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -544,6 +544,7 @@ static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length)
>   int dwc3_event_buffers_setup(struct dwc3 *dwc)
>   {
>   	struct dwc3_event_buffer	*evt;
> +	u32				reg;
>   
>   	if (!dwc->ev_buf)
>   		return 0;
> @@ -556,7 +557,10 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc)
>   			upper_32_bits(evt->dma));
>   	dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
>   			DWC3_GEVNTSIZ_SIZE(evt->length));
> -	dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 0);
> +
> +	/* Clear any stale event */
> +	reg = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> +	dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), reg);
>   
>   	return 0;
>   }
> @@ -2484,7 +2488,11 @@ static int dwc3_runtime_resume(struct device *dev)
>   
>   	switch (dwc->current_dr_role) {
>   	case DWC3_GCTL_PRTCAP_DEVICE:
> -		dwc3_gadget_process_pending_events(dwc);
> +		if (dwc->pending_events) {
> +			pm_runtime_put(dwc->dev);
> +			dwc->pending_events = false;
> +			enable_irq(dwc->irq_gadget);
> +		}
>   		break;
>   	case DWC3_GCTL_PRTCAP_HOST:
>   	default:
> @@ -2574,6 +2582,12 @@ static void dwc3_complete(struct device *dev)
>   static const struct dev_pm_ops dwc3_dev_pm_ops = {
>   	SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
>   	.complete = dwc3_complete,
> +
> +	/*
> +	 * Runtime suspend halts the controller on disconnection. It replies on
> +	 * platforms with custom connection notification to start the controller
> +	 * again.
> +	 */
>   	SET_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume,
>   			dwc3_runtime_idle)
>   };
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 1e561fd8b86e..2fa3fd55eb32 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1673,7 +1673,6 @@ static inline void dwc3_otg_host_init(struct dwc3 *dwc)
>   #if !IS_ENABLED(CONFIG_USB_DWC3_HOST)
>   int dwc3_gadget_suspend(struct dwc3 *dwc);
>   int dwc3_gadget_resume(struct dwc3 *dwc);
> -void dwc3_gadget_process_pending_events(struct dwc3 *dwc);
>   #else
>   static inline int dwc3_gadget_suspend(struct dwc3 *dwc)
>   {
> @@ -1684,10 +1683,6 @@ static inline int dwc3_gadget_resume(struct dwc3 *dwc)
>   {
>   	return 0;
>   }
> -
> -static inline void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
> -{
> -}
>   #endif /* !IS_ENABLED(CONFIG_USB_DWC3_HOST) */
>   
>   #if IS_ENABLED(CONFIG_USB_DWC3_ULPI)
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 89fc690fdf34..a525f7ea5886 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -4490,6 +4490,17 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
>   		return IRQ_HANDLED;
>   
>   	count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> +
> +	/*
> +	 * If the controller is halted, the event count is stale/invalid. Ignore
> +	 * them. This happens if the interrupt assertion is from an out-of-band
> +	 * resume notification.
> +	 */
> +	if (!dwc->pullups_connected && count) {
> +		dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
> +		return IRQ_HANDLED;
> +	}
> +
>   	count &= DWC3_GEVNTCOUNT_MASK;
>   	if (!count)
>   		return IRQ_NONE;
> @@ -4735,14 +4746,3 @@ int dwc3_gadget_resume(struct dwc3 *dwc)
>   
>   	return dwc3_gadget_soft_connect(dwc);
>   }
> -
> -void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
> -{
> -	if (dwc->pending_events) {
> -		dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
> -		dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
> -		pm_runtime_put(dwc->dev);
> -		dwc->pending_events = false;
> -		enable_irq(dwc->irq_gadget);
> -	}
> -}
>
>
> ---
>
> Thanks,
> Thinh