| Message ID | 1537021801-23896-1-git-send-email-anurag.kumar.vulisha@xilinx.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver | expand |
Hello Anurag, On 9/15/2018 8:00 PM, Anurag Kumar Vulisha wrote: > These patch series fixes the broken BULK streaming support in > dwc3 gadget driver. > > Changes in v5: > 1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen" > > Changes in v4: > 1. Corrected the commit messgae and stream timeout description > as suggested by "Thinh Nguyen" > > Changes in v3: > 1. Added the changes suggested by "Thinh Nguyen" > > Changes in v2: > 1. Added "usb: dwc3:" in subject heading > > Anurag Kumar Vulisha (8): > usb: dwc3: Correct the logic for checking TRB full in > __dwc3_prepare_one_trb() > usb: dwc3: update stream id in depcmd > usb: dwc3: make controller clear transfer resources after complete > usb: dwc3: implement stream transfer timeout > usb: dwc3: don't issue no-op trb for stream capable endpoints > usb: dwc3: check for requests in started list for stream capable > endpoints > usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl > fields > usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints > > drivers/usb/dwc3/core.h | 7 ++++ > drivers/usb/dwc3/gadget.c | 85 ++++++++++++++++++++++++++++++++++++++++++----- > 2 files changed, 84 insertions(+), 8 deletions(-) > Tested-By: Tejas Joglekar <tejas.joglekar@synopsys.com> I have tested this patch series except the stream transfer timeout patch on HAPS-DX platform. I am not aware of exact scenarios where I can test the timeout patch, and don't have test to perform timeout testing. Thanks & Regards, Tejas Joglekar
Hello Anurag, On 9/15/2018 8:00 PM, Anurag Kumar Vulisha wrote: > These patch series fixes the broken BULK streaming support in > dwc3 gadget driver. > > Changes in v5: > 1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen" > > Changes in v4: > 1. Corrected the commit messgae and stream timeout description > as suggested by "Thinh Nguyen" > > Changes in v3: > 1. Added the changes suggested by "Thinh Nguyen" > > Changes in v2: > 1. Added "usb: dwc3:" in subject heading > > Anurag Kumar Vulisha (8): > usb: dwc3: Correct the logic for checking TRB full in > __dwc3_prepare_one_trb() > usb: dwc3: update stream id in depcmd > usb: dwc3: make controller clear transfer resources after complete > usb: dwc3: implement stream transfer timeout > usb: dwc3: don't issue no-op trb for stream capable endpoints > usb: dwc3: check for requests in started list for stream capable > endpoints > usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl > fields > usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints > > drivers/usb/dwc3/core.h | 7 ++++ > drivers/usb/dwc3/gadget.c | 85 ++++++++++++++++++++++++++++++++++++++++++----- > 2 files changed, 84 insertions(+), 8 deletions(-) > Tested-By: Tejas Joglekar <tejas.joglekar@synopsys.com> I have tested this patch series except the stream transfer timeout patch on HAPS-DX platform. I am not aware of exact scenarios to test the timeout patch and don't have a test for the same. Thanks, Tejas Joglekar
Hi Tejas, >-----Original Message----- >From: Tejas Joglekar [mailto:Tejas.Joglekar@synopsys.com] >Sent: Friday, September 21, 2018 7:01 PM >To: Anurag Kumar Vulisha <anuragku@xilinx.com>; balbi@kernel.org; >gregkh@linuxfoundation.org >Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux- >kernel@vger.kernel.org; Thinh.Nguyen@synopsys.com; Ajay Yugalkishore Pandey ><APANDEY@xilinx.com>; joglekartejas@gmail.com >Subject: Re: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 >gadget driver > >Hello Anurag, >On 9/15/2018 8:00 PM, Anurag Kumar Vulisha wrote: >> These patch series fixes the broken BULK streaming support in >> dwc3 gadget driver. >> >> Changes in v5: >> 1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen" >> >> Changes in v4: >> 1. Corrected the commit messgae and stream timeout description >> as suggested by "Thinh Nguyen" >> >> Changes in v3: >> 1. Added the changes suggested by "Thinh Nguyen" >> >> Changes in v2: >> 1. Added "usb: dwc3:" in subject heading >> >> Anurag Kumar Vulisha (8): >> usb: dwc3: Correct the logic for checking TRB full in >> __dwc3_prepare_one_trb() >> usb: dwc3: update stream id in depcmd >> usb: dwc3: make controller clear transfer resources after complete >> usb: dwc3: implement stream transfer timeout >> usb: dwc3: don't issue no-op trb for stream capable endpoints >> usb: dwc3: check for requests in started list for stream capable >> endpoints >> usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl >> fields >> usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints >> >> drivers/usb/dwc3/core.h | 7 ++++ >> drivers/usb/dwc3/gadget.c | 85 >++++++++++++++++++++++++++++++++++++++++++----- >> 2 files changed, 84 insertions(+), 8 deletions(-) >> >Tested-By: Tejas Joglekar <tejas.joglekar@synopsys.com> >I have tested this patch series except the stream transfer timeout patch on HAPS-DX >platform. I am not aware of exact scenarios to test the timeout patch and don't have >a test for the same. Thanks for testing the patches. The issue mentioned in the timeout patch (Patch 4) will occur very rarely on the long runs and only when tested with stream capable host. This issue happens only when the host & dwc3 controller go out of sync, where the dwc3 controller may wait for host to issue prime transaction and host may wait for the gadget to issue ERDY. I used controller version 2.90A for testing this issue. This issue is mentioned in databook section 9.5.2 Thanks, Anurag Kumar Vulisha
Hello Anurag, On 9/21/2018 7:36 PM, Anurag Kumar Vulisha wrote: > Hi Tejas, > >> -----Original Message----- >> From: Tejas Joglekar [mailto:Tejas.Joglekar@synopsys.com] >> Sent: Friday, September 21, 2018 7:01 PM >> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; balbi@kernel.org; >> gregkh@linuxfoundation.org >> Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux- >> kernel@vger.kernel.org; Thinh.Nguyen@synopsys.com; Ajay Yugalkishore Pandey >> <APANDEY@xilinx.com>; joglekartejas@gmail.com >> Subject: Re: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 >> gadget driver >> >> Hello Anurag, >> On 9/15/2018 8:00 PM, Anurag Kumar Vulisha wrote: >>> These patch series fixes the broken BULK streaming support in >>> dwc3 gadget driver. >>> >>> Changes in v5: >>> 1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen" >>> >>> Changes in v4: >>> 1. Corrected the commit messgae and stream timeout description >>> as suggested by "Thinh Nguyen" >>> >>> Changes in v3: >>> 1. Added the changes suggested by "Thinh Nguyen" >>> >>> Changes in v2: >>> 1. Added "usb: dwc3:" in subject heading >>> >>> Anurag Kumar Vulisha (8): >>> usb: dwc3: Correct the logic for checking TRB full in >>> __dwc3_prepare_one_trb() >>> usb: dwc3: update stream id in depcmd >>> usb: dwc3: make controller clear transfer resources after complete >>> usb: dwc3: implement stream transfer timeout >>> usb: dwc3: don't issue no-op trb for stream capable endpoints >>> usb: dwc3: check for requests in started list for stream capable >>> endpoints >>> usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl >>> fields >>> usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints >>> >>> drivers/usb/dwc3/core.h | 7 ++++ >>> drivers/usb/dwc3/gadget.c | 85 >> ++++++++++++++++++++++++++++++++++++++++++----- >>> 2 files changed, 84 insertions(+), 8 deletions(-) >>> >> Tested-By: Tejas Joglekar <tejas.joglekar@synopsys.com> >> I have tested this patch series except the stream transfer timeout patch on HAPS-DX >> platform. I am not aware of exact scenarios to test the timeout patch and don't have >> a test for the same. > Thanks for testing the patches. The issue mentioned in the timeout patch (Patch 4) will > occur very rarely on the long runs and only when tested with stream capable host. This > issue happens only when the host & dwc3 controller go out of sync, where the dwc3 > controller may wait for host to issue prime transaction and host may wait for the gadget > to issue ERDY. I used controller version 2.90A for testing this issue. This issue is mentioned > in databook section 9.5.2 > > Thanks, > Anurag Kumar Vulisha > > I see, as it is rare so maybe it is not seen here. I have used 3.30A for testing. I will keep watch on this issue during my tests. --- Thanks, Tejas Joglekar
Hi Felipe, Please let us know if you have any suggestions / comments on this patch series. If you feel this patch series are okay, can we proceed with them? Thanks, Anurag Kumar Vulisha >-----Original Message----- >From: Anurag Kumar Vulisha [mailto:anurag.kumar.vulisha@xilinx.com] >Sent: Saturday, September 15, 2018 8:00 PM >To: balbi@kernel.org; gregkh@linuxfoundation.org >Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux- >kernel@vger.kernel.org; Thinh.Nguyen@synopsys.com; Ajay Yugalkishore Pandey ><APANDEY@xilinx.com>; Anurag Kumar Vulisha <anuragku@xilinx.com> >Subject: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget >driver > >These patch series fixes the broken BULK streaming support in >dwc3 gadget driver. > >Changes in v5: > 1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen" > >Changes in v4: > 1. Corrected the commit messgae and stream timeout description > as suggested by "Thinh Nguyen" > >Changes in v3: > 1. Added the changes suggested by "Thinh Nguyen" > >Changes in v2: > 1. Added "usb: dwc3:" in subject heading > >Anurag Kumar Vulisha (8): > usb: dwc3: Correct the logic for checking TRB full in > __dwc3_prepare_one_trb() > usb: dwc3: update stream id in depcmd > usb: dwc3: make controller clear transfer resources after complete > usb: dwc3: implement stream transfer timeout > usb: dwc3: don't issue no-op trb for stream capable endpoints > usb: dwc3: check for requests in started list for stream capable > endpoints > usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl > fields > usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints > > drivers/usb/dwc3/core.h | 7 ++++ > drivers/usb/dwc3/gadget.c | 85 ++++++++++++++++++++++++++++++++++++++++++- >---- > 2 files changed, 84 insertions(+), 8 deletions(-) > >-- >2.1.1
(no top-posting, please) Hi, Anurag Kumar Vulisha <anuragku@xilinx.com> writes: > Hi Felipe, > > Please let us know if you have any suggestions / comments on this patch series. > If you feel this patch series are okay, can we proceed with them? I really don't like this dwc3-specific timer. The best way here would be to add a timer on udc/core.c which can be reused by any udc. This would mean, of course, teaching udc/core about streams and lettting it do part of the handling. Best
Hi Felipe, >-----Original Message----- >From: Felipe Balbi [mailto:balbi@kernel.org] >Sent: Tuesday, October 09, 2018 11:07 AM >To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Anurag Kumar Vulisha ><anuragku@xilinx.com>; gregkh@linuxfoundation.org >Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux- >kernel@vger.kernel.org; Thinh.Nguyen@synopsys.com; Ajay Yugalkishore Pandey ><APANDEY@xilinx.com> >Subject: RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 >gadget driver > > >(no top-posting, please) > Will ensure this won't happen again >Hi, > > >Anurag Kumar Vulisha <anuragku@xilinx.com> writes: > >> Hi Felipe, >> >> Please let us know if you have any suggestions / comments on this patch series. >> If you feel this patch series are okay, can we proceed with them? > >I really don't like this dwc3-specific timer. The best way here would be >to add a timer on udc/core.c which can be reused by any udc. This would >mean, of course, teaching udc/core about streams and lettting it do part >of the handling. > Thanks for spending your time in reviewing this patch. The reason for adding the timer is when streams are enabled there could be chances for the host and gadget controller to become out of sync, the gadget may wait for the host to issue prime transaction and the host may wait for the gadget to issue ERDY. To avoid such a potential deadlock conditions, timeout needs to be implemented in dwc3 driver. After timeout occurs, gadget will first stop transfer and restart the transfer again. This issue is mentioned in databook 2.90A section 9.5.2. I am not aware of how other controllers are handling the streams, but since this issue looks more like a dwc3 specific issue, I think it would be more convincing to add the timer in dwc3 gadget driver rather than adding in udc framework. Also we are stopping the timer when a valid StreamEvnt is found, which would be difficult to handle if the timer is moved into udc. Please help me by correcting , if I am missing something or my understanding is wrong. Thanks, Anurag Kumar Vulisha >Best > >-- >balbi
Hi, Anurag Kumar Vulisha <anuragku@xilinx.com> writes: >>> Please let us know if you have any suggestions / comments on this patch series. >>> If you feel this patch series are okay, can we proceed with them? >> >>I really don't like this dwc3-specific timer. The best way here would be >>to add a timer on udc/core.c which can be reused by any udc. This would >>mean, of course, teaching udc/core about streams and lettting it do part >>of the handling. >> > > Thanks for spending your time in reviewing this patch. The reason for adding the > timer is when streams are enabled there could be chances for the host and gadget > controller to become out of sync, the gadget may wait for the host to issue prime > transaction and the host may wait for the gadget to issue ERDY. To avoid such a > potential deadlock conditions, timeout needs to be implemented in dwc3 driver. "in dwc3 driver" is an implementation choice. The situation you describe could happen with any UDC, right? > After timeout occurs, gadget will first stop transfer and restart the transfer again. > This issue is mentioned in databook 2.90A section 9.5.2. I am not aware of how > other controllers are handling the streams, but since this issue looks more like a We should get in touch with other UDC authors. We have at least Renesas, net2280, bcd_udc and mtu3 supporting superspeed. > dwc3 specific issue, I think it would be more convincing to add the timer in dwc3 > gadget driver rather than adding in udc framework. Also we are stopping the timer why? When the situation you describe is something that can happen with any udc, why should we reimplement the solution on all UDCs supporting streams when we can give generic support for handling certain situations? > when a valid StreamEvnt is found, which would be difficult to handle if the timer is Why difficult? udc-core would call: mod_timer(gadget->stream_timeout_timer, msecs_to_jiffies(50)); Once you receive stream event, dwc3 would call: if (timer_pending(dwc->gadget.stream_timeout_timer)) del_timer(dwc->gadget.stream_timeout_timer); Why is that difficult? You could even avoid anything to be written in dwc3 and put the del_timer() inside usb_gadget_giveback_request() itself. That why, dwc3 doesn't even have to know that there's a timer running. Also, you're timer function, instead of calling dwc3's private functions, should be relying on the gadget API. Your timer, apparently, should be fired per-request, then your timer function would call: usb_ep_dequeue(request); usb_ep_queue(request); If the timer expires. This would work for any UDC, not only dwc3. Then, this is something we document for all UDCs and they'd have to adhere to the API. In summary, not that many changes needed to dwc3. Nothing related to timers inside dwc3. Almost everythin can, and should, be done generically.
Hi Felipe, >-----Original Message----- >From: Felipe Balbi [mailto:balbi@kernel.org] >Sent: Tuesday, October 09, 2018 12:51 PM >To: Anurag Kumar Vulisha <anuragku@xilinx.com>; gregkh@linuxfoundation.org >Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux- >kernel@vger.kernel.org; Thinh.Nguyen@synopsys.com; Ajay Yugalkishore Pandey ><APANDEY@xilinx.com> >Subject: RE: [PATCH v5 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 >gadget driver > > >Hi, > >Anurag Kumar Vulisha <anuragku@xilinx.com> writes: >>>> Please let us know if you have any suggestions / comments on this patch series. >>>> If you feel this patch series are okay, can we proceed with them? >>> >>>I really don't like this dwc3-specific timer. The best way here would be >>>to add a timer on udc/core.c which can be reused by any udc. This would >>>mean, of course, teaching udc/core about streams and lettting it do part >>>of the handling. >>> >> >> Thanks for spending your time in reviewing this patch. The reason for adding the >> timer is when streams are enabled there could be chances for the host and gadget >> controller to become out of sync, the gadget may wait for the host to issue prime >> transaction and the host may wait for the gadget to issue ERDY. To avoid such a >> potential deadlock conditions, timeout needs to be implemented in dwc3 driver. > >"in dwc3 driver" is an implementation choice. The situation you describe >could happen with any UDC, right? > Yes this could happen to other UDC drivers also, unless controller is capable of handling >> After timeout occurs, gadget will first stop transfer and restart the transfer again. >> This issue is mentioned in databook 2.90A section 9.5.2. I am not aware of how >> other controllers are handling the streams, but since this issue looks more like a > >We should get in touch with other UDC authors. We have at least Renesas, >net2280, bcd_udc and mtu3 supporting superspeed. > Thanks for pointing other drivers. Will refer these drivers to see how they are handling streams >> dwc3 specific issue, I think it would be more convincing to add the timer in dwc3 >> gadget driver rather than adding in udc framework. Also we are stopping the timer > >why? When the situation you describe is something that can happen with >any udc, why should we reimplement the solution on all UDCs supporting >streams when we can give generic support for handling certain >situations? > I agree with you. As you suggested will work on implementing changes in UDC >> when a valid StreamEvnt is found, which would be difficult to handle if the timer is > >Why difficult? udc-core would call: > >mod_timer(gadget->stream_timeout_timer, msecs_to_jiffies(50)); > >Once you receive stream event, dwc3 would call: > >if (timer_pending(dwc->gadget.stream_timeout_timer)) > del_timer(dwc->gadget.stream_timeout_timer); > >Why is that difficult? You could even avoid anything to be written in >dwc3 and put the del_timer() inside usb_gadget_giveback_request() >itself. That why, dwc3 doesn't even have to know that there's a timer >running. Also, you're timer function, instead of calling dwc3's private >functions, should be relying on the gadget API. > >Your timer, apparently, should be fired per-request, then your timer >function would call: > >usb_ep_dequeue(request); >usb_ep_queue(request); > >If the timer expires. This would work for any UDC, not only dwc3. Then, >this is something we document for all UDCs and they'd have to adhere to >the API. > >In summary, not that many changes needed to dwc3. Nothing related to >timers inside dwc3. Almost everythin can, and should, be done >generically. Thanks a lot for giving a detailed explanation. Will implement the timeout changes into UDC core. Best Regards, Anurag Kumar Vulisha
Hi, Anurag Kumar Vulisha <anuragku@xilinx.com> writes: >>> Thanks for spending your time in reviewing this patch. The reason for adding the >>> timer is when streams are enabled there could be chances for the host and gadget >>> controller to become out of sync, the gadget may wait for the host to issue prime >>> transaction and the host may wait for the gadget to issue ERDY. To avoid such a >>> potential deadlock conditions, timeout needs to be implemented in dwc3 driver. >> >>"in dwc3 driver" is an implementation choice. The situation you describe >>could happen with any UDC, right? >> > > Yes this could happen to other UDC drivers also, unless controller is capable of handling > >>> After timeout occurs, gadget will first stop transfer and restart the transfer again. >>> This issue is mentioned in databook 2.90A section 9.5.2. I am not aware of how >>> other controllers are handling the streams, but since this issue looks more like a >> >>We should get in touch with other UDC authors. We have at least Renesas, >>net2280, bcd_udc and mtu3 supporting superspeed. >> > > Thanks for pointing other drivers. Will refer these drivers to see how they are handling streams > >>> dwc3 specific issue, I think it would be more convincing to add the timer in dwc3 >>> gadget driver rather than adding in udc framework. Also we are stopping the timer >> >>why? When the situation you describe is something that can happen with >>any udc, why should we reimplement the solution on all UDCs supporting >>streams when we can give generic support for handling certain >>situations? >> > > I agree with you. As you suggested will work on implementing changes in UDC > >>> when a valid StreamEvnt is found, which would be difficult to handle if the timer is >> >>Why difficult? udc-core would call: >> >>mod_timer(gadget->stream_timeout_timer, msecs_to_jiffies(50)); >> >>Once you receive stream event, dwc3 would call: >> >>if (timer_pending(dwc->gadget.stream_timeout_timer)) >> del_timer(dwc->gadget.stream_timeout_timer); >> >>Why is that difficult? You could even avoid anything to be written in >>dwc3 and put the del_timer() inside usb_gadget_giveback_request() >>itself. That why, dwc3 doesn't even have to know that there's a timer >>running. Also, you're timer function, instead of calling dwc3's private >>functions, should be relying on the gadget API. >> >>Your timer, apparently, should be fired per-request, then your timer >>function would call: >> >>usb_ep_dequeue(request); >>usb_ep_queue(request); >> >>If the timer expires. This would work for any UDC, not only dwc3. Then, >>this is something we document for all UDCs and they'd have to adhere to >>the API. >> >>In summary, not that many changes needed to dwc3. Nothing related to >>timers inside dwc3. Almost everythin can, and should, be done >>generically. > > Thanks a lot for giving a detailed explanation. Will implement the timeout > changes into UDC core. no problem. I just wanna make sure that this work happens in one place and one place only :) cheers
These patch series fixes the broken BULK streaming support in dwc3 gadget driver. Changes in v5: 1. Removed the dev_dbg prints as suggested bt "Thinh Nguyen" Changes in v4: 1. Corrected the commit messgae and stream timeout description as suggested by "Thinh Nguyen" Changes in v3: 1. Added the changes suggested by "Thinh Nguyen" Changes in v2: 1. Added "usb: dwc3:" in subject heading Anurag Kumar Vulisha (8): usb: dwc3: Correct the logic for checking TRB full in __dwc3_prepare_one_trb() usb: dwc3: update stream id in depcmd usb: dwc3: make controller clear transfer resources after complete usb: dwc3: implement stream transfer timeout usb: dwc3: don't issue no-op trb for stream capable endpoints usb: dwc3: check for requests in started list for stream capable endpoints usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints drivers/usb/dwc3/core.h | 7 ++++ drivers/usb/dwc3/gadget.c | 85 ++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 84 insertions(+), 8 deletions(-)