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@xxxxxxxxxx> writes:
>>
>>> Hi,
>>>
>>> Felipe Balbi <balbi@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
