usb: dwc3: gadget: fix miss isoc issue introduced by IRQ latency

Commit Message

Zengtao (B) Dec. 14, 2018, 4:32 p.m. UTC

If it's a busy system, some times when we start an isoc transfer, the
framenumber get from the event buffer may be already elasped, in this
 case, we will get all the packets dropped due to miss isoc. And we turn
into transfer nothing, to fix this issue, we need to fix the framenumber
to make sure that it's not out of date.

Signed-off-by: Liang Shengjun <[e-mail]>
Signed-off-by: Zeng Tao <[e-mail]>
---
 drivers/usb/dwc3/core.h   |  3 +++
 drivers/usb/dwc3/gadget.c | 12 ++++++++++++
 2 files changed, 15 insertions(+)

Comments

Felipe Balbi Dec. 14, 2018, 8:51 a.m. UTC | #1

Zeng Tao <[e-mail]> writes:

> If it's a busy system, some times when we start an isoc transfer, the
> framenumber get from the event buffer may be already elasped, in this
>  case, we will get all the packets dropped due to miss isoc. And we turn
> into transfer nothing, to fix this issue, we need to fix the framenumber
> to make sure that it's not out of date.
>
> Signed-off-by: Liang Shengjun <[e-mail]>
> Signed-off-by: Zeng Tao <[e-mail]>

There's a patch going upstream already doing this:

https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit/?h=next&id=d53701067f048b8b11635e964b6d3bd9a248c622

Zengtao (B) Dec. 14, 2018, 9:11 a.m. UTC | #2

>-----Original Message-----
>From: Felipe Balbi [mailto:[e-mail]]
>Sent: Friday, December 14, 2018 4:52 PM
>To: Zengtao (B) <[e-mail]>
>Cc: liangshengjun <[e-mail]>; Zengtao (B)
><[e-mail]>; Greg Kroah-Hartman
><[e-mail]>; [e-mail];
>[e-mail]
>Subject: Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by
>IRQ latency
>
>* PGP Signed by an unknown key
>
>Zeng Tao <[e-mail]> writes:
>
>> If it's a busy system, some times when we start an isoc transfer, the
>> framenumber get from the event buffer may be already elasped, in this
>> case, we will get all the packets dropped due to miss isoc. And we
>> turn into transfer nothing, to fix this issue, we need to fix the
>> framenumber to make sure that it's not out of date.
>>
>> Signed-off-by: Liang Shengjun <[e-mail]>
>> Signed-off-by: Zeng Tao <[e-mail]>
>
>There's a patch going upstream already doing this:
>
>https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit/?h
>=next&id=d53701067f048b8b11635e964b6d3bd9a248c622
>

Sorry, I think I missed to update to the latest version. But I think my 
patch is more efficient. Because I just sync the frame from the HW, it
 won't fail and there is no need to extra tries.

What do you think about it?

Regards
Zengtao
>--
>balbi
>
>* Unknown Key
>* 0xE11A9906

Sergei Shtylyov Dec. 14, 2018, 11:17 a.m. UTC | #3

Hello!

On 12/14/2018 07:32 PM, Zeng Tao wrote:

> If it's a busy system, some times when we start an isoc transfer, the
> framenumber get from the event buffer may be already elasped, in this

   Frame number? Else my spell checker trips. :-)

>  case, we will get all the packets dropped due to miss isoc. And we turn

   Remove the leading space, please.

> into transfer nothing, to fix this issue, we need to fix the framenumber
> to make sure that it's not out of date.
> 
> Signed-off-by: Liang Shengjun <[e-mail]>
> Signed-off-by: Zeng Tao <[e-mail]>
[...]
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 9f92ee0..b63bd72 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1263,6 +1263,15 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>  	return DWC3_DSTS_SOFFN(reg);
>  }
>  
> +static bool __dwc3_gadget_target_frame_elapsed(struct dwc3_ep *dep)
> +{
> +	u16 cframe =  __dwc3_gadget_get_frame(dep->dwc);
> +	u16 eframe = dep->frame_number & DWC3_EVENT_PRAM_SOFFN_MASK;
> +
> +	return (((eframe - cframe) & DWC3_EVENT_PRAM_SOFFN_MASK)
> +		> DWC3_EVENT_PRAM_MAX_SOFFN / 2);

   Please leave > on the previous line.
   And you surely know that the outer parens are unnecessary? :-)

[...]

MBR, Sergei

Felipe Balbi Dec. 14, 2018, 11:23 a.m. UTC | #4

Hi,

"Zengtao (B)" <[e-mail]> writes:
>>-----Original Message-----
>>From: Felipe Balbi [mailto:[e-mail]]
>>Sent: Friday, December 14, 2018 4:52 PM
>>To: Zengtao (B) <[e-mail]>
>>Cc: liangshengjun <[e-mail]>; Zengtao (B)
>><[e-mail]>; Greg Kroah-Hartman
>><[e-mail]>; [e-mail];
>>[e-mail]
>>Subject: Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by
>>IRQ latency
>>
>>* PGP Signed by an unknown key
>>
>>Zeng Tao <[e-mail]> writes:
>>
>>> If it's a busy system, some times when we start an isoc transfer, the
>>> framenumber get from the event buffer may be already elasped, in this
>>> case, we will get all the packets dropped due to miss isoc. And we
>>> turn into transfer nothing, to fix this issue, we need to fix the
>>> framenumber to make sure that it's not out of date.
>>>
>>> Signed-off-by: Liang Shengjun <[e-mail]>
>>> Signed-off-by: Zeng Tao <[e-mail]>
>>
>>There's a patch going upstream already doing this:
>>
>>https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit/?h
>>=next&id=d53701067f048b8b11635e964b6d3bd9a248c622
>>
>
> Sorry, I think I missed to update to the latest version. But I think my 
> patch is more efficient. Because I just sync the frame from the HW, it
>  won't fail and there is no need to extra tries.
>
> What do you think about it?

the 14 bits you use for your check is not representative of the actual
micro-frame you're currently in. Thinh explained that in the discussion
we had until we came to the patch I pointed you to above. Please look at
the mailing list archives for details.

Thinh Nguyen Dec. 14, 2018, 9:42 p.m. UTC | #5

Hi Zengtao,

On 12/14/2018 3:24 AM, Felipe Balbi wrote:
> Hi,
>
> "Zengtao (B)" <[e-mail]> writes:
>>> -----Original Message-----
>>> From: Felipe Balbi [mailto:[e-mail]]
>>> Sent: Friday, December 14, 2018 4:52 PM
>>> To: Zengtao (B) <[e-mail]>
>>> Cc: liangshengjun <[e-mail]>; Zengtao (B)
>>> <[e-mail]>; Greg Kroah-Hartman
>>> <[e-mail]>; [e-mail];
>>> [e-mail]
>>> Subject: Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by
>>> IRQ latency
>>>
>>> * PGP Signed by an unknown key
>>>
>>> Zeng Tao <[e-mail]> writes:
>>>
>>>> If it's a busy system, some times when we start an isoc transfer, the
>>>> framenumber get from the event buffer may be already elasped, in this
>>>> case, we will get all the packets dropped due to miss isoc. And we
>>>> turn into transfer nothing, to fix this issue, we need to fix the
>>>> framenumber to make sure that it's not out of date.
>>>>
>>>> Signed-off-by: Liang Shengjun <[e-mail]>
>>>> Signed-off-by: Zeng Tao <[e-mail]>
>>> There's a patch going upstream already doing this:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit/?h
>>> =next&id=d53701067f048b8b11635e964b6d3bd9a248c622
>>>
>> Sorry, I think I missed to update to the latest version. But I think my 
>> patch is more efficient. Because I just sync the frame from the HW, it
>>  won't fail and there is no need to extra tries.
>>
>> What do you think about it?
> the 14 bits you use for your check is not representative of the actual
> micro-frame you're currently in. Thinh explained that in the discussion
> we had until we came to the patch I pointed you to above. Please look at
> the mailing list archives for details.
>

There are several issues with this patch.
1) Your frame elapsed time check is not based on interval but statically
DWC3_EVENT_PRAM_SOFFN / 2. That's about 1 second. So it doesn't account
for isoc transfers with large service interval of 1 sec or more.
2) This function __dwc3_gadget_target_frame_elapsed() should have
comments for what it does. The name implies that this function checks
for eframe > cframe, and not eframe > cframe + 1s.
3) If this check fails, then it will do DWC3_ALIGN_FRAME() at least
twice. The isoc transfer will start 1 more interval into the future than
it needs to.

Also, I think the role of this check should be from the controller as it
has more information and its own logic to decide if the selected future
uframe has elapsed.

BR,
Thinh

Zengtao (B) Dec. 17, 2018, 1:45 a.m. UTC | #6

Hi Thinh:

>-----Original Message-----
>From: Thinh Nguyen [mailto:[e-mail]]
>Sent: Saturday, December 15, 2018 5:43 AM
>To: Felipe Balbi <[e-mail]>; Zengtao (B)
><[e-mail]>
>Cc: liangshengjun <[e-mail]>; Greg Kroah-Hartman
><[e-mail]>; [e-mail];
>[e-mail]; Thinh Nguyen
><[e-mail]>
>Subject: Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by
>IRQ latency
>
>Hi Zengtao,
>
>On 12/14/2018 3:24 AM, Felipe Balbi wrote:
>> Hi,
>>
>> "Zengtao (B)" <[e-mail]> writes:
>>>> -----Original Message-----
>>>> From: Felipe Balbi [mailto:[e-mail]]
>>>> Sent: Friday, December 14, 2018 4:52 PM
>>>> To: Zengtao (B) <[e-mail]>
>>>> Cc: liangshengjun <[e-mail]>; Zengtao (B)
>>>> <[e-mail]>; Greg Kroah-Hartman
>>>> <[e-mail]>; [e-mail];
>>>> [e-mail]
>>>> Subject: Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue
>>>> introduced by IRQ latency
>>>>
>>>> * PGP Signed by an unknown key
>>>>
>>>> Zeng Tao <[e-mail]> writes:
>>>>
>>>>> If it's a busy system, some times when we start an isoc transfer,
>>>>> the framenumber get from the event buffer may be already elasped,
>>>>> in this case, we will get all the packets dropped due to miss isoc.
>>>>> And we turn into transfer nothing, to fix this issue, we need to
>>>>> fix the framenumber to make sure that it's not out of date.
>>>>>
>>>>> Signed-off-by: Liang Shengjun <[e-mail]>
>>>>> Signed-off-by: Zeng Tao <[e-mail]>
>>>> There's a patch going upstream already doing this:
>>>>
>>>>
>https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit
>>>> /?h
>>>> =next&id=d53701067f048b8b11635e964b6d3bd9a248c622
>>>>
>>> Sorry, I think I missed to update to the latest version. But I think
>>> my patch is more efficient. Because I just sync the frame from the
>>> HW, it  won't fail and there is no need to extra tries.
>>>
>>> What do you think about it?
>> the 14 bits you use for your check is not representative of the actual
>> micro-frame you're currently in. Thinh explained that in the
>> discussion we had until we came to the patch I pointed you to above.
>> Please look at the mailing list archives for details.
>>
>
>There are several issues with this patch.
>1) Your frame elapsed time check is not based on interval but statically
>DWC3_EVENT_PRAM_SOFFN / 2. That's about 1 second. So it doesn't
>account for isoc transfers with large service interval of 1 sec or more.


This is just for checking whether the Frame number is overflow or not. So
1 second should a reason value.

>2) This function __dwc3_gadget_target_frame_elapsed() should have
>comments for what it does. The name implies that this function checks
>for eframe > cframe, and not eframe > cframe + 1s.
eframe > cframe + 1s is used to deal with the Frame number overflow.

>3) If this check fails, then it will do DWC3_ALIGN_FRAME() at least twice.
>The isoc transfer will start 1 more interval into the future than it needs to.
>
If the interval is one, 1 more interval should be more reasonable because the
core always fetch the trb one frame ahead.

>Also, I think the role of this check should be from the controller as it has
>more information and its own logic to decide if the selected future uframe
>has elapsed.

Yes, agree, but I think if the sw can be used to do the same thing and more
effective, Why not? 

BR
Zengtao

Zengtao (B) Dec. 17, 2018, 1:54 a.m. UTC | #7

Hi Sergei:

>-----Original Message-----
>From: Sergei Shtylyov [mailto:[e-mail]]
>Sent: Friday, December 14, 2018 7:18 PM
>To: Zengtao (B) <[e-mail]>; [e-mail]
>Cc: liangshengjun <[e-mail]>; Greg Kroah-Hartman
><[e-mail]>; [e-mail];
>[e-mail]
>Subject: Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by
>IRQ latency
>
>Hello!
>
>On 12/14/2018 07:32 PM, Zeng Tao wrote:
>
>> If it's a busy system, some times when we start an isoc transfer, the
>> framenumber get from the event buffer may be already elasped, in this
>
>   Frame number? Else my spell checker trips. :-)
>
>>  case, we will get all the packets dropped due to miss isoc. And we
>> turn
>
>   Remove the leading space, please.
>
>> into transfer nothing, to fix this issue, we need to fix the
>> framenumber to make sure that it's not out of date.
>>
>> Signed-off-by: Liang Shengjun <[e-mail]>
>> Signed-off-by: Zeng Tao <[e-mail]>
>[...]
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 9f92ee0..b63bd72 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1263,6 +1263,15 @@ static int __dwc3_gadget_get_frame(struct
>dwc3 *dwc)
>>  	return DWC3_DSTS_SOFFN(reg);
>>  }
>>
>> +static bool __dwc3_gadget_target_frame_elapsed(struct dwc3_ep
>*dep) {
>> +	u16 cframe =  __dwc3_gadget_get_frame(dep->dwc);
>> +	u16 eframe = dep->frame_number &
>DWC3_EVENT_PRAM_SOFFN_MASK;
>> +
>> +	return (((eframe - cframe) & DWC3_EVENT_PRAM_SOFFN_MASK)
>> +		> DWC3_EVENT_PRAM_MAX_SOFFN / 2);
>
>   Please leave > on the previous line.
>   And you surely know that the outer parens are unnecessary? :-)
>

Thanks for your kindly corrections. :-), I will apply them in the patch if we need.

>[...]
>
>MBR, Sergei

Thinh Nguyen Dec. 17, 2018, 8:12 p.m. UTC | #8

Hi Zengtao,

On 12/16/2018 5:45 PM, Zengtao (B) wrote:
>>>>>> If it's a busy system, some times when we start an isoc transfer,
>>>>>> the framenumber get from the event buffer may be already elasped,
>>>>>> in this case, we will get all the packets dropped due to miss isoc.
>>>>>> And we turn into transfer nothing, to fix this issue, we need to
>>>>>> fix the framenumber to make sure that it's not out of date.
>>>>>>
>>>>>> Signed-off-by: Liang Shengjun <[e-mail]>
>>>>>> Signed-off-by: Zeng Tao <[e-mail]>
>>>>> There's a patch going upstream already doing this:
>>>>>
>>>>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_balbi_usb.git_commit&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w&m=yGrN_JnamczN1G0wY4Th67vAfxSJEouaUgJdu0u6Av8&s=v4xeWOLyCSNoHxHS6kI8_bJfHgddFsPkqzgLFTJZOG8&e=
>>>>> /?h
>>>>> =next&id=d53701067f048b8b11635e964b6d3bd9a248c622
>>>>>
>>>> Sorry, I think I missed to update to the latest version. But I think
>>>> my patch is more efficient. Because I just sync the frame from the
>>>> HW, it  won't fail and there is no need to extra tries.
>>>>
>>>> What do you think about it?
>>> the 14 bits you use for your check is not representative of the actual
>>> micro-frame you're currently in. Thinh explained that in the
>>> discussion we had until we came to the patch I pointed you to above.
>>> Please look at the mailing list archives for details.
>>>
>> There are several issues with this patch.
>> 1) Your frame elapsed time check is not based on interval but statically
>> DWC3_EVENT_PRAM_SOFFN / 2. That's about 1 second. So it doesn't
>> account for isoc transfers with large service interval of 1 sec or more.
>
> This is just for checking whether the Frame number is overflow or not. So
> 1 second should a reason value.

Why when there's another way to solve this issue without creating a new
logic that only works within 1 second limit.

>
>> 2) This function __dwc3_gadget_target_frame_elapsed() should have
>> comments for what it does. The name implies that this function checks
>> for eframe > cframe, and not eframe > cframe + 1s.
> eframe > cframe + 1s is used to deal with the Frame number overflow.

Right. You need to provide comments for that. It's not obvious that
DWC3_EVENT_PRAM_SOFFN / 2 is around 1 second.

>
>> 3) If this check fails, then it will do DWC3_ALIGN_FRAME() at least twice.
>> The isoc transfer will start 1 more interval into the future than it needs to.
>>
> If the interval is one, 1 more interval should be more reasonable because the
> core always fetch the trb one frame ahead.

Did it fail with your setup when you test with interval of 1? It didn't
fail from my testing setup. Your check applies for all intervals and not
just interval of 1. Also, the databook doesn't mention this limitation
that you have to do 2 intervals into the future if the service interval
is 1.

Regardless, if the __dwc3_gadget_target_frame_elapsed() passes for
service interval of 1, then you'd only do DWC3_ALIGN_FRAME() once. This
is different than what you said/wanted to do.

>
>> Also, I think the role of this check should be from the controller as it has
>> more information and its own logic to decide if the selected future uframe
>> has elapsed.
> Yes, agree, but I think if the sw can be used to do the same thing and more
> effective, Why not? 
>

No. The SW cannot do the same thing. As Felipe mentioned previously,
__dwc3_gadget_get_frame() can only get a 14-bit frame number. The
controller keeps track of the frame number with at least 16 bits. It has
more information and can work with larger service intervals.

Can you test with Felipe's patches to see if they work for you? We can
come back and review if there are still issues.

Thanks,
Thinh

Patch

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5bfb625..8742d96 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -452,6 +452,9 @@ 
 
 #define DWC3_DSTS_RXFIFOEMPTY		BIT(17)
 
+#define DWC3_EVENT_PRAM_MAX_SOFFN      0x3fff
+#define DWC3_EVENT_PRAM_SOFFN_MASK     0x3fff
+
 #define DWC3_DSTS_SOFFN_MASK		(0x3fff << 3)
 #define DWC3_DSTS_SOFFN(n)		(((n) & DWC3_DSTS_SOFFN_MASK) >> 3)
 
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9f92ee0..b63bd72 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1263,6 +1263,15 @@  static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
 	return DWC3_DSTS_SOFFN(reg);
 }
 
+static bool __dwc3_gadget_target_frame_elapsed(struct dwc3_ep *dep)
+{
+	u16 cframe =  __dwc3_gadget_get_frame(dep->dwc);
+	u16 eframe = dep->frame_number & DWC3_EVENT_PRAM_SOFFN_MASK;
+
+	return (((eframe - cframe) & DWC3_EVENT_PRAM_SOFFN_MASK)
+		> DWC3_EVENT_PRAM_MAX_SOFFN / 2);
+}
+
 static void __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 {
 	if (list_empty(&dep->pending_list)) {
@@ -1272,6 +1281,9 @@  static void __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 		return;
 	}
 
+	while (__dwc3_gadget_target_frame_elapsed(dep))
+		dep->frame_number = DWC3_ALIGN_FRAME(dep);
+
 	dep->frame_number = DWC3_ALIGN_FRAME(dep);
 	__dwc3_gadget_kick_transfer(dep);
 }