usb: dwc3: gadget: check link trb after free_slot is increased

PATCH In review Zhuang Jin Can 3 participants 6 messages May 15, 2014 – May 19, 2014

Subsystems (2)

USB DesignWare USB3 DRD IP driver

lore

Share & tools

lore

Zhuang Jin Can

author

2014-05-15 09:58 UTC #0 lore raw

PATCH 1/1 usb: dwc3: gadget: check link trb after free_slot is increased

In ISOC transfers, when free_slot points to the last TRB (i.e. Link TRB), and all queued requests meet Missed Interval Isoc error, busy_slot points to trb0.

	busy_slot->trb0
		   trb1
		   ...
	free_slot->trb31(Link TRB)

After end transfer and receiving the XferNotReady event, trb_left is caculated as 1 which is wrong, and no TRB will be primed to the endpoint.

The root cause is free_slot is not increased the same way as busy_slot. When busy_slot is increased by one, it checks if points to a link TRB after increasement, but free_slot checks it before increasement. free_slot should behave the same as busy_slot to make the trb_left caculation correct.

Signed-off-by Zhuang Jin Can <[email protected]>

Signed-off-by Jiebing Li <[email protected]>

---
 drivers/usb/dwc3/gadget.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/usb/dwc3/gadget.c +4 -4

index 54da8c8..2ebe82b 100644
@@ -828,10 +828,6 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 			length, last ? " last" : "",
 			chain ? " chain" : "");
 
-	/* Skip the LINK-TRB on ISOC */
-	if (((dep->free_slot & DWC3_TRB_MASK) == DWC3_TRB_NUM - 1) &&
-			usb_endpoint_xfer_isoc(dep->endpoint.desc))
-		dep->free_slot++;
 
 	trb = &dep->trb_pool[dep->free_slot & DWC3_TRB_MASK];

@@ -843,6 +839,10 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 	}
 
 	dep->free_slot++;
+	/* Skip the LINK-TRB on ISOC */
+	if (((dep->free_slot & DWC3_TRB_MASK) == DWC3_TRB_NUM - 1) &&
+			usb_endpoint_xfer_isoc(dep->endpoint.desc))
+		dep->free_slot++;
 
 	trb->size = DWC3_TRB_SIZE_LENGTH(length);
 	trb->bpl = lower_32_bits(dma);

Signature · 1 line

1.7.9.5

Felipe Balbi

2014-05-15 15:37 UTC #1 lore raw

Quoted 29 lines · Zhuang Jin Can

In ISOC transfers, when free_slot points to the last TRB (i.e. Link TRB), and all queued requests meet Missed Interval Isoc error, busy_slot points to trb0.
	busy_slot->trb0
		   trb1
		   ...
	free_slot->trb31(Link TRB)
After end transfer and receiving the XferNotReady event, trb_left is caculated as 1 which is wrong, and no TRB will be primed to the endpoint.
The root cause is free_slot is not increased the same way as busy_slot. When busy_slot is increased by one, it checks if points to a link TRB after increasement, but free_slot checks it before increasement. free_slot should behave the same as busy_slot to make the trb_left caculation correct.
Signed-off-by Zhuang Jin Can <[email protected]>
Signed-off-by Jiebing Li <[email protected]>
---
 drivers/usb/dwc3/gadget.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/dwc3/gadget.c +0 -0
index 54da8c8..2ebe82b 100644

drivers/usb/dwc3/gadget.c static void dwc3_prepare_one_trb(struct dwc3_ep *dep, · line 832

Quoted 9 lines

drivers/usb/dwc3/gadget.c +0 -4

@@ -828,10 +828,6 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 			length, last ? " last" : "",
 			chain ? " chain" : "");
 
-	/* Skip the LINK-TRB on ISOC */
-	if (((dep->free_slot & DWC3_TRB_MASK) == DWC3_TRB_NUM - 1) &&
-			usb_endpoint_xfer_isoc(dep->endpoint.desc))
-		dep->free_slot++;
 
 	trb = &dep->trb_pool[dep->free_slot & DWC3_TRB_MASK];

I have a feeling this has a negative side effect of letting us use the link TRB for data transfer... I mean, if we don't increment free_slot before accessing our trb_pool, we have no way to skip link trb on this access here.

How did you find the bug ? do you have good instructions on how to reproduce it ? How did you test the patch and for how long ?

cheers

Signature · 1 line

balbi

Zhuang Jin Can

author

2014-05-16 03:50 UTC #2 lore raw

Quoted 40 lines · Felipe Balbi

Hi
Quoted 37 lines · Zhuang Jin Can
In ISOC transfers, when free_slot points to the last TRB (i.e. Link TRB), and all queued requests meet Missed Interval Isoc error, busy_slot points to trb0.
	busy_slot->trb0
		   trb1
		   ...
	free_slot->trb31(Link TRB)
After end transfer and receiving the XferNotReady event, trb_left is caculated as 1 which is wrong, and no TRB will be primed to the endpoint.
The root cause is free_slot is not increased the same way as busy_slot. When busy_slot is increased by one, it checks if points to a link TRB after increasement, but free_slot checks it before increasement. free_slot should behave the same as busy_slot to make the trb_left caculation correct.
Signed-off-by Zhuang Jin Can <[email protected]>
Signed-off-by Jiebing Li <[email protected]>
---
 drivers/usb/dwc3/gadget.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/dwc3/gadget.c +0 -4
index 54da8c8..2ebe82b 100644
@@ -828,10 +828,6 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 			length, last ? " last" : "",
 			chain ? " chain" : "");
 
-	/* Skip the LINK-TRB on ISOC */
-	if (((dep->free_slot & DWC3_TRB_MASK) == DWC3_TRB_NUM - 1) &&
-			usb_endpoint_xfer_isoc(dep->endpoint.desc))
-		dep->free_slot++;
 

Quoted 6 lines

drivers/usb/dwc3/gadget.c static void dwc3_prepare_one_trb(struct dwc3_ep *dep, · line 832
Quoted 1 line
drivers/usb/dwc3/gadget.c +0 -0
 	trb = &dep->trb_pool[dep->free_slot & DWC3_TRB_MASK];
I have a feeling this has a negative side effect of letting us use the link TRB for data transfer... I mean, if we don't increment free_slot before accessing our trb_pool, we have no way to skip link trb on this access here.

After every free_slot++ Link TRB is checked and increased if appropriate, this guarantees you next time access free_slot, it can't be a Link TRB.

Quoted 3 lines

How did you find the bug ? do you have good instructions on how to reproduce it ? How did you test the patch and for how long ?

The bug is reproduced on Android with f_audio_source.c enabled, which has an isoc-in endpoint keeps sending audio data to host in an interval of 1 ms. Normally, you need to run for 12+ hours to hit the issue. So I think you can just run some isoc transfers for a long time to reproduce it. To accelarte the reproducing, you can run some concurrent data transfer as well, so the possibility to meet missed interval error is larger.

The patch is tested for basic functionality like enumeration, data transfers. For this bug, it was tested for 20+ hours.

Thanks
Jincan

Felipe Balbi

2014-05-16 12:41 UTC #3 lore raw

Hi,

Quoted 44 lines · Zhuang Jin Can

Quoted 38 lines · Zhuang Jin Can
Quoted 37 lines
In ISOC transfers, when free_slot points to the last TRB (i.e. Link TRB), and all queued requests meet Missed Interval Isoc error, busy_slot points to trb0.
	busy_slot->trb0
		   trb1
		   ...
	free_slot->trb31(Link TRB)
After end transfer and receiving the XferNotReady event, trb_left is caculated as 1 which is wrong, and no TRB will be primed to the endpoint.
The root cause is free_slot is not increased the same way as busy_slot. When busy_slot is increased by one, it checks if points to a link TRB after increasement, but free_slot checks it before increasement. free_slot should behave the same as busy_slot to make the trb_left caculation correct.
Signed-off-by Zhuang Jin Can <[email protected]>
Signed-off-by Jiebing Li <[email protected]>
---
 drivers/usb/dwc3/gadget.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/dwc3/gadget.c +0 -4
index 54da8c8..2ebe82b 100644
@@ -828,10 +828,6 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 			length, last ? " last" : "",
 			chain ? " chain" : "");
 
-	/* Skip the LINK-TRB on ISOC */
-	if (((dep->free_slot & DWC3_TRB_MASK) == DWC3_TRB_NUM - 1) &&
-			usb_endpoint_xfer_isoc(dep->endpoint.desc))
-		dep->free_slot++;
 
Quoted 6 lines
drivers/usb/dwc3/gadget.c static void dwc3_prepare_one_trb(struct dwc3_ep *dep, · line 832
Quoted 1 line
drivers/usb/dwc3/gadget.c +0 -0
 	trb = &dep->trb_pool[dep->free_slot & DWC3_TRB_MASK];
I have a feeling this has a negative side effect of letting us use the link TRB for data transfer... I mean, if we don't increment free_slot before accessing our trb_pool, we have no way to skip link trb on this access here.

Quoted 3 lines

After every free_slot++ Link TRB is checked and increased if appropriate, this guarantees you next time access free_slot, it can't be a Link TRB.

right, next access will be fine, but you're forgetting about current access.

Quoted 10 lines

Quoted 2 lines
How did you find the bug ? do you have good instructions on how to reproduce it ? How did you test the patch and for how long ?
The bug is reproduced on Android with f_audio_source.c enabled, which has an isoc-in endpoint keeps sending audio data to host in an interval of 1 ms. Normally, you need to run for 12+ hours to hit the issue. So I think you can just run some isoc transfers for a long time to reproduce it. To accelarte the reproducing, you can run some concurrent data transfer as well, so the possibility to meet missed interval error is larger.

Quoted 2 lines

The patch is tested for basic functionality like enumeration, data transfers. For this bug, it was tested for 20+ hours.

thanks, g_audio loop should be fine.

Signature · 1 line

balbi

Zhuang Jin Can

author

2014-05-16 14:47 UTC #4 lore raw

Hi,

Quoted 45 lines · Felipe Balbi

Quoted 44 lines · Zhuang Jin Can
Quoted 44 lines · Zhuang Jin Can
Quoted 29 lines
In ISOC transfers, when free_slot points to the last TRB (i.e. Link TRB), and all queued requests meet Missed Interval Isoc error, busy_slot points to trb0.
	busy_slot->trb0
		   trb1
		   ...
	free_slot->trb31(Link TRB)
After end transfer and receiving the XferNotReady event, trb_left is caculated as 1 which is wrong, and no TRB will be primed to the endpoint.
The root cause is free_slot is not increased the same way as busy_slot. When busy_slot is increased by one, it checks if points to a link TRB after increasement, but free_slot checks it before increasement. free_slot should behave the same as busy_slot to make the trb_left caculation correct.
Signed-off-by Zhuang Jin Can <[email protected]>
Signed-off-by Jiebing Li <[email protected]>
---
 drivers/usb/dwc3/gadget.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/dwc3/gadget.c +0 -0
index 54da8c8..2ebe82b 100644
drivers/usb/dwc3/gadget.c static void dwc3_prepare_one_trb(struct dwc3_ep *dep, · line 832
Quoted 9 lines
drivers/usb/dwc3/gadget.c +0 -4
@@ -828,10 +828,6 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 			length, last ? " last" : "",
 			chain ? " chain" : "");
 
-	/* Skip the LINK-TRB on ISOC */
-	if (((dep->free_slot & DWC3_TRB_MASK) == DWC3_TRB_NUM - 1) &&
-			usb_endpoint_xfer_isoc(dep->endpoint.desc))
-		dep->free_slot++;
 
 	trb = &dep->trb_pool[dep->free_slot & DWC3_TRB_MASK];
I have a feeling this has a negative side effect of letting us use the link TRB for data transfer... I mean, if we don't increment free_slot before accessing our trb_pool, we have no way to skip link trb on this access here.

Quoted 7 lines

Quoted 3 lines
After every free_slot++ Link TRB is checked and increased if appropriate, this guarantees you next time access free_slot, it can't be a Link TRB.
right, next access will be fine, but you're forgetting about current access.

The current access is the next access relative to last access. So if the first access is fine, succeeding accesses should be fine. And the first access happens on when the free_slot points to slot 1 which is not a link trb.

Quoted 10 lines

Quoted 10 lines
Quoted 2 lines
How did you find the bug ? do you have good instructions on how to reproduce it ? How did you test the patch and for how long ?
The bug is reproduced on Android with f_audio_source.c enabled, which has an isoc-in endpoint keeps sending audio data to host in an interval of 1 ms. Normally, you need to run for 12+ hours to hit the issue. So I think you can just run some isoc transfers for a long time to reproduce it. To accelarte the reproducing, you can run some concurrent data transfer as well, so the possibility to meet missed interval error is larger.

Quoted 5 lines

Quoted 2 lines
The patch is tested for basic functionality like enumeration, data transfers. For this bug, it was tested for 20+ hours.
thanks, g_audio loop should be fine.

np.

Regards
Jincan

Pratyush Anand

Reviewed-by

2014-05-19 11:45 UTC #5 lore raw

Quoted 22 lines · Zhuang Jin Can

Hi,
Quoted 20 lines · Felipe Balbi
Quoted 19 lines · Zhuang Jin Can
Quoted 19 lines · Zhuang Jin Can
Quoted 18 lines
In ISOC transfers, when free_slot points to the last TRB (i.e. Link TRB), and all queued requests meet Missed Interval Isoc error, busy_slot points to trb0.
	busy_slot->trb0
		   trb1
		   ...
	free_slot->trb31(Link TRB)
After end transfer and receiving the XferNotReady event, trb_left is caculated as 1 which is wrong, and no TRB will be primed to the endpoint.
The root cause is free_slot is not increased the same way as busy_slot. When busy_slot is increased by one, it checks if points to a link TRB after increasement, but free_slot checks it before increasement. free_slot should behave the same as busy_slot to make the trb_left caculation correct.

Reviewing the code it seems that observation of Zhuang Jin Can is correct. Year back I did lot of testing with isoc and even with missed path, but may be I could not get this issue because I would not have landed into a situation where last missed TRB is 31st.

Infact, you could be able to see that bug even in other case, like submit 31 back to back ep_queue request from gadget and then wait as if no application data is available. They all have been transmitted to host and callback has been called for all of them. After some time application is ready with new data. So when gadget does ep_queue of this new data you should be able to see this issue.

Reviewed-by Pratyush Anand <[email protected]>

Regards
Pratyush
been

Quoted 61 lines

Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
Quoted 11 lines
Signed-off-by Zhuang Jin Can <[email protected]>
Signed-off-by Jiebing Li <[email protected]>
---
 drivers/usb/dwc3/gadget.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/usb/dwc3/gadget.c +0 -0
index 54da8c8..2ebe82b 100644
drivers/usb/dwc3/gadget.c static void dwc3_prepare_one_trb(struct dwc3_ep *dep, · line 832
Quoted 9 lines
drivers/usb/dwc3/gadget.c +0 -4
@@ -828,10 +828,6 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 			length, last ? " last" : "",
 			chain ? " chain" : "");
 
-	/* Skip the LINK-TRB on ISOC */
-	if (((dep->free_slot & DWC3_TRB_MASK) == DWC3_TRB_NUM - 1) &&
-			usb_endpoint_xfer_isoc(dep->endpoint.desc))
-		dep->free_slot++;
 
 	trb = &dep->trb_pool[dep->free_slot & DWC3_TRB_MASK];
I have a feeling this has a negative side effect of letting us use the link TRB for data transfer... I mean, if we don't increment free_slot before accessing our trb_pool, we have no way to skip link trb on this access here.
Quoted 7 lines
Quoted 3 lines
After every free_slot++ Link TRB is checked and increased if appropriate, this guarantees you next time access free_slot, it can't be a Link TRB.
right, next access will be fine, but you're forgetting about current access.
The current access is the next access relative to last access. So if the first access is fine, succeeding accesses should be fine. And the first access happens on when the free_slot points to slot 1 which is not a link trb.
Quoted 10 lines
Quoted 10 lines
Quoted 2 lines
How did you find the bug ? do you have good instructions on how to reproduce it ? How did you test the patch and for how long ?
The bug is reproduced on Android with f_audio_source.c enabled, which has an isoc-in endpoint keeps sending audio data to host in an interval of 1 ms. Normally, you need to run for 12+ hours to hit the issue. So I think you can just run some isoc transfers for a long time to reproduce it. To accelarte the reproducing, you can run some concurrent data transfer as well, so the possibility to meet missed interval error is larger.
Quoted 5 lines
Quoted 2 lines
The patch is tested for basic functionality like enumeration, data transfers. For this bug, it was tested for 20+ hours.
thanks, g_audio loop should be fine.
np.
Regards
Jincan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html