Hi, Jack Pham <[email protected]> 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 <[email protected]> >> > --- >> > 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
signature.asc
Description: PGP signature
