diff mbox series

[1/2] net: pegasus: convert control messages to the new send/recv scheme.

Message ID 20200925093124.22483-2-petkan@nucleusys.com (mailing list archive)
State New, archived
Headers show
Series | expand

Commit Message

Petko Manolov Sept. 25, 2020, 9:31 a.m. UTC 
From: Petko Manolov <petko.manolov@konsulko.com>

Signed-off-by: Petko Manolov <petko.manolov@konsulko.com>
---
 drivers/net/usb/pegasus.c | 56 +++++++--------------------------------
 1 file changed, 9 insertions(+), 47 deletions(-)

Comments

Greg KH Sept. 25, 2020, 2:37 p.m. UTC | #1 
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
Petko Manolov Sept. 25, 2020, 4:05 p.m. UTC | #2 
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
Greg KH Sept. 26, 2020, 5:45 a.m. UTC | #3 
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 mbox series

Patch

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)