Hi Felipe,

Do you have any comments on this?

Thanks,
Jack

On Tue, Aug 01, 2017 at 02:00:56AM -0700, Jack Pham wrote:
> In the SG case this is already handled since a non-zero
> request->num_mapped_sgs is a clear indicator that dma_map_sg()
> had been called. While it would be nice to do the same for the
> singly mapped case by simply checking for non-zero request->dma,
> it's conceivable that 0 is a valid dma_addr_t handle. Hence add
> a flag 'dma_mapped' to struct usb_request and use this to
> determine the need to call dma_unmap_single(). Otherwise, if a
> request is not DMA mapped then the result of calling
> usb_request_unmap_request() would safely be a no-op.
> 
> Signed-off-by: Jack Pham <ja...@codeaurora.org>
> ---
> Hi Felipe,
> 
> Here's what I came up with after our discussion back in [1]. It
> turned out to be pretty dead-simple and hopefully doesn't need to
> approach the number of URB flags that the host core uses.
> 
> I did a quick survey of all callers of usb_gadget_{map,unmap}_request
> and besides the instance I reported & patched in dwc3, from what I
> can tell it looks like all the other gadget drivers seem to be calling
> these APIs sanely--i.e. a request is only queued after being
> successfully mapped, and unmap only ever gets called on requests that
> had been queued.
> 
> Although in one instance (renesas_usb3.c), there is a 'used' flag as
> part of struct renesas_usb3_dma that looks like it's tracking the
> status of whether a request is mapped or not, but it's not obvious from
> cursory glance whether it has a use besides that or if it would be
> sufficient to just rely on this new flag in struct usb_gadget.
> Maybe Yoshihiro-san can comment.
> 
> [1] http://marc.info/?l=linux-usb&m=149872369422476&w=2
> 
> Thanks,
> Jack
> 
>  drivers/usb/gadget/udc/core.c | 5 ++++-
>  include/linux/usb/gadget.h    | 2 ++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index e6f04ee..c1cef6a 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -812,6 +812,8 @@ int usb_gadget_map_request_by_dev(struct device *dev,
>                       dev_err(dev, "failed to map buffer\n");
>                       return -EFAULT;
>               }
> +
> +             req->dma_mapped = 1;
>       }
>  
>       return 0;
> @@ -836,9 +838,10 @@ void usb_gadget_unmap_request_by_dev(struct device *dev,
>                               is_in ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
>  
>               req->num_mapped_sgs = 0;
> -     } else {
> +     } else if (req->dma_mapped) {
>               dma_unmap_single(dev, req->dma, req->length,
>                               is_in ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
> +             req->dma_mapped = 0;
>       }
>  }
>  EXPORT_SYMBOL_GPL(usb_gadget_unmap_request_by_dev);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 1a4a4ba..21468a7 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -48,6 +48,7 @@ struct usb_ep;
>   *     by adding a zero length packet as needed;
>   * @short_not_ok: When reading data, makes short packets be
>   *     treated as errors (queue stops advancing till cleanup).
> + * @dma_mapped: Indicates if request has been mapped to DMA (internal)
>   * @complete: Function called when request completes, so this request and
>   *   its buffer may be re-used.  The function will always be called with
>   *   interrupts disabled, and it must not sleep.
> @@ -103,6 +104,7 @@ struct usb_request {
>       unsigned                no_interrupt:1;
>       unsigned                zero:1;
>       unsigned                short_not_ok:1;
> +     unsigned                dma_mapped:1;
>  
>       void                    (*complete)(struct usb_ep *ep,
>                                       struct usb_request *req);

-- 
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to