On Thu, Sep 07, 2017 at 02:20:17AM +0000, Thinh Nguyen wrote:
> Hi,
>
> 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.
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?
Jack
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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
