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

[Johan Hovold <johan@kernel.org>]

On Wed, Nov 04, 2020 at 12:17:03PM +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/kl5kusb105.c | 94 +++++++++++++++------------------
>  1 file changed, 44 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/usb/serial/kl5kusb105.c b/drivers/usb/serial/kl5kusb105.c
> index 5ee48b0650c4..75cfd1c907f3 100644
> --- a/drivers/usb/serial/kl5kusb105.c
> +++ b/drivers/usb/serial/kl5kusb105.c
> @@ -124,16 +124,17 @@ static int klsi_105_chg_port_settings(struct usb_serial_port *port,
>  {
>  	int rc;
>  
> -	rc = usb_control_msg(port->serial->dev,
> -			usb_sndctrlpipe(port->serial->dev, 0),
> -			KL5KUSB105A_SIO_SET_DATA,
> -			USB_TYPE_VENDOR | USB_DIR_OUT | USB_RECIP_INTERFACE,
> -			0, /* value */
> -			0, /* index */
> -			settings,
> -			sizeof(struct klsi_105_port_settings),
> -			KLSI_TIMEOUT);
> -	if (rc < 0)
> +	rc = usb_control_msg_send(port->serial->dev, 0,
> +				  KL5KUSB105A_SIO_SET_DATA,
> +				  USB_TYPE_VENDOR | USB_DIR_OUT |
> +				  USB_RECIP_INTERFACE,
> +				  0, /* value */
> +				  0, /* index */
> +				  settings,
> +				  sizeof(struct klsi_105_port_settings),
> +				  KLSI_TIMEOUT,
> +				  GFP_KERNEL);
> +	if (rc)
>  		dev_err(&port->dev,
>  			"Change port settings failed (error = %d)\n", rc);

The callers of this function already allocates a transfer-buffer which
you should now remove to avoid introducing an additional memdup() per
call.

Note that there was a related bug in open() which failed to release it's
transfer buffer on successful open. I CCed you on the fix which we
should get merged and backported before converting to using the new
helper.

> @@ -283,16 +276,17 @@ static int  klsi_105_open(struct tty_struct *tty, struct usb_serial_port *port)
>  		goto err_free_cfg;
>  	}
>  
> -	rc = usb_control_msg(port->serial->dev,
> -			     usb_sndctrlpipe(port->serial->dev, 0),
> -			     KL5KUSB105A_SIO_CONFIGURE,
> -			     USB_TYPE_VENDOR|USB_DIR_OUT|USB_RECIP_INTERFACE,
> -			     KL5KUSB105A_SIO_CONFIGURE_READ_ON,
> -			     0, /* index */
> -			     NULL,
> -			     0,
> -			     KLSI_TIMEOUT);
> -	if (rc < 0) {
> +	rc  = usb_control_msg_send(port->serial->dev, 0,
> +				   KL5KUSB105A_SIO_CONFIGURE,
> +				   USB_TYPE_VENDOR | USB_DIR_OUT |
> +				   USB_RECIP_INTERFACE,
> +				   KL5KUSB105A_SIO_CONFIGURE_READ_ON,
> +				   0, /* index */
> +				   NULL,
> +				   0,
> +				   KLSI_TIMEOUT,
> +				   GFP_KERNEL);
> +	if (rc) {

And again there's no need to change the control transfers without a data
stage.

Johan