Message ID | 20171011103646.11879-2-tiwai@suse.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
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 >
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 --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,