Hi,
Jack Pham <jackp@xxxxxxxxxxxxxx> writes:
>> On 6/29/2017 12:54 AM, Jack Pham wrote:
>> > A recent optimization was made so that a request put on the
>> > pending_list wouldn't get mapped for DMA until just before
>> > preparing a TRB for it. However, this poses a problem in case
>> > the request is dequeued or the endpoint is disabled before the
>> > mapping is done as that would lead to dwc3_gadget_giveback()
>> > unconditionally calling usb_gadget_unmap_request_for_dev() with
>> > an invalid request->dma handle. Depending on the platform's DMA
>> > implementation the unmap operation could result in a panic.
>> >
>> > Since we know a successful mapping is a prerequisite for getting
>> > a TRB, the unmap can be conditionally called only when req->trb
>> > is non-NULL.
>> >
>> > Fixes: cdb55b39fab8 ("usb: dwc3: gadget: lazily map requests for DMA")
>> > Signed-off-by: Jack Pham <jackp@xxxxxxxxxxxxxx>
>> > ---
>> > 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?
> 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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
