linux-kernel - Re: [PATCH 08/15] usb: serial: ftdi_sio: use usb_control_msg_recv() and usb_control_msg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <X8oJimVHyGxGodZd@localhost>
Date:   Fri, 4 Dec 2020 11:03:54 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Himadri Pandya <himadrispandya@...il.com>
Cc:     johan@...nel.org, gregkh@...uxfoundation.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH 08/15] usb: serial: ftdi_sio: use usb_control_msg_recv()
 and usb_control_msg_send()

On Wed, Nov 04, 2020 at 12:16:56PM +0530, Himadri Pandya wrote:
> The new usb_control_msg_recv() and usb_control_msg_send() nicely wraps
> usb_control_msg() with proper error check. Hence use the wrappers
> instead of calling usb_control_msg() directly.
> 
> Signed-off-by: Himadri Pandya <himadrispandya@...il.com>
> ---
>  drivers/usb/serial/ftdi_sio.c | 182 +++++++++++++++-------------------
>  1 file changed, 78 insertions(+), 104 deletions(-)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index e0f4c3d9649c..37e9e75b90d0 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1245,13 +1245,12 @@ static int update_mctrl(struct usb_serial_port *port, unsigned int set,
>  		value |= FTDI_SIO_SET_DTR_HIGH;
>  	if (set & TIOCM_RTS)
>  		value |= FTDI_SIO_SET_RTS_HIGH;
> -	rv = usb_control_msg(port->serial->dev,
> -			       usb_sndctrlpipe(port->serial->dev, 0),
> -			       FTDI_SIO_SET_MODEM_CTRL_REQUEST,
> -			       FTDI_SIO_SET_MODEM_CTRL_REQUEST_TYPE,
> -			       value, priv->interface,
> -			       NULL, 0, WDR_TIMEOUT);
> -	if (rv < 0) {
> +	rv = usb_control_msg_send(port->serial->dev, 0,
> +				  FTDI_SIO_SET_MODEM_CTRL_REQUEST,
> +				  FTDI_SIO_SET_MODEM_CTRL_REQUEST_TYPE,
> +				  value, priv->interface,
> +				  NULL, 0, WDR_TIMEOUT, GFP_KERNEL);
> +	if (rv) {
>  		dev_dbg(dev, "%s Error from MODEM_CTRL urb: DTR %s, RTS %s\n",
>  			__func__,
>  			(set & TIOCM_DTR) ? "HIGH" : (clear & TIOCM_DTR) ? "LOW" : "unchanged",

As I mentioned earlier there's no point in using these helper for
control transfers without a data stage so please drop those conversions
and only update _read_latency_timer() ftdi_read_cbus_pins().

> @@ -2311,6 +2285,7 @@ static int ftdi_NDI_device_setup(struct usb_serial *serial)
>  {
>  	struct usb_device *udev = serial->dev;
>  	int latency = ndi_latency_timer;
> +	int ret;
>  
>  	if (latency == 0)
>  		latency = 1;
> @@ -2320,12 +2295,12 @@ static int ftdi_NDI_device_setup(struct usb_serial *serial)
>  	dev_dbg(&udev->dev, "%s setting NDI device latency to %d\n", __func__, latency);
>  	dev_info(&udev->dev, "NDI device with a latency value of %d\n", latency);
>  
> -	/* FIXME: errors are not returned */
> -	usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> -				FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
> -				FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
> -				latency, 0, NULL, 0, WDR_TIMEOUT);
> -	return 0;
> +	ret = usb_control_msg_send(udev, 0,
> +				   FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
> +				   FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
> +				   latency, 0, NULL, 0, WDR_TIMEOUT,
> +				   GFP_KERNEL);
> +	return ret;

Also note that returning ret here is an unrelated change which could
potentially break probing in case there are devices which do not support
this request (and would need to be done in a separate patch if at all).

Johan