usb: dwc3: Fixes and code cleanup
Subsystems (2)
cover usb: dwc3: Fixes and code cleanup
The first two patches are bug fixes in TRB preparation when scatter gather is used. The next two patches are basically trivial and part of code cleanup.
The patches are rebased on Balbi's testing/next branch.
Amit Virdi (4):
usb: dwc3: gadget: Fix TRB preparation during SG
usb: dwc3: gadget: Stop TRB preparation after limit is reached
usb: dwc3: gadget: Remove redundant check
usb: dwc3: Remove current_trb as it is unused
drivers/usb/dwc3/core.h | 3 --- drivers/usb/dwc3/gadget.c | 9 ++++----- 2 files changed, 4 insertions(+), 8 deletions(-)
Signature · 6 lines
1.8.0 -- 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
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 2/4 usb: dwc3: gadget: Stop TRB preparation after limit is reached
When SG is used, there are two loops iterating to prepare TRBs:
- Outer loop over the request_list
- Inner loop over the SG list
The driver must stop preparing TRBs when the max TRBs have been prepared. The code was missing break to get out of the outer loop.
--- drivers/usb/dwc3/gadget.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/usb/dwc3/gadget.c +3 -0
@@ -900,6 +900,9 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool starting)if (last_one)break ; } + +if (last_one) +break ; }else { dma = req->request.dma; length = req->request.length;
Signature · 6 lines
1.8.0 -- 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
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Quoted 8 lines · Amit Virdi
When SG is used, there are two loops iterating to prepare TRBs:
- Outer loop over therequest_list
- Inner loop over the SG listThe driver must stop preparing TRBs when the max TRBs have been prepared. The code was missing break to get out of the outer loop.
Signed-off-by Amit Virdi <[email protected]>
which bug is this fixing ? Which kernels are affected ? This need to be backported to which kernel ? Which commit introduced this bug ? How can I validate this to be a valid fix ?
Quoted 21 lines
--- drivers/usb/dwc3/gadget.c | 3 +++ 1 file changed, 3 insertions(+)drivers/usb/dwc3/gadget.c +3
-0@@ -900,6 +900,9 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool starting)if (last_one)break ; } + +if (last_one) +break ; }else { dma = req->request.dma; length = req->request.length;Signature · 1 line
1.8.0
Signature · 1 line
balbi
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Quoted 5 lines · Felipe Balbi
Quoted 4 lines · Amit Virdi
When SG is used, there are two loops iterating to prepare TRBs:
- Outer loop over therequest_list
- Inner loop over the SG list
Quoted 9 lines
Quoted 4 lines
The driver must stop preparing TRBs when the max TRBs have been prepared. The code was missing break to get out of the outer loop.
Signed-off-by Amit Virdi <[email protected]>which bug is this fixing ? Which kernels are affected ? This need to be backported to which kernel ? Which commit introduced this bug ? How can I validate this to be a valid fix ?
Problem description:
DWC3 gadget sets up a pool of 32 TRBs for each EP during initialization. This means, the max TRBs that can be submitted for an EP is fixed to 32. Since the request queue for an EP is a linked list, any number of requests can be queued to it by the gadget layer. However, the dwc3 driver must not submit TRBs more than the pool it has created for. This limit wasn't respected when SG was used resulting in submitting more than the max TRBs, eventually leading to non-transfer of the TRBs submitted over the max limit.
Commit that introduced this bug:
This bug is present from the day support for sg list was added to dwc3 gadget., i.e. since commit eeb720fb21d61dfc3aac780e721150998ef603af ("usb: dwc3: gadget: add support for SG lists") - kernel version v3.2-rc7, hence v3.2
Kernels affected:
It is best to backport this fix to kernel v3.8+ as there were many enhancements/bug fixes from v3.2..v3.8
Generic setup to reproduce/test this fix:
This bug is reproduced whenever the number of TRBs that need to be submitted to the hardware from the software queue (request_list) becomes more than 32 on bulk endpoint. eg. if num_mapped_sgs is 2 and the request_list has 17 entries (hence, 34 potential TRBs), the scenario is reproduced.
My setup details:
For reproducing and testing the patches [1/4 and 2/4], I used an inhouse developed user space application that run on device side. This user space application interacts with the customized uvc webcam gadget to submit the video data to the DWC3 driver. The host side was running standard webcam application (cheese).
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Hi,
Quoted 16 lines · Amit Virdi
Quoted 5 lines · Felipe Balbi
Quoted 4 lines · Amit Virdi
When SG is used, there are two loops iterating to prepare TRBs:
- Outer loop over therequest_list
- Inner loop over the SG listQuoted 9 lines
Quoted 4 lines
The driver must stop preparing TRBs when the max TRBs have been prepared. The code was missing break to get out of the outer loop.
Signed-off-by Amit Virdi <[email protected]>which bug is this fixing ? Which kernels are affected ? This need to be backported to which kernel ? Which commit introduced this bug ? How can I validate this to be a valid fix ?
Quoted 9 lines
Problem description:
DWC3 gadget sets up a pool of 32 TRBs for each EP during initialization. This means, the max TRBs that can be submitted for an EP is fixed to 32. Since the request queue for an EP is a linked list, any number of requests can be queued to it by the gadget layer. However, the dwc3 driver must not submit TRBs more than the pool it has created for. This limit wasn't respected when SG was used resulting in submitting more than the max TRBs, eventually leading to non-transfer of the TRBs submitted over the max limit.
this would become a much, much better commit log than the one you provided. It's a much more verbose description of the issue.
Quoted 17 lines
Commit that introduced this bug:
This bug is present from the day support for sg list was added to dwc3 gadget., i.e. since commiteeb720fb21d61dfc3aac780e721150998ef603af("usb: dwc3: gadget: add support for SG lists") - kernel version v3.2-rc7, hence v3.2Kernels affected:
It is best to backport this fix to kernel v3.8+ as there were many enhancements/bug fixes from v3.2..v3.8Generic setup to reproduce/test this fix:
This bug is reproduced whenever the number of TRBs that need to be submitted to the hardware from the software queue (request_list) becomes more than 32 on bulk endpoint. eg. ifnum_mapped_sgsis 2 and therequest_listhas 17 entries (hence, 34 potential TRBs), the scenario is reproduced.
Quoted 6 lines
My setup details:
For reproducing and testing the patches [1/4 and 2/4], I used an inhouse developed user space application that run on device side. This user space application interacts with the customized uvc webcam gadget to submit the video data to the DWC3 driver. The host side was running standard webcam application (cheese).
oh, so this is something I can't easily reproduce myself. Then I'll need full boot logs, register dump and full trace output of dwc3 running and exhibiting the problem. Also, make sure you use either v3.18 or v3.19rc1 to validate your changes.
Signature · 1 line
balbi
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Quoted 29 lines · Felipe Balbi
Hi,
Quoted 16 lines · Amit Virdi
Quoted 5 lines · Felipe Balbi
Quoted 4 lines · Amit Virdi
When SG is used, there are two loops iterating to prepare TRBs:
- Outer loop over therequest_list
- Inner loop over the SG listQuoted 9 lines
Quoted 4 lines
The driver must stop preparing TRBs when the max TRBs have been prepared. The code was missing break to get out of the outer loop.
Signed-off-by Amit Virdi <[email protected]>which bug is this fixing ? Which kernels are affected ? This need to be backported to which kernel ? Which commit introduced this bug ? How can I validate this to be a valid fix ?
Quoted 9 lines
Problem description:
DWC3 gadget sets up a pool of 32 TRBs for each EP during initialization. This means, the max TRBs that can be submitted for an EP is fixed to 32. Since the request queue for an EP is a linked list, any number of requests can be queued to it by the gadget layer. However, the dwc3 driver must not submit TRBs more than the pool it has created for. This limit wasn't respected when SG was used resulting in submitting more than the max TRBs, eventually leading to non-transfer of the TRBs submitted over the max limit.
Quoted 3 lines
this would become a much, much better commit log than the one you provided. It's a much more verbose description of the issue.
Okay, I'll edit the commit message.
Quoted 24 lines
Quoted 17 lines
Commit that introduced this bug:
This bug is present from the day support for sg list was added to dwc3 gadget., i.e. since commiteeb720fb21d61dfc3aac780e721150998ef603af("usb: dwc3: gadget: add support for SG lists") - kernel version v3.2-rc7, hence v3.2Kernels affected:
It is best to backport this fix to kernel v3.8+ as there were many enhancements/bug fixes from v3.2..v3.8Generic setup to reproduce/test this fix:
This bug is reproduced whenever the number of TRBs that need to be submitted to the hardware from the software queue (request_list) becomes more than 32 on bulk endpoint. eg. ifnum_mapped_sgsis 2 and therequest_listhas 17 entries (hence, 34 potential TRBs), the scenario is reproduced.Quoted 6 lines
My setup details:
For reproducing and testing the patches [1/4 and 2/4], I used an inhouse developed user space application that run on device side. This user space application interacts with the customized uvc webcam gadget to submit the video data to the DWC3 driver. The host side was running standard webcam application (cheese).
Quoted 5 lines
oh, so this is something I can't easily reproduce myself. Then I'll need full boot logs, register dump and full trace output of dwc3 running and exhibiting the problem. Also, make sure you use either v3.18 or v3.19rc1 to validate your changes.
[Quoting you from the thread of patch 1/4]
Quoted 14 lines
Quoted 11 lines
endpoint. Following is the log snippet once this bug is reproduced:
----
dwc3 dwc3.0: ep2in-bulk: Transfer Not Ready
dwc3 dwc3.0: queing request24cc5780to ep2in-bulk length 960002 dwc3 dwc3.0: ep2in-bulk: req24cc5780dma24eb6400length 2 chain dwc3 dwc3.0: ep2in-bulk: req24cc5780dma 25901800 length 960000 dwc3 dwc3.0: queing request24cc5000to ep2in-bulk length 960002 dwc3 dwc3.0: ep2in-bulk: Transfer Not Ready
dwc3 dwc3.0: queing request24cc5900to ep2in-bulk length 960002
-----Quoted 2 lines
Without this fix, the hardware never generates "Transfer Complete" event for the corresponding EP and goes into an unknown state.
Quoted 10 lines
whiuch kernel are you using to develop this ? Most of these messages don't exist anymore with v3.19-rc because I have converted them into tracepoints, considering you're showing me logs with these messages, this tells me that you're sending me a patch developed/tested against a kernel older than v3.18. I need to see full bootup logs, a register dump (
/sys/kernel/debug/*dwc3*/regdump) and a much larger debugging log from dwc3 in order to accept patches from you. Sorry, but I cannot take patches which were not tested against, at a minimum, the latest major release.
Yes you're right that both the fixes [1/4] and [2/4] have only been build tested with the latest kernel. I should have mentioned this information in the commit message or the cover-letter itself.
The only reason why I have not been able to test these fixes on the latest is because the customized webcam gadget is not ported on the latest kernel. There have been a lot of changes in the video framework lately and that is not my area of expertise. So porting the customized webcam gadget to latest kernel and testing these fixes is not feasible for me.
Having said all this, the fact remains that these are bugs in the dwc3 driver from kernel v3.9 onwards. The log messages clearly depict this. These bugs are obviously present till date.
I ask you to give a more deeper thought on the whole situation and not undermine the importance of code review.
- [Patch 2/4] fixes a bug that could have been found in the code review.
- [Patch 1/4] fixes a bug which was very very subtle but a deeper
code review can certainly boost the confidence about the change made. list_is_last won't work when SG is used because, for the last request in the request_list, the TRB for the first sg entry modifies the list's next and prev pointers while moving the URB to req_queued.
I can certainly provide the dwc3 specific kernel bootup logs, full regdump and any loglevel you want me to, if that helps
--
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
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Hi,
Quoted 24 lines · Amit Virdi
Quoted 24 lines
Quoted 23 lines
Quoted 4 lines
Quoted 4 lines
When SG is used, there are two loops iterating to prepare TRBs:
- Outer loop over therequest_list
- Inner loop over the SG listQuoted 9 lines
Quoted 4 lines
The driver must stop preparing TRBs when the max TRBs have been prepared. The code was missing break to get out of the outer loop.
Signed-off-by Amit Virdi <[email protected]>which bug is this fixing ? Which kernels are affected ? This need to be backported to which kernel ? Which commit introduced this bug ? How can I validate this to be a valid fix ?
Problem description:
DWC3 gadget sets up a pool of 32 TRBs for each EP during initialization. This means, the max TRBs that can be submitted for an EP is fixed to 32. Since the request queue for an EP is a linked list, any number of requests can be queued to it by the gadget layer. However, the dwc3 driver must not submit TRBs more than the pool it has created for. This limit wasn't respected when SG was used resulting in submitting more than the max TRBs, eventually leading to non-transfer of the TRBs submitted over the max limit.
Quoted 4 lines
Quoted 2 lines
this would become a much, much better commit log than the one you provided. It's a much more verbose description of the issue.
Okay, I'll edit the commit message.
thanks
Quoted 57 lines
Quoted 24 lines
Quoted 17 lines
Commit that introduced this bug:
This bug is present from the day support for sg list was added to dwc3 gadget., i.e. since commiteeb720fb21d61dfc3aac780e721150998ef603af("usb: dwc3: gadget: add support for SG lists") - kernel version v3.2-rc7, hence v3.2Kernels affected:
It is best to backport this fix to kernel v3.8+ as there were many enhancements/bug fixes from v3.2..v3.8Generic setup to reproduce/test this fix:
This bug is reproduced whenever the number of TRBs that need to be submitted to the hardware from the software queue (request_list) becomes more than 32 on bulk endpoint. eg. ifnum_mapped_sgsis 2 and therequest_listhas 17 entries (hence, 34 potential TRBs), the scenario is reproduced.Quoted 6 lines
My setup details:
For reproducing and testing the patches [1/4 and 2/4], I used an inhouse developed user space application that run on device side. This user space application interacts with the customized uvc webcam gadget to submit the video data to the DWC3 driver. The host side was running standard webcam application (cheese).Quoted 5 lines
oh, so this is something I can't easily reproduce myself. Then I'll need full boot logs, register dump and full trace output of dwc3 running and exhibiting the problem. Also, make sure you use either v3.18 or v3.19rc1 to validate your changes.
[Quoting you from the thread of patch 1/4]
Quoted 14 lines
Quoted 11 lines
endpoint. Following is the log snippet once this bug is reproduced:
----
dwc3 dwc3.0: ep2in-bulk: Transfer Not Ready
dwc3 dwc3.0: queing request24cc5780to ep2in-bulk length 960002 dwc3 dwc3.0: ep2in-bulk: req24cc5780dma24eb6400length 2 chain dwc3 dwc3.0: ep2in-bulk: req24cc5780dma 25901800 length 960000 dwc3 dwc3.0: queing request24cc5000to ep2in-bulk length 960002 dwc3 dwc3.0: ep2in-bulk: Transfer Not Ready
dwc3 dwc3.0: queing request24cc5900to ep2in-bulk length 960002
-----Quoted 2 lines
Without this fix, the hardware never generates "Transfer Complete" event for the corresponding EP and goes into an unknown state.
Quoted 10 lines
whiuch kernel are you using to develop this ? Most of these messages don't exist anymore with v3.19-rc because I have converted them into tracepoints, considering you're showing me logs with these messages, this tells me that you're sending me a patch developed/tested against a kernel older than v3.18. I need to see full bootup logs, a register dump (
/sys/kernel/debug/*dwc3*/regdump) and a much larger debugging log from dwc3 in order to accept patches from you. Sorry, but I cannot take patches which were not tested against, at a minimum, the latest major release.
Quoted 3 lines
Yes you're right that both the fixes [1/4] and [2/4] have only been build tested with the latest kernel. I should have mentioned this information in the commit message or the cover-letter itself.
yeah, you should've :-)
Quoted 6 lines
The only reason why I have not been able to test these fixes on the latest is because the customized webcam gadget is not ported on the latest kernel. There have been a lot of changes in the video framework lately and that is not my area of expertise. So porting the customized webcam gadget to latest kernel and testing these fixes is not feasible for me.
right, so while I can see that both patches are very minimal and likely to be correct, I have no way of guaranteeing that. The only thing I can do, is run the tests which I already run (none of which exercise the cases you found though, other I'd have found them already) to make sure your patches don't break what's already working.
Quoted 2 lines
Having said all this, the fact remains that these are bugs in the dwc3 driver from kernel v3.9 onwards. The log messages clearly depict this.
no, it doesn't :-) your log snippet is so small that I cannot see how the driver got to that point :-) The only thing I can see is that you started two requests, one for 960000 bytes and another one for 2 bytes, but I don't know how many TRBs we had available, nor do I see a Start Transfer command, so I can't validate Start Transfer command parameters were correct or not.
Quoted 1 line
These bugs are obviously present till date.
Right, the code is the same :-)
Quoted 2 lines
I ask you to give a more deeper thought on the whole situation and not undermine the importance of code review.
heh
Quoted 9 lines
- [Patch 2/4] fixes a bug that could have been found in the code review.
- [Patch 1/4] fixes a bug which was very very subtle but a deeper
code review can certainly boost the confidence about the change made.list_is_lastwon't work when SG is used because, for the last request in therequest_list, the TRB for the first sg entry modifies the list's next and prev pointers while moving the URB toreq_queued.I can certainly provide the dwc3 specific kernel bootup logs, full regdump and any loglevel you want me to, if that helps
Yeah, if you can provide those, then that'll help me verifying. Full logs from boot to failure point with VERBOSE_DEBUG enabled (considering you're not running on anything recent).
Signature · 1 line
balbi
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Quoted 7 lines
Quoted 6 lines
The only reason why I have not been able to test these fixes on the latest is because the customized webcam gadget is not ported on the latest kernel. There have been a lot of changes in the video framework lately and that is not my area of expertise. So porting the customized webcam gadget to latest kernel and testing these fixes is not feasible for me.
Quoted 6 lines
right, so while I can see that both patches are very minimal and likely to be correct, I have no way of guaranteeing that. The only thing I can do, is run the tests which I already run (none of which exercise the cases you found though, other I'd have found them already) to make sure your patches don't break what's already working.
Okay
Quoted 26 lines
Quoted 2 lines
Having said all this, the fact remains that these are bugs in the dwc3 driver from kernel v3.9 onwards. The log messages clearly depict this.
no, it doesn't :-) your log snippet is so small that I cannot see how the driver got to that point :-) The only thing I can see is that you started two requests, one for 960000 bytes and another one for 2 bytes, but I don't know how many TRBs we had available, nor do I see a Start Transfer command, so I can't validate Start Transfer command parameters were correct or not.
Quoted 1 line
These bugs are obviously present till date.
Right, the code is the same :-)
Quoted 2 lines
I ask you to give a more deeper thought on the whole situation and not undermine the importance of code review.
heh
Quoted 7 lines
- [Patch 2/4] fixes a bug that could have been found in the code review.
- [Patch 1/4] fixes a bug which was very very subtle but a deeper
code review can certainly boost the confidence about the change made.list_is_lastwon't work when SG is used because, for the last request in therequest_list, the TRB for the first sg entry modifies the list's next and prev pointers while moving the URB toreq_queued.
Quoted 7 lines
Quoted 2 lines
I can certainly provide the dwc3 specific kernel bootup logs, full regdump and any loglevel you want me to, if that helps
Yeah, if you can provide those, then that'll help me verifying. Full logs from boot to failure point with
VERBOSE_DEBUGenabled (considering you're not running on anything recent).
Okay. I'll provide full verbose logs, along with the register dump, when I'm back to the office next week, for the working and non-working case, but how - as attachment or as inline?
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Quoted 45 lines · Amit Virdi
Quoted 7 lines
Quoted 6 lines
The only reason why I have not been able to test these fixes on the latest is because the customized webcam gadget is not ported on the latest kernel. There have been a lot of changes in the video framework lately and that is not my area of expertise. So porting the customized webcam gadget to latest kernel and testing these fixes is not feasible for me.
Quoted 6 lines
right, so while I can see that both patches are very minimal and likely to be correct, I have no way of guaranteeing that. The only thing I can do, is run the tests which I already run (none of which exercise the cases you found though, other I'd have found them already) to make sure your patches don't break what's already working.
Okay
Quoted 29 lines
Quoted 2 lines
Having said all this, the fact remains that these are bugs in the dwc3 driver from kernel v3.9 onwards. The log messages clearly depict this.
no, it doesn't :-) your log snippet is so small that I cannot see how the driver got to that point :-) The only thing I can see is that you started two requests, one for 960000 bytes and another one for 2 bytes, but I don't know how many TRBs we had available, nor do I see a Start Transfer command, so I can't validate Start Transfer command parameters were correct or not.
Quoted 1 line
These bugs are obviously present till date.
Right, the code is the same :-)
Quoted 2 lines
I ask you to give a more deeper thought on the whole situation and not undermine the importance of code review.
heh
Quoted 9 lines
- [Patch 2/4] fixes a bug that could have been found in the code review.
- [Patch 1/4] fixes a bug which was very very subtle but a deeper
code review can certainly boost the confidence about the change made.list_is_lastwon't work when SG is used because, for the last request in therequest_list, the TRB for the first sg entry modifies the list's next and prev pointers while moving the URB toreq_queued.I can certainly provide the dwc3 specific kernel bootup logs, full regdump and any loglevel you want me to, if that helps
Quoted 8 lines
Quoted 4 lines
Yeah, if you can provide those, then that'll help me verifying. Full logs from boot to failure point with
VERBOSE_DEBUGenabled (considering you're not running on anything recent).Okay. I'll provide full verbose logs, along with the register dump, when I'm back to the office next week, for the working and non-working case, but how - as attachment or as inline?
Either way will do but I have a feeling mailing list attachment size will bite you, so maybe it's best to make a tarball of both logs and send that as attachment. Compressed text will be very small.
Signature · 1 line
balbi
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
I can certainly provide the dwc3 specific kernel bootup logs, full regdump and any loglevel you want me to, if that helps
Yeah, if you can provide those, then that'll help me verifying. Full logs from boot to failure point with
VERBOSE_DEBUGenabled (considering you're not running on anything recent).
Quoted 8 lines
Quoted 3 lines
Okay. I'll provide full verbose logs, along with the register dump, when I'm back to the office next week, for the working and non-working case, but how - as attachment or as inline?
Either way will do but I have a feeling mailing list attachment size will bite you, so maybe it's best to make a tarball of both logs and send that as attachment. Compressed text will be very small.
Attached are the dwc3 verbose logs and register dump without and with the fixes in this patchset.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Hi,
Quoted 12 lines · Amit Virdi
Quoted 12 lines
Quoted 11 lines
Quoted 7 lines
Quoted 2 lines
I can certainly provide the dwc3 specific kernel bootup logs, full regdump and any loglevel you want me to, if that helps
Yeah, if you can provide those, then that'll help me verifying. Full logs from boot to failure point with
VERBOSE_DEBUGenabled (considering you're not running on anything recent).Okay. I'll provide full verbose logs, along with the register dump, when I'm back to the office next week, for the working and non-working case, but how - as attachment or as inline?
Quoted 7 lines
Quoted 4 lines
Either way will do but I have a feeling mailing list attachment size will bite you, so maybe it's best to make a tarball of both logs and send that as attachment. Compressed text will be very small.
Attached are the dwc3 verbose logs and register dump without and with the fixes in this patchset.
Sorry for the long delay, it has been a bit hectic.
I can see from your logs that we end up with a Transfer Not Ready (Transfer Active) event and endpoint has BUSY flag set. I also see that you're queueing 31 requests and all of them will use 2 TRBs, so we clearly go over our TRB table.
This fix is, indeed, necessary but we need to improve commit log a bit so it's extremely clear what the error is. Later, we can improve how we handle requests in this driver, but that is outside of the scope of your patchset.
Please improve commit log and resend your series so it can be applied.
cheers
Signature · 1 line
balbi
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Quoted 20 lines · Felipe Balbi
Hi,
Quoted 17 lines · Amit Virdi
Quoted 16 lines
Quoted 11 lines
Quoted 7 lines
Quoted 2 lines
I can certainly provide the dwc3 specific kernel bootup logs, full regdump and any loglevel you want me to, if that helps
Yeah, if you can provide those, then that'll help me verifying. Full logs from boot to failure point with
VERBOSE_DEBUGenabled (considering you're not running on anything recent).Okay. I'll provide full verbose logs, along with the register dump, when I'm back to the office next week, for the working and non-working case, but how - as attachment or as inline?
Either way will do but I have a feeling mailing list attachment size will bite you, so maybe it's best to make a tarball of both logs and send that as attachment. Compressed text will be very small.
Quoted 5 lines
Quoted 2 lines
Attached are the dwc3 verbose logs and register dump without and with the fixes in this patchset.
Sorry for the long delay, it has been a bit hectic.
It's okay!
Quoted 5 lines
I can see from your logs that we end up with a Transfer Not Ready (Transfer Active) event and endpoint has BUSY flag set. I also see that you're queueing 31 requests and all of them will use 2 TRBs, so we clearly go over our TRB table.
Quoted 7 lines
This fix is, indeed, necessary but we need to improve commit log a bit so it's extremely clear what the error is. Later, we can improve how we handle requests in this driver, but that is outside of the scope of your patchset.
Please improve commit log and resend your series so it can be applied.
Alright! I'll improve the commit messages and, also, cc stable list while resending the patches. As I see, you have already picked patches [3/4] and [4/4]. So, I'll resend only [1/4] and [2/4]
Quoted 2 lines
cheers
Thank you for your patience and kind understanding.
--
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
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Hi,
Quoted 42 lines · Amit Virdi
Quoted 20 lines · Felipe Balbi
Hi,
Quoted 17 lines · Amit Virdi
Quoted 16 lines
Quoted 11 lines
Quoted 7 lines
Quoted 2 lines
I can certainly provide the dwc3 specific kernel bootup logs, full regdump and any loglevel you want me to, if that helps
Yeah, if you can provide those, then that'll help me verifying. Full logs from boot to failure point with
VERBOSE_DEBUGenabled (considering you're not running on anything recent).Okay. I'll provide full verbose logs, along with the register dump, when I'm back to the office next week, for the working and non-working case, but how - as attachment or as inline?
Either way will do but I have a feeling mailing list attachment size will bite you, so maybe it's best to make a tarball of both logs and send that as attachment. Compressed text will be very small.
Quoted 5 lines
Quoted 2 lines
Attached are the dwc3 verbose logs and register dump without and with the fixes in this patchset.
Sorry for the long delay, it has been a bit hectic.
It's okay!
Quoted 5 lines
I can see from your logs that we end up with a Transfer Not Ready (Transfer Active) event and endpoint has BUSY flag set. I also see that you're queueing 31 requests and all of them will use 2 TRBs, so we clearly go over our TRB table.
Quoted 7 lines
This fix is, indeed, necessary but we need to improve commit log a bit so it's extremely clear what the error is. Later, we can improve how we handle requests in this driver, but that is outside of the scope of your patchset.
Please improve commit log and resend your series so it can be applied.
Quoted 8 lines
Alright! I'll improve the commit messages and, also, cc stable list while resending the patches. As I see, you have already picked patches [3/4] and [4/4]. So, I'll resend only [1/4] and [2/4]
Quoted 2 lines
cheers
Thank you for your patience and kind understanding.
Alright, I just applied your patches to testing/fixes. I'll start testing today and should be able to send a pull request to Greg by the end of the week, hopefully.
cheers
Signature · 1 line
balbi
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Quoted 4 lines
Alright, I just applied your patches to testing/fixes. I'll start testing today and should be able to send a pull request to Greg by the end of the week, hopefully.
Thanks! Just a small clarification - git failed to send patches to stable kernel list again (unfortunately I used the older configuration; so older git version - my bad).
Does these patches land up in the stable trees through maintainers or I should explicitly cc [email protected] now?
Re: [PATCH 2/4] usb: dwc3: gadget: Stop TRB preparation after limit is reached
Quoted 11 lines · Amit Virdi
Quoted 4 lines
Alright, I just applied your patches to testing/fixes. I'll start testing today and should be able to send a pull request to Greg by the end of the week, hopefully.
Thanks! Just a small clarification - git failed to send patches to stable kernel list again (unfortunately I used the older configuration; so older git version - my bad).
Does these patches land up in the stable trees through maintainers or I should explicitly cc [email protected] now?
Please read Documentation/stable_kernel_rules.txt for how to get patches accepted into the stable trees.
thanks,
greg k-h
PATCH 4/4 usb: dwc3: Remove current_trb as it is unused
In reply to a message not in the archiveThis field was introduced but never used. So, remove it.
--- drivers/usb/dwc3/core.h | 3 --- 1 file changed, 3 deletions(-)
drivers/usb/dwc3/core.h +0 -2
@@ -431,7 +431,6 @@ struct dwc3_event_buffer {* @dwc: pointer to DWC controller * @saved_state: ep state saved during hibernation * @flags: endpoint flags (wedged, stalled, ...) -* @current_trb: index of current used trb * @number: endpoint number (1 - 15) * @type: set to bmAttributes & USB_ENDPOINT_XFERTYPE_MASK * @resource_index: Resource transfer index
@@ -464,8 +463,6 @@ struct dwc3_ep {/* This last one is specific to EP0 */ #define DWC3_EP0_DIR_IN (1 << 31) -unsigned current_trb;
PATCH 1/4 usb: dwc3: gadget: Fix TRB preparation during SG
In reply to Amit VirdiWhen scatter gather is used, multiple TRBs are prepared from one DWC3 request. Hence, we must set the 'last' flag when the SG is last as well as the TRB is last. The current implementation uses list_is_last to check if the dwc3_request is the last request in the request_list.
This doesn't work when SG is used. This is because, when it is the last request, the first TRB preparation (in dwc3_prepare_one_trb) modifies the dwc3_request list's next and prev pointers while moving the URB to req_queued.
Hence, list_is_last always return false no matter what. The correct way is not to access the modified pointers of dwc3_request but to use list_empty macro instead.
gather implementation"
--- drivers/usb/dwc3/gadget.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/dwc3/gadget.c +1 -2
@@ -882,8 +882,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool starting)if (i == (request->num_mapped_sgs -1 ) ||sg_is_last (s)) { -if (list_is_last (&req->list, - &dep->request_list)) +if (list_empty (&dep->request_list)) last_one = true; chain = false; }
Signature · 1 line
1.8.0
Re: [PATCH 1/4] usb: dwc3: gadget: Fix TRB preparation during SG
Quoted 9 lines · Amit Virdi
When scatter gather is used, multiple TRBs are prepared from one DWC3 request. Hence, we must set the 'last' flag when the SG is last as well as the TRB is last. The current implementation uses
list_is_lastto check if thedwc3_requestis the last request in therequest_list.This doesn't work when SG is used. This is because, when it is the last request, the first TRB preparation (in
dwc3_prepare_one_trb) modifies thedwc3_requestlist's next and prev pointers while moving the URB toreq_queued.
Quoted 8 lines
Hence,
list_is_lastalways return false no matter what. The correct way is not to access the modified pointers ofdwc3_requestbut to uselist_emptymacro instead.Fixes e5ba5ec833aa4a76980b512d6a6779643516b850 ("usb: dwc3: gadget: fix scattergather implementation"
Signed-off-by Amit Virdi <[email protected]>
you need to Cc stable here and make sure you point out which kernel versions this should be backported to. Looks like this sould be:
Also, how have you tested this ? I need a test case to make sure it fails here and this patch really fixes the problem.
Quoted 21 lines
--- drivers/usb/dwc3/gadget.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/usb/dwc3/gadget.c +1
-2@@ -882,8 +882,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool starting)if (i == (request->num_mapped_sgs -1 ) ||sg_is_last (s)) { -if (list_is_last (&req->list, - &dep->request_list)) +if (list_empty (&dep->request_list)) last_one = true; chain = false; }Signature · 1 line
1.8.0
Signature · 1 line
balbi
Re: [PATCH 1/4] usb: dwc3: gadget: Fix TRB preparation during SG
Quoted 17 lines · Felipe Balbi
Quoted 16 lines · Amit Virdi
When scatter gather is used, multiple TRBs are prepared from one DWC3 request. Hence, we must set the 'last' flag when the SG is last as well as the TRB is last. The current implementation uses
list_is_lastto check if thedwc3_requestis the last request in therequest_list.This doesn't work when SG is used. This is because, when it is the last request, the first TRB preparation (in
dwc3_prepare_one_trb) modifies thedwc3_requestlist's next and prev pointers while moving the URB toreq_queued.Hence,
list_is_lastalways return false no matter what. The correct way is not to access the modified pointers ofdwc3_requestbut to uselist_emptymacro instead.Fixes e5ba5ec833aa4a76980b512d6a6779643516b850 ("usb: dwc3: gadget: fix scattergather implementation"
Quoted 6 lines
Quoted 1 line
Signed-off-by Amit Virdi <[email protected]>you need to Cc stable here and make sure you point out which kernel versions this should be backported to. Looks like this sould be:
Cc <[email protected]> # v3.9+
Okay. I checked the git log. The commit ("usb: dwc3: gadget: fix scatter gather implementation") was introduced in v3.8-rc5, hence v3.8, so I need to
Quoted 4 lines
Also, how have you tested this ? I need a test case to make sure it fails here and this patch really fixes the problem.
This bug can be easily reproduced/tested if the gadget driver submits a urb having number of sg entries mapped to DMA more than 1 on bulk endpoint. Following is the log snippet once this bug is reproduced:
----
dwc3 dwc3.0: ep2in-bulk: Transfer Not Ready
dwc3 dwc3.0: queing request 24cc5780 to ep2in-bulk length 960002 dwc3 dwc3.0: ep2in-bulk: req 24cc5780 dma 24eb6400 length 2 chain dwc3 dwc3.0: ep2in-bulk: req 24cc5780 dma 25901800 length 960000 dwc3 dwc3.0: queing request 24cc5000 to ep2in-bulk length 960002 dwc3 dwc3.0: ep2in-bulk: Transfer Not Ready
dwc3 dwc3.0: queing request 24cc5900 to ep2in-bulk length 960002
-----
Without this fix, the hardware never generates "Transfer Complete" event for the corresponding EP and goes into an unknown state.
Re: [PATCH 1/4] usb: dwc3: gadget: Fix TRB preparation during SG
Hi,
Quoted 23 lines · Amit Virdi
Quoted 22 lines · Felipe Balbi
Quoted 9 lines · Amit Virdi
When scatter gather is used, multiple TRBs are prepared from one DWC3 request. Hence, we must set the 'last' flag when the SG is last as well as the TRB is last. The current implementation uses
list_is_lastto check if thedwc3_requestis the last request in therequest_list.This doesn't work when SG is used. This is because, when it is the last request, the first TRB preparation (in
dwc3_prepare_one_trb) modifies thedwc3_requestlist's next and prev pointers while moving the URB toreq_queued.Quoted 8 lines
Hence,
list_is_lastalways return false no matter what. The correct way is not to access the modified pointers ofdwc3_requestbut to uselist_emptymacro instead.Fixes e5ba5ec833aa4a76980b512d6a6779643516b850 ("usb: dwc3: gadget: fix scattergather implementation"
Signed-off-by Amit Virdi <[email protected]>you need to Cc stable here and make sure you point out which kernel versions this should be backported to. Looks like this sould be:
Quoted 7 lines
Quoted 1 line
Cc <[email protected]> # v3.9+Okay. I checked the git log. The commit ("usb: dwc3: gadget: fix scatter gather implementation") was introduced in v3.8-rc5, hence v3.8, so I need to
Cc <[email protected]> # v3.8+
hehe, many folks get confused by this. New features will never get merged upstream during the -rc cycle. -rc5 is when I applied it to my tree so it could be merged on the following merge window, which was v3.9.
Quoted 6 lines
Quoted 3 lines
Also, how have you tested this ? I need a test case to make sure it fails here and this patch really fixes the problem.
This bug can be easily reproduced/tested if the gadget driver submits a urb having number of sg entries mapped to DMA more than 1 on bulk
which gadget driver does this ?
Quoted 11 lines
endpoint. Following is the log snippet once this bug is reproduced:
----
dwc3 dwc3.0: ep2in-bulk: Transfer Not Ready
dwc3 dwc3.0: queing request24cc5780to ep2in-bulk length 960002 dwc3 dwc3.0: ep2in-bulk: req24cc5780dma24eb6400length 2 chain dwc3 dwc3.0: ep2in-bulk: req24cc5780dma 25901800 length 960000 dwc3 dwc3.0: queing request24cc5000to ep2in-bulk length 960002 dwc3 dwc3.0: ep2in-bulk: Transfer Not Ready
dwc3 dwc3.0: queing request24cc5900to ep2in-bulk length 960002
-----
Quoted 2 lines
Without this fix, the hardware never generates "Transfer Complete" event for the corresponding EP and goes into an unknown state.
whiuch kernel are you using to develop this ? Most of these messages don't exist anymore with v3.19-rc because I have converted them into tracepoints, considering you're showing me logs with these messages, this tells me that you're sending me a patch developed/tested against a kernel older than v3.18. I need to see full bootup logs, a register dump (/sys/kernel/debug/*dwc3*/regdump) and a much larger debugging log from dwc3 in order to accept patches from you. Sorry, but I cannot take patches which were not tested against, at a minimum, the latest major release.
It would be much better if you developed/tested against v3.19-rc1, considering we have 40 different patches which were merged since v3.18 was tagged.
Signature · 1 line
balbi
Re: [PATCH 1/4] usb: dwc3: gadget: Fix TRB preparation during SG
Quoted 32 lines · Felipe Balbi
Hi,
Quoted 29 lines · Amit Virdi
Quoted 17 lines · Felipe Balbi
Quoted 16 lines · Amit Virdi
When scatter gather is used, multiple TRBs are prepared from one DWC3 request. Hence, we must set the 'last' flag when the SG is last as well as the TRB is last. The current implementation uses
list_is_lastto check if thedwc3_requestis the last request in therequest_list.This doesn't work when SG is used. This is because, when it is the last request, the first TRB preparation (in
dwc3_prepare_one_trb) modifies thedwc3_requestlist's next and prev pointers while moving the URB toreq_queued.Hence,
list_is_lastalways return false no matter what. The correct way is not to access the modified pointers ofdwc3_requestbut to uselist_emptymacro instead.Fixes e5ba5ec833aa4a76980b512d6a6779643516b850 ("usb: dwc3: gadget: fix scattergather implementation"
Quoted 6 lines
Quoted 1 line
Signed-off-by Amit Virdi <[email protected]>you need to Cc stable here and make sure you point out which kernel versions this should be backported to. Looks like this sould be:
Cc <[email protected]> # v3.9+Okay. I checked the git log. The commit ("usb: dwc3: gadget: fix scatter gather implementation") was introduced in v3.8-rc5, hence v3.8, so I need to
Quoted 7 lines
Quoted 1 line
Cc <[email protected]> # v3.8+hehe, many folks get confused by this. New features will never get merged upstream during the -rc cycle. -rc5 is when I applied it to my tree so it could be merged on the following merge window, which was v3.9.
Got it, thanks!
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Also, how have you tested this ? I need a test case to make sure it fails here and this patch really fixes the problem.
This bug can be easily reproduced/tested if the gadget driver submits a urb having number of sg entries mapped to DMA more than 1 on bulk
which gadget driver does this ?
I discovered the bug fixed in this patch and the other [2/4], when I was using our customized uvc webcam gadget.
Quoted 24 lines
Quoted 11 lines
endpoint. Following is the log snippet once this bug is reproduced:
----
dwc3 dwc3.0: ep2in-bulk: Transfer Not Ready
dwc3 dwc3.0: queing request24cc5780to ep2in-bulk length 960002 dwc3 dwc3.0: ep2in-bulk: req24cc5780dma24eb6400length 2 chain dwc3 dwc3.0: ep2in-bulk: req24cc5780dma 25901800 length 960000 dwc3 dwc3.0: queing request24cc5000to ep2in-bulk length 960002 dwc3 dwc3.0: ep2in-bulk: Transfer Not Ready
dwc3 dwc3.0: queing request24cc5900to ep2in-bulk length 960002
-----Quoted 2 lines
Without this fix, the hardware never generates "Transfer Complete" event for the corresponding EP and goes into an unknown state.
whiuch kernel are you using to develop this ? Most of these messages don't exist anymore with v3.19-rc because I have converted them into tracepoints, considering you're showing me logs with these messages, this tells me that you're sending me a patch developed/tested against a kernel older than v3.18. I need to see full bootup logs, a register dump (
/sys/kernel/debug/*dwc3*/regdump) and a much larger debugging log from dwc3 in order to accept patches from you. Sorry, but I cannot take patches which were not tested against, at a minimum, the latest major release.
Quoted 4 lines
It would be much better if you developed/tested against v3.19-rc1, considering we have 40 different patches which were merged since v3.18 was tagged.
Your comments for this patch and the other are similar. So, I'm quoting your comments from here to the other thread (of patch 2/4) and replying there to avoid any duplicacy.
--
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
PATCH 3/4 usb: dwc3: gadget: Remove redundant check
In reply to Amit Virdidwc3_gadget_init_hw_endpoints calls dwc3_alloc_trb_pool only if epnum is not equal to 0 or 1. Hence, rechecking it in the called function is redundant.
--- drivers/usb/dwc3/gadget.c | 3 --- 1 file changed, 3 deletions(-)
drivers/usb/dwc3/gadget.c +0 -3
@@ -352,9 +352,6 @@ static int dwc3_alloc_trb_pool(struct dwc3_ep *dep)if (dep->trb_pool)return 0 ; -if (dep->number ==0 || dep->number ==1 ) -return 0 ; - dep->trb_pool =dma_alloc_coherent (dwc->dev,sizeof (struct dwc3_trb ) *DWC3_TRB_NUM , &dep->trb_pool_dma,GFP_KERNEL );
Signature · 1 line
1.8.0