diff mbox

[v2,1/9] usb: core: Add a helper function to check the validity of EP type in URB

Message ID 20171011103646.11879-2-tiwai@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Iwai Oct. 11, 2017, 10:36 a.m. UTC 
This patch adds a new helper function to perform a sanity check of the
given URB to see whether it contains a valid endpoint.  It's a light-
weight version of what usb_submit_urb() does, but without the kernel
warning followed by the stack trace, just returns an error code.

Especially for a driver that doesn't parse the descriptor but fills
the URB with the fixed endpoint (e.g. some quirks for non-compliant
devices), this kind of check is preferable at the probe phase before
actually submitting the urb.

Tested-by: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2:
* Fix function name typos
* Call usb_urb_ep_type_check() in usb_submit_urb(), too

 drivers/usb/core/urb.c | 30 ++++++++++++++++++++++++++----
 include/linux/usb.h    |  2 ++
 2 files changed, 28 insertions(+), 4 deletions(-)

Comments

Johan Hovold Oct. 11, 2017, 2:14 p.m. UTC | #1 
On Wed, Oct 11, 2017 at 12:36:38PM +0200, Takashi Iwai wrote:
> This patch adds a new helper function to perform a sanity check of the
> given URB to see whether it contains a valid endpoint.  It's a light-
> weight version of what usb_submit_urb() does, but without the kernel
> warning followed by the stack trace, just returns an error code.
> 
> Especially for a driver that doesn't parse the descriptor but fills
> the URB with the fixed endpoint (e.g. some quirks for non-compliant
> devices), this kind of check is preferable at the probe phase before
> actually submitting the urb.
> 
> Tested-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> v1->v2:
> * Fix function name typos
> * Call usb_urb_ep_type_check() in usb_submit_urb(), too
> 
>  drivers/usb/core/urb.c | 30 ++++++++++++++++++++++++++----
>  include/linux/usb.h    |  2 ++
>  2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 47903d510955..8b800e34407b 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -187,6 +187,31 @@ EXPORT_SYMBOL_GPL(usb_unanchor_urb);
>  
>  /*-------------------------------------------------------------------*/
>  
> +static const int pipetypes[4] = {
> +	PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> +};

No this one is no longer used outside of usb_urb_ep_type_check().

> +
> +/**
> + * usb_urb_ep_type_check - sanity check of endpoint in the given urb
> + * @urb: urb to be checked
> + *
> + * This performs a light-weight sanity check for the endpoint in the
> + * given urb.  It returns 0 if the urb contains a valid endpoint, otherwise
> + * a negative error code.
> + */
> +int usb_urb_ep_type_check(const struct urb *urb)
> +{
> +	const struct usb_host_endpoint *ep;
> +
> +	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> +	if (!ep)
> +		return -EINVAL;

And I know Greg asked you to call this function from usb_submit_urb()
which have already done the above ep-lookup once. Maybe not that many
extra cycles, but still seems unnecessary to me.

> +	if (usb_pipetype(urb->pipe) != pipetypes[usb_endpoint_type(&ep->desc)])
> +		return -EINVAL;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_urb_ep_type_check);
> +
>  /**
>   * usb_submit_urb - issue an asynchronous transfer request for an endpoint
>   * @urb: pointer to the urb describing the request
> @@ -326,9 +351,6 @@ EXPORT_SYMBOL_GPL(usb_unanchor_urb);
>   */
>  int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>  {
> -	static int			pipetypes[4] = {
> -		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> -	};
>  	int				xfertype, max;
>  	struct usb_device		*dev;
>  	struct usb_host_endpoint	*ep;
> @@ -444,7 +466,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>  	 */
>  
>  	/* Check that the pipe's type matches the endpoint's type */
> -	if (usb_pipetype(urb->pipe) != pipetypes[xfertype])
> +	if (usb_urb_ep_type_check(urb))
>  		dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
>  			usb_pipetype(urb->pipe), pipetypes[xfertype]);

Johan
Takashi Iwai Oct. 11, 2017, 2:31 p.m. UTC | #2 
On Wed, 11 Oct 2017 16:14:55 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 11, 2017 at 12:36:38PM +0200, Takashi Iwai wrote:
> > This patch adds a new helper function to perform a sanity check of the
> > given URB to see whether it contains a valid endpoint.  It's a light-
> > weight version of what usb_submit_urb() does, but without the kernel
> > warning followed by the stack trace, just returns an error code.
> > 
> > Especially for a driver that doesn't parse the descriptor but fills
> > the URB with the fixed endpoint (e.g. some quirks for non-compliant
> > devices), this kind of check is preferable at the probe phase before
> > actually submitting the urb.
> > 
> > Tested-by: Andrey Konovalov <andreyknvl@google.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > v1->v2:
> > * Fix function name typos
> > * Call usb_urb_ep_type_check() in usb_submit_urb(), too
> > 
> >  drivers/usb/core/urb.c | 30 ++++++++++++++++++++++++++----
> >  include/linux/usb.h    |  2 ++
> >  2 files changed, 28 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> > index 47903d510955..8b800e34407b 100644
> > --- a/drivers/usb/core/urb.c
> > +++ b/drivers/usb/core/urb.c
> > @@ -187,6 +187,31 @@ EXPORT_SYMBOL_GPL(usb_unanchor_urb);
> >  
> >  /*-------------------------------------------------------------------*/
> >  
> > +static const int pipetypes[4] = {
> > +	PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > +};
> 
> No this one is no longer used outside of usb_urb_ep_type_check().

It is used in the error message, so I kept it here.


> > +
> > +/**
> > + * usb_urb_ep_type_check - sanity check of endpoint in the given urb
> > + * @urb: urb to be checked
> > + *
> > + * This performs a light-weight sanity check for the endpoint in the
> > + * given urb.  It returns 0 if the urb contains a valid endpoint, otherwise
> > + * a negative error code.
> > + */
> > +int usb_urb_ep_type_check(const struct urb *urb)
> > +{
> > +	const struct usb_host_endpoint *ep;
> > +
> > +	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> > +	if (!ep)
> > +		return -EINVAL;
> 
> And I know Greg asked you to call this function from usb_submit_urb()
> which have already done the above ep-lookup once. Maybe not that many
> extra cycles, but still seems unnecessary to me.

Yeah, we can do micro-optimization later if needed, e.g. splitting the
function.  It's a question of balance between code optimization and
readability...


thanks,

Takashi

> > +	if (usb_pipetype(urb->pipe) != pipetypes[usb_endpoint_type(&ep->desc)])
> > +		return -EINVAL;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_urb_ep_type_check);
> > +
> >  /**
> >   * usb_submit_urb - issue an asynchronous transfer request for an endpoint
> >   * @urb: pointer to the urb describing the request
> > @@ -326,9 +351,6 @@ EXPORT_SYMBOL_GPL(usb_unanchor_urb);
> >   */
> >  int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
> >  {
> > -	static int			pipetypes[4] = {
> > -		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > -	};
> >  	int				xfertype, max;
> >  	struct usb_device		*dev;
> >  	struct usb_host_endpoint	*ep;
> > @@ -444,7 +466,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
> >  	 */
> >  
> >  	/* Check that the pipe's type matches the endpoint's type */
> > -	if (usb_pipetype(urb->pipe) != pipetypes[xfertype])
> > +	if (usb_urb_ep_type_check(urb))
> >  		dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
> >  			usb_pipetype(urb->pipe), pipetypes[xfertype]);
> 
> Johan
>
Johan Hovold Oct. 11, 2017, 2:58 p.m. UTC | #3 
On Wed, Oct 11, 2017 at 04:31:07PM +0200, Takashi Iwai wrote:
> On Wed, 11 Oct 2017 16:14:55 +0200,
> Johan Hovold wrote:
> > 
> > On Wed, Oct 11, 2017 at 12:36:38PM +0200, Takashi Iwai wrote:
> > > This patch adds a new helper function to perform a sanity check of the
> > > given URB to see whether it contains a valid endpoint.  It's a light-
> > > weight version of what usb_submit_urb() does, but without the kernel
> > > warning followed by the stack trace, just returns an error code.
> > > 
> > > Especially for a driver that doesn't parse the descriptor but fills
> > > the URB with the fixed endpoint (e.g. some quirks for non-compliant
> > > devices), this kind of check is preferable at the probe phase before
> > > actually submitting the urb.
> > > 
> > > Tested-by: Andrey Konovalov <andreyknvl@google.com>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > > v1->v2:
> > > * Fix function name typos
> > > * Call usb_urb_ep_type_check() in usb_submit_urb(), too
> > > 
> > >  drivers/usb/core/urb.c | 30 ++++++++++++++++++++++++++----
> > >  include/linux/usb.h    |  2 ++
> > >  2 files changed, 28 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> > > index 47903d510955..8b800e34407b 100644
> > > --- a/drivers/usb/core/urb.c
> > > +++ b/drivers/usb/core/urb.c
> > > @@ -187,6 +187,31 @@ EXPORT_SYMBOL_GPL(usb_unanchor_urb);
> > >  
> > >  /*-------------------------------------------------------------------*/
> > >  
> > > +static const int pipetypes[4] = {
> > > +	PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > > +};
> > 
> > No this one is no longer used outside of usb_urb_ep_type_check().
> 
> It is used in the error message, so I kept it here.

Ah, my bad, sorry.

Johan
diff mbox

Patch

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 47903d510955..8b800e34407b 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -187,6 +187,31 @@  EXPORT_SYMBOL_GPL(usb_unanchor_urb);
 
 /*-------------------------------------------------------------------*/
 
+static const int pipetypes[4] = {
+	PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
+};
+
+/**
+ * usb_urb_ep_type_check - sanity check of endpoint in the given urb
+ * @urb: urb to be checked
+ *
+ * This performs a light-weight sanity check for the endpoint in the
+ * given urb.  It returns 0 if the urb contains a valid endpoint, otherwise
+ * a negative error code.
+ */
+int usb_urb_ep_type_check(const struct urb *urb)
+{
+	const struct usb_host_endpoint *ep;
+
+	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
+	if (!ep)
+		return -EINVAL;
+	if (usb_pipetype(urb->pipe) != pipetypes[usb_endpoint_type(&ep->desc)])
+		return -EINVAL;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_urb_ep_type_check);
+
 /**
  * usb_submit_urb - issue an asynchronous transfer request for an endpoint
  * @urb: pointer to the urb describing the request
@@ -326,9 +351,6 @@  EXPORT_SYMBOL_GPL(usb_unanchor_urb);
  */
 int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 {
-	static int			pipetypes[4] = {
-		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
-	};
 	int				xfertype, max;
 	struct usb_device		*dev;
 	struct usb_host_endpoint	*ep;
@@ -444,7 +466,7 @@  int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 	 */
 
 	/* Check that the pipe's type matches the endpoint's type */
-	if (usb_pipetype(urb->pipe) != pipetypes[xfertype])
+	if (usb_urb_ep_type_check(urb))
 		dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
 			usb_pipetype(urb->pipe), pipetypes[xfertype]);
 
diff --git a/include/linux/usb.h b/include/linux/usb.h
index cb9fbd54386e..2b861804fffa 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1728,6 +1728,8 @@  static inline int usb_urb_dir_out(struct urb *urb)
 	return (urb->transfer_flags & URB_DIR_MASK) == URB_DIR_OUT;
 }
 
+int usb_urb_ep_type_check(const struct urb *urb);
+
 void *usb_alloc_coherent(struct usb_device *dev, size_t size,
 	gfp_t mem_flags, dma_addr_t *dma);
 void usb_free_coherent(struct usb_device *dev, size_t size,