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 <andreyk...@google.com>
> > Signed-off-by: Takashi Iwai <ti...@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
>
--
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
