Hi Felipe,
On Thu, Jun 29, 2017 at 08:48:17AM +0300, Felipe Balbi wrote:
> Hi Jack,
>
> Jack Pham <jackp@xxxxxxxxxxxxxx> writes:
> > On Wed, May 17, 2017 at 01:31:12PM +0300, Felipe Balbi wrote:
> >> Some functions might want to have very, very long request queues. We
> >> can't make any assumptions about how many requests we *are* able to
> >> map, so instead of mapping requests early, let's map them late. This
> >> way, functions can queue as many requests as they'd like but we won't
> >> take DMA resources until they are needed.
> >>
> >> Also, we can now stop processing requests when we run out of DMA
> >> resources but still keep requests in the queue for late processing.
> >>
> >> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
> >> ---
> >>
> >> Tested with g_mass_storage using 1024 requests. Note that we have
> >> space for 255 TRBs in our ring. TRB #256 is our link TRB back to
> >> the head of the ring.
> >>
> >> drivers/usb/dwc3/gadget.c | 21 ++++++++++++---------
> >> 1 file changed, 12 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> >> index 7113a9f53ca9..750364eb11e1 100644
> >> --- a/drivers/usb/dwc3/gadget.c
> >> +++ b/drivers/usb/dwc3/gadget.c
> >> @@ -1099,6 +1099,17 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
> >> }
> >>
> >> list_for_each_entry_safe(req, n, &dep->pending_list, list) {
> >> + struct dwc3 *dwc = dep->dwc;
> >> + int ret;
> >> +
> >> + ret = usb_gadget_map_request_by_dev(dwc->sysdev, &req->request,
> >> + dep->direction);
> >> + if (ret)
> >> + return;
> >> +
> >> + req->sg = req->request.sg;
> >> + req->num_pending_sgs = req->request.num_mapped_sgs;
> >> +
> >
> > Consider the case when the request is dequeued or ep_disable() is called
> > before its TRB was prepared (such as when there aren't any TRBs
> > available yet), and therefore dma_map_{single,sg}() hasn't been called
> > yet. Wouldn't then dwc3_gadget_giveback() end up unconditionally calling
> > dma_unmap_{single,sg}() with an invalid request->dma (or sg) handle?
> > Depending on the platform's DMA implementation the unmap operation could
> > result in a panic.
>
> Nice catch :-) completely missed that.
>
> > I'm wondering if we should try to fix this more generally, that is allow
> > usb_gadget_unmap_request_by_dev() to know not to call dma_unmap_*()
> > unless it was peviously mapped. The host core does this already by way
> > of flags such as URB_DMA_MAP_SINGLE and URB_DMA_MAP_SG which are applied
> > to urb->transfer_flags and used in the urb map/unmap routines in
> > hcd.c. Perhaps we can add something similar to udc/core.c?
>
> yeah, sounds like a plan to me. Several UDC drivers are already carrying
> their own flags for such things. We might as well fix it up
> generically. In any case, we still need a dwc3-specific patch for the
> -rc cycle. Then for v4.14 merge window we introduce a generic flag. How
> about that?
>
> Are you going to be working on this, btw?
Cool. I'll submit a patch for dwc3 shortly. And sure I can take a
stab at cooking something up for udc/core.c as well.
On that matter, I was tempted to take the easy path and come up with
something along the lines of simply checking request.dma != 0 before
calling dma_unmap_single() but not knowing DMA implementations very
well, I'm afraid someone would come back and say that 0 is a perfectly
valid dma_addr_t handle... so a flag is probably most correct then?
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
