[v4,1/4] usb: gadget: Support already-mapped DMA SGs

Message ID 20240117122646.41616-2-paul@crapouillou.net (mailing list archive)
State Superseded
Headers
Series usb: gadget: functionfs: DMABUF import interface |

Commit Message

Paul Cercueil Jan. 17, 2024, 12:26 p.m. UTC
  Add a new 'sg_was_mapped' field to the struct usb_request. This field
can be used to indicate that the scatterlist associated to the USB
transfer has already been mapped into the DMA space, and it does not
have to be done internally.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/usb/gadget/udc/core.c | 7 ++++++-
 include/linux/usb/gadget.h    | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)
  

Comments

Frank Li Jan. 19, 2024, 9:49 p.m. UTC | #1
On Wed, Jan 17, 2024 at 01:26:43PM +0100, Paul Cercueil wrote:
> Add a new 'sg_was_mapped' field to the struct usb_request. This field
> can be used to indicate that the scatterlist associated to the USB
> transfer has already been mapped into the DMA space, and it does not
> have to be done internally.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/usb/gadget/udc/core.c | 7 ++++++-
>  include/linux/usb/gadget.h    | 2 ++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index d59f94464b87..9d4150124fdb 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -903,6 +903,11 @@ int usb_gadget_map_request_by_dev(struct device *dev,
>  	if (req->length == 0)
>  		return 0;
>  
> +	if (req->sg_was_mapped) {
> +		req->num_mapped_sgs = req->num_sgs;
> +		return 0;
> +	}
> +
>  	if (req->num_sgs) {
>  		int     mapped;
>  
> @@ -948,7 +953,7 @@ EXPORT_SYMBOL_GPL(usb_gadget_map_request);
>  void usb_gadget_unmap_request_by_dev(struct device *dev,
>  		struct usb_request *req, int is_in)
>  {
> -	if (req->length == 0)
> +	if (req->length == 0 || req->sg_was_mapped)
>  		return;
>  
>  	if (req->num_mapped_sgs) {
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index a771ccc038ac..c529e4e06997 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -52,6 +52,7 @@ struct usb_ep;
>   * @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)
> + * @sg_was_mapped: Set if the scatterlist has been mapped before the request
>   * @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.
> @@ -111,6 +112,7 @@ struct usb_request {
>  	unsigned		zero:1;
>  	unsigned		short_not_ok:1;
>  	unsigned		dma_mapped:1;
> +	unsigned		sg_was_mapped:1;

why not use dma_mapped direclty?

Frank

>  
>  	void			(*complete)(struct usb_ep *ep,
>  					struct usb_request *req);
> -- 
> 2.43.0
>
  
Paul Cercueil Jan. 20, 2024, 12:14 a.m. UTC | #2
Hi Frank,

Le vendredi 19 janvier 2024 à 16:49 -0500, Frank Li a écrit :
> On Wed, Jan 17, 2024 at 01:26:43PM +0100, Paul Cercueil wrote:
> > Add a new 'sg_was_mapped' field to the struct usb_request. This
> > field
> > can be used to indicate that the scatterlist associated to the USB
> > transfer has already been mapped into the DMA space, and it does
> > not
> > have to be done internally.
> > 
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > ---
> >  drivers/usb/gadget/udc/core.c | 7 ++++++-
> >  include/linux/usb/gadget.h    | 2 ++
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/gadget/udc/core.c
> > b/drivers/usb/gadget/udc/core.c
> > index d59f94464b87..9d4150124fdb 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -903,6 +903,11 @@ int usb_gadget_map_request_by_dev(struct
> > device *dev,
> >  	if (req->length == 0)
> >  		return 0;
> >  
> > +	if (req->sg_was_mapped) {
> > +		req->num_mapped_sgs = req->num_sgs;
> > +		return 0;
> > +	}
> > +
> >  	if (req->num_sgs) {
> >  		int     mapped;
> >  
> > @@ -948,7 +953,7 @@ EXPORT_SYMBOL_GPL(usb_gadget_map_request);
> >  void usb_gadget_unmap_request_by_dev(struct device *dev,
> >  		struct usb_request *req, int is_in)
> >  {
> > -	if (req->length == 0)
> > +	if (req->length == 0 || req->sg_was_mapped)
> >  		return;
> >  
> >  	if (req->num_mapped_sgs) {
> > diff --git a/include/linux/usb/gadget.h
> > b/include/linux/usb/gadget.h
> > index a771ccc038ac..c529e4e06997 100644
> > --- a/include/linux/usb/gadget.h
> > +++ b/include/linux/usb/gadget.h
> > @@ -52,6 +52,7 @@ struct usb_ep;
> >   * @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)
> > + * @sg_was_mapped: Set if the scatterlist has been mapped before
> > the request
> >   * @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.
> > @@ -111,6 +112,7 @@ struct usb_request {
> >  	unsigned		zero:1;
> >  	unsigned		short_not_ok:1;
> >  	unsigned		dma_mapped:1;
> > +	unsigned		sg_was_mapped:1;
> 
> why not use dma_mapped direclty?

Because of the unmap case. We want to know whether we should unmap or
not.

> 
> Frank

Cheers,
-Paul

> 
> >  
> >  	void			(*complete)(struct usb_ep *ep,
> >  					struct usb_request *req);
> > -- 
> > 2.43.0
> >
  
Frank Li Jan. 20, 2024, 2:01 a.m. UTC | #3
On Sat, Jan 20, 2024 at 01:14:52AM +0100, Paul Cercueil wrote:
> Hi Frank,
> 
> Le vendredi 19 janvier 2024 à 16:49 -0500, Frank Li a écrit :
> > On Wed, Jan 17, 2024 at 01:26:43PM +0100, Paul Cercueil wrote:
> > > Add a new 'sg_was_mapped' field to the struct usb_request. This
> > > field
> > > can be used to indicate that the scatterlist associated to the USB
> > > transfer has already been mapped into the DMA space, and it does
> > > not
> > > have to be done internally.
> > > 
> > > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > ---
> > >  drivers/usb/gadget/udc/core.c | 7 ++++++-
> > >  include/linux/usb/gadget.h    | 2 ++
> > >  2 files changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/gadget/udc/core.c
> > > b/drivers/usb/gadget/udc/core.c
> > > index d59f94464b87..9d4150124fdb 100644
> > > --- a/drivers/usb/gadget/udc/core.c
> > > +++ b/drivers/usb/gadget/udc/core.c
> > > @@ -903,6 +903,11 @@ int usb_gadget_map_request_by_dev(struct
> > > device *dev,
> > >  	if (req->length == 0)
> > >  		return 0;
> > >  
> > > +	if (req->sg_was_mapped) {
> > > +		req->num_mapped_sgs = req->num_sgs;
> > > +		return 0;
> > > +	}
> > > +
> > >  	if (req->num_sgs) {
> > >  		int     mapped;
> > >  
> > > @@ -948,7 +953,7 @@ EXPORT_SYMBOL_GPL(usb_gadget_map_request);
> > >  void usb_gadget_unmap_request_by_dev(struct device *dev,
> > >  		struct usb_request *req, int is_in)
> > >  {
> > > -	if (req->length == 0)
> > > +	if (req->length == 0 || req->sg_was_mapped)
> > >  		return;
> > >  
> > >  	if (req->num_mapped_sgs) {
> > > diff --git a/include/linux/usb/gadget.h
> > > b/include/linux/usb/gadget.h
> > > index a771ccc038ac..c529e4e06997 100644
> > > --- a/include/linux/usb/gadget.h
> > > +++ b/include/linux/usb/gadget.h
> > > @@ -52,6 +52,7 @@ struct usb_ep;
> > >   * @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)
> > > + * @sg_was_mapped: Set if the scatterlist has been mapped before
> > > the request
> > >   * @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.
> > > @@ -111,6 +112,7 @@ struct usb_request {
> > >  	unsigned		zero:1;
> > >  	unsigned		short_not_ok:1;
> > >  	unsigned		dma_mapped:1;
> > > +	unsigned		sg_was_mapped:1;
> > 
> > why not use dma_mapped direclty?
> 
> Because of the unmap case. We want to know whether we should unmap or
> not.

I see, Thanks
Frank

> 
> > 
> > Frank
> 
> Cheers,
> -Paul
> 
> > 
> > >  
> > >  	void			(*complete)(struct usb_ep *ep,
> > >  					struct usb_request *req);
> > > -- 
> > > 2.43.0
> > > 
>
  

Patch

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index d59f94464b87..9d4150124fdb 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -903,6 +903,11 @@  int usb_gadget_map_request_by_dev(struct device *dev,
 	if (req->length == 0)
 		return 0;
 
+	if (req->sg_was_mapped) {
+		req->num_mapped_sgs = req->num_sgs;
+		return 0;
+	}
+
 	if (req->num_sgs) {
 		int     mapped;
 
@@ -948,7 +953,7 @@  EXPORT_SYMBOL_GPL(usb_gadget_map_request);
 void usb_gadget_unmap_request_by_dev(struct device *dev,
 		struct usb_request *req, int is_in)
 {
-	if (req->length == 0)
+	if (req->length == 0 || req->sg_was_mapped)
 		return;
 
 	if (req->num_mapped_sgs) {
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index a771ccc038ac..c529e4e06997 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -52,6 +52,7 @@  struct usb_ep;
  * @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)
+ * @sg_was_mapped: Set if the scatterlist has been mapped before the request
  * @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.
@@ -111,6 +112,7 @@  struct usb_request {
 	unsigned		zero:1;
 	unsigned		short_not_ok:1;
 	unsigned		dma_mapped:1;
+	unsigned		sg_was_mapped:1;
 
 	void			(*complete)(struct usb_ep *ep,
 					struct usb_request *req);