Hi,
Jack Pham <jackp@xxxxxxxxxxxxxx> writes:
>> 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?
I would go for the flag myself :-)
--
balbi
Attachment:
signature.asc
Description: PGP signature
