[PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[JaeHun Jung]

Sometimes very rarely, The count is 0 and the DWC3 flag is set has status.
It must not have these status. Because, It can make happen interrupt storming
status.
So, It have to clean up DWC3_EVENT_PENDING flags set when count is 0.
It means "There are no interrupts to handle.".

(struct dwc3_event_buffer *) ev_buf = 0xFFFFFF883DBF1180 (
	(void *) buf = 0xFFFFFFC00DBDD000 = end+0x337D000,
	(void *) cache = 0xFFFFFF8839F54080,
	(unsigned int) length = 0x1000,
	(unsigned int) lpos = 0x0,
	(unsigned int) count = 0x0,
	(unsigned int) flags = 0x00000001,
	(dma_addr_t) dma = 0x00000008BD7D7000,
	(struct dwc3 *) dwc = 0xFFFFFF8839CBC880,
	(u64) android_kabi_reserved1 = 0x0),

(time = 47557628930999, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628931268, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
(time = 47557628932383, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628932652, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
(time = 47557628933768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628934037, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
(time = 47557628935152, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628935460, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
(time = 47557628936575, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628936845, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
(time = 47557628937960, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628938229, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
(time = 47557628939345, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628939652, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
(time = 47557628940768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628941037, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
(time = 47557628942152, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628942422, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
(time = 47557628943537, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628943806, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
(time = 47557628944922, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628945229, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
(time = 47557628946345, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628946614, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
(time = 47557628947729, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
(time = 47557628947999, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),

Signed-off-by: JaeHun Jung <jh0801.jung@samsung.com>
---
 drivers/usb/dwc3/gadget.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 789976567f9f..5d2d5a9b9915 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -4355,8 +4355,11 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
 	 * irq event handler completes before caching new event to prevent
 	 * losing events.
 	 */
-	if (evt->flags & DWC3_EVENT_PENDING)
+	if (evt->flags & DWC3_EVENT_PENDING) {
+		if (!evt->count)
+			evt->flags &= ~DWC3_EVENT_PENDING;
 		return IRQ_HANDLED;
+	}
 
 	count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
 	count &= DWC3_GEVNTCOUNT_MASK;
-- 
2.31.1

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Felipe Balbi]

Hi,

JaeHun Jung <jh0801.jung@samsung.com> writes:
> Sometimes very rarely, The count is 0 and the DWC3 flag is set has status.
> It must not have these status. Because, It can make happen interrupt storming
> status.
> So, It have to clean up DWC3_EVENT_PENDING flags set when count is 0.
> It means "There are no interrupts to handle.".
>
> (struct dwc3_event_buffer *) ev_buf = 0xFFFFFF883DBF1180 (
> 	(void *) buf = 0xFFFFFFC00DBDD000 = end+0x337D000,
> 	(void *) cache = 0xFFFFFF8839F54080,
> 	(unsigned int) length = 0x1000,
> 	(unsigned int) lpos = 0x0,
> 	(unsigned int) count = 0x0,
> 	(unsigned int) flags = 0x00000001,
> 	(dma_addr_t) dma = 0x00000008BD7D7000,
> 	(struct dwc3 *) dwc = 0xFFFFFF8839CBC880,
> 	(u64) android_kabi_reserved1 = 0x0),
>
> (time = 47557628930999, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628931268, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628932383, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628932652, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628933768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628934037, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628935152, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628935460, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628936575, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628936845, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628937960, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628938229, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628939345, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628939652, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628940768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628941037, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628942152, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628942422, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628943537, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628943806, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628944922, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628945229, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628946345, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628946614, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628947729, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628947999, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
>
> Signed-off-by: JaeHun Jung <jh0801.jung@samsung.com>

Please get in the habit of running patches through
scripts/get_maintainer.pl. If you did, it would tell you Thinh is the
new maintainer for dwc3.

-- 
balbi

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Linyu Yuan]

On 1/2/2023 1:08 PM, JaeHun Jung wrote:
> Sometimes very rarely, The count is 0 and the DWC3 flag is set has status.
> It must not have these status. Because, It can make happen interrupt storming
> status.

could you help explain without clear the flag, how interrupt storming 
happen ?

as your change didn't touch any hardware register, i don't know how it 
fix storming.

storming from software layer ?

> So, It have to clean up DWC3_EVENT_PENDING flags set when count is 0.
> It means "There are no interrupts to handle.".
>
> (struct dwc3_event_buffer *) ev_buf = 0xFFFFFF883DBF1180 (
> 	(void *) buf = 0xFFFFFFC00DBDD000 = end+0x337D000,
> 	(void *) cache = 0xFFFFFF8839F54080,
> 	(unsigned int) length = 0x1000,
> 	(unsigned int) lpos = 0x0,
> 	(unsigned int) count = 0x0,
> 	(unsigned int) flags = 0x00000001,
> 	(dma_addr_t) dma = 0x00000008BD7D7000,
> 	(struct dwc3 *) dwc = 0xFFFFFF8839CBC880,
> 	(u64) android_kabi_reserved1 = 0x0),
>
> (time = 47557628930999, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628931268, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628932383, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628932652, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628933768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628934037, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628935152, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628935460, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628936575, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628936845, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628937960, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628938229, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628939345, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628939652, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628940768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628941037, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628942152, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628942422, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628943537, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628943806, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628944922, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628945229, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628946345, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628946614, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628947729, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628947999, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
>
> Signed-off-by: JaeHun Jung <jh0801.jung@samsung.com>
> ---
>   drivers/usb/dwc3/gadget.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 789976567f9f..5d2d5a9b9915 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -4355,8 +4355,11 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
>   	 * irq event handler completes before caching new event to prevent
>   	 * losing events.
>   	 */
> -	if (evt->flags & DWC3_EVENT_PENDING)
> +	if (evt->flags & DWC3_EVENT_PENDING) {
> +		if (!evt->count)
> +			evt->flags &= ~DWC3_EVENT_PENDING;
>   		return IRQ_HANDLED;
> +	}
>   
>   	count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>   	count &= DWC3_GEVNTCOUNT_MASK;

how about below change ?

         count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
         count &= DWC3_GEVNTCOUNT_MASK;
-       if (!count)
+       if (!count) {
+               evt->flags &= ~DWC3_EVENT_PENDING;
                 return IRQ_NONE;
+       }

         evt->count = count;
         evt->flags |= DWC3_EVENT_PENDING;

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Linyu Yuan]

On 1/5/2023 11:29 AM, Linyu Yuan wrote:
> On 1/2/2023 1:08 PM, JaeHun Jung wrote:
>> Sometimes very rarely, The count is 0 and the DWC3 flag is set has 
>> status.
>> It must not have these status. Because, It can make happen interrupt 
>> storming
>> status.
>
> could you help explain without clear the flag, how interrupt storming 
> happen ?
>
> as your change didn't touch any hardware register, i don't know how it 
> fix storming.
>
> storming from software layer ?
>
>> So, It have to clean up DWC3_EVENT_PENDING flags set when count is 0.
>> It means "There are no interrupts to handle.".
>>
>> (struct dwc3_event_buffer *) ev_buf = 0xFFFFFF883DBF1180 (
>>     (void *) buf = 0xFFFFFFC00DBDD000 = end+0x337D000,
>>     (void *) cache = 0xFFFFFF8839F54080,
>>     (unsigned int) length = 0x1000,
>>     (unsigned int) lpos = 0x0,
>>     (unsigned int) count = 0x0,
>>     (unsigned int) flags = 0x00000001,
>>     (dma_addr_t) dma = 0x00000008BD7D7000,
>>     (struct dwc3 *) dwc = 0xFFFFFF8839CBC880,
>>     (u64) android_kabi_reserved1 = 0x0),
>>
>> (time = 47557628930999, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628931268, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>> (time = 47557628932383, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628932652, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>> (time = 47557628933768, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628934037, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>> (time = 47557628935152, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628935460, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>> (time = 47557628936575, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628936845, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>> (time = 47557628937960, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628938229, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>> (time = 47557628939345, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628939652, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>> (time = 47557628940768, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628941037, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>> (time = 47557628942152, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628942422, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>> (time = 47557628943537, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628943806, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>> (time = 47557628944922, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628945229, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>> (time = 47557628946345, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628946614, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>> (time = 47557628947729, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 1),
>> (time = 47557628947999, irq = 165, fn = dwc3_interrupt, latency = 0, 
>> en = 3),
>>
>> Signed-off-by: JaeHun Jung <jh0801.jung@samsung.com>
>> ---
>>   drivers/usb/dwc3/gadget.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 789976567f9f..5d2d5a9b9915 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -4355,8 +4355,11 @@ static irqreturn_t dwc3_check_event_buf(struct 
>> dwc3_event_buffer *evt)
>>        * irq event handler completes before caching new event to prevent
>>        * losing events.
>>        */
>> -    if (evt->flags & DWC3_EVENT_PENDING)
>> +    if (evt->flags & DWC3_EVENT_PENDING) {
>> +        if (!evt->count)
>> +            evt->flags &= ~DWC3_EVENT_PENDING;
>>           return IRQ_HANDLED;
>> +    }
>>         count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>>       count &= DWC3_GEVNTCOUNT_MASK;
>
> how about below change ?
>
>         count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>         count &= DWC3_GEVNTCOUNT_MASK;
> -       if (!count)
> +       if (!count) {
> +               evt->flags &= ~DWC3_EVENT_PENDING;
>                 return IRQ_NONE;
> +       }
ignore my suggestion, add Thinh to comment.
>
>         evt->count = count;
>         evt->flags |= DWC3_EVENT_PENDING;
>

RE: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[정재훈]

> -----Original Message-----
> From: Linyu Yuan [mailto:quic_linyyuan@quicinc.com]
> Sent: Thursday, January 5, 2023 12:35 PM
> To: JaeHun Jung; Felipe Balbi; Greg Kroah-Hartman; Thinh Nguyen
> Cc: open list:USB XHCI DRIVER; open list; Seungchull Suh; Daehwan Jung
> Subject: Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0
> 
> 
> On 1/5/2023 11:29 AM, Linyu Yuan wrote:
> > On 1/2/2023 1:08 PM, JaeHun Jung wrote:
> >> Sometimes very rarely, The count is 0 and the DWC3 flag is set has
> >> status.
> >> It must not have these status. Because, It can make happen interrupt
> >> storming status.
> >
> > could you help explain without clear the flag, how interrupt storming
> > happen ?
> >
> > as your change didn't touch any hardware register, i don't know how it
> > fix storming.
> >
H/W interrupts are still occur on IP.
But. ISR doesn't handle it. Because of this
"if (evt->flags & DWC3_EVENT_PENDING)"

This is event values.
(struct dwc3_event_buffer *) ev_buf = 0xFFFFFF883DBF1180 = kernel_size_le_lo32+0xFFFFFF883B391180 -> (
        (void *) buf = 0xFFFFFFC00DBDD000 = end+0x337D000 -> ,
        (void *) cache = 0xFFFFFF8839F54080 = kernel_size_le_lo32+0xFFFFFF88376F4080 -> ,
        (unsigned int) length = 0x1000,
        (unsigned int) lpos = 0x0,
        (unsigned int) count = 0x0,
        (unsigned int) flags = 0x00100001,
        (dma_addr_t) dma = 0x00000008BD7D7000,

*((struct dwc3_event_type *) 0xFFFFFF8839F54080) = (
    is_devspec = 1,
    type = 0,
    reserved8_31 = 774)

*((struct dwc3_event_devt  *) 0xFFFFFF8839F54080) = (
    one_bit = 1,
    device_event = 0,
    type = 6, << DWC3_DEVICE_EVENT_SUSPEND
    reserved15_12 = 0,
    event_info = 3,
    reserved31_25 = 0)

Occurred interrupts are "DWC3_DEVICE_EVENT_SUSPEND".
If when "count has 0" and DWC3_EVENT_PENDING are set at the same time than
 ISR and bottom thread couldn't handle next interrupt.
We guessing connected with "dwc3_gadget_process_pending_events()" function.
But, We could not find reproduce way.


> > storming from software layer ?
> >
> >> So, It have to clean up DWC3_EVENT_PENDING flags set when count is 0.
> >> It means "There are no interrupts to handle.".
> >>
> >> (struct dwc3_event_buffer *) ev_buf = 0xFFFFFF883DBF1180 (
> >>     (void *) buf = 0xFFFFFFC00DBDD000 = end+0x337D000,
> >>     (void *) cache = 0xFFFFFF8839F54080,
> >>     (unsigned int) length = 0x1000,
> >>     (unsigned int) lpos = 0x0,
> >>     (unsigned int) count = 0x0,
> >>     (unsigned int) flags = 0x00000001,
> >>     (dma_addr_t) dma = 0x00000008BD7D7000,
> >>     (struct dwc3 *) dwc = 0xFFFFFF8839CBC880,
> >>     (u64) android_kabi_reserved1 = 0x0),
> >>
> >> (time = 47557628930999, irq = 165, fn = dwc3_interrupt, latency = 0,
> >> en = 1), (time = 47557628931268, irq = 165, fn = dwc3_interrupt,
> >> latency = 0, en = 3), (time = 47557628932383, irq = 165, fn =
> >> dwc3_interrupt, latency = 0, en = 1), (time = 47557628932652, irq =
> >> 165, fn = dwc3_interrupt, latency = 0, en = 3), (time =
> >> 47557628933768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> >> (time = 47557628934037, irq = 165, fn = dwc3_interrupt, latency = 0,
> >> en = 3), (time = 47557628935152, irq = 165, fn = dwc3_interrupt,
> >> latency = 0, en = 1), (time = 47557628935460, irq = 165, fn =
> >> dwc3_interrupt, latency = 0, en = 3), (time = 47557628936575, irq =
> >> 165, fn = dwc3_interrupt, latency = 0, en = 1), (time =
> >> 47557628936845, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> >> (time = 47557628937960, irq = 165, fn = dwc3_interrupt, latency = 0,
> >> en = 1), (time = 47557628938229, irq = 165, fn = dwc3_interrupt,
> >> latency = 0, en = 3), (time = 47557628939345, irq = 165, fn =
> >> dwc3_interrupt, latency = 0, en = 1), (time = 47557628939652, irq =
> >> 165, fn = dwc3_interrupt, latency = 0, en = 3), (time =
> >> 47557628940768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> >> (time = 47557628941037, irq = 165, fn = dwc3_interrupt, latency = 0,
> >> en = 3), (time = 47557628942152, irq = 165, fn = dwc3_interrupt,
> >> latency = 0, en = 1), (time = 47557628942422, irq = 165, fn =
> >> dwc3_interrupt, latency = 0, en = 3), (time = 47557628943537, irq =
> >> 165, fn = dwc3_interrupt, latency = 0, en = 1), (time =
> >> 47557628943806, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> >> (time = 47557628944922, irq = 165, fn = dwc3_interrupt, latency = 0,
> >> en = 1), (time = 47557628945229, irq = 165, fn = dwc3_interrupt,
> >> latency = 0, en = 3), (time = 47557628946345, irq = 165, fn =
> >> dwc3_interrupt, latency = 0, en = 1), (time = 47557628946614, irq =
> >> 165, fn = dwc3_interrupt, latency = 0, en = 3), (time =
> >> 47557628947729, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> >> (time = 47557628947999, irq = 165, fn = dwc3_interrupt, latency = 0,
> >> en = 3),
> >>
> >> Signed-off-by: JaeHun Jung <jh0801.jung@samsung.com>
> >> ---
> >>   drivers/usb/dwc3/gadget.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-) diff --git
> >> a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index
> >> 789976567f9f..5d2d5a9b9915 100644
> >> --- a/drivers/usb/dwc3/gadget.c
> >> +++ b/drivers/usb/dwc3/gadget.c
> >> @@ -4355,8 +4355,11 @@ static irqreturn_t dwc3_check_event_buf(struct
> >> dwc3_event_buffer *evt)
> >>        * irq event handler completes before caching new event to
> >> prevent
> >>        * losing events.
> >>        */
> >> -    if (evt->flags & DWC3_EVENT_PENDING)
> >> +    if (evt->flags & DWC3_EVENT_PENDING) {
> >> +        if (!evt->count)
> >> +            evt->flags &= ~DWC3_EVENT_PENDING;
> >>           return IRQ_HANDLED;
> >> +    }
> >>         count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> >>       count &= DWC3_GEVNTCOUNT_MASK;
> >
> > how about below change ?
> >
> >         count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> >         count &= DWC3_GEVNTCOUNT_MASK;
> > -       if (!count)
> > +       if (!count) {
> > +               evt->flags &= ~DWC3_EVENT_PENDING;
> >                 return IRQ_NONE;
> > +       }
> ignore my suggestion, add Thinh to comment.
> >
> >         evt->count = count;
> >         evt->flags |= DWC3_EVENT_PENDING;
> >

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Linyu Yuan]

On 1/5/2023 5:54 PM, 정재훈 wrote:
>
>> -----Original Message-----
>> From: Linyu Yuan [mailto:quic_linyyuan@quicinc.com]
>> Sent: Thursday, January 5, 2023 12:35 PM
>> To: JaeHun Jung; Felipe Balbi; Greg Kroah-Hartman; Thinh Nguyen
>> Cc: open list:USB XHCI DRIVER; open list; Seungchull Suh; Daehwan Jung
>> Subject: Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0
>>
>>
>> On 1/5/2023 11:29 AM, Linyu Yuan wrote:
>>> On 1/2/2023 1:08 PM, JaeHun Jung wrote:
>>>> Sometimes very rarely, The count is 0 and the DWC3 flag is set has
>>>> status.
>>>> It must not have these status. Because, It can make happen interrupt
>>>> storming status.
>>> could you help explain without clear the flag, how interrupt storming
>>> happen ?
>>>
>>> as your change didn't touch any hardware register, i don't know how it
>>> fix storming.
>>>
> H/W interrupts are still occur on IP.

I guess we should fix it from IP layer.


but when checking DWC3_EVENT_PENDING flag, it will auto clear in dwc3 
thread irq handler.

there is one possible root cause is it cleared only after irq enabled in 
dwc3_process_event_buf(),

we should move unmask irq operation at end of this function.


> But. ISR doesn't handle it. Because of this
> "if (evt->flags & DWC3_EVENT_PENDING)"
>
> This is event values.
> (struct dwc3_event_buffer *) ev_buf = 0xFFFFFF883DBF1180 = kernel_size_le_lo32+0xFFFFFF883B391180 -> (
>          (void *) buf = 0xFFFFFFC00DBDD000 = end+0x337D000 -> ,
>          (void *) cache = 0xFFFFFF8839F54080 = kernel_size_le_lo32+0xFFFFFF88376F4080 -> ,
>          (unsigned int) length = 0x1000,
>          (unsigned int) lpos = 0x0,
>          (unsigned int) count = 0x0,
>          (unsigned int) flags = 0x00100001,
>          (dma_addr_t) dma = 0x00000008BD7D7000,
>
> *((struct dwc3_event_type *) 0xFFFFFF8839F54080) = (
>      is_devspec = 1,
>      type = 0,
>      reserved8_31 = 774)
>
> *((struct dwc3_event_devt  *) 0xFFFFFF8839F54080) = (
>      one_bit = 1,
>      device_event = 0,
>      type = 6, << DWC3_DEVICE_EVENT_SUSPEND
>      reserved15_12 = 0,
>      event_info = 3,
>      reserved31_25 = 0)
>
> Occurred interrupts are "DWC3_DEVICE_EVENT_SUSPEND".
> If when "count has 0" and DWC3_EVENT_PENDING are set at the same time than
>   ISR and bottom thread couldn't handle next interrupt.
> We guessing connected with "dwc3_gadget_process_pending_events()" function.
> But, We could not find reproduce way.
>
>
>>> storming from software layer ?
>>>
>>>> So, It have to clean up DWC3_EVENT_PENDING flags set when count is 0.
>>>> It means "There are no interrupts to handle.".
>>>>
>>>> (struct dwc3_event_buffer *) ev_buf = 0xFFFFFF883DBF1180 (
>>>>      (void *) buf = 0xFFFFFFC00DBDD000 = end+0x337D000,
>>>>      (void *) cache = 0xFFFFFF8839F54080,
>>>>      (unsigned int) length = 0x1000,
>>>>      (unsigned int) lpos = 0x0,
>>>>      (unsigned int) count = 0x0,
>>>>      (unsigned int) flags = 0x00000001,
>>>>      (dma_addr_t) dma = 0x00000008BD7D7000,
>>>>      (struct dwc3 *) dwc = 0xFFFFFF8839CBC880,
>>>>      (u64) android_kabi_reserved1 = 0x0),
>>>>
>>>> (time = 47557628930999, irq = 165, fn = dwc3_interrupt, latency = 0,
>>>> en = 1), (time = 47557628931268, irq = 165, fn = dwc3_interrupt,
>>>> latency = 0, en = 3), (time = 47557628932383, irq = 165, fn =
>>>> dwc3_interrupt, latency = 0, en = 1), (time = 47557628932652, irq =
>>>> 165, fn = dwc3_interrupt, latency = 0, en = 3), (time =
>>>> 47557628933768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
>>>> (time = 47557628934037, irq = 165, fn = dwc3_interrupt, latency = 0,
>>>> en = 3), (time = 47557628935152, irq = 165, fn = dwc3_interrupt,
>>>> latency = 0, en = 1), (time = 47557628935460, irq = 165, fn =
>>>> dwc3_interrupt, latency = 0, en = 3), (time = 47557628936575, irq =
>>>> 165, fn = dwc3_interrupt, latency = 0, en = 1), (time =
>>>> 47557628936845, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
>>>> (time = 47557628937960, irq = 165, fn = dwc3_interrupt, latency = 0,
>>>> en = 1), (time = 47557628938229, irq = 165, fn = dwc3_interrupt,
>>>> latency = 0, en = 3), (time = 47557628939345, irq = 165, fn =
>>>> dwc3_interrupt, latency = 0, en = 1), (time = 47557628939652, irq =
>>>> 165, fn = dwc3_interrupt, latency = 0, en = 3), (time =
>>>> 47557628940768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
>>>> (time = 47557628941037, irq = 165, fn = dwc3_interrupt, latency = 0,
>>>> en = 3), (time = 47557628942152, irq = 165, fn = dwc3_interrupt,
>>>> latency = 0, en = 1), (time = 47557628942422, irq = 165, fn =
>>>> dwc3_interrupt, latency = 0, en = 3), (time = 47557628943537, irq =
>>>> 165, fn = dwc3_interrupt, latency = 0, en = 1), (time =
>>>> 47557628943806, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
>>>> (time = 47557628944922, irq = 165, fn = dwc3_interrupt, latency = 0,
>>>> en = 1), (time = 47557628945229, irq = 165, fn = dwc3_interrupt,
>>>> latency = 0, en = 3), (time = 47557628946345, irq = 165, fn =
>>>> dwc3_interrupt, latency = 0, en = 1), (time = 47557628946614, irq =
>>>> 165, fn = dwc3_interrupt, latency = 0, en = 3), (time =
>>>> 47557628947729, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
>>>> (time = 47557628947999, irq = 165, fn = dwc3_interrupt, latency = 0,
>>>> en = 3),
>>>>
>>>> Signed-off-by: JaeHun Jung <jh0801.jung@samsung.com>
>>>> ---
>>>>    drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-) diff --git
>>>> a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index
>>>> 789976567f9f..5d2d5a9b9915 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -4355,8 +4355,11 @@ static irqreturn_t dwc3_check_event_buf(struct
>>>> dwc3_event_buffer *evt)
>>>>         * irq event handler completes before caching new event to
>>>> prevent
>>>>         * losing events.
>>>>         */
>>>> -    if (evt->flags & DWC3_EVENT_PENDING)
>>>> +    if (evt->flags & DWC3_EVENT_PENDING) {
>>>> +        if (!evt->count)
>>>> +            evt->flags &= ~DWC3_EVENT_PENDING;
>>>>            return IRQ_HANDLED;
>>>> +    }
>>>>          count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>>>>        count &= DWC3_GEVNTCOUNT_MASK;
>>> how about below change ?
>>>
>>>          count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>>>          count &= DWC3_GEVNTCOUNT_MASK;
>>> -       if (!count)
>>> +       if (!count) {
>>> +               evt->flags &= ~DWC3_EVENT_PENDING;
>>>                  return IRQ_NONE;
>>> +       }
>> ignore my suggestion, add Thinh to comment.
>>>          evt->count = count;
>>>          evt->flags |= DWC3_EVENT_PENDING;
>>>

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Thinh Nguyen]

On Fri, Jan 06, 2023, Linyu Yuan wrote:
> On 1/5/2023 5:54 PM, 정재훈 wrote:
> > 
> > > -----Original Message-----
> > > From: Linyu Yuan [mailto:quic_linyyuan@quicinc.com]
> > > Sent: Thursday, January 5, 2023 12:35 PM
> > > To: JaeHun Jung; Felipe Balbi; Greg Kroah-Hartman; Thinh Nguyen
> > > Cc: open list:USB XHCI DRIVER; open list; Seungchull Suh; Daehwan Jung
> > > Subject: Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0
> > > 
> > > 
> > > On 1/5/2023 11:29 AM, Linyu Yuan wrote:
> > > > On 1/2/2023 1:08 PM, JaeHun Jung wrote:
> > > > > Sometimes very rarely, The count is 0 and the DWC3 flag is set has
> > > > > status.
> > > > > It must not have these status. Because, It can make happen interrupt
> > > > > storming status.
> > > > could you help explain without clear the flag, how interrupt storming
> > > > happen ?
> > > > 
> > > > as your change didn't touch any hardware register, i don't know how it
> > > > fix storming.
> > > > 
> > H/W interrupts are still occur on IP.
> 
> I guess we should fix it from IP layer.
> 

How are you certain the problem is from IP layer?

> 
> but when checking DWC3_EVENT_PENDING flag, it will auto clear in dwc3 thread
> irq handler.
> 
> there is one possible root cause is it cleared only after irq enabled in
> dwc3_process_event_buf(),
> 
> we should move unmask irq operation at end of this function.
> 

This interrupt storm can happen because we clear the evt->flags _after_
we unmask the interrupt. This was done to prevent false interrupt from
delay interrupt deassertion, which can be a problem for legacy pci
interrupt.

see 7441b273388b ("usb: dwc3: gadget: Fix event pending check")

The change JaeHun Jung did should be fine.

BR,
Thinh

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Thinh Nguyen]

Hi,

On Mon, Jan 02, 2023, JaeHun Jung wrote:
> Sometimes very rarely, The count is 0 and the DWC3 flag is set has status.
> It must not have these status. Because, It can make happen interrupt storming
> status.
> So, It have to clean up DWC3_EVENT_PENDING flags set when count is 0.
> It means "There are no interrupts to handle.".

Can you reword to include the explanation from my response [*] and add a
fixes tag to 7441b273388b ("usb: dwc3: gadget: Fix event pending check")

Thanks,
Thinh

[*] https://lore.kernel.org/linux-usb/20230109182813.sle5h34wdgglnlph@synopsys.com/T/#md32deea7e6b3c6d309aadba3d1cd2800d173685e


> 
> (struct dwc3_event_buffer *) ev_buf = 0xFFFFFF883DBF1180 (
> 	(void *) buf = 0xFFFFFFC00DBDD000 = end+0x337D000,
> 	(void *) cache = 0xFFFFFF8839F54080,
> 	(unsigned int) length = 0x1000,
> 	(unsigned int) lpos = 0x0,
> 	(unsigned int) count = 0x0,
> 	(unsigned int) flags = 0x00000001,
> 	(dma_addr_t) dma = 0x00000008BD7D7000,
> 	(struct dwc3 *) dwc = 0xFFFFFF8839CBC880,
> 	(u64) android_kabi_reserved1 = 0x0),
> 
> (time = 47557628930999, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628931268, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628932383, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628932652, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628933768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628934037, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628935152, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628935460, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628936575, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628936845, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628937960, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628938229, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628939345, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628939652, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628940768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628941037, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628942152, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628942422, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628943537, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628943806, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628944922, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628945229, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628946345, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628946614, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> (time = 47557628947729, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> (time = 47557628947999, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> 
> Signed-off-by: JaeHun Jung <jh0801.jung@samsung.com>
> ---
>  drivers/usb/dwc3/gadget.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 789976567f9f..5d2d5a9b9915 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -4355,8 +4355,11 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
>  	 * irq event handler completes before caching new event to prevent
>  	 * losing events.
>  	 */
> -	if (evt->flags & DWC3_EVENT_PENDING)
> +	if (evt->flags & DWC3_EVENT_PENDING) {
> +		if (!evt->count)
> +			evt->flags &= ~DWC3_EVENT_PENDING;
>  		return IRQ_HANDLED;
> +	}
>  
>  	count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>  	count &= DWC3_GEVNTCOUNT_MASK;
> -- 
> 2.31.1
> 

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Thinh Nguyen]

On Mon, Jan 09, 2023, Thinh Nguyen wrote:
> Hi,
> 
> On Mon, Jan 02, 2023, JaeHun Jung wrote:
> > Sometimes very rarely, The count is 0 and the DWC3 flag is set has status.
> > It must not have these status. Because, It can make happen interrupt storming
> > status.
> > So, It have to clean up DWC3_EVENT_PENDING flags set when count is 0.
> > It means "There are no interrupts to handle.".
> 
> Can you reword to include the explanation from my response [*] and add a
> fixes tag to 7441b273388b ("usb: dwc3: gadget: Fix event pending check")
> 

Actually, the old problem that the commit 7441b273388b fixes may
resurface because of this change.

Can you try this instead?

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 65500246323b..3c36dfdb88f0 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -5515,8 +5515,15 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
         * irq event handler completes before caching new event to prevent
         * losing events.
         */
-       if (evt->flags & DWC3_EVENT_PENDING)
+       if (evt->flags & DWC3_EVENT_PENDING) {
+               if (!evt->count) {
+                       u32 reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
+
+                       if (!(reg & DWC3_GEVNTSIZ_INTMASK))
+                               evt->flags &= ~DWC3_EVENT_PENDING;
+               }
                return IRQ_HANDLED;
+       }


This is a quick solution at the moment. Let me know if you have have a
better option. Note that reading a register may take some time, albeit
short. That's why we may want to keep the evt->count check to help with
performance.

Thanks,
Thinh


> 
> 
> > 
> > (struct dwc3_event_buffer *) ev_buf = 0xFFFFFF883DBF1180 (
> > 	(void *) buf = 0xFFFFFFC00DBDD000 = end+0x337D000,
> > 	(void *) cache = 0xFFFFFF8839F54080,
> > 	(unsigned int) length = 0x1000,
> > 	(unsigned int) lpos = 0x0,
> > 	(unsigned int) count = 0x0,
> > 	(unsigned int) flags = 0x00000001,
> > 	(dma_addr_t) dma = 0x00000008BD7D7000,
> > 	(struct dwc3 *) dwc = 0xFFFFFF8839CBC880,
> > 	(u64) android_kabi_reserved1 = 0x0),
> > 
> > (time = 47557628930999, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628931268, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > (time = 47557628932383, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628932652, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > (time = 47557628933768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628934037, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > (time = 47557628935152, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628935460, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > (time = 47557628936575, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628936845, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > (time = 47557628937960, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628938229, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > (time = 47557628939345, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628939652, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > (time = 47557628940768, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628941037, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > (time = 47557628942152, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628942422, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > (time = 47557628943537, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628943806, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > (time = 47557628944922, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628945229, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > (time = 47557628946345, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628946614, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > (time = 47557628947729, irq = 165, fn = dwc3_interrupt, latency = 0, en = 1),
> > (time = 47557628947999, irq = 165, fn = dwc3_interrupt, latency = 0, en = 3),
> > 
> > Signed-off-by: JaeHun Jung <jh0801.jung@samsung.com>
> > ---
> >  drivers/usb/dwc3/gadget.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 789976567f9f..5d2d5a9b9915 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -4355,8 +4355,11 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
> >  	 * irq event handler completes before caching new event to prevent
> >  	 * losing events.
> >  	 */
> > -	if (evt->flags & DWC3_EVENT_PENDING)
> > +	if (evt->flags & DWC3_EVENT_PENDING) {
> > +		if (!evt->count)
> > +			evt->flags &= ~DWC3_EVENT_PENDING;
> >  		return IRQ_HANDLED;
> > +	}
> >  
> >  	count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> >  	count &= DWC3_GEVNTCOUNT_MASK;
> > -- 
> > 2.31.1
> > 

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Linyu Yuan]

On 1/10/2023 2:28 AM, Thinh Nguyen wrote:
> On Fri, Jan 06, 2023, Linyu Yuan wrote:
>> On 1/5/2023 5:54 PM, 정재훈 wrote:
>>>> -----Original Message-----
>>>> From: Linyu Yuan [mailto:quic_linyyuan@quicinc.com]
>>>> Sent: Thursday, January 5, 2023 12:35 PM
>>>> To: JaeHun Jung; Felipe Balbi; Greg Kroah-Hartman; Thinh Nguyen
>>>> Cc: open list:USB XHCI DRIVER; open list; Seungchull Suh; Daehwan Jung
>>>> Subject: Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0
>>>>
>>>>
>>>> On 1/5/2023 11:29 AM, Linyu Yuan wrote:
>>>>> On 1/2/2023 1:08 PM, JaeHun Jung wrote:
>>>>>> Sometimes very rarely, The count is 0 and the DWC3 flag is set has
>>>>>> status.
>>>>>> It must not have these status. Because, It can make happen interrupt
>>>>>> storming status.
>>>>> could you help explain without clear the flag, how interrupt storming
>>>>> happen ?
>>>>>
>>>>> as your change didn't touch any hardware register, i don't know how it
>>>>> fix storming.
>>>>>
>>> H/W interrupts are still occur on IP.
>> I guess we should fix it from IP layer.
>>
> How are you certain the problem is from IP layer?

I think all IRQ is from DWC3 controller IP. if it is not IP layer, could 
you share how to prevent from SW layer ?

seem IRQ can happen when event count is zero ,  why this can happen ? 
does it mean event count register is not trust ?

>
>> but when checking DWC3_EVENT_PENDING flag, it will auto clear in dwc3 thread
>> irq handler.
>>
>> there is one possible root cause is it cleared only after irq enabled in
>> dwc3_process_event_buf(),
>>
>> we should move unmask irq operation at end of this function.
>>
> This interrupt storm can happen because we clear the evt->flags _after_
> we unmask the interrupt. This was done to prevent false interrupt from
> delay interrupt deassertion, which can be a problem for legacy pci
> interrupt.
thanks for explain, i didn't know that.
>
> see 7441b273388b ("usb: dwc3: gadget: Fix event pending check")
>
> The change JaeHun Jung did should be fine.
agree.
> BR,
> Thinh

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Thinh Nguyen]

On Tue, Jan 10, 2023, Linyu Yuan wrote:
> 
> On 1/10/2023 2:28 AM, Thinh Nguyen wrote:
> > On Fri, Jan 06, 2023, Linyu Yuan wrote:
> > > On 1/5/2023 5:54 PM, 정재훈 wrote:
> > > > > -----Original Message-----
> > > > > From: Linyu Yuan [mailto:quic_linyyuan@quicinc.com]
> > > > > Sent: Thursday, January 5, 2023 12:35 PM
> > > > > To: JaeHun Jung; Felipe Balbi; Greg Kroah-Hartman; Thinh Nguyen
> > > > > Cc: open list:USB XHCI DRIVER; open list; Seungchull Suh; Daehwan Jung
> > > > > Subject: Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0
> > > > > 
> > > > > 
> > > > > On 1/5/2023 11:29 AM, Linyu Yuan wrote:
> > > > > > On 1/2/2023 1:08 PM, JaeHun Jung wrote:
> > > > > > > Sometimes very rarely, The count is 0 and the DWC3 flag is set has
> > > > > > > status.
> > > > > > > It must not have these status. Because, It can make happen interrupt
> > > > > > > storming status.
> > > > > > could you help explain without clear the flag, how interrupt storming
> > > > > > happen ?
> > > > > > 
> > > > > > as your change didn't touch any hardware register, i don't know how it
> > > > > > fix storming.
> > > > > > 
> > > > H/W interrupts are still occur on IP.
> > > I guess we should fix it from IP layer.
> > > 
> > How are you certain the problem is from IP layer?
> 
> I think all IRQ is from DWC3 controller IP. if it is not IP layer, could you
> share how to prevent from SW layer ?
> 
> seem IRQ can happen when event count is zero ,  why this can happen ? does
> it mean event count register is not trust ?

When the interrupt is unmasked, the controller will generate interrupts
as events are received. Normally, the flag checking for pending event
should be cleared before unmasking the interrupt, but we clear it after
to account for possible false interrupt due to the nature of legacy pci
interrupt. This exposes a gap where the interrupts can come but the flag
isn't cleared. While it should be rare and shouldn't be too much of a
problem, we can avoid this scenario with some additional checks.

> 
> > 
> > > but when checking DWC3_EVENT_PENDING flag, it will auto clear in dwc3 thread
> > > irq handler.
> > > 
> > > there is one possible root cause is it cleared only after irq enabled in
> > > dwc3_process_event_buf(),
> > > 
> > > we should move unmask irq operation at end of this function.
> > > 
> > This interrupt storm can happen because we clear the evt->flags _after_
> > we unmask the interrupt. This was done to prevent false interrupt from
> > delay interrupt deassertion, which can be a problem for legacy pci
> > interrupt.
> thanks for explain, i didn't know that.
> > 
> > see 7441b273388b ("usb: dwc3: gadget: Fix event pending check")
> > 
> > The change JaeHun Jung did should be fine.
> agree.

The change may still need some additional check as suggested in my
response:
https://lore.kernel.org/linux-usb/20230109190914.3blihjfjdcszazdd@synopsys.com/T/#m7b907aa6da4023cb20fa00a57813d31fd84e081f

BR,
Thinh

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Linyu Yuan]

On 1/10/2023 10:53 AM, Thinh Nguyen wrote:
> On Tue, Jan 10, 2023, Linyu Yuan wrote:
>> On 1/10/2023 2:28 AM, Thinh Nguyen wrote:
>>> On Fri, Jan 06, 2023, Linyu Yuan wrote:
>>>> On 1/5/2023 5:54 PM, 정재훈 wrote:
>>>>>> -----Original Message-----
>>>>>> From: Linyu Yuan [mailto:quic_linyyuan@quicinc.com]
>>>>>> Sent: Thursday, January 5, 2023 12:35 PM
>>>>>> To: JaeHun Jung; Felipe Balbi; Greg Kroah-Hartman; Thinh Nguyen
>>>>>> Cc: open list:USB XHCI DRIVER; open list; Seungchull Suh; Daehwan Jung
>>>>>> Subject: Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0
>>>>>>
>>>>>>
>>>>>> On 1/5/2023 11:29 AM, Linyu Yuan wrote:
>>>>>>> On 1/2/2023 1:08 PM, JaeHun Jung wrote:
>>>>>>>> Sometimes very rarely, The count is 0 and the DWC3 flag is set has
>>>>>>>> status.
>>>>>>>> It must not have these status. Because, It can make happen interrupt
>>>>>>>> storming status.
>>>>>>> could you help explain without clear the flag, how interrupt storming
>>>>>>> happen ?
>>>>>>>
>>>>>>> as your change didn't touch any hardware register, i don't know how it
>>>>>>> fix storming.
>>>>>>>
>>>>> H/W interrupts are still occur on IP.
>>>> I guess we should fix it from IP layer.
>>>>
>>> How are you certain the problem is from IP layer?
>> I think all IRQ is from DWC3 controller IP. if it is not IP layer, could you
>> share how to prevent from SW layer ?
>>
>> seem IRQ can happen when event count is zero ,  why this can happen ? does
>> it mean event count register is not trust ?
> When the interrupt is unmasked, the controller will generate interrupts
> as events are received. Normally, the flag checking for pending event
> should be cleared before unmasking the interrupt, but we clear it after
> to account for possible false interrupt due to the nature of legacy pci
> interrupt. This exposes a gap where the interrupts can come but the flag
> isn't cleared. While it should be rare and shouldn't be too much of a
> problem, we can avoid this scenario with some additional checks.
>
>>>> but when checking DWC3_EVENT_PENDING flag, it will auto clear in dwc3 thread
>>>> irq handler.
>>>>
>>>> there is one possible root cause is it cleared only after irq enabled in
>>>> dwc3_process_event_buf(),
>>>>
>>>> we should move unmask irq operation at end of this function.
>>>>
>>> This interrupt storm can happen because we clear the evt->flags _after_
>>> we unmask the interrupt. This was done to prevent false interrupt from
>>> delay interrupt deassertion, which can be a problem for legacy pci
>>> interrupt.
>> thanks for explain, i didn't know that.
>>> see 7441b273388b ("usb: dwc3: gadget: Fix event pending check")
>>>
>>> The change JaeHun Jung did should be fine.
>> agree.
> The change may still need some additional check as suggested in my
> response:
> https://lore.kernel.org/linux-usb/20230109190914.3blihjfjdcszazdd@synopsys.com/T/#m7b907aa6da4023cb20fa00a57813d31fd84e081f
  do you think we need to read event count before checking 
DWC3_EVENT_PENDING  ?
>
> BR,
> Thinh

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Linyu Yuan]

On 1/10/2023 11:05 AM, Linyu Yuan wrote:
>
> On 1/10/2023 10:53 AM, Thinh Nguyen wrote:
>> On Tue, Jan 10, 2023, Linyu Yuan wrote:
>>> On 1/10/2023 2:28 AM, Thinh Nguyen wrote:
>>>> On Fri, Jan 06, 2023, Linyu Yuan wrote:
>>>>> On 1/5/2023 5:54 PM, 정재훈 wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Linyu Yuan [mailto:quic_linyyuan@quicinc.com]
>>>>>>> Sent: Thursday, January 5, 2023 12:35 PM
>>>>>>> To: JaeHun Jung; Felipe Balbi; Greg Kroah-Hartman; Thinh Nguyen
>>>>>>> Cc: open list:USB XHCI DRIVER; open list; Seungchull Suh; 
>>>>>>> Daehwan Jung
>>>>>>> Subject: Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when 
>>>>>>> count is 0
>>>>>>>
>>>>>>>
>>>>>>> On 1/5/2023 11:29 AM, Linyu Yuan wrote:
>>>>>>>> On 1/2/2023 1:08 PM, JaeHun Jung wrote:
>>>>>>>>> Sometimes very rarely, The count is 0 and the DWC3 flag is set 
>>>>>>>>> has
>>>>>>>>> status.
>>>>>>>>> It must not have these status. Because, It can make happen 
>>>>>>>>> interrupt
>>>>>>>>> storming status.
>>>>>>>> could you help explain without clear the flag, how interrupt 
>>>>>>>> storming
>>>>>>>> happen ?
>>>>>>>>
>>>>>>>> as your change didn't touch any hardware register, i don't know 
>>>>>>>> how it
>>>>>>>> fix storming.
>>>>>>>>
>>>>>> H/W interrupts are still occur on IP.
>>>>> I guess we should fix it from IP layer.
>>>>>
>>>> How are you certain the problem is from IP layer?
>>> I think all IRQ is from DWC3 controller IP. if it is not IP layer, 
>>> could you
>>> share how to prevent from SW layer ?
>>>
>>> seem IRQ can happen when event count is zero ,  why this can happen 
>>> ? does
>>> it mean event count register is not trust ?
>> When the interrupt is unmasked, the controller will generate interrupts
>> as events are received. Normally, the flag checking for pending event
>> should be cleared before unmasking the interrupt, but we clear it after
>> to account for possible false interrupt due to the nature of legacy pci
>> interrupt. This exposes a gap where the interrupts can come but the flag
>> isn't cleared. While it should be rare and shouldn't be too much of a
>> problem, we can avoid this scenario with some additional checks.
>>
>>>>> but when checking DWC3_EVENT_PENDING flag, it will auto clear in 
>>>>> dwc3 thread
>>>>> irq handler.
>>>>>
>>>>> there is one possible root cause is it cleared only after irq 
>>>>> enabled in
>>>>> dwc3_process_event_buf(),
>>>>>
>>>>> we should move unmask irq operation at end of this function.
>>>>>
>>>> This interrupt storm can happen because we clear the evt->flags 
>>>> _after_
>>>> we unmask the interrupt. This was done to prevent false interrupt from
>>>> delay interrupt deassertion, which can be a problem for legacy pci
>>>> interrupt.
>>> thanks for explain, i didn't know that.
>>>> see 7441b273388b ("usb: dwc3: gadget: Fix event pending check")
>>>>
>>>> The change JaeHun Jung did should be fine.
>>> agree.
>> The change may still need some additional check as suggested in my
>> response:
>> https://lore.kernel.org/linux-usb/20230109190914.3blihjfjdcszazdd@synopsys.com/T/#m7b907aa6da4023cb20fa00a57813d31fd84e081f 
>>
>  do you think we need to read event count before checking 
> DWC3_EVENT_PENDING  ?
sorry for this noise, may be i have a little understanding of the legcy 
pci issue now.
>>
>> BR,
>> Thinh

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Linyu Yuan]

On 1/10/2023 11:13 AM, Linyu Yuan wrote:
>
> On 1/10/2023 11:05 AM, Linyu Yuan wrote:
>>
>> On 1/10/2023 10:53 AM, Thinh Nguyen wrote:
>>> On Tue, Jan 10, 2023, Linyu Yuan wrote:
>>>> On 1/10/2023 2:28 AM, Thinh Nguyen wrote:
>>>>> On Fri, Jan 06, 2023, Linyu Yuan wrote:
>>>>>> On 1/5/2023 5:54 PM, 정재훈 wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Linyu Yuan [mailto:quic_linyyuan@quicinc.com]
>>>>>>>> Sent: Thursday, January 5, 2023 12:35 PM
>>>>>>>> To: JaeHun Jung; Felipe Balbi; Greg Kroah-Hartman; Thinh Nguyen
>>>>>>>> Cc: open list:USB XHCI DRIVER; open list; Seungchull Suh; 
>>>>>>>> Daehwan Jung
>>>>>>>> Subject: Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when 
>>>>>>>> count is 0
>>>>>>>>
>>>>>>>>
>>>>>>>> On 1/5/2023 11:29 AM, Linyu Yuan wrote:
>>>>>>>>> On 1/2/2023 1:08 PM, JaeHun Jung wrote:
>>>>>>>>>> Sometimes very rarely, The count is 0 and the DWC3 flag is 
>>>>>>>>>> set has
>>>>>>>>>> status.
>>>>>>>>>> It must not have these status. Because, It can make happen 
>>>>>>>>>> interrupt
>>>>>>>>>> storming status.
>>>>>>>>> could you help explain without clear the flag, how interrupt 
>>>>>>>>> storming
>>>>>>>>> happen ?
>>>>>>>>>
>>>>>>>>> as your change didn't touch any hardware register, i don't 
>>>>>>>>> know how it
>>>>>>>>> fix storming.
>>>>>>>>>
>>>>>>> H/W interrupts are still occur on IP.
>>>>>> I guess we should fix it from IP layer.
>>>>>>
>>>>> How are you certain the problem is from IP layer?
>>>> I think all IRQ is from DWC3 controller IP. if it is not IP layer, 
>>>> could you
>>>> share how to prevent from SW layer ?
>>>>
>>>> seem IRQ can happen when event count is zero ,  why this can happen 
>>>> ? does
>>>> it mean event count register is not trust ?
>>> When the interrupt is unmasked, the controller will generate interrupts
>>> as events are received. Normally, the flag checking for pending event
>>> should be cleared before unmasking the interrupt, but we clear it after
>>> to account for possible false interrupt due to the nature of legacy pci
>>> interrupt. This exposes a gap where the interrupts can come but the 
>>> flag
>>> isn't cleared. While it should be rare and shouldn't be too much of a
>>> problem, we can avoid this scenario with some additional checks.
>>>
>>>>>> but when checking DWC3_EVENT_PENDING flag, it will auto clear in 
>>>>>> dwc3 thread
>>>>>> irq handler.
>>>>>>
>>>>>> there is one possible root cause is it cleared only after irq 
>>>>>> enabled in
>>>>>> dwc3_process_event_buf(),
>>>>>>
>>>>>> we should move unmask irq operation at end of this function.
>>>>>>
>>>>> This interrupt storm can happen because we clear the evt->flags 
>>>>> _after_
>>>>> we unmask the interrupt. This was done to prevent false interrupt 
>>>>> from
>>>>> delay interrupt deassertion, which can be a problem for legacy pci
>>>>> interrupt.
>>>> thanks for explain, i didn't know that.
>>>>> see 7441b273388b ("usb: dwc3: gadget: Fix event pending check")
>>>>>
>>>>> The change JaeHun Jung did should be fine.
>>>> agree.
>>> The change may still need some additional check as suggested in my
>>> response:
>>> https://lore.kernel.org/linux-usb/20230109190914.3blihjfjdcszazdd@synopsys.com/T/#m7b907aa6da4023cb20fa00a57813d31fd84e081f 
>>>
>>  do you think we need to read event count before checking 
>> DWC3_EVENT_PENDING  ?
> sorry for this noise, may be i have a little understanding of the 
> legcy pci issue now.
one more question, is it legacy PCIe device still exist in real world ? 
and any VID/PID info ?
>>>
>>> BR,
>>> Thinh

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Thinh Nguyen]

On Tue, Jan 10, 2023, Linyu Yuan wrote:
> 
> On 1/10/2023 11:13 AM, Linyu Yuan wrote:
> > 
> > On 1/10/2023 11:05 AM, Linyu Yuan wrote:
> > > 
> > > On 1/10/2023 10:53 AM, Thinh Nguyen wrote:
> > > > On Tue, Jan 10, 2023, Linyu Yuan wrote:
> > > > > On 1/10/2023 2:28 AM, Thinh Nguyen wrote:
> > > > > > On Fri, Jan 06, 2023, Linyu Yuan wrote:
> > > > > > > On 1/5/2023 5:54 PM, 정재훈 wrote:
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Linyu Yuan [mailto:quic_linyyuan@quicinc.com]
> > > > > > > > > Sent: Thursday, January 5, 2023 12:35 PM
> > > > > > > > > To: JaeHun Jung; Felipe Balbi; Greg Kroah-Hartman; Thinh Nguyen
> > > > > > > > > Cc: open list:USB XHCI DRIVER; open list;
> > > > > > > > > Seungchull Suh; Daehwan Jung
> > > > > > > > > Subject: Re: [PATCH] usb: dwc3: Clear
> > > > > > > > > DWC3_EVENT_PENDING when count is 0
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 1/5/2023 11:29 AM, Linyu Yuan wrote:
> > > > > > > > > > On 1/2/2023 1:08 PM, JaeHun Jung wrote:
> > > > > > > > > > > Sometimes very rarely, The count is
> > > > > > > > > > > 0 and the DWC3 flag is set has
> > > > > > > > > > > status.
> > > > > > > > > > > It must not have these status.
> > > > > > > > > > > Because, It can make happen
> > > > > > > > > > > interrupt
> > > > > > > > > > > storming status.
> > > > > > > > > > could you help explain without clear the
> > > > > > > > > > flag, how interrupt storming
> > > > > > > > > > happen ?
> > > > > > > > > > 
> > > > > > > > > > as your change didn't touch any hardware
> > > > > > > > > > register, i don't know how it
> > > > > > > > > > fix storming.
> > > > > > > > > > 
> > > > > > > > H/W interrupts are still occur on IP.
> > > > > > > I guess we should fix it from IP layer.
> > > > > > > 
> > > > > > How are you certain the problem is from IP layer?
> > > > > I think all IRQ is from DWC3 controller IP. if it is not IP
> > > > > layer, could you
> > > > > share how to prevent from SW layer ?
> > > > > 
> > > > > seem IRQ can happen when event count is zero ,  why this can
> > > > > happen ? does
> > > > > it mean event count register is not trust ?
> > > > When the interrupt is unmasked, the controller will generate interrupts
> > > > as events are received. Normally, the flag checking for pending event
> > > > should be cleared before unmasking the interrupt, but we clear it after
> > > > to account for possible false interrupt due to the nature of legacy pci
> > > > interrupt. This exposes a gap where the interrupts can come but
> > > > the flag
> > > > isn't cleared. While it should be rare and shouldn't be too much of a
> > > > problem, we can avoid this scenario with some additional checks.
> > > > 
> > > > > > > but when checking DWC3_EVENT_PENDING flag, it will
> > > > > > > auto clear in dwc3 thread
> > > > > > > irq handler.
> > > > > > > 
> > > > > > > there is one possible root cause is it cleared only
> > > > > > > after irq enabled in
> > > > > > > dwc3_process_event_buf(),
> > > > > > > 
> > > > > > > we should move unmask irq operation at end of this function.
> > > > > > > 
> > > > > > This interrupt storm can happen because we clear the
> > > > > > evt->flags _after_
> > > > > > we unmask the interrupt. This was done to prevent false
> > > > > > interrupt from
> > > > > > delay interrupt deassertion, which can be a problem for legacy pci
> > > > > > interrupt.
> > > > > thanks for explain, i didn't know that.
> > > > > > see 7441b273388b ("usb: dwc3: gadget: Fix event pending check")
> > > > > > 
> > > > > > The change JaeHun Jung did should be fine.
> > > > > agree.
> > > > The change may still need some additional check as suggested in my
> > > > response:
> > > > https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20230109190914.3blihjfjdcszazdd@synopsys.com/T/*m7b907aa6da4023cb20fa00a57813d31fd84e081f__;Iw!!A4F2R9G_pg!dchc0UAeZnm7PcA-aav_58rsw7agMygwPSGC_kN8Ga_BS3oTbLMNh3DkfQ9B2njbva-tJzTj7Cb2dJnE5RbEW6KJnDmtFA$
> > > > 
> > >  do you think we need to read event count before checking
> > > DWC3_EVENT_PENDING  ?
> > sorry for this noise, may be i have a little understanding of the legcy
> > pci issue now.
> one more question, is it legacy PCIe device still exist in real world ? and
> any VID/PID info ?

Currently, all dwc3 PCIe devices are affected. Some setups are more
noticeable than others. The dwc3 driver is implemented to probe platform
devices. So, dwc3 PCIe devices are wrapped as platform devices for the
dwc3 driver. Since we're going through the platform device code path,
the pci layer falls back to using legacy interrupt instead of MSI (last
I check awhile ago).

A little more detail on this problem:
PCIe legacy interrupt will emulate interrupt line by sending an
interrupt assert and deassert messages. After the interrupt assert
message is sent, interrupts are continuously generated until the
deassert message is sent. If there's a register write to unmask/mask
interrupt or clearing events falls in between these messages, then there
may be a race.

Let's say we don't have event pending check, this can happen:

Normal scenario
---------------
    event_count += n # controller generates new events
  interrupt asserts
    write(mask irq)
    event_count -= n # dwc3 clears events
  interrupt deasserts
    write(unmask irq)


Race scenario
-------------
    event_count += n # new events
  interrupt asserts
    write(mask irq)
    event_count -= n # clear events
    event_count += n # more events come and hard irq handler gets called
		     # again as interrupt is generated, but cached
		     # events haven't been handled. This breaks
		     # serialization and causes lost events.
    write(mask irq)

    event_count -= n # clear events
  interrupt deasserts
    write(unmask irq) # events handled


For MSI, this won't be a problem because it's edge-triggered and the way
it sends interrupt is different.

BR,
Thinh

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Linyu Yuan]

On 1/11/2023 8:00 AM, Thinh Nguyen wrote:
>
>> one more question, is it legacy PCIe device still exist in real world ? and
>> any VID/PID info ?
> Currently, all dwc3 PCIe devices are affected. Some setups are more


if non PCIe device have no such issue, can we do some improvement for it ?

like new flag or static key/jump label to improve interrupt handler ?


> noticeable than others. The dwc3 driver is implemented to probe platform
> devices. So, dwc3 PCIe devices are wrapped as platform devices for the
> dwc3 driver. Since we're going through the platform device code path,
> the pci layer falls back to using legacy interrupt instead of MSI (last
> I check awhile ago).
>
> A little more detail on this problem:
> PCIe legacy interrupt will emulate interrupt line by sending an
> interrupt assert and deassert messages. After the interrupt assert
> message is sent, interrupts are continuously generated until the
> deassert message is sent. If there's a register write to unmask/mask
> interrupt or clearing events falls in between these messages, then there
> may be a race.
>
> Let's say we don't have event pending check, this can happen:
>
> Normal scenario
> ---------------
>      event_count += n # controller generates new events
>    interrupt asserts
>      write(mask irq)
>      event_count -= n # dwc3 clears events
>    interrupt deasserts
>      write(unmask irq)
>
>
> Race scenario
> -------------
>      event_count += n # new events
>    interrupt asserts
>      write(mask irq)
>      event_count -= n # clear events
>      event_count += n # more events come and hard irq handler gets called
> 		     # again as interrupt is generated, but cached
> 		     # events haven't been handled. This breaks
> 		     # serialization and causes lost events.
>      write(mask irq)
>
>      event_count -= n # clear events
>    interrupt deasserts
>      write(unmask irq) # events handled


if mask irq is not working, the race will happen like this, thanks for 
explanation.


>
> For MSI, this won't be a problem because it's edge-triggered and the way
> it sends interrupt is different.
>
> BR,
> Thinh

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Thinh Nguyen]

On Wed, Jan 11, 2023, Linyu Yuan wrote:
> 
> On 1/11/2023 8:00 AM, Thinh Nguyen wrote:
> > 
> > > one more question, is it legacy PCIe device still exist in real world ? and
> > > any VID/PID info ?
> > Currently, all dwc3 PCIe devices are affected. Some setups are more
> 
> 
> if non PCIe device have no such issue, can we do some improvement for it ?
> 
> like new flag or static key/jump label to improve interrupt handler ?
> 

There are different ways to handle this. At the time, it was (and is?)
the simplest solution. There isn't any observable performance
degradation from testing so there's no incentive to change it yet.

BR,
Thinh

> 
> > noticeable than others. The dwc3 driver is implemented to probe platform
> > devices. So, dwc3 PCIe devices are wrapped as platform devices for the
> > dwc3 driver. Since we're going through the platform device code path,
> > the pci layer falls back to using legacy interrupt instead of MSI (last
> > I check awhile ago).
> > 
> > A little more detail on this problem:
> > PCIe legacy interrupt will emulate interrupt line by sending an
> > interrupt assert and deassert messages. After the interrupt assert
> > message is sent, interrupts are continuously generated until the
> > deassert message is sent. If there's a register write to unmask/mask
> > interrupt or clearing events falls in between these messages, then there
> > may be a race.
> > 
> > Let's say we don't have event pending check, this can happen:
> > 
> > Normal scenario
> > ---------------
> >      event_count += n # controller generates new events
> >    interrupt asserts
> >      write(mask irq)
> >      event_count -= n # dwc3 clears events
> >    interrupt deasserts
> >      write(unmask irq)
> > 
> > 
> > Race scenario
> > -------------
> >      event_count += n # new events
> >    interrupt asserts
> >      write(mask irq)
> >      event_count -= n # clear events
> >      event_count += n # more events come and hard irq handler gets called
> > 		     # again as interrupt is generated, but cached
> > 		     # events haven't been handled. This breaks
> > 		     # serialization and causes lost events.
> >      write(mask irq)
> > 
> >      event_count -= n # clear events
> >    interrupt deasserts
> >      write(unmask irq) # events handled
> 
> 
> if mask irq is not working, the race will happen like this, thanks for
> explanation.
> 
> 
> > 
> > For MSI, this won't be a problem because it's edge-triggered and the way
> > it sends interrupt is different.
> > 

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Linyu Yuan]

hi Thinh,


regarding your suggestion, assume it is not PCIe type,  still have one 
question,


-       if (evt->flags & DWC3_EVENT_PENDING)
+       if (evt->flags & DWC3_EVENT_PENDING) {
+               if (!evt->count) {
+                       u32 reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
+
+                       if (!(reg & DWC3_GEVNTSIZ_INTMASK))
+                               evt->flags &= ~DWC3_EVENT_PENDING;

do we need to return IRQ_WAKE_THREAD  ?

+               }
                 return IRQ_HANDLED;

as here return IRQ HANDLED, how can we make sure a new IRQ will be 
handled after previous IRQ thread clean PENDING flag ?

+       }


also for non-PCIe controller, consider IRQ mask register working correctly,

consider a case IRQ happen before IRQ thread exit,  here just return 
IRQ_HANDLED.

once IRQ thread exit, it will clean PENDING flag, so next IRQ event will 
run normally.

if 정재훈 saw PENDING flag is not cleared, does it mean IRQ thread have no 
chance to exit ?

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Thinh Nguyen]

On Tue, Jan 31, 2023, Linyu Yuan wrote:
> hi Thinh,
> 
> 
> regarding your suggestion, assume it is not PCIe type,  still have one
> question,
> 
> 
> -       if (evt->flags & DWC3_EVENT_PENDING)
> +       if (evt->flags & DWC3_EVENT_PENDING) {
> +               if (!evt->count) {
> +                       u32 reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
> +
> +                       if (!(reg & DWC3_GEVNTSIZ_INTMASK))
> +                               evt->flags &= ~DWC3_EVENT_PENDING;
> 
> do we need to return IRQ_WAKE_THREAD  ?

No, if evt->count is 0, but GEVNTCOUNT is > 0, the controller will
generate interrupt. The evt->count will be updated and the events will
be handled on the next interrupt.

> 
> +               }
>                 return IRQ_HANDLED;
> 
> as here return IRQ HANDLED, how can we make sure a new IRQ will be handled
> after previous IRQ thread clean PENDING flag ?

If evt->count > 0, that means the bottom half is still running. So,
leave it be. If evt->count == 0, then the cached events are processed,
we're safe to clear the PENDING flag. New interrupt will be generated if
GEVNTCOUNT is > 0.

> 
> +       }
> 
> 
> also for non-PCIe controller, consider IRQ mask register working correctly,
> 
> consider a case IRQ happen before IRQ thread exit,  here just return
> IRQ_HANDLED.
> 
> once IRQ thread exit, it will clean PENDING flag, so next IRQ event will run
> normally.
> 
> if 정재훈 saw PENDING flag is not cleared, does it mean IRQ thread have no
> chance to exit ?

The PENDING flag should be cleared eventually when the bottom half
completes. I don't expect the interrupt storm to block the IRQ thread
forever, but I can't guarantee the device behavor. 정재훈 can confirm.
This change should resolve the interrupt storm.

BR,
Thinh

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Linyu Yuan]

On 2/2/2023 2:57 AM, Thinh Nguyen wrote:
> On Tue, Jan 31, 2023, Linyu Yuan wrote:
>> hi Thinh,
>>
>>
>> regarding your suggestion, assume it is not PCIe type,  still have one
>> question,
>>
>>
>> -       if (evt->flags & DWC3_EVENT_PENDING)
>> +       if (evt->flags & DWC3_EVENT_PENDING) {
>> +               if (!evt->count) {
>> +                       u32 reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
>> +
>> +                       if (!(reg & DWC3_GEVNTSIZ_INTMASK))
>> +                               evt->flags &= ~DWC3_EVENT_PENDING;
>>
>> do we need to return IRQ_WAKE_THREAD  ?
> No, if evt->count is 0, but GEVNTCOUNT is > 0, the controller will
> generate interrupt. The evt->count will be updated and the events will
> be handled on the next interrupt.


when will next interrupt happen ?

as when enter here, i guess GEVENTCOUNT is already > 0, but we didn't 
read it.


>
>> +               }
>>                  return IRQ_HANDLED;
>>
>> as here return IRQ HANDLED, how can we make sure a new IRQ will be handled
>> after previous IRQ thread clean PENDING flag ?
> If evt->count > 0, that means the bottom half is still running. So,
> leave it be. If evt->count == 0, then the cached events are processed,
> we're safe to clear the PENDING flag. New interrupt will be generated if
> GEVNTCOUNT is > 0.
>
>> +       }
>>
>>
>> also for non-PCIe controller, consider IRQ mask register working correctly,
>>
>> consider a case IRQ happen before IRQ thread exit,  here just return
>> IRQ_HANDLED.
>>
>> once IRQ thread exit, it will clean PENDING flag, so next IRQ event will run
>> normally.
>>
>> if 정재훈 saw PENDING flag is not cleared, does it mean IRQ thread have no
>> chance to exit ?
> The PENDING flag should be cleared eventually when the bottom half
> completes. I don't expect the interrupt storm to block the IRQ thread
> forever, but I can't guarantee the device behavor. 정재훈 can confirm.
> This change should resolve the interrupt storm.
>
> BR,
> Thinh

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Thinh Nguyen]

On Thu, Feb 02, 2023, Linyu Yuan wrote:
> 
> On 2/2/2023 2:57 AM, Thinh Nguyen wrote:
> > On Tue, Jan 31, 2023, Linyu Yuan wrote:
> > > hi Thinh,
> > > 
> > > 
> > > regarding your suggestion, assume it is not PCIe type,  still have one
> > > question,
> > > 
> > > 
> > > -       if (evt->flags & DWC3_EVENT_PENDING)
> > > +       if (evt->flags & DWC3_EVENT_PENDING) {
> > > +               if (!evt->count) {
> > > +                       u32 reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
> > > +
> > > +                       if (!(reg & DWC3_GEVNTSIZ_INTMASK))
> > > +                               evt->flags &= ~DWC3_EVENT_PENDING;
> > > 
> > > do we need to return IRQ_WAKE_THREAD  ?
> > No, if evt->count is 0, but GEVNTCOUNT is > 0, the controller will
> > generate interrupt. The evt->count will be updated and the events will
> > be handled on the next interrupt.
> 
> 
> when will next interrupt happen ?

Immediately after. You can test this by just return IRQ_HANDLED and not
clear the GEVNTCOUNT to see its behavior.

> 
> as when enter here, i guess GEVENTCOUNT is already > 0, but we didn't read
> it.

GEVNTCOUNT is always updating as new events are generated. We only clear
however many events we process, but that doesn't stop it from
incrementing.

BR,
Thinh

> 
> 
> > 
> > > +               }
> > >                  return IRQ_HANDLED;
> > > 
> > > as here return IRQ HANDLED, how can we make sure a new IRQ will be handled
> > > after previous IRQ thread clean PENDING flag ?
> > If evt->count > 0, that means the bottom half is still running. So,
> > leave it be. If evt->count == 0, then the cached events are processed,
> > we're safe to clear the PENDING flag. New interrupt will be generated if
> > GEVNTCOUNT is > 0.
> > 
> > > +       }
> > > 
> > > 
> > > also for non-PCIe controller, consider IRQ mask register working correctly,
> > > 
> > > consider a case IRQ happen before IRQ thread exit,  here just return
> > > IRQ_HANDLED.
> > > 
> > > once IRQ thread exit, it will clean PENDING flag, so next IRQ event will run
> > > normally.
> > > 
> > > if 정재훈 saw PENDING flag is not cleared, does it mean IRQ thread have no
> > > chance to exit ?
> > The PENDING flag should be cleared eventually when the bottom half
> > completes. I don't expect the interrupt storm to block the IRQ thread
> > forever, but I can't guarantee the device behavor. 정재훈 can confirm.
> > This change should resolve the interrupt storm.
> > 
> > BR,
> > Thinh

Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

[Linyu Yuan]

On 2/3/2023 4:06 AM, Thinh Nguyen wrote:
> On Thu, Feb 02, 2023, Linyu Yuan wrote:
>> On 2/2/2023 2:57 AM, Thinh Nguyen wrote:
>>> On Tue, Jan 31, 2023, Linyu Yuan wrote:
>>>> hi Thinh,
>>>>
>>>>
>>>> regarding your suggestion, assume it is not PCIe type,  still have one
>>>> question,
>>>>
>>>>
>>>> -       if (evt->flags & DWC3_EVENT_PENDING)
>>>> +       if (evt->flags & DWC3_EVENT_PENDING) {
>>>> +               if (!evt->count) {
>>>> +                       u32 reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
>>>> +
>>>> +                       if (!(reg & DWC3_GEVNTSIZ_INTMASK))
>>>> +                               evt->flags &= ~DWC3_EVENT_PENDING;
>>>>
>>>> do we need to return IRQ_WAKE_THREAD  ?
>>> No, if evt->count is 0, but GEVNTCOUNT is > 0, the controller will
>>> generate interrupt. The evt->count will be updated and the events will
>>> be handled on the next interrupt.
>>
>> when will next interrupt happen ?
> Immediately after. You can test this by just return IRQ_HANDLED and not
> clear the GEVNTCOUNT to see its behavior.


if it immediately, it will be good.


정재훈  could you update a new patch which Thinh suggest.

maybe we didn't find the root cause of irq strom, but the change have no side effect.


>
>> as when enter here, i guess GEVENTCOUNT is already > 0, but we didn't read
>> it.
> GEVNTCOUNT is always updating as new events are generated. We only clear
> however many events we process, but that doesn't stop it from
> incrementing.


just consider if there is a case that next GEVNETCOUNT increase which 
happen long time later,

maybe think too much.


>
> BR,
> Thinh
>
>>
>>>> +               }
>>>>                   return IRQ_HANDLED;
>>>>
>>>> as here return IRQ HANDLED, how can we make sure a new IRQ will be handled
>>>> after previous IRQ thread clean PENDING flag ?
>>> If evt->count > 0, that means the bottom half is still running. So,
>>> leave it be. If evt->count == 0, then the cached events are processed,
>>> we're safe to clear the PENDING flag. New interrupt will be generated if
>>> GEVNTCOUNT is > 0.
>>>
>>>> +       }
>>>>
>>>>
>>>> also for non-PCIe controller, consider IRQ mask register working correctly,
>>>>
>>>> consider a case IRQ happen before IRQ thread exit,  here just return
>>>> IRQ_HANDLED.
>>>>
>>>> once IRQ thread exit, it will clean PENDING flag, so next IRQ event will run
>>>> normally.
>>>>
>>>> if 정재훈 saw PENDING flag is not cleared, does it mean IRQ thread have no
>>>> chance to exit ?
>>> The PENDING flag should be cleared eventually when the bottom half
>>> completes. I don't expect the interrupt storm to block the IRQ thread
>>> forever, but I can't guarantee the device behavor. 정재훈 can confirm.
>>> This change should resolve the interrupt storm.
>>>
>>> BR,
>>> Thinh