| Message ID | 20200925160200.4364-2-petkan@nucleusys.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | utilize the new control message send and receive API | expand |
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
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); } static int update_eth_regs_async(pegasus_t *pegasus)