On Fri, Sep 25, 2020 at 12:31:23PM +0300, Petko Manolov wrote:
> From: Petko Manolov <petko.manolov@xxxxxxxxxxxx>
>
> Signed-off-by: Petko Manolov <petko.manolov@xxxxxxxxxxxx>
I really do not like to take patches without any changelog text at all
:(
Can you fix this up?
> ---
> 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);
> }
>
This change actually fixes a number of problems where the driver never
was checking for "short reads" and so could have had "bad" data used by
the driver.
At the least you should mention that in the changelog :)
> 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);
Just call set_registers() above instead of "open coding" this?
thanks,
greg k-h
