| Message ID | 19a1dd73d68507543e955f508fe47eb7ffef9e73.1532742607.git.thinhn@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] usb: dwc3: gadget: Check MaxPacketSize from descriptor | expand |
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > DWC3 must check for the BUFSIZ and update the req->remaining > regardless of transfer alignment. Returning early from transfer OUT > unalignment will skip updating the req->remaining. > > Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize") > Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> > --- > drivers/usb/dwc3/gadget.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 032ea7d709ba..a5b8387a37ba 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2246,6 +2246,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, > if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO)) > trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > > + count = trb->size & DWC3_TRB_SIZE_MASK; > + req->remaining += count; > + > /* > * If we're dealing with unaligned size OUT transfer, we will be left > * with one TRB pending in the ring. We need to manually clear HWO bit > @@ -2256,9 +2259,6 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, > return 1; > } > > - count = trb->size & DWC3_TRB_SIZE_MASK; > - req->remaining += count; > - this shouldn't be necessary, actually. The remaining TRB which was added to fix alignment issues, is simply because DWC3 doesn't like OUT transfers whose size are not aligned to wMaxPacketSize. We *know* we have e.g. 511 bytes to receive from host, but dwc3 wants me to write 1024 bytes on the TRB (for superspeed), so I need to add this chained TRB for the remaining 513 bytes. I don't expect host to send us more than 511 bytes, if it does, the host is faulty and I don't want to operate on those bytes anyway.
Hi Felipe, On 7/29/2018 11:08 PM, Felipe Balbi wrote: > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> DWC3 must check for the BUFSIZ and update the req->remaining >> regardless of transfer alignment. Returning early from transfer OUT >> unalignment will skip updating the req->remaining. >> >> Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize") >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >> --- >> drivers/usb/dwc3/gadget.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 032ea7d709ba..a5b8387a37ba 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2246,6 +2246,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, >> if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO)) >> trb->ctrl &= ~DWC3_TRB_CTRL_HWO; >> >> + count = trb->size & DWC3_TRB_SIZE_MASK; >> + req->remaining += count; >> + >> /* >> * If we're dealing with unaligned size OUT transfer, we will be left >> * with one TRB pending in the ring. We need to manually clear HWO bit >> @@ -2256,9 +2259,6 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, >> return 1; >> } >> >> - count = trb->size & DWC3_TRB_SIZE_MASK; >> - req->remaining += count; >> - > this shouldn't be necessary, actually. The remaining TRB which was added > to fix alignment issues, is simply because DWC3 doesn't like OUT > transfers whose size are not aligned to wMaxPacketSize. We *know* we > have e.g. 511 bytes to receive from host, but dwc3 wants me to write > 1024 bytes on the TRB (for superspeed), so I need to add this chained > TRB for the remaining 513 bytes. > > I don't expect host to send us more than 511 bytes, if it does, the host > is faulty and I don't want to operate on those bytes anyway. > Right. But that's not the issue here. I realized that I did not describe the issue fully... The issue here is the reporting of the actual number of bytes transferred. For isoc OUT transfers, the last TRB of the Buffer Descriptor will be retired with HWO = 0 (dwc_usb3 and dwc_usb31 programming guide 4.3.7). So, the check for unaligned && HWO will be false and will not return early. As a result, req->remaining will still be updated with the BUFSIZ count, and the remaining will be 513+ in your example. The 'actual' bytes written calculation will be wrong (e.g 511 - 513 = -2). We can't rely on the current check to determine whether it's the last TRB. My current solution is to update req->remaining for all TRB completions. However, we have to also update the 'actual' bytes calculation for OUT transfers. Thanks, Thinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > Hi Felipe, > > On 7/29/2018 11:08 PM, Felipe Balbi wrote: >> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> DWC3 must check for the BUFSIZ and update the req->remaining >>> regardless of transfer alignment. Returning early from transfer OUT >>> unalignment will skip updating the req->remaining. >>> >>> Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize") >>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >>> --- >>> drivers/usb/dwc3/gadget.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 032ea7d709ba..a5b8387a37ba 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -2246,6 +2246,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, >>> if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO)) >>> trb->ctrl &= ~DWC3_TRB_CTRL_HWO; >>> >>> + count = trb->size & DWC3_TRB_SIZE_MASK; >>> + req->remaining += count; >>> + >>> /* >>> * If we're dealing with unaligned size OUT transfer, we will be left >>> * with one TRB pending in the ring. We need to manually clear HWO bit >>> @@ -2256,9 +2259,6 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, >>> return 1; >>> } >>> >>> - count = trb->size & DWC3_TRB_SIZE_MASK; >>> - req->remaining += count; >>> - >> this shouldn't be necessary, actually. The remaining TRB which was added >> to fix alignment issues, is simply because DWC3 doesn't like OUT >> transfers whose size are not aligned to wMaxPacketSize. We *know* we >> have e.g. 511 bytes to receive from host, but dwc3 wants me to write >> 1024 bytes on the TRB (for superspeed), so I need to add this chained >> TRB for the remaining 513 bytes. >> >> I don't expect host to send us more than 511 bytes, if it does, the host >> is faulty and I don't want to operate on those bytes anyway. >> > > Right. But that's not the issue here. I realized that I did not describe > the issue fully... > > The issue here is the reporting of the actual number of bytes > transferred. For isoc OUT transfers, the last TRB of the Buffer > Descriptor will be retired with HWO = 0 (dwc_usb3 and dwc_usb31 > programming guide 4.3.7). So, the check for unaligned && HWO will be > false and will not return early. As a result, req->remaining will still > be updated with the BUFSIZ count, and the remaining will be 513+ in your > example. The 'actual' bytes written calculation will be wrong (e.g 511 - > 513 = -2). > > We can't rely on the current check to determine whether it's the last > TRB. My current solution is to update req->remaining for all TRB > completions. However, we have to also update the 'actual' bytes > calculation for OUT transfers. I think the bug here is another one. Look at the TRB types: irq/16-dwc3-2463 [003] d... 3589.933478: dwc3_complete_trb: ep1out: trb 000000007d3fbc0d buf 00000000b8ed3800 size 0 ctrl 2e46446c (hlCS:Sc:isoc-first) irq/16-dwc3-2463 [003] d... 3589.933492: dwc3_complete_trb: ep1out: trb 00000000d1fcb0c4 buf 000000003787e000 size 1023 ctrl 2e464c68 (hlcS:SC:isoc-first) Shouldn't the one be of type ISOC instead of ISOC_FIRST? Can you test patch below and see if things behave better? Please also capture tracepoints :) modified drivers/usb/dwc3/gadget.c @@ -1073,7 +1073,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep, /* Now prepare one extra TRB to align transfer size */ trb = &dep->trb_pool[dep->trb_enqueue]; __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, - maxp - rem, false, 0, + maxp - rem, false, 1, req->request.stream_id, req->request.short_not_ok, req->request.no_interrupt); @@ -1117,7 +1117,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep, /* Now prepare one extra TRB to align transfer size */ trb = &dep->trb_pool[dep->trb_enqueue]; __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - rem, - false, 0, req->request.stream_id, + false, 1, req->request.stream_id, req->request.short_not_ok, req->request.no_interrupt); } else if (req->request.zero && req->request.length &&
Hi, First of all, not receiving your mails through the mailing list. Can you check that you're not being blocked for some reason? You may wanna send trace data compressed with gzip or xz. On Wed, Aug 1, 2018 at 2:27 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > On 7/30/2018 11:58 PM, Felipe Balbi wrote: > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > >> Hi Felipe, > >> > >> On 7/29/2018 11:08 PM, Felipe Balbi wrote: > >>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > >>>> DWC3 must check for the BUFSIZ and update the req->remaining > >>>> regardless of transfer alignment. Returning early from transfer OUT > >>>> unalignment will skip updating the req->remaining. > >>>> > >>>> Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to > wMaxPacketSize") > >>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> > >>>> --- > >>>> drivers/usb/dwc3/gadget.c | 6 +++--- > >>>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > >>>> index 032ea7d709ba..a5b8387a37ba 100644 > >>>> --- a/drivers/usb/dwc3/gadget.c > >>>> +++ b/drivers/usb/dwc3/gadget.c > >>>> @@ -2246,6 +2246,9 @@ static int > dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, > >>>> if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO)) > >>>> trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > >>>> > >>>> + count = trb->size & DWC3_TRB_SIZE_MASK; > >>>> + req->remaining += count; > >>>> + > >>>> /* > >>>> * If we're dealing with unaligned size OUT transfer, we will be > left > >>>> * with one TRB pending in the ring. We need to manually clear HWO > bit > >>>> @@ -2256,9 +2259,6 @@ static int > dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, > >>>> return 1; > >>>> } > >>>> > >>>> - count = trb->size & DWC3_TRB_SIZE_MASK; > >>>> - req->remaining += count; > >>>> - > >>> this shouldn't be necessary, actually. The remaining TRB which was > added > >>> to fix alignment issues, is simply because DWC3 doesn't like OUT > >>> transfers whose size are not aligned to wMaxPacketSize. We *know* we > >>> have e.g. 511 bytes to receive from host, but dwc3 wants me to write > >>> 1024 bytes on the TRB (for superspeed), so I need to add this chained > >>> TRB for the remaining 513 bytes. > >>> > >>> I don't expect host to send us more than 511 bytes, if it does, the > host > >>> is faulty and I don't want to operate on those bytes anyway. > >>> > >> Right. But that's not the issue here. I realized that I did not describe > >> the issue fully... > >> > >> The issue here is the reporting of the actual number of bytes > >> transferred. For isoc OUT transfers, the last TRB of the Buffer > >> Descriptor will be retired with HWO = 0 (dwc_usb3 and dwc_usb31 > >> programming guide 4.3.7). So, the check for unaligned && HWO will be > >> false and will not return early. As a result, req->remaining will still > >> be updated with the BUFSIZ count, and the remaining will be 513+ in your > >> example. The 'actual' bytes written calculation will be wrong (e.g 511 - > >> 513 = -2). > >> > >> We can't rely on the current check to determine whether it's the last > >> TRB. My current solution is to update req->remaining for all TRB > >> completions. However, we have to also update the 'actual' bytes > >> calculation for OUT transfers. > > I think the bug here is another one. Look at the TRB types: > > > > irq/16-dwc3-2463 [003] d... 3589.933478: dwc3_complete_trb: > ep1out: trb 000000007d3fbc0d buf 00000000b8ed3800 size 0 ctrl 2e46446c > (hlCS:Sc:isoc-first) > irq/16-dwc3-2463 [003] d... 3589.933492: dwc3_complete_trb: > ep1out: trb 00000000d1fcb0c4 buf 000000003787e000 size 1023 ctrl 2e464c68 > (hlcS:SC:isoc-first) > > > Shouldn't the one be of type ISOC instead of ISOC_FIRST? Can you test > > patch below and see if things behave better? Please also capture > > tracepoints :) > > > > modified drivers/usb/dwc3/gadget.c > > @@ -1073,7 +1073,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep > *dep, > > /* Now prepare one extra TRB to align transfer > size */ > > trb = &dep->trb_pool[dep->trb_enqueue]; > > __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, > > - maxp - rem, false, 0, > > + maxp - rem, false, 1, > > req->request.stream_id, > > req->request.short_not_ok, > > req->request.no_interrupt); > > @@ -1117,7 +1117,7 @@ static void dwc3_prepare_one_trb_linear(struct > dwc3_ep *dep, > > /* Now prepare one extra TRB to align transfer size */ > > trb = &dep->trb_pool[dep->trb_enqueue]; > > __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - > rem, > > - false, 0, req->request.stream_id, > > + false, 1, req->request.stream_id, > > req->request.short_not_ok, > > req->request.no_interrupt); > > } else if (req->request.zero && req->request.length && > > > > > > This is an issue, but it's not the same one. > > irq/16-dwc3-5032 [003] d... 65.404194: dwc3_complete_trb: > ep1out: trb 00000000890522d5 buf 00000000b8d6d000 size 0 ctrl 3b56446c > (hlCS:Sc:isoc-first) > So this is chained to the next one, that's correct. > irq/16-dwc3-5032 [003] d... 65.404209: dwc3_complete_trb: > ep1out: trb 00000000c15f388f buf 0000000037821000 size 1023 ctrl > 3b564c78 (hlcS:SC:isoc) > But here, HWO should've been left as is, because of the short packet. That's what the databook says on the two notes on section 8.2.3 after table 8-8 of Databook 2.60a: 1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a short packet, the chained TRBs that follow it are not written back (e.g. the BUFSIZ and HWO fields remain the same as the software-prepared value) 2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is also set), and a short packet is received, the core retires the TRB in progress and skip past the TRB where CHN=0, accumulating the ISP and IOC bits from each TRB. If ISP or IOC is set in any TRB, the core generates an XferInProgress event. Hardware does not set the HWO bit to 0 in skipped TRBs. If the endpoint type is isochronous, the CHN=0 TRB will also be retired and its buffer size field updated with the total number of bytes remaining in the BD. Note that at most we have confirmation that SIZE will be updated in case of Isoc endpoints, but there's nothing there about HWO, so I expected it to remain set according to the rest of the text. There's one possibility that "TRB will also be *retired*" means that it's not skipped, which would mean that HWO is, indeed, set to 0. To patch that, however, we don't need all the extra checking you have implemented. I'll try to propose a slightly simpler fix if you're willing to test it out. > Are you going to make a patch for this one? > Yes, I'll prepare an official patch. cheers <div dir="ltr">Hi,<br><br>First of all, not receiving your mails through the mailing list. Can you check that you're not<div>being blocked for some reason? You may wanna send trace data compressed with gzip or xz.<div><br><div class="gmail_quote"></div></div><div dir="ltr"><div><div class="gmail_quote"><div dir="ltr">On Wed, Aug 1, 2018 at 2:27 AM Thinh Nguyen <<a href="mailto:Thinh.Nguyen@synopsys.com" target="_blank">Thinh.Nguyen@synopsys.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 7/30/2018 11:58 PM, Felipe Balbi wrote:<br> > Thinh Nguyen <<a href="mailto:Thinh.Nguyen@synopsys.com" target="_blank">Thinh.Nguyen@synopsys.com</a>> writes:<br> >> Hi Felipe,<br> >><br> >> On 7/29/2018 11:08 PM, Felipe Balbi wrote:<br> >>> Thinh Nguyen <<a href="mailto:Thinh.Nguyen@synopsys.com" target="_blank">Thinh.Nguyen@synopsys.com</a>> writes:<br> >>>> DWC3 must check for the BUFSIZ and update the req->remaining<br> >>>> regardless of transfer alignment. Returning early from transfer OUT<br> >>>> unalignment will skip updating the req->remaining.<br> >>>><br> >>>> Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")<br> >>>> Signed-off-by: Thinh Nguyen <<a href="mailto:thinhn@synopsys.com" target="_blank">thinhn@synopsys.com</a>><br> >>>> ---<br> >>>> drivers/usb/dwc3/gadget.c | 6 +++---<br> >>>> 1 file changed, 3 insertions(+), 3 deletions(-)<br> >>>><br> >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c<br> >>>> index 032ea7d709ba..a5b8387a37ba 100644<br> >>>> --- a/drivers/usb/dwc3/gadget.c<br> >>>> +++ b/drivers/usb/dwc3/gadget.c<br> >>>> @@ -2246,6 +2246,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,<br> >>>> if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))<br> >>>> trb->ctrl &= ~DWC3_TRB_CTRL_HWO;<br> >>>> <br> >>>> + count = trb->size & DWC3_TRB_SIZE_MASK;<br> >>>> + req->remaining += count;<br> >>>> +<br> >>>> /*<br> >>>> * If we're dealing with unaligned size OUT transfer, we will be left<br> >>>> * with one TRB pending in the ring. We need to manually clear HWO bit<br> >>>> @@ -2256,9 +2259,6 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,<br> >>>> return 1;<br> >>>> }<br> >>>> <br> >>>> - count = trb->size & DWC3_TRB_SIZE_MASK;<br> >>>> - req->remaining += count;<br> >>>> -<br> >>> this shouldn't be necessary, actually. The remaining TRB which was added<br> >>> to fix alignment issues, is simply because DWC3 doesn't like OUT<br> >>> transfers whose size are not aligned to wMaxPacketSize. We *know* we<br> >>> have e.g. 511 bytes to receive from host, but dwc3 wants me to write<br> >>> 1024 bytes on the TRB (for superspeed), so I need to add this chained<br> >>> TRB for the remaining 513 bytes.<br> >>><br> >>> I don't expect host to send us more than 511 bytes, if it does, the host<br> >>> is faulty and I don't want to operate on those bytes anyway.<br> >>><br> >> Right. But that's not the issue here. I realized that I did not describe<br> >> the issue fully...<br> >><br> >> The issue here is the reporting of the actual number of bytes<br> >> transferred. For isoc OUT transfers, the last TRB of the Buffer<br> >> Descriptor will be retired with HWO = 0 (dwc_usb3 and dwc_usb31<br> >> programming guide 4.3.7). So, the check for unaligned && HWO will be<br> >> false and will not return early. As a result, req->remaining will still<br> >> be updated with the BUFSIZ count, and the remaining will be 513+ in your<br> >> example. The 'actual' bytes written calculation will be wrong (e.g 511 -<br> >> 513 = -2).<br> >><br> >> We can't rely on the current check to determine whether it's the last<br> >> TRB. My current solution is to update req->remaining for all TRB<br> >> completions. However, we have to also update the 'actual' bytes<br> >> calculation for OUT transfers.<br> > I think the bug here is another one. Look at the TRB types:<br> ><br> > irq/16-dwc3-2463 [003] d... 3589.933478: dwc3_complete_trb: ep1out: trb 000000007d3fbc0d buf 00000000b8ed3800 size 0 ctrl 2e46446c (hlCS:Sc:isoc-first) </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > irq/16-dwc3-2463 [003] d... 3589.933492: dwc3_complete_trb: ep1out: trb 00000000d1fcb0c4 buf 000000003787e000 size 1023 ctrl 2e464c68 (hlcS:SC:isoc-first)</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> ><br> > Shouldn't the one be of type ISOC instead of ISOC_FIRST? Can you test<br> > patch below and see if things behave better? Please also capture<br> > tracepoints :)<br> ><br> > modified drivers/usb/dwc3/gadget.c<br> > @@ -1073,7 +1073,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,<br> > /* Now prepare one extra TRB to align transfer size */<br> > trb = &dep->trb_pool[dep->trb_enqueue];<br> > __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,<br> > - maxp - rem, false, 0,<br> > + maxp - rem, false, 1,<br> > req->request.stream_id,<br> > req->request.short_not_ok,<br> > req->request.no_interrupt);<br> > @@ -1117,7 +1117,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,<br> > /* Now prepare one extra TRB to align transfer size */<br> > trb = &dep->trb_pool[dep->trb_enqueue];<br> > __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - rem,<br> > - false, 0, req->request.stream_id,<br> > + false, 1, req->request.stream_id,<br> > req->request.short_not_ok,<br> > req->request.no_interrupt);<br> > } else if (req->request.zero && req->request.length &&<br> ><br> ><br> <br> This is an issue, but it's not the same one.<br> <br> irq/16-dwc3-5032 [003] d... 65.404194: dwc3_complete_trb:<br> ep1out: trb 00000000890522d5 buf 00000000b8d6d000 size 0 ctrl 3b56446c<br> (hlCS:Sc:isoc-first)<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div><div class="gmail_quote"><div>So this is chained to the next one, that's correct.<br></div></div></div></div><div dir="ltr"><div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> irq/16-dwc3-5032 [003] d... 65.404209: dwc3_complete_trb:<br> ep1out: trb 00000000c15f388f buf 0000000037821000 size 1023 ctrl<br> 3b564c78 (hlcS:SC:isoc)<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div><div class="gmail_quote"><div><div>But here, HWO should've been left as is, because of the short packet. That's what the</div><div>databook says on the two notes on section 8.2.3 after table 8-8 of Databook 2.60a:</div><div><br></div><div><div>1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a short packet,</div><div>the chained TRBs that follow it are not written back (e.g. the BUFSIZ and HWO fields</div><div>remain the same as the software-prepared value)</div><div><br></div><div>2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is also set), and a short</div><div>packet is received, the core retires the TRB in progress and skip past the TRB where</div><div>CHN=0, accumulating the ISP and IOC bits from each TRB. If ISP or IOC is set in any TRB,</div><div>the core generates an XferInProgress event. Hardware does not set the HWO bit to 0 in</div><div>skipped TRBs. If the endpoint type is isochronous, the CHN=0 TRB will also be retired and</div><div>its buffer size field updated with the total number of bytes remaining in the BD.</div></div><div><br></div><div>Note that at most we have confirmation that SIZE will be updated in case of Isoc endpoints,</div><div>but there's nothing there about HWO, so I expected it to remain set according to the rest of</div><div>the text.</div></div><div><br></div><div>There's one possibility that "TRB will also be *retired*" means that it's not skipped, which</div><div>would mean that HWO is, indeed, set to 0. To patch that, however, we don't need all the</div><div>extra checking you have implemented. I'll try to propose a slightly simpler fix if you're</div><div>willing to test it out.</div></div></div></div><div dir="ltr"><div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Are you going to make a patch for this one?<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div><div class="gmail_quote"><div>Yes, I'll prepare an official patch.</div><div> </div><div>cheers</div></div></div></div></div></div>
Hi, Felipe Balbi <balbi@kernel.org> writes: <snip> >> This is an issue, but it's not the same one. >> >> irq/16-dwc3-5032 [003] d... 65.404194: dwc3_complete_trb: >> ep1out: trb 00000000890522d5 buf 00000000b8d6d000 size 0 ctrl 3b56446c >> (hlCS:Sc:isoc-first) >> > > So this is chained to the next one, that's correct. > > >> irq/16-dwc3-5032 [003] d... 65.404209: dwc3_complete_trb: >> ep1out: trb 00000000c15f388f buf 0000000037821000 size 1023 ctrl >> 3b564c78 (hlcS:SC:isoc) >> > > But here, HWO should've been left as is, because of the short packet. > That's what the databook says on the two notes on section 8.2.3 after > table 8-8 of Databook 2.60a: > > 1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a > short packet, the chained TRBs that follow it are not written back > (e.g. the BUFSIZ and HWO fields remain the same as the > software-prepared value) > > 2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is > also set), and a short packet is received, the core retires the TRB in > progress and skip past the TRB where CHN=0, accumulating the ISP and > IOC bits from each TRB. If ISP or IOC is set in any TRB, the core > generates an XferInProgress event. Hardware does not set the HWO bit > to 0 in skipped TRBs. If the endpoint type is isochronous, the CHN=0 > TRB will also be retired and its buffer size field updated with the > total number of bytes remaining in the BD. > > Note that at most we have confirmation that SIZE will be updated in > case of Isoc endpoints, but there's nothing there about HWO, so I > expected it to remain set according to the rest of the text. > > There's one possibility that "TRB will also be *retired*" means that > it's not skipped, which would mean that HWO is, indeed, set to 0. To > patch that, however, we don't need all the extra checking you have > implemented. I'll try to propose a slightly simpler fix if you're > willing to test it out. Something like below: diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 9ba614032d5d..a8c7271a83b5 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2227,6 +2227,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, struct dwc3_request *req, struct dwc3_trb *trb, const struct dwc3_event_depevt *event, int status, int chain) { + const struct usb_endpoint_descriptor *desc = dep->endpoint.desc; unsigned int count; dwc3_ep_inc_deq(dep); @@ -2257,7 +2258,17 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, } count = trb->size & DWC3_TRB_SIZE_MASK; - req->remaining += count; + + /* + * In case of ISOC endpoints and Short or Zero packets, the + * last TRB will be retired and its size field will be updated + * to contain the full remaining size; meaning req->remaining + * will be count from the last TRB in the chain. + */ + if ((req->zero || req->unaligned) && usb_endpoint_xfer_isoc(desc)) + req->remaining = count; + else + req->remaining += count; if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN) return 1; If this minimal fix works, then as a follow-up I'll make a cleanup patch to make this part of the code slightly better. We're starting to get way too many conditionals here and they can be cleaned up a little. I just wanna know if the minimal fix works for you and if it does, we can mark it for stable and make sure the backport goes as far back as possible. cheers
Felipe Balbi <balbi@kernel.org> writes: > Hi, > > Felipe Balbi <balbi@kernel.org> writes: > > <snip> > >>> This is an issue, but it's not the same one. >>> >>> irq/16-dwc3-5032 [003] d... 65.404194: dwc3_complete_trb: >>> ep1out: trb 00000000890522d5 buf 00000000b8d6d000 size 0 ctrl 3b56446c >>> (hlCS:Sc:isoc-first) >>> >> >> So this is chained to the next one, that's correct. >> >> >>> irq/16-dwc3-5032 [003] d... 65.404209: dwc3_complete_trb: >>> ep1out: trb 00000000c15f388f buf 0000000037821000 size 1023 ctrl >>> 3b564c78 (hlcS:SC:isoc) >>> >> >> But here, HWO should've been left as is, because of the short packet. >> That's what the databook says on the two notes on section 8.2.3 after >> table 8-8 of Databook 2.60a: >> >> 1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a >> short packet, the chained TRBs that follow it are not written back >> (e.g. the BUFSIZ and HWO fields remain the same as the >> software-prepared value) >> >> 2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is >> also set), and a short packet is received, the core retires the TRB in >> progress and skip past the TRB where CHN=0, accumulating the ISP and >> IOC bits from each TRB. If ISP or IOC is set in any TRB, the core >> generates an XferInProgress event. Hardware does not set the HWO bit >> to 0 in skipped TRBs. If the endpoint type is isochronous, the CHN=0 >> TRB will also be retired and its buffer size field updated with the >> total number of bytes remaining in the BD. >> >> Note that at most we have confirmation that SIZE will be updated in >> case of Isoc endpoints, but there's nothing there about HWO, so I >> expected it to remain set according to the rest of the text. >> >> There's one possibility that "TRB will also be *retired*" means that >> it's not skipped, which would mean that HWO is, indeed, set to 0. To >> patch that, however, we don't need all the extra checking you have >> implemented. I'll try to propose a slightly simpler fix if you're >> willing to test it out. > > Something like below: and here's another possibility. This makes it clearer that we want to skip all TRBs with CHN bit set: diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 9ba614032d5d..56e2a2ebae66 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2227,6 +2227,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, struct dwc3_request *req, struct dwc3_trb *trb, const struct dwc3_event_depevt *event, int status, int chain) { + const struct usb_endpoint_descriptor *desc = dep->endpoint.desc; unsigned int count; dwc3_ep_inc_deq(dep); @@ -2256,6 +2257,16 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, return 1; } + /* + * In case of ISOC endpoints and Short or Zero packets, the + * last TRB will be retired and its size field will be updated + * to contain the full remaining size; meaning req->remaining + * will be count from the last TRB in the chain. + */ + if ((req->zero || req->unaligned) && usb_endpoint_xfer_isoc(desc) + && chain) + return 0; + count = trb->size & DWC3_TRB_SIZE_MASK; req->remaining += count;
Hi Felipe, On 8/1/2018 1:33 AM, Felipe Balbi wrote: > Felipe Balbi <balbi@kernel.org> writes: > >> Hi, >> >> Felipe Balbi <balbi@kernel.org> writes: >> >> <snip> >> >>>> This is an issue, but it's not the same one. >>>> >>>> irq/16-dwc3-5032 [003] d... 65.404194: dwc3_complete_trb: >>>> ep1out: trb 00000000890522d5 buf 00000000b8d6d000 size 0 ctrl 3b56446c >>>> (hlCS:Sc:isoc-first) >>>> >>> So this is chained to the next one, that's correct. >>> >>> >>>> irq/16-dwc3-5032 [003] d... 65.404209: dwc3_complete_trb: >>>> ep1out: trb 00000000c15f388f buf 0000000037821000 size 1023 ctrl >>>> 3b564c78 (hlcS:SC:isoc) >>>> >>> But here, HWO should've been left as is, because of the short packet. >>> That's what the databook says on the two notes on section 8.2.3 after >>> table 8-8 of Databook 2.60a: My proposal doesn't change how we handle the TRB's HWO currently. >>> >>> 1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a >>> short packet, the chained TRBs that follow it are not written back >>> (e.g. the BUFSIZ and HWO fields remain the same as the >>> software-prepared value) >>> >>> 2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is >>> also set), and a short packet is received, the core retires the TRB in >>> progress and skip past the TRB where CHN=0, accumulating the ISP and >>> IOC bits from each TRB. If ISP or IOC is set in any TRB, the core >>> generates an XferInProgress event. Hardware does not set the HWO bit >>> to 0 in skipped TRBs. If the endpoint type is isochronous, the CHN=0 >>> TRB will also be retired and its buffer size field updated with the >>> total number of bytes remaining in the BD. >>> >>> Note that at most we have confirmation that SIZE will be updated in >>> case of Isoc endpoints, but there's nothing there about HWO, so I >>> expected it to remain set according to the rest of the text. >>> >>> There's one possibility that "TRB will also be *retired*" means that >>> it's not skipped, which would mean that HWO is, indeed, set to 0. To Right. And the programming guide also says that for isoc OUT transfer, - Any non-first TRBs with CHN=1 that had not already been retired will not be written back. HWO will still be 1, and software can reclaim them for another transfer. - The last TRB of the Buffer Descriptor will be retired with: * HWO = 0. * BUFSIZ = The total remaining buffer size of the Buffer Descriptor. So we know that the last isoc TRB will have CHN=0 and HWO=0. >>> patch that, however, we don't need all the extra checking you have >>> implemented. I'll try to propose a slightly simpler fix if you're >>> willing to test it out. >> Something like below: > and here's another possibility. This makes it clearer that we want to > skip all TRBs with CHN bit set: > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 9ba614032d5d..56e2a2ebae66 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2227,6 +2227,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, > struct dwc3_request *req, struct dwc3_trb *trb, > const struct dwc3_event_depevt *event, int status, int chain) > { > + const struct usb_endpoint_descriptor *desc = dep->endpoint.desc; > unsigned int count; > > dwc3_ep_inc_deq(dep); > @@ -2256,6 +2257,16 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, > return 1; > } > > + /* > + * In case of ISOC endpoints and Short or Zero packets, the > + * last TRB will be retired and its size field will be updated > + * to contain the full remaining size; meaning req->remaining > + * will be count from the last TRB in the chain. > + */ > + if ((req->zero || req->unaligned) && usb_endpoint_xfer_isoc(desc) > + && chain) > + return 0; > + > count = trb->size & DWC3_TRB_SIZE_MASK; > req->remaining += count; > These patches will not fix the issue. Let's take a look at the problem again. static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, struct dwc3_request *req, struct dwc3_trb *trb, const struct dwc3_event_depevt *event, int status, int chain) { unsigned int count; dwc3_ep_inc_deq(dep); trace_dwc3_complete_trb(dep, trb); /* * If we're in the middle of series of chained TRBs and we * receive a short transfer along the way, DWC3 will skip * through all TRBs including the last TRB in the chain (the * where CHN bit is zero. DWC3 will also avoid clearing HWO * bit and SW has to do it manually. * * We're going to do that here to avoid problems of HW trying * to use bogus TRBs for transfers. */ if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO)) trb->ctrl &= ~DWC3_TRB_CTRL_HWO; /* * If we're dealing with unaligned size OUT transfer, we will be left * with one TRB pending in the ring. We need to manually clear HWO bit * from that TRB. */ if ((req->zero || req->unaligned) && (trb->ctrl & DWC3_TRB_CTRL_HWO)) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>>>>>>> This tries to check for the last TRB and return early. This works in normal cases and it stops incrementing the req->remaining. But for isoc OUT transfers, this case won't hit due to reason mention previously. <<<<<<<<<< trb->ctrl &= ~DWC3_TRB_CTRL_HWO; return 1; } count = trb->size & DWC3_TRB_SIZE_MASK; req->remaining += count; ^^^^^^^^^^^^^^^^^^^^^^^ >>>>>>>>>> So, your proposals will still update the req->remaining with the BUFSIZ count of the last TRB. <<<<<<<<<< if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN) return 1; if (event->status & DEPEVT_STATUS_SHORT && !chain) return 1; if (event->status & DEPEVT_STATUS_IOC) return 1; return 0; } static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, const struct dwc3_event_depevt *event, struct dwc3_request *req, int status) { .... req->request.actual = req->request.length - req->remaining; ^^^^^^^^^^^^^^ >>>>>>>>>> And the calculation is off because req->remaining includes the remaining of the last TRB BUFSIZ count. <<<<<<<<<< .... } I included the traces. Hopefully it will go out to the mailing list. Thanks for making the patches, and of course I will test them. :) Thinh
On 8/1/2018 6:26 PM, Thinh Nguyen wrote: > Hi Felipe, > > > On 8/1/2018 1:33 AM, Felipe Balbi wrote: >> Felipe Balbi <balbi@kernel.org> writes: >> >>> Hi, >>> >>> Felipe Balbi <balbi@kernel.org> writes: >>> >>> <snip> >>> >>>>> This is an issue, but it's not the same one. >>>>> >>>>> irq/16-dwc3-5032 [003] d... 65.404194: dwc3_complete_trb: >>>>> ep1out: trb 00000000890522d5 buf 00000000b8d6d000 size 0 ctrl 3b56446c >>>>> (hlCS:Sc:isoc-first) >>>>> >>>> So this is chained to the next one, that's correct. >>>> >>>> >>>>> irq/16-dwc3-5032 [003] d... 65.404209: dwc3_complete_trb: >>>>> ep1out: trb 00000000c15f388f buf 0000000037821000 size 1023 ctrl >>>>> 3b564c78 (hlcS:SC:isoc) >>>>> >>>> But here, HWO should've been left as is, because of the short packet. >>>> That's what the databook says on the two notes on section 8.2.3 after >>>> table 8-8 of Databook 2.60a: > My proposal doesn't change how we handle the TRB's HWO currently. > >>>> 1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a >>>> short packet, the chained TRBs that follow it are not written back >>>> (e.g. the BUFSIZ and HWO fields remain the same as the >>>> software-prepared value) >>>> >>>> 2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is >>>> also set), and a short packet is received, the core retires the TRB in >>>> progress and skip past the TRB where CHN=0, accumulating the ISP and >>>> IOC bits from each TRB. If ISP or IOC is set in any TRB, the core >>>> generates an XferInProgress event. Hardware does not set the HWO bit >>>> to 0 in skipped TRBs. If the endpoint type is isochronous, the CHN=0 >>>> TRB will also be retired and its buffer size field updated with the >>>> total number of bytes remaining in the BD. >>>> >>>> Note that at most we have confirmation that SIZE will be updated in >>>> case of Isoc endpoints, but there's nothing there about HWO, so I >>>> expected it to remain set according to the rest of the text. >>>> >>>> There's one possibility that "TRB will also be *retired*" means that >>>> it's not skipped, which would mean that HWO is, indeed, set to 0. To > Right. And the programming guide also says that for isoc OUT transfer, > - Any non-first TRBs with CHN=1 that had not already been retired will > not be written back. HWO will still be 1, and software can reclaim them > for another transfer. > - The last TRB of the Buffer Descriptor will be retired with: > * HWO = 0. > * BUFSIZ = The total remaining buffer size of the Buffer Descriptor. > > So we know that the last isoc TRB will have CHN=0 and HWO=0. > > >>>> patch that, however, we don't need all the extra checking you have >>>> implemented. I'll try to propose a slightly simpler fix if you're >>>> willing to test it out. >>> Something like below: >> and here's another possibility. This makes it clearer that we want to >> skip all TRBs with CHN bit set: >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 9ba614032d5d..56e2a2ebae66 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2227,6 +2227,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, >> struct dwc3_request *req, struct dwc3_trb *trb, >> const struct dwc3_event_depevt *event, int status, int chain) >> { >> + const struct usb_endpoint_descriptor *desc = dep->endpoint.desc; >> unsigned int count; >> >> dwc3_ep_inc_deq(dep); >> @@ -2256,6 +2257,16 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, >> return 1; >> } >> >> + /* >> + * In case of ISOC endpoints and Short or Zero packets, the >> + * last TRB will be retired and its size field will be updated >> + * to contain the full remaining size; meaning req->remaining >> + * will be count from the last TRB in the chain. >> + */ >> + if ((req->zero || req->unaligned) && usb_endpoint_xfer_isoc(desc) >> + && chain) >> + return 0; >> + >> count = trb->size & DWC3_TRB_SIZE_MASK; >> req->remaining += count; >> > These patches will not fix the issue. > What do you think of this fix? diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index f452ab708ffc..338f7ab8a8b4 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2277,8 +2277,10 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, * with one TRB pending in the ring. We need to manually clear HWO bit * from that TRB. */ - if ((req->zero || req->unaligned) && (trb->ctrl & DWC3_TRB_CTRL_HWO)) { - trb->ctrl &= ~DWC3_TRB_CTRL_HWO; + if ((req->zero || req->unaligned) && !chain) { + if (trb->ctrl & DWC3_TRB_CTRL_HWO) + trb->ctrl &= ~DWC3_TRB_CTRL_HWO; + return 1; } Thinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>> >>>> 1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a >>>> short packet, the chained TRBs that follow it are not written back >>>> (e.g. the BUFSIZ and HWO fields remain the same as the >>>> software-prepared value) >>>> >>>> 2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is >>>> also set), and a short packet is received, the core retires the TRB in >>>> progress and skip past the TRB where CHN=0, accumulating the ISP and >>>> IOC bits from each TRB. If ISP or IOC is set in any TRB, the core >>>> generates an XferInProgress event. Hardware does not set the HWO bit >>>> to 0 in skipped TRBs. If the endpoint type is isochronous, the CHN=0 >>>> TRB will also be retired and its buffer size field updated with the >>>> total number of bytes remaining in the BD. >>>> >>>> Note that at most we have confirmation that SIZE will be updated in >>>> case of Isoc endpoints, but there's nothing there about HWO, so I >>>> expected it to remain set according to the rest of the text. >>>> >>>> There's one possibility that "TRB will also be *retired*" means that >>>> it's not skipped, which would mean that HWO is, indeed, set to 0. To > > Right. And the programming guide also says that for isoc OUT transfer, > - Any non-first TRBs with CHN=1 that had not already been retired will > not be written back. HWO will still be 1, and software can reclaim them > for another transfer. > - The last TRB of the Buffer Descriptor will be retired with: > * HWO = 0. > * BUFSIZ = The total remaining buffer size of the Buffer Descriptor. > > So we know that the last isoc TRB will have CHN=0 and HWO=0. yeah, only now I understood the actual problem. req->length is 1, so reading BUFSIZ from last TRB will, actually, cause the problem. > static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, > struct dwc3_request *req, struct dwc3_trb *trb, > const struct dwc3_event_depevt *event, int status, int chain) > { > unsigned int count; > > dwc3_ep_inc_deq(dep); > > trace_dwc3_complete_trb(dep, trb); > > /* > * If we're in the middle of series of chained TRBs and we > * receive a short transfer along the way, DWC3 will skip > * through all TRBs including the last TRB in the chain (the > * where CHN bit is zero. DWC3 will also avoid clearing HWO > * bit and SW has to do it manually. > * > * We're going to do that here to avoid problems of HW trying > * to use bogus TRBs for transfers. > */ > if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO)) > trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > > /* > * If we're dealing with unaligned size OUT transfer, we will be left > * with one TRB pending in the ring. We need to manually clear HWO bit > * from that TRB. > */ > if ((req->zero || req->unaligned) && (trb->ctrl & DWC3_TRB_CTRL_HWO)) { > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>>>>>>>> > This tries to check for the last TRB and return early. This works in normal > cases and it stops incrementing the req->remaining. But for isoc OUT > transfers, > this case won't hit due to reason mention previously. > <<<<<<<<<< > > trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > return 1; > } > > count = trb->size & DWC3_TRB_SIZE_MASK; > req->remaining += count; > ^^^^^^^^^^^^^^^^^^^^^^^ >>>>>>>>>>> > So, your proposals will still update the req->remaining with the BUFSIZ > count of > the last TRB. > <<<<<<<<<< > > > if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN) > return 1; > > if (event->status & DEPEVT_STATUS_SHORT && !chain) > return 1; > > if (event->status & DEPEVT_STATUS_IOC) > return 1; > > return 0; > } > > > > static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, > const struct dwc3_event_depevt *event, > struct dwc3_request *req, int status) > { > .... > req->request.actual = req->request.length - req->remaining; > ^^^^^^^^^^^^^^ >>>>>>>>>>> > And the calculation is off because req->remaining includes the remaining > of the > last TRB BUFSIZ count. > <<<<<<<<<< > .... > } > > I included the traces. Hopefully it will go out to the mailing list. > Thanks for making the patches, and of course I will test them. :) I'll read your traces shortly and reply again.
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: <snip> >> These patches will not fix the issue. >> > > What do you think of this fix? > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index f452ab708ffc..338f7ab8a8b4 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2277,8 +2277,10 @@ static int > dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, > * with one TRB pending in the ring. We need to manually clear > HWO bit > * from that TRB. > */ > - if ((req->zero || req->unaligned) && (trb->ctrl & DWC3_TRB_CTRL_HWO)) { > - trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > + if ((req->zero || req->unaligned) && !chain) { > + if (trb->ctrl & DWC3_TRB_CTRL_HWO) > + trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > + > return 1; > } This is a rathher minimal fix. I like it. So this together with the one I wrote for the TRB type, right? Can you send this one as a proper patch and add the correct Cc stable and Fixes tags? thanks
On 8/2/2018 12:46 AM, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > > <snip> > >>> These patches will not fix the issue. >>> >> What do you think of this fix? >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index f452ab708ffc..338f7ab8a8b4 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2277,8 +2277,10 @@ static int >> dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, >> * with one TRB pending in the ring. We need to manually clear >> HWO bit >> * from that TRB. >> */ >> - if ((req->zero || req->unaligned) && (trb->ctrl & DWC3_TRB_CTRL_HWO)) { >> - trb->ctrl &= ~DWC3_TRB_CTRL_HWO; >> + if ((req->zero || req->unaligned) && !chain) { >> + if (trb->ctrl & DWC3_TRB_CTRL_HWO) >> + trb->ctrl &= ~DWC3_TRB_CTRL_HWO; >> + >> return 1; >> } > This is a rathher minimal fix. I like it. So this together with the one > I wrote for the TRB type, right? Can you send this one as a proper patch > and add the correct Cc stable and Fixes tags? > Yes. Can you create an official patch for that TRB type issue you found? Thanks, Thinh -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > On 8/2/2018 12:46 AM, Felipe Balbi wrote: >> Hi, >> >> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> >> <snip> >> >>>> These patches will not fix the issue. >>>> >>> What do you think of this fix? >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index f452ab708ffc..338f7ab8a8b4 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -2277,8 +2277,10 @@ static int >>> dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, >>> * with one TRB pending in the ring. We need to manually clear >>> HWO bit >>> * from that TRB. >>> */ >>> - if ((req->zero || req->unaligned) && (trb->ctrl & DWC3_TRB_CTRL_HWO)) { >>> - trb->ctrl &= ~DWC3_TRB_CTRL_HWO; >>> + if ((req->zero || req->unaligned) && !chain) { >>> + if (trb->ctrl & DWC3_TRB_CTRL_HWO) >>> + trb->ctrl &= ~DWC3_TRB_CTRL_HWO; >>> + >>> return 1; >>> } >> This is a rathher minimal fix. I like it. So this together with the one >> I wrote for the TRB type, right? Can you send this one as a proper patch >> and add the correct Cc stable and Fixes tags? >> > Yes. Can you create an official patch for that TRB type issue you found? Just sent it out, you're on Cc
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 032ea7d709ba..a5b8387a37ba 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2246,6 +2246,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO)) trb->ctrl &= ~DWC3_TRB_CTRL_HWO; + count = trb->size & DWC3_TRB_SIZE_MASK; + req->remaining += count; + /* * If we're dealing with unaligned size OUT transfer, we will be left * with one TRB pending in the ring. We need to manually clear HWO bit @@ -2256,9 +2259,6 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, return 1; } - count = trb->size & DWC3_TRB_SIZE_MASK; - req->remaining += count; - if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN) return 1;
DWC3 must check for the BUFSIZ and update the req->remaining regardless of transfer alignment. Returning early from transfer OUT unalignment will skip updating the req->remaining. Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize") Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> --- drivers/usb/dwc3/gadget.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)