Hi Felipe,
On 9/7/2017 12:16 AM, Felipe Balbi wrote:
>>>> drivers/usb/dwc3/gadget.c | 8 +++++---
>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index 9e41605a..6b299c7 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -191,14 +191,16 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
>>>>
>>>> req->started = false;
>>>> list_del(&req->list);
>>>> - req->trb = NULL;
>>>> req->remaining = 0;
>>>>
>>>> if (req->request.status == -EINPROGRESS)
>>>> req->request.status = status;
>>>>
>>>> - usb_gadget_unmap_request_by_dev(dwc->sysdev,
>>>> - &req->request, req->direction);
>>>> + if (req->trb)
>>> This check does not account for control data transfer. TRBs for ep0 are
>>> not set to its req->trb. ep0out request needs to be unmapped, otherwise
>>> device will receive bogus data.
>>>
>>> Our internal test showed that the device failed to interpret control
>>> data from host. I bisected to this patch.
>
> what was the testing? How can I reproduce it?
This is a regression. The internal test found the issue when it does a
3-stage Control Write Transfer. You can reproduce this issue with just 1
out data packet of size > 0. Read and check the data on control request
completion.
>
>> Hi Thinh,
>>
>> Thanks for catching this. I can think of two ways to address this:
>>
>> 1. Make sure req->trb is populated for ep0/1 as well. This should be
>> easily done since the TRB corresponding to the mapped buffer is always
>> dwc->ep0_trb. We can assign the pointer after each of the map_request()
>> calls in __dwc3_ep0_do_control_data. And req->trb already gets zeroed
>> in dwc3_giveback() already. Hopefully this can be taken for 4.13.y
>> stable as well.
>>
>> 2. In 4.14-rc1 there is now commit 31fe084ffaaf ("usb: gadget: core:
>> unmap request from DMA only if previously mapped") which handles
>> $SUBJECT in a generic way so obviates the need for this patch, so
>> maybe this patch can simply be reverted. However this might not backport
>> so well for 4.13.y since reverting would bring us back to the behavior I
>> originally intended to fix.
>>
>> Felipe, what do you think?
>
> I think we need to make sure req->trb is set in control transfers,
> too. But let's see, I want to be able to reproduce it first.
>
Let me know if you need more info.
BR,
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
