| Message ID | 20200925093124.22483-2-petkan@nucleusys.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] net: pegasus: convert control messages to the new send/recv scheme. | expand |
On Fri, Sep 25, 2020 at 12:31:23PM +0300, Petko Manolov wrote: > From: Petko Manolov <petko.manolov@konsulko.com> > > Signed-off-by: Petko Manolov <petko.manolov@konsulko.com> 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
On 20-09-25 16:37:30, Greg KH wrote: > On Fri, Sep 25, 2020 at 12:31:23PM +0300, Petko Manolov wrote: > > From: Petko Manolov <petko.manolov@konsulko.com> > > > > Signed-off-by: Petko Manolov <petko.manolov@konsulko.com> > > I really do not like to take patches without any changelog text at all > :( > > Can you fix this up? I am sorry about this. Hope v2 is better. cheers, Petko
On Fri, Sep 25, 2020 at 07:05:37PM +0300, Petko Manolov wrote: > On 20-09-25 16:37:30, Greg KH wrote: > > On Fri, Sep 25, 2020 at 12:31:23PM +0300, Petko Manolov wrote: > > > From: Petko Manolov <petko.manolov@konsulko.com> > > > > > > Signed-off-by: Petko Manolov <petko.manolov@konsulko.com> > > > > I really do not like to take patches without any changelog text at all > > :( > > > > Can you fix this up? > > I am sorry about this. Hope v2 is better. Better, but still a bit more work is needed :) 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)