Re: [PATCH 1/2] net: pegasus: convert control messages to the new send/recv scheme.

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Sep 25, 2020 at 07:01:59PM +0300, Petko Manolov wrote:
> From: Petko Manolov <petko.manolov@konsulko.com>
> 
> Move all control transfers to safer usb_control_msg_send/recv() API.

This says _what_ the patch does, but we can see that from the diff.  You
should describe _why_ we are doing what we are doing, so that everyone
understands the need for the change.

Also, can you add something like:
	This fixes a number of issues where short reads were not
	properly handled by the driver.

Take a look at "The canonical patch format" in the kernel file,
Documentation/SubmittingPatches for a description of how to write good
changelogs that we can understand 5 years in the future when we have to
look back at the files.

> Signed-off-by: Petko Manolov <petko.manolov@konsulko.com>
> ---
>  drivers/net/usb/pegasus.c | 56 +++++++--------------------------------
>  1 file changed, 9 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index e92cb51a2c77..0ecc1eb2e71b 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -124,62 +124,24 @@ static void async_ctrl_callback(struct urb *urb)
>  
>  static int get_registers(pegasus_t *pegasus, __u16 indx, __u16 size, void *data)
>  {
> -	u8 *buf;
> -	int ret;
> -
> -	buf = kmalloc(size, GFP_NOIO);
> -	if (!buf)
> -		return -ENOMEM;
> -
> -	ret = usb_control_msg(pegasus->usb, usb_rcvctrlpipe(pegasus->usb, 0),
> -			      PEGASUS_REQ_GET_REGS, PEGASUS_REQT_READ, 0,
> -			      indx, buf, size, 1000);
> -	if (ret < 0)
> -		netif_dbg(pegasus, drv, pegasus->net,
> -			  "%s returned %d\n", __func__, ret);
> -	else if (ret <= size)
> -		memcpy(data, buf, ret);
> -	kfree(buf);
> -	return ret;
> +	return usb_control_msg_recv(pegasus->usb, 0, PEGASUS_REQ_GET_REGS,
> +				    PEGASUS_REQT_READ, 0, indx, data, size,
> +				    1000, GFP_NOIO);
>  }
>  
>  static int set_registers(pegasus_t *pegasus, __u16 indx, __u16 size,
>  			 const void *data)
>  {
> -	u8 *buf;
> -	int ret;
> -
> -	buf = kmemdup(data, size, GFP_NOIO);
> -	if (!buf)
> -		return -ENOMEM;
> -
> -	ret = usb_control_msg(pegasus->usb, usb_sndctrlpipe(pegasus->usb, 0),
> -			      PEGASUS_REQ_SET_REGS, PEGASUS_REQT_WRITE, 0,
> -			      indx, buf, size, 100);
> -	if (ret < 0)
> -		netif_dbg(pegasus, drv, pegasus->net,
> -			  "%s returned %d\n", __func__, ret);
> -	kfree(buf);
> -	return ret;
> +	return usb_control_msg_send(pegasus->usb, 0, PEGASUS_REQ_SET_REGS,
> +			      PEGASUS_REQT_WRITE, 0, indx, data, size, 1000,
> +			      GFP_NOIO);
>  }
>  
>  static int set_register(pegasus_t *pegasus, __u16 indx, __u8 data)
>  {
> -	u8 *buf;
> -	int ret;
> -
> -	buf = kmemdup(&data, 1, GFP_NOIO);
> -	if (!buf)
> -		return -ENOMEM;
> -
> -	ret = usb_control_msg(pegasus->usb, usb_sndctrlpipe(pegasus->usb, 0),
> -			      PEGASUS_REQ_SET_REG, PEGASUS_REQT_WRITE, data,
> -			      indx, buf, 1, 1000);
> -	if (ret < 0)
> -		netif_dbg(pegasus, drv, pegasus->net,
> -			  "%s returned %d\n", __func__, ret);
> -	kfree(buf);
> -	return ret;
> +	return usb_control_msg_send(pegasus->usb, 0, PEGASUS_REQ_SET_REG,
> +			            PEGASUS_REQT_WRITE, data, indx, &data, 1,
> +				    1000, GFP_NOIO);
>  }

Again, why isn't set_register() now rewritten to just be:

static int set_register(pegasus_t *pegasus, __u16 indx, __u8 data)
{
	return set_registers(pegasus, indx, 1, data);
}

Much easier to show that it's all converted properly :)

thanks,

greg k-h