| Message ID | 20200925160200.4364-2-petkan@nucleusys.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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
On 20-09-26 07:44:57, Greg KH wrote: > 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. Didn't want to parrot the reason for usb_control_msg_send/recv() existence, but i guess you're right. 5 years from now i would not remember anything. :) > > 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 :) There's a catch - adm8511-based devices can only write to a single register via specific control request. This request expects the register number in wIndex and the value in wValue. A bit of a brain fart which is fixed in adm8515. Admittedly, I could open code set_register() and avoid a kmemdup() call since in this case 'data' is going to be NULL. However, set_register() isn't heavily used, except for the setup phase, so i went for the prettier/simpler approach. Which reminds me that i should put a comment explaining why is that. cheers, Petko
On Sat, Sep 26, 2020 at 11:21:08AM +0300, Petko Manolov wrote: > On 20-09-26 07:44:57, Greg KH wrote: > > 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. > > Didn't want to parrot the reason for usb_control_msg_send/recv() existence, but > i guess you're right. 5 years from now i would not remember anything. :) > > > > 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 :) > > There's a catch - adm8511-based devices can only write to a single register via > specific control request. This request expects the register number in wIndex > and the value in wValue. A bit of a brain fart which is fixed in adm8515. Ah, I missed that, good catch, sorry about that. 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)