| Message ID | 20200902110115.1994491-2-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: new USB control message helper functions | expand |
On Wed, 02 Sep 2020 13:01:03 +0200, Greg Kroah-Hartman wrote: > > snd_usb_pipe_sanity_check() is a great function, so let's move it into > the USB core so that other parts of the kernel, including the USB core, > can call it. > > Name it usb_pipe_type_check() to match the existing > usb_urb_ep_type_check() call, which now uses this function. > > Cc: Jaroslav Kysela <perex@perex.cz> > Cc: Takashi Iwai <tiwai@suse.com> > Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org> > Cc: Eli Billauer <eli.billauer@gmail.com> > Cc: Emiliano Ingrassia <ingrassia@epigenesys.com> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Alexander Tsoy <alexander@tsoy.me> > Cc: "Geoffrey D. Bennett" <g@b4.vu> > Cc: Jussi Laako <jussi@sonarnerd.net> > Cc: Nick Kossifidis <mickflemm@gmail.com> > Cc: Dmitry Panchenko <dmitry@d-systems.ee> > Cc: Chris Wulff <crwulff@gmail.com> > Cc: Jesus Ramos <jesus-ramos@live.com> > Cc: linux-usb@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: alsa-devel@alsa-project.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Reviewed-by: Takashi Iwai <tiwai@suse.de> thanks, Takashi > --- > drivers/usb/core/urb.c | 29 ++++++++++++++++++++++------- > include/linux/usb.h | 1 + > sound/usb/helper.c | 16 +--------------- > sound/usb/helper.h | 1 - > sound/usb/mixer_scarlett_gen2.c | 2 +- > sound/usb/quirks.c | 12 ++++++------ > 6 files changed, 31 insertions(+), 30 deletions(-) > > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c > index 27e83e55a590..45bc2914c1ba 100644 > --- a/drivers/usb/core/urb.c > +++ b/drivers/usb/core/urb.c > @@ -192,24 +192,39 @@ static const int pipetypes[4] = { > }; > > /** > - * usb_urb_ep_type_check - sanity check of endpoint in the given urb > - * @urb: urb to be checked > + * usb_pipe_type_check - sanity check of a specific pipe for a usb device > + * @dev: struct usb_device to be checked > + * @pipe: pipe to check > * > * 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. > + * given usb device. It returns 0 if the pipe is a valid for the specific usb > + * device, otherwise a negative error code. > */ > -int usb_urb_ep_type_check(const struct urb *urb) > +int usb_pipe_type_check(struct usb_device *dev, unsigned int pipe) > { > const struct usb_host_endpoint *ep; > > - ep = usb_pipe_endpoint(urb->dev, urb->pipe); > + ep = usb_pipe_endpoint(dev, pipe); > if (!ep) > return -EINVAL; > - if (usb_pipetype(urb->pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) > + if (usb_pipetype(pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) > return -EINVAL; > return 0; > } > +EXPORT_SYMBOL_GPL(usb_pipe_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) > +{ > + return usb_pipe_type_check(urb->dev, urb->pipe); > +} > EXPORT_SYMBOL_GPL(usb_urb_ep_type_check); > > /** > diff --git a/include/linux/usb.h b/include/linux/usb.h > index 20c555db4621..0b3963d7ec38 100644 > --- a/include/linux/usb.h > +++ b/include/linux/usb.h > @@ -1764,6 +1764,7 @@ static inline int usb_urb_dir_out(struct urb *urb) > return (urb->transfer_flags & URB_DIR_MASK) == URB_DIR_OUT; > } > > +int usb_pipe_type_check(struct usb_device *dev, unsigned int pipe); > int usb_urb_ep_type_check(const struct urb *urb); > > void *usb_alloc_coherent(struct usb_device *dev, size_t size, > diff --git a/sound/usb/helper.c b/sound/usb/helper.c > index 4c12cc5b53fd..cf92d7110773 100644 > --- a/sound/usb/helper.c > +++ b/sound/usb/helper.c > @@ -63,20 +63,6 @@ void *snd_usb_find_csint_desc(void *buffer, int buflen, void *after, u8 dsubtype > return NULL; > } > > -/* check the validity of pipe and EP types */ > -int snd_usb_pipe_sanity_check(struct usb_device *dev, unsigned int pipe) > -{ > - static const int pipetypes[4] = { > - PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT > - }; > - struct usb_host_endpoint *ep; > - > - ep = usb_pipe_endpoint(dev, pipe); > - if (!ep || usb_pipetype(pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) > - return -EINVAL; > - return 0; > -} > - > /* > * Wrapper for usb_control_msg(). > * Allocates a temp buffer to prevent dmaing from/to the stack. > @@ -89,7 +75,7 @@ int snd_usb_ctl_msg(struct usb_device *dev, unsigned int pipe, __u8 request, > void *buf = NULL; > int timeout; > > - if (snd_usb_pipe_sanity_check(dev, pipe)) > + if (usb_pipe_type_check(dev, pipe)) > return -EINVAL; > > if (size > 0) { > diff --git a/sound/usb/helper.h b/sound/usb/helper.h > index 5e8a18b4e7b9..f5b4c6647e4d 100644 > --- a/sound/usb/helper.h > +++ b/sound/usb/helper.h > @@ -7,7 +7,6 @@ unsigned int snd_usb_combine_bytes(unsigned char *bytes, int size); > void *snd_usb_find_desc(void *descstart, int desclen, void *after, u8 dtype); > void *snd_usb_find_csint_desc(void *descstart, int desclen, void *after, u8 dsubtype); > > -int snd_usb_pipe_sanity_check(struct usb_device *dev, unsigned int pipe); > int snd_usb_ctl_msg(struct usb_device *dev, unsigned int pipe, > __u8 request, __u8 requesttype, __u16 value, __u16 index, > void *data, __u16 size); > diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c > index 0ffff7640892..9609c6d9655c 100644 > --- a/sound/usb/mixer_scarlett_gen2.c > +++ b/sound/usb/mixer_scarlett_gen2.c > @@ -1978,7 +1978,7 @@ static int scarlett2_mixer_status_create(struct usb_mixer_interface *mixer) > return 0; > } > > - if (snd_usb_pipe_sanity_check(dev, pipe)) > + if (usb_pipe_type_check(dev, pipe)) > return -EINVAL; > > mixer->urb = usb_alloc_urb(0, GFP_KERNEL); > diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c > index abf99b814a0f..fc3aab04a0bc 100644 > --- a/sound/usb/quirks.c > +++ b/sound/usb/quirks.c > @@ -846,7 +846,7 @@ static int snd_usb_accessmusic_boot_quirk(struct usb_device *dev) > static const u8 seq[] = { 0x4e, 0x73, 0x52, 0x01 }; > void *buf; > > - if (snd_usb_pipe_sanity_check(dev, usb_sndintpipe(dev, 0x05))) > + if (usb_pipe_type_check(dev, usb_sndintpipe(dev, 0x05))) > return -EINVAL; > buf = kmemdup(seq, ARRAY_SIZE(seq), GFP_KERNEL); > if (!buf) > @@ -875,7 +875,7 @@ static int snd_usb_nativeinstruments_boot_quirk(struct usb_device *dev) > { > int ret; > > - if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) > + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) > return -EINVAL; > ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), > 0xaf, USB_TYPE_VENDOR | USB_RECIP_DEVICE, > @@ -984,7 +984,7 @@ static int snd_usb_axefx3_boot_quirk(struct usb_device *dev) > > dev_dbg(&dev->dev, "Waiting for Axe-Fx III to boot up...\n"); > > - if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) > + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) > return -EINVAL; > /* If the Axe-Fx III has not fully booted, it will timeout when trying > * to enable the audio streaming interface. A more generous timeout is > @@ -1018,7 +1018,7 @@ static int snd_usb_motu_microbookii_communicate(struct usb_device *dev, u8 *buf, > { > int err, actual_length; > > - if (snd_usb_pipe_sanity_check(dev, usb_sndintpipe(dev, 0x01))) > + if (usb_pipe_type_check(dev, usb_sndintpipe(dev, 0x01))) > return -EINVAL; > err = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x01), buf, *length, > &actual_length, 1000); > @@ -1030,7 +1030,7 @@ static int snd_usb_motu_microbookii_communicate(struct usb_device *dev, u8 *buf, > > memset(buf, 0, buf_size); > > - if (snd_usb_pipe_sanity_check(dev, usb_rcvintpipe(dev, 0x82))) > + if (usb_pipe_type_check(dev, usb_rcvintpipe(dev, 0x82))) > return -EINVAL; > err = usb_interrupt_msg(dev, usb_rcvintpipe(dev, 0x82), buf, buf_size, > &actual_length, 1000); > @@ -1117,7 +1117,7 @@ static int snd_usb_motu_m_series_boot_quirk(struct usb_device *dev) > { > int ret; > > - if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) > + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) > return -EINVAL; > ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), > 1, USB_TYPE_VENDOR | USB_RECIP_DEVICE, > -- > 2.28.0 >
On Wed, Sep 02, 2020 at 01:01:03PM +0200, Greg Kroah-Hartman wrote: > snd_usb_pipe_sanity_check() is a great function, so let's move it into > the USB core so that other parts of the kernel, including the USB core, > can call it. > > Name it usb_pipe_type_check() to match the existing > usb_urb_ep_type_check() call, which now uses this function. > > Cc: Jaroslav Kysela <perex@perex.cz> > Cc: Takashi Iwai <tiwai@suse.com> > Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org> > Cc: Eli Billauer <eli.billauer@gmail.com> > Cc: Emiliano Ingrassia <ingrassia@epigenesys.com> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Alexander Tsoy <alexander@tsoy.me> > Cc: "Geoffrey D. Bennett" <g@b4.vu> > Cc: Jussi Laako <jussi@sonarnerd.net> > Cc: Nick Kossifidis <mickflemm@gmail.com> > Cc: Dmitry Panchenko <dmitry@d-systems.ee> > Cc: Chris Wulff <crwulff@gmail.com> > Cc: Jesus Ramos <jesus-ramos@live.com> > Cc: linux-usb@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: alsa-devel@alsa-project.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c > index 27e83e55a590..45bc2914c1ba 100644 > --- a/drivers/usb/core/urb.c > +++ b/drivers/usb/core/urb.c > @@ -192,24 +192,39 @@ static const int pipetypes[4] = { > }; > > /** > - * usb_urb_ep_type_check - sanity check of endpoint in the given urb > - * @urb: urb to be checked > + * usb_pipe_type_check - sanity check of a specific pipe for a usb device > + * @dev: struct usb_device to be checked > + * @pipe: pipe to check > * > * 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. > + * given usb device. It returns 0 if the pipe is a valid for the specific usb -----------------------------------------------------^ Typo. > + * device, otherwise a negative error code. > */ > -int usb_urb_ep_type_check(const struct urb *urb) > +int usb_pipe_type_check(struct usb_device *dev, unsigned int pipe) > { > const struct usb_host_endpoint *ep; > > - ep = usb_pipe_endpoint(urb->dev, urb->pipe); > + ep = usb_pipe_endpoint(dev, pipe); > if (!ep) > return -EINVAL; > - if (usb_pipetype(urb->pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) > + if (usb_pipetype(pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) > return -EINVAL; > return 0; > } > +EXPORT_SYMBOL_GPL(usb_pipe_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) > +{ > + return usb_pipe_type_check(urb->dev, urb->pipe); > +} > EXPORT_SYMBOL_GPL(usb_urb_ep_type_check); Since this routine is used in only one place in the entire kernel, you might as well inline the code there and get rid of the function entirely. > diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c > index abf99b814a0f..fc3aab04a0bc 100644 > --- a/sound/usb/quirks.c > +++ b/sound/usb/quirks.c > @@ -846,7 +846,7 @@ static int snd_usb_accessmusic_boot_quirk(struct usb_device *dev) > static const u8 seq[] = { 0x4e, 0x73, 0x52, 0x01 }; > void *buf; > > - if (snd_usb_pipe_sanity_check(dev, usb_sndintpipe(dev, 0x05))) > + if (usb_pipe_type_check(dev, usb_sndintpipe(dev, 0x05))) > return -EINVAL; > buf = kmemdup(seq, ARRAY_SIZE(seq), GFP_KERNEL); > if (!buf) > @@ -875,7 +875,7 @@ static int snd_usb_nativeinstruments_boot_quirk(struct usb_device *dev) > { > int ret; > > - if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) > + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) > return -EINVAL; In a few places here this check is completely unnecessary. All it does is verify that the device does have an endpoint 0 and the the type of the endpoint matches the type of the pipe. Well, every USB device always has an endpoint 0, and it is always a bidirectional control endpoint. Therefore a simple static check is all you need: There's no point calling usb_pipe_type_check() when the pipe is of the form usb_{snd|rcv}ctrlpipe(dev, 0). In short, this check should be removed completely; it does nothing. > ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), > 0xaf, USB_TYPE_VENDOR | USB_RECIP_DEVICE, > @@ -984,7 +984,7 @@ static int snd_usb_axefx3_boot_quirk(struct usb_device *dev) > > dev_dbg(&dev->dev, "Waiting for Axe-Fx III to boot up...\n"); > > - if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) > + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) Same for this check. > return -EINVAL; > /* If the Axe-Fx III has not fully booted, it will timeout when trying > * to enable the audio streaming interface. A more generous timeout is > @@ -1018,7 +1018,7 @@ static int snd_usb_motu_microbookii_communicate(struct usb_device *dev, u8 *buf, > { > int err, actual_length; > > - if (snd_usb_pipe_sanity_check(dev, usb_sndintpipe(dev, 0x01))) > + if (usb_pipe_type_check(dev, usb_sndintpipe(dev, 0x01))) > return -EINVAL; > err = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x01), buf, *length, > &actual_length, 1000); > @@ -1030,7 +1030,7 @@ static int snd_usb_motu_microbookii_communicate(struct usb_device *dev, u8 *buf, > > memset(buf, 0, buf_size); > > - if (snd_usb_pipe_sanity_check(dev, usb_rcvintpipe(dev, 0x82))) > + if (usb_pipe_type_check(dev, usb_rcvintpipe(dev, 0x82))) > return -EINVAL; > err = usb_interrupt_msg(dev, usb_rcvintpipe(dev, 0x82), buf, buf_size, > &actual_length, 1000); > @@ -1117,7 +1117,7 @@ static int snd_usb_motu_m_series_boot_quirk(struct usb_device *dev) > { > int ret; > > - if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) > + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) And this one. > return -EINVAL; > ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), > 1, USB_TYPE_VENDOR | USB_RECIP_DEVICE, Alan Stern
On Thu, 03 Sep 2020 02:45:53 +0200, Alan Stern wrote: > > In a few places here this check is completely unnecessary. All it does > is verify that the device does have an endpoint 0 and the the type of > the endpoint matches the type of the pipe. Well, every USB device > always has an endpoint 0, and it is always a bidirectional control > endpoint. Therefore a simple static check is all you need: There's no > point calling usb_pipe_type_check() when the pipe is of the form > usb_{snd|rcv}ctrlpipe(dev, 0). > > In short, this check should be removed completely; it does nothing. Fair enough, but I think those removals should be in another patch. thanks, Takashi
On Wed, Sep 02, 2020 at 08:45:53PM -0400, Alan Stern wrote: > On Wed, Sep 02, 2020 at 01:01:03PM +0200, Greg Kroah-Hartman wrote: > > snd_usb_pipe_sanity_check() is a great function, so let's move it into > > the USB core so that other parts of the kernel, including the USB core, > > can call it. > > > > Name it usb_pipe_type_check() to match the existing > > usb_urb_ep_type_check() call, which now uses this function. > > > > Cc: Jaroslav Kysela <perex@perex.cz> > > Cc: Takashi Iwai <tiwai@suse.com> > > Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org> > > Cc: Eli Billauer <eli.billauer@gmail.com> > > Cc: Emiliano Ingrassia <ingrassia@epigenesys.com> > > Cc: Alan Stern <stern@rowland.harvard.edu> > > Cc: Alexander Tsoy <alexander@tsoy.me> > > Cc: "Geoffrey D. Bennett" <g@b4.vu> > > Cc: Jussi Laako <jussi@sonarnerd.net> > > Cc: Nick Kossifidis <mickflemm@gmail.com> > > Cc: Dmitry Panchenko <dmitry@d-systems.ee> > > Cc: Chris Wulff <crwulff@gmail.com> > > Cc: Jesus Ramos <jesus-ramos@live.com> > > Cc: linux-usb@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > Cc: alsa-devel@alsa-project.org > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > > > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c > > index 27e83e55a590..45bc2914c1ba 100644 > > --- a/drivers/usb/core/urb.c > > +++ b/drivers/usb/core/urb.c > > @@ -192,24 +192,39 @@ static const int pipetypes[4] = { > > }; > > > > /** > > - * usb_urb_ep_type_check - sanity check of endpoint in the given urb > > - * @urb: urb to be checked > > + * usb_pipe_type_check - sanity check of a specific pipe for a usb device > > + * @dev: struct usb_device to be checked > > + * @pipe: pipe to check > > * > > * 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. > > + * given usb device. It returns 0 if the pipe is a valid for the specific usb > -----------------------------------------------------^ > Typo. Oops, will fix, thanks. > > > + * device, otherwise a negative error code. > > */ > > -int usb_urb_ep_type_check(const struct urb *urb) > > +int usb_pipe_type_check(struct usb_device *dev, unsigned int pipe) > > { > > const struct usb_host_endpoint *ep; > > > > - ep = usb_pipe_endpoint(urb->dev, urb->pipe); > > + ep = usb_pipe_endpoint(dev, pipe); > > if (!ep) > > return -EINVAL; > > - if (usb_pipetype(urb->pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) > > + if (usb_pipetype(pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) > > return -EINVAL; > > return 0; > > } > > +EXPORT_SYMBOL_GPL(usb_pipe_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) > > +{ > > + return usb_pipe_type_check(urb->dev, urb->pipe); > > +} > > EXPORT_SYMBOL_GPL(usb_urb_ep_type_check); > > Since this routine is used in only one place in the entire kernel, you > might as well inline the code there and get rid of the function > entirely. Good idea, will do. > > diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c > > index abf99b814a0f..fc3aab04a0bc 100644 > > --- a/sound/usb/quirks.c > > +++ b/sound/usb/quirks.c > > @@ -846,7 +846,7 @@ static int snd_usb_accessmusic_boot_quirk(struct usb_device *dev) > > static const u8 seq[] = { 0x4e, 0x73, 0x52, 0x01 }; > > void *buf; > > > > - if (snd_usb_pipe_sanity_check(dev, usb_sndintpipe(dev, 0x05))) > > + if (usb_pipe_type_check(dev, usb_sndintpipe(dev, 0x05))) > > return -EINVAL; > > buf = kmemdup(seq, ARRAY_SIZE(seq), GFP_KERNEL); > > if (!buf) > > @@ -875,7 +875,7 @@ static int snd_usb_nativeinstruments_boot_quirk(struct usb_device *dev) > > { > > int ret; > > > > - if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) > > + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) > > return -EINVAL; > > In a few places here this check is completely unnecessary. All it does > is verify that the device does have an endpoint 0 and the the type of > the endpoint matches the type of the pipe. Well, every USB device > always has an endpoint 0, and it is always a bidirectional control > endpoint. I think this was probably added to handle syzbot issues. As long as the USB core does ensure that a USB device has endpoint 0, I agree, these can be removed. I'll go check that and add a follow-on patch to the series to do this, thanks. thanks, greg k-h
On Thu, Sep 03, 2020 at 09:32:30AM +0200, Greg Kroah-Hartman wrote: > On Wed, Sep 02, 2020 at 08:45:53PM -0400, Alan Stern wrote: > > On Wed, Sep 02, 2020 at 01:01:03PM +0200, Greg Kroah-Hartman wrote: > > > snd_usb_pipe_sanity_check() is a great function, so let's move it into > > > the USB core so that other parts of the kernel, including the USB core, > > > can call it. > > > > > > Name it usb_pipe_type_check() to match the existing > > > usb_urb_ep_type_check() call, which now uses this function. > > > > > > Cc: Jaroslav Kysela <perex@perex.cz> > > > Cc: Takashi Iwai <tiwai@suse.com> > > > Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org> > > > Cc: Eli Billauer <eli.billauer@gmail.com> > > > Cc: Emiliano Ingrassia <ingrassia@epigenesys.com> > > > Cc: Alan Stern <stern@rowland.harvard.edu> > > > Cc: Alexander Tsoy <alexander@tsoy.me> > > > Cc: "Geoffrey D. Bennett" <g@b4.vu> > > > Cc: Jussi Laako <jussi@sonarnerd.net> > > > Cc: Nick Kossifidis <mickflemm@gmail.com> > > > Cc: Dmitry Panchenko <dmitry@d-systems.ee> > > > Cc: Chris Wulff <crwulff@gmail.com> > > > Cc: Jesus Ramos <jesus-ramos@live.com> > > > Cc: linux-usb@vger.kernel.org > > > Cc: linux-kernel@vger.kernel.org > > > Cc: alsa-devel@alsa-project.org > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > --- > > > > > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c > > > index 27e83e55a590..45bc2914c1ba 100644 > > > --- a/drivers/usb/core/urb.c > > > +++ b/drivers/usb/core/urb.c > > > @@ -192,24 +192,39 @@ static const int pipetypes[4] = { > > > }; > > > > > > /** > > > - * usb_urb_ep_type_check - sanity check of endpoint in the given urb > > > - * @urb: urb to be checked > > > + * usb_pipe_type_check - sanity check of a specific pipe for a usb device > > > + * @dev: struct usb_device to be checked > > > + * @pipe: pipe to check > > > * > > > * 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. > > > + * given usb device. It returns 0 if the pipe is a valid for the specific usb > > -----------------------------------------------------^ > > Typo. > > Oops, will fix, thanks. > > > > > > > + * device, otherwise a negative error code. > > > */ > > > -int usb_urb_ep_type_check(const struct urb *urb) > > > +int usb_pipe_type_check(struct usb_device *dev, unsigned int pipe) > > > { > > > const struct usb_host_endpoint *ep; > > > > > > - ep = usb_pipe_endpoint(urb->dev, urb->pipe); > > > + ep = usb_pipe_endpoint(dev, pipe); > > > if (!ep) > > > return -EINVAL; > > > - if (usb_pipetype(urb->pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) > > > + if (usb_pipetype(pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) > > > return -EINVAL; > > > return 0; > > > } > > > +EXPORT_SYMBOL_GPL(usb_pipe_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) > > > +{ > > > + return usb_pipe_type_check(urb->dev, urb->pipe); > > > +} > > > EXPORT_SYMBOL_GPL(usb_urb_ep_type_check); > > > > Since this routine is used in only one place in the entire kernel, you > > might as well inline the code there and get rid of the function > > entirely. > > Good idea, will do. No, wait, the USB sound drivers call it a lot, so it needs to stick around for now until we clean that up. thanks, greg k-h
On Mon, Sep 07, 2020 at 04:16:34PM +0200, Greg Kroah-Hartman wrote: > On Thu, Sep 03, 2020 at 09:32:30AM +0200, Greg Kroah-Hartman wrote: > > On Wed, Sep 02, 2020 at 08:45:53PM -0400, Alan Stern wrote: > > > Since this routine is used in only one place in the entire kernel, you > > > might as well inline the code there and get rid of the function > > > entirely. > > > > Good idea, will do. > > No, wait, the USB sound drivers call it a lot, so it needs to stick > around for now until we clean that up. Argh. I must have run "git grep" from within drivers/usb/core. My mistake. Alan Stern
On Wed, Sep 02, 2020 at 04:35:33PM +0200, Takashi Iwai wrote: > On Wed, 02 Sep 2020 13:01:03 +0200, > Greg Kroah-Hartman wrote: > > > > snd_usb_pipe_sanity_check() is a great function, so let's move it into > > the USB core so that other parts of the kernel, including the USB core, > > can call it. > > > > Name it usb_pipe_type_check() to match the existing > > usb_urb_ep_type_check() call, which now uses this function. > > > > Cc: Jaroslav Kysela <perex@perex.cz> > > Cc: Takashi Iwai <tiwai@suse.com> > > Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org> > > Cc: Eli Billauer <eli.billauer@gmail.com> > > Cc: Emiliano Ingrassia <ingrassia@epigenesys.com> > > Cc: Alan Stern <stern@rowland.harvard.edu> > > Cc: Alexander Tsoy <alexander@tsoy.me> > > Cc: "Geoffrey D. Bennett" <g@b4.vu> > > Cc: Jussi Laako <jussi@sonarnerd.net> > > Cc: Nick Kossifidis <mickflemm@gmail.com> > > Cc: Dmitry Panchenko <dmitry@d-systems.ee> > > Cc: Chris Wulff <crwulff@gmail.com> > > Cc: Jesus Ramos <jesus-ramos@live.com> > > Cc: linux-usb@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > Cc: alsa-devel@alsa-project.org > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Reviewed-by: Takashi Iwai <tiwai@suse.de> Thanks for the reviews of all of these. greg k-h
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c index 27e83e55a590..45bc2914c1ba 100644 --- a/drivers/usb/core/urb.c +++ b/drivers/usb/core/urb.c @@ -192,24 +192,39 @@ static const int pipetypes[4] = { }; /** - * usb_urb_ep_type_check - sanity check of endpoint in the given urb - * @urb: urb to be checked + * usb_pipe_type_check - sanity check of a specific pipe for a usb device + * @dev: struct usb_device to be checked + * @pipe: pipe to check * * 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. + * given usb device. It returns 0 if the pipe is a valid for the specific usb + * device, otherwise a negative error code. */ -int usb_urb_ep_type_check(const struct urb *urb) +int usb_pipe_type_check(struct usb_device *dev, unsigned int pipe) { const struct usb_host_endpoint *ep; - ep = usb_pipe_endpoint(urb->dev, urb->pipe); + ep = usb_pipe_endpoint(dev, pipe); if (!ep) return -EINVAL; - if (usb_pipetype(urb->pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) + if (usb_pipetype(pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) return -EINVAL; return 0; } +EXPORT_SYMBOL_GPL(usb_pipe_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) +{ + return usb_pipe_type_check(urb->dev, urb->pipe); +} EXPORT_SYMBOL_GPL(usb_urb_ep_type_check); /** diff --git a/include/linux/usb.h b/include/linux/usb.h index 20c555db4621..0b3963d7ec38 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -1764,6 +1764,7 @@ static inline int usb_urb_dir_out(struct urb *urb) return (urb->transfer_flags & URB_DIR_MASK) == URB_DIR_OUT; } +int usb_pipe_type_check(struct usb_device *dev, unsigned int pipe); int usb_urb_ep_type_check(const struct urb *urb); void *usb_alloc_coherent(struct usb_device *dev, size_t size, diff --git a/sound/usb/helper.c b/sound/usb/helper.c index 4c12cc5b53fd..cf92d7110773 100644 --- a/sound/usb/helper.c +++ b/sound/usb/helper.c @@ -63,20 +63,6 @@ void *snd_usb_find_csint_desc(void *buffer, int buflen, void *after, u8 dsubtype return NULL; } -/* check the validity of pipe and EP types */ -int snd_usb_pipe_sanity_check(struct usb_device *dev, unsigned int pipe) -{ - static const int pipetypes[4] = { - PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT - }; - struct usb_host_endpoint *ep; - - ep = usb_pipe_endpoint(dev, pipe); - if (!ep || usb_pipetype(pipe) != pipetypes[usb_endpoint_type(&ep->desc)]) - return -EINVAL; - return 0; -} - /* * Wrapper for usb_control_msg(). * Allocates a temp buffer to prevent dmaing from/to the stack. @@ -89,7 +75,7 @@ int snd_usb_ctl_msg(struct usb_device *dev, unsigned int pipe, __u8 request, void *buf = NULL; int timeout; - if (snd_usb_pipe_sanity_check(dev, pipe)) + if (usb_pipe_type_check(dev, pipe)) return -EINVAL; if (size > 0) { diff --git a/sound/usb/helper.h b/sound/usb/helper.h index 5e8a18b4e7b9..f5b4c6647e4d 100644 --- a/sound/usb/helper.h +++ b/sound/usb/helper.h @@ -7,7 +7,6 @@ unsigned int snd_usb_combine_bytes(unsigned char *bytes, int size); void *snd_usb_find_desc(void *descstart, int desclen, void *after, u8 dtype); void *snd_usb_find_csint_desc(void *descstart, int desclen, void *after, u8 dsubtype); -int snd_usb_pipe_sanity_check(struct usb_device *dev, unsigned int pipe); int snd_usb_ctl_msg(struct usb_device *dev, unsigned int pipe, __u8 request, __u8 requesttype, __u16 value, __u16 index, void *data, __u16 size); diff --git a/sound/usb/mixer_scarlett_gen2.c b/sound/usb/mixer_scarlett_gen2.c index 0ffff7640892..9609c6d9655c 100644 --- a/sound/usb/mixer_scarlett_gen2.c +++ b/sound/usb/mixer_scarlett_gen2.c @@ -1978,7 +1978,7 @@ static int scarlett2_mixer_status_create(struct usb_mixer_interface *mixer) return 0; } - if (snd_usb_pipe_sanity_check(dev, pipe)) + if (usb_pipe_type_check(dev, pipe)) return -EINVAL; mixer->urb = usb_alloc_urb(0, GFP_KERNEL); diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index abf99b814a0f..fc3aab04a0bc 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -846,7 +846,7 @@ static int snd_usb_accessmusic_boot_quirk(struct usb_device *dev) static const u8 seq[] = { 0x4e, 0x73, 0x52, 0x01 }; void *buf; - if (snd_usb_pipe_sanity_check(dev, usb_sndintpipe(dev, 0x05))) + if (usb_pipe_type_check(dev, usb_sndintpipe(dev, 0x05))) return -EINVAL; buf = kmemdup(seq, ARRAY_SIZE(seq), GFP_KERNEL); if (!buf) @@ -875,7 +875,7 @@ static int snd_usb_nativeinstruments_boot_quirk(struct usb_device *dev) { int ret; - if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) return -EINVAL; ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), 0xaf, USB_TYPE_VENDOR | USB_RECIP_DEVICE, @@ -984,7 +984,7 @@ static int snd_usb_axefx3_boot_quirk(struct usb_device *dev) dev_dbg(&dev->dev, "Waiting for Axe-Fx III to boot up...\n"); - if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) return -EINVAL; /* If the Axe-Fx III has not fully booted, it will timeout when trying * to enable the audio streaming interface. A more generous timeout is @@ -1018,7 +1018,7 @@ static int snd_usb_motu_microbookii_communicate(struct usb_device *dev, u8 *buf, { int err, actual_length; - if (snd_usb_pipe_sanity_check(dev, usb_sndintpipe(dev, 0x01))) + if (usb_pipe_type_check(dev, usb_sndintpipe(dev, 0x01))) return -EINVAL; err = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x01), buf, *length, &actual_length, 1000); @@ -1030,7 +1030,7 @@ static int snd_usb_motu_microbookii_communicate(struct usb_device *dev, u8 *buf, memset(buf, 0, buf_size); - if (snd_usb_pipe_sanity_check(dev, usb_rcvintpipe(dev, 0x82))) + if (usb_pipe_type_check(dev, usb_rcvintpipe(dev, 0x82))) return -EINVAL; err = usb_interrupt_msg(dev, usb_rcvintpipe(dev, 0x82), buf, buf_size, &actual_length, 1000); @@ -1117,7 +1117,7 @@ static int snd_usb_motu_m_series_boot_quirk(struct usb_device *dev) { int ret; - if (snd_usb_pipe_sanity_check(dev, usb_sndctrlpipe(dev, 0))) + if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0))) return -EINVAL; ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), 1, USB_TYPE_VENDOR | USB_RECIP_DEVICE,
snd_usb_pipe_sanity_check() is a great function, so let's move it into the USB core so that other parts of the kernel, including the USB core, can call it. Name it usb_pipe_type_check() to match the existing usb_urb_ep_type_check() call, which now uses this function. Cc: Jaroslav Kysela <perex@perex.cz> Cc: Takashi Iwai <tiwai@suse.com> Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org> Cc: Eli Billauer <eli.billauer@gmail.com> Cc: Emiliano Ingrassia <ingrassia@epigenesys.com> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Alexander Tsoy <alexander@tsoy.me> Cc: "Geoffrey D. Bennett" <g@b4.vu> Cc: Jussi Laako <jussi@sonarnerd.net> Cc: Nick Kossifidis <mickflemm@gmail.com> Cc: Dmitry Panchenko <dmitry@d-systems.ee> Cc: Chris Wulff <crwulff@gmail.com> Cc: Jesus Ramos <jesus-ramos@live.com> Cc: linux-usb@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: alsa-devel@alsa-project.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/usb/core/urb.c | 29 ++++++++++++++++++++++------- include/linux/usb.h | 1 + sound/usb/helper.c | 16 +--------------- sound/usb/helper.h | 1 - sound/usb/mixer_scarlett_gen2.c | 2 +- sound/usb/quirks.c | 12 ++++++------ 6 files changed, 31 insertions(+), 30 deletions(-)