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?
--
balbi
Attachment:
signature.asc
Description: PGP signature
