Re: [PATCH 08/15] usb: serial: ftdi_sio: use usb_control_msg_recv() and usb_control_msg_send()

From: Johan Hovold <johan@kernel.org>
To: Himadri Pandya <himadrispandya@gmail.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH 08/15] usb: serial: ftdi_sio: use usb_control_msg_recv() and usb_control_msg_send()
Date: Fri, 4 Dec 2020 11:03:54 +0100	[thread overview]
Message-ID: <X8oJimVHyGxGodZd@localhost> (raw)
In-Reply-To: <20201104064703.15123-9-himadrispandya@gmail.com>

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@gmail.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