Re: [PATCH v2] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API

[Anant Thazhemadam]

On 29/10/20 6:52 pm, Anant Thazhemadam wrote:
> Currently, __usbnet_{read|write}_cmd() use usb_control_msg(),
> and thus consider potential partial reads/writes being done to 
> be perfectly valid.
> Quite a few callers of usbnet_{read|write}_cmd() don't enforce
> checking for partial reads/writes into account either, automatically
> assuming that a complete read/write occurs.
>
> However, the new usb_control_msg_{send|recv}() APIs don't allow partial
> reads and writes.
> Using the new APIs also relaxes the return value checking that must
> be done after usbnet_{read|write}_cmd() is called.
>
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@xxxxxxxxx> <mailto:anant.thazhemadam@xxxxxxxxx>
> ---
> Changes in v2:
> 	* Fix build error
>
> This patch has been compile and build tested with a .config file that
> was generated using make allyesconfig, and the build error has been 
> fixed.
> Unfortunately, I wasn't able to get my hands on a usbnet adapter for testing,
> and would appreciate it if someone could do that.
>
>  drivers/net/usb/usbnet.c | 52 ++++++++--------------------------------
>  1 file changed, 10 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index bf6c58240bd4..2f7c7b7f4047 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1982,64 +1982,32 @@ EXPORT_SYMBOL(usbnet_link_change);
>  static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
>  			     u16 value, u16 index, void *data, u16 size)
>  {
> -	void *buf = NULL;
> -	int err = -ENOMEM;
>  
>  	netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x"
>  		   " value=0x%04x index=0x%04x size=%d\n",
>  		   cmd, reqtype, value, index, size);
>  
> -	if (size) {
> -		buf = kmalloc(size, GFP_KERNEL);
> -		if (!buf)
> -			goto out;
> -	}
> -
> -	err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> -			      cmd, reqtype, value, index, buf, size,
> -			      USB_CTRL_GET_TIMEOUT);
> -	if (err > 0 && err <= size) {
> -        if (data)
> -            memcpy(data, buf, err);
> -        else
> -            netdev_dbg(dev->net,
> -                "Huh? Data requested but thrown away.\n");
> -    }
> -	kfree(buf);
> -out:
> -	return err;
> +	return usb_control_msg_recv(dev->udev, 0,
> +			      cmd, reqtype, value, index, data, size,
> +			      USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
>  }
>  
>  static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
>  			      u16 value, u16 index, const void *data,
>  			      u16 size)
>  {
> -	void *buf = NULL;
> -	int err = -ENOMEM;
> -
>  	netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
>  		   " value=0x%04x index=0x%04x size=%d\n",
>  		   cmd, reqtype, value, index, size);
>  
> -	if (data) {
> -		buf = kmemdup(data, size, GFP_KERNEL);
> -		if (!buf)
> -			goto out;
> -	} else {
> -        if (size) {
> -            WARN_ON_ONCE(1);
> -            err = -EINVAL;
> -            goto out;
> -        }
> -    }
> -
> -	err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
> -			      cmd, reqtype, value, index, buf, size,
> -			      USB_CTRL_SET_TIMEOUT);
> -	kfree(buf);
> +	if (size && !data) {
> +		WARN_ON_ONCE(1);
> +		return -EINVAL;
> +	}
>  
> -out:
> -	return err;
> +	return usb_control_msg_send(dev->udev, 0,
> +			cmd, reqtype, value, index, data, size,
> +			USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
>  }
>  
>  /*

I had a v2 prepared and ready but was told to wait for a week before sending it in,
since usb_control_msg_{send|recv}() that were being used were not present in the
networking tree at the time, and all the trees would be converged by then.
So, just to be on the safer side, I waited for two weeks.
I checked the net tree, and found the APIs there too (defined in usb.h).

However the build seems to fail here,
    https://lists.01.org/hyperkitty/list/kbuild-all@xxxxxxxxxxxx/thread/O2BERGN7SYYC6LNOOKNUGPS2IJLDWYT7/

I'm not entirely sure at this point why this is happening, and would appreciate it if
someone could take the time to tell me if and how this might be an issue with my
patch.

Thanks,
Anant