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  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]
Date:   Fri, 4 Dec 2020 10:16:07 +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 01/15] usb: serial: ark3116: use usb_control_msg_recv()
 and usb_control_msg_send()

On Wed, Nov 04, 2020 at 12:16:49PM +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/ark3116.c | 29 ++++-------------------------
>  1 file changed, 4 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/usb/serial/ark3116.c b/drivers/usb/serial/ark3116.c
> index 71a9206ea1e2..51302892c779 100644
> --- a/drivers/usb/serial/ark3116.c
> +++ b/drivers/usb/serial/ark3116.c
> @@ -77,38 +77,17 @@ struct ark3116_private {
>  static int ark3116_write_reg(struct usb_serial *serial,
>  			     unsigned reg, __u8 val)
>  {
> -	int result;
>  	 /* 0xfe 0x40 are magic values taken from original driver */
> -	result = usb_control_msg(serial->dev,
> -				 usb_sndctrlpipe(serial->dev, 0),
> -				 0xfe, 0x40, val, reg,
> -				 NULL, 0, ARK_TIMEOUT);
> -	if (result)
> -		return result;
> -
> -	return 0;
> +	return usb_control_msg_send(serial->dev, 0, 0xfe, 0x40, val, reg, NULL, 0,
> +				    ARK_TIMEOUT, GFP_KERNEL);

For control transfers without a data stage there's no point in using
usb_control_msg_send() as it already returns a negative errno on error
or 0 on success.

>  }
>  
>  static int ark3116_read_reg(struct usb_serial *serial,
>  			    unsigned reg, unsigned char *buf)
>  {
> -	int result;
>  	/* 0xfe 0xc0 are magic values taken from original driver */
> -	result = usb_control_msg(serial->dev,
> -				 usb_rcvctrlpipe(serial->dev, 0),
> -				 0xfe, 0xc0, 0, reg,
> -				 buf, 1, ARK_TIMEOUT);
> -	if (result < 1) {
> -		dev_err(&serial->interface->dev,
> -				"failed to read register %u: %d\n",
> -				reg, result);
> -		if (result >= 0)
> -			result = -EIO;
> -
> -		return result;
> -	}
> -
> -	return 0;
> +	return usb_control_msg_recv(serial->dev, 0, 0xfe, 0xc0, 0, reg, buf, 1,
> +				    ARK_TIMEOUT, GFP_KERNEL);

This driver already use a DMA-able transfer buffer which is allocated
once and then passed to this helper repeatedly. This change would
introduce additional redandant memdup + memcpy for every call and for no
real gain.

You also have an unrelated change here as you simply remove an existing
error message.

Please drop this patch.

Johan

Powered by blists - more mailing lists