On Fri, Sep 25, 2020 at 07:01:59PM +0300, Petko Manolov wrote:
> From: Petko Manolov <petko.manolov@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
> 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
