[OPW,kernel] GDM72xx checkpatch.pl fixes: Replacing printk(KERN_DEBUG) with netdev_dbg Signed-off-by: minniux <minniux@minniux.com>
diff mbox

Message ID 1381077367-10604-1-git-send-email-minniux@minniux.com
State Changes Requested
Headers show

Commit Message

minniux Oct. 6, 2013, 4:36 p.m. UTC
---
 drivers/staging/gdm72xx/gdm_sdio.c  |   14 +++++++-------
 drivers/staging/gdm72xx/gdm_usb.c   |   12 ++++++------
 drivers/staging/gdm72xx/gdm_wimax.c |   16 ++++++++--------
 drivers/staging/gdm72xx/gdm_wimax.h |    2 +-
 4 files changed, 22 insertions(+), 22 deletions(-)

Comments

Greg Kroah-Hartman Oct. 6, 2013, 7:22 p.m. UTC | #1
On Sun, Oct 06, 2013 at 06:36:07PM +0200, minniux wrote:
> ---
>  drivers/staging/gdm72xx/gdm_sdio.c  |   14 +++++++-------
>  drivers/staging/gdm72xx/gdm_usb.c   |   12 ++++++------
>  drivers/staging/gdm72xx/gdm_wimax.c |   16 ++++++++--------
>  drivers/staging/gdm72xx/gdm_wimax.h |    2 +-
>  4 files changed, 22 insertions(+), 22 deletions(-)


Nice job, but take a look at that Subject line, I think it's a bit
messed up, don't you agree?

The trick with git is you have to have an empty line after the title,
before the body of the changelog, in order for it to show up properly
when you create a patch out of it.

Also, we need a real, and full, name for the signed-off-by line, and the
 From: line in the patch.

> diff --git a/drivers/staging/gdm72xx/gdm_sdio.c b/drivers/staging/gdm72xx/gdm_sdio.c
> index 047a4d7..c1eeb30 100644
> --- a/drivers/staging/gdm72xx/gdm_sdio.c
> +++ b/drivers/staging/gdm72xx/gdm_sdio.c
> @@ -48,13 +48,13 @@ static void hexdump(char *title, u8 *data, int len)
>  {
>  	int i;
>  
> -	printk(KERN_DEBUG "%s: length = %d\n", title, len);
> +	netdev_dbg("%s: length = %d\n", title, len);
>  	for (i = 0; i < len; i++) {
> -		printk(KERN_DEBUG "%02x ", data[i]);
> +		netdev_dbg("%02x ", data[i]);
>  		if ((i & 0xf) == 0xf)
> -			printk(KERN_DEBUG "\n");
> +			netdev_dbg("\n");
>  	}
> -	printk(KERN_DEBUG "\n");
> +	netdev_dbg("\n");
>  }
>  #endif

This is a "tricky" function, and should just be deleted as the kernel
already has a function that does this.  Also, as it is, it's way wrong,
and will never output the data you think it does (all of the
KERNEL_DEBUG characters in the middle of the lines, looks like someone
(probably me) took a cleanup patch for this and didn't pay attention to
it, sorry...)

> @@ -475,12 +475,12 @@ static int control_sdu_tx_flow(struct sdiowm_dev *sdev, u8 *hci_data, int len)
>  
>  	if (hci_data[4] == 0) {
>  #ifdef DEBUG
> -		printk(KERN_DEBUG "WIMAX ==> STOP SDU TX\n");
> +		netdev_dbg("WIMAX ==> STOP SDU TX\n");
>  #endif

I don't think you compiled your patch, as all of your changes would
totally break the build.  netdev_dbg() needs a pointer to a network
device object, and isn't a drop-in-replacement for printk().

Also, if you switch to netdev_dbg() all of the #ifdef DEBUG lines around
the calls can be removed, as they would no longer be needed.

Care to fix this all up and resend it?

thanks,

greg k-h
minniux Oct. 6, 2013, 8:20 p.m. UTC | #2
I got that, will fix it and and resend the updated patch.

Thanks Greg for the feedback :)


On Sun, Oct 6, 2013 at 9:22 PM, Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sun, Oct 06, 2013 at 06:36:07PM +0200, minniux wrote:
> > ---
> >  drivers/staging/gdm72xx/gdm_sdio.c  |   14 +++++++-------
> >  drivers/staging/gdm72xx/gdm_usb.c   |   12 ++++++------
> >  drivers/staging/gdm72xx/gdm_wimax.c |   16 ++++++++--------
> >  drivers/staging/gdm72xx/gdm_wimax.h |    2 +-
> >  4 files changed, 22 insertions(+), 22 deletions(-)
>
>
> Nice job, but take a look at that Subject line, I think it's a bit
> messed up, don't you agree?
>
> The trick with git is you have to have an empty line after the title,
> before the body of the changelog, in order for it to show up properly
> when you create a patch out of it.
>
> Also, we need a real, and full, name for the signed-off-by line, and the
>  From: line in the patch.
>
> > diff --git a/drivers/staging/gdm72xx/gdm_sdio.c
> b/drivers/staging/gdm72xx/gdm_sdio.c
> > index 047a4d7..c1eeb30 100644
> > --- a/drivers/staging/gdm72xx/gdm_sdio.c
> > +++ b/drivers/staging/gdm72xx/gdm_sdio.c
> > @@ -48,13 +48,13 @@ static void hexdump(char *title, u8 *data, int len)
> >  {
> >       int i;
> >
> > -     printk(KERN_DEBUG "%s: length = %d\n", title, len);
> > +     netdev_dbg("%s: length = %d\n", title, len);
> >       for (i = 0; i < len; i++) {
> > -             printk(KERN_DEBUG "%02x ", data[i]);
> > +             netdev_dbg("%02x ", data[i]);
> >               if ((i & 0xf) == 0xf)
> > -                     printk(KERN_DEBUG "\n");
> > +                     netdev_dbg("\n");
> >       }
> > -     printk(KERN_DEBUG "\n");
> > +     netdev_dbg("\n");
> >  }
> >  #endif
>
> This is a "tricky" function, and should just be deleted as the kernel
> already has a function that does this.  Also, as it is, it's way wrong,
> and will never output the data you think it does (all of the
> KERNEL_DEBUG characters in the middle of the lines, looks like someone
> (probably me) took a cleanup patch for this and didn't pay attention to
> it, sorry...)
>
> > @@ -475,12 +475,12 @@ static int control_sdu_tx_flow(struct sdiowm_dev
> *sdev, u8 *hci_data, int len)
> >
> >       if (hci_data[4] == 0) {
> >  #ifdef DEBUG
> > -             printk(KERN_DEBUG "WIMAX ==> STOP SDU TX\n");
> > +             netdev_dbg("WIMAX ==> STOP SDU TX\n");
> >  #endif
>
> I don't think you compiled your patch, as all of your changes would
> totally break the build.  netdev_dbg() needs a pointer to a network
> device object, and isn't a drop-in-replacement for printk().
>
> Also, if you switch to netdev_dbg() all of the #ifdef DEBUG lines around
> the calls can be removed, as they would no longer be needed.
>
> Care to fix this all up and resend it?
>
> thanks,
>
> greg k-h
>
>
Josh Triplett Oct. 6, 2013, 10:07 p.m. UTC | #3
On Sun, Oct 06, 2013 at 12:22:37PM -0700, Greg KH wrote:
> On Sun, Oct 06, 2013 at 06:36:07PM +0200, minniux wrote:
> > @@ -475,12 +475,12 @@ static int control_sdu_tx_flow(struct sdiowm_dev *sdev, u8 *hci_data, int len)
> >  
> >  	if (hci_data[4] == 0) {
> >  #ifdef DEBUG
> > -		printk(KERN_DEBUG "WIMAX ==> STOP SDU TX\n");
> > +		netdev_dbg("WIMAX ==> STOP SDU TX\n");
> >  #endif
> 
> I don't think you compiled your patch, as all of your changes would
> totally break the build.  netdev_dbg() needs a pointer to a network
> device object, and isn't a drop-in-replacement for printk().

This is likely the fault of checkpatch, which blindly suggests
replacing printk(KERN_FOO with:

Prefer netdev_$level2(netdev, ... then dev_$level2(dev, ... then pr_$level(...  to printk(KERN_$orig ...

One of the OPW interns mentioned via IRC that she might try fixing this
test in checkpatch to only suggest netdev_* for files in net/ or
drivers/net/ .

- Josh Triplett
Amany El-Guindy Oct. 6, 2013, 10:35 p.m. UTC | #4
On Mon, Oct 7, 2013 at 12:07 AM, Josh Triplett <josh@joshtriplett.org>wrote:
>
> This is likely the fault of checkpatch, which blindly suggests
> replacing printk(KERN_FOO with:
>
> Prefer netdev_$level2(netdev, ... then dev_$level2(dev, ... then
> pr_$level(...  to printk(KERN_$orig ...
>
> One of the OPW interns mentioned via IRC that she might try fixing this
> test in checkpatch to only suggest netdev_* for files in net/ or
> drivers/net/ .
>
> - Josh Triplett
>
>
So, If i understand this right, replacing with dev_* would make more sense
in this case, isn't it?
Josh Triplett Oct. 6, 2013, 11:15 p.m. UTC | #5
On Mon, Oct 07, 2013 at 12:35:07AM +0200, Amany El-Guindy wrote:
> On Mon, Oct 7, 2013 at 12:07 AM, Josh Triplett <josh@joshtriplett.org>wrote:
> >
> > This is likely the fault of checkpatch, which blindly suggests
> > replacing printk(KERN_FOO with:
> >
> > Prefer netdev_$level2(netdev, ... then dev_$level2(dev, ... then
> > pr_$level(...  to printk(KERN_$orig ...
> >
> > One of the OPW interns mentioned via IRC that she might try fixing this
> > test in checkpatch to only suggest netdev_* for files in net/ or
> > drivers/net/ .
> >
> > - Josh Triplett
> >
> >
> So, If i understand this right, replacing with dev_* would make more sense
> in this case, isn't it?

Actually, in this case, it looks like there *is* a net device available
to call netdev_* with, at least in some of the functions.  You'd just
need to rework the code to pass down that device into the sub-functions
that call printk (to the extent you don't eliminate those functions as
Greg suggested).

- Josh Triplett

Patch
diff mbox

diff --git a/drivers/staging/gdm72xx/gdm_sdio.c b/drivers/staging/gdm72xx/gdm_sdio.c
index 047a4d7..c1eeb30 100644
--- a/drivers/staging/gdm72xx/gdm_sdio.c
+++ b/drivers/staging/gdm72xx/gdm_sdio.c
@@ -48,13 +48,13 @@  static void hexdump(char *title, u8 *data, int len)
 {
 	int i;
 
-	printk(KERN_DEBUG "%s: length = %d\n", title, len);
+	netdev_dbg("%s: length = %d\n", title, len);
 	for (i = 0; i < len; i++) {
-		printk(KERN_DEBUG "%02x ", data[i]);
+		netdev_dbg("%02x ", data[i]);
 		if ((i & 0xf) == 0xf)
-			printk(KERN_DEBUG "\n");
+			netdev_dbg("\n");
 	}
-	printk(KERN_DEBUG "\n");
+	netdev_dbg("\n");
 }
 #endif
 
@@ -475,12 +475,12 @@  static int control_sdu_tx_flow(struct sdiowm_dev *sdev, u8 *hci_data, int len)
 
 	if (hci_data[4] == 0) {
 #ifdef DEBUG
-		printk(KERN_DEBUG "WIMAX ==> STOP SDU TX\n");
+		netdev_dbg("WIMAX ==> STOP SDU TX\n");
 #endif
 		tx->stop_sdu_tx = 1;
 	} else if (hci_data[4] == 1) {
 #ifdef DEBUG
-		printk(KERN_DEBUG "WIMAX ==> START SDU TX\n");
+		netdev_dbg("WIMAX ==> START SDU TX\n");
 #endif
 		tx->stop_sdu_tx = 0;
 		if (tx->can_send)
@@ -542,7 +542,7 @@  static void gdm_sdio_irq(struct sdio_func *func)
 			schedule_work(&sdev->ws);
 		spin_unlock_irqrestore(&tx->lock, flags);
 #ifdef DEBUG
-		printk(KERN_DEBUG "Ack... %0x\n", ntohl(*ack_seq));
+		netdev_dbg("Ack... %0x\n", ntohl(*ack_seq));
 #endif
 		goto done;
 	}
diff --git a/drivers/staging/gdm72xx/gdm_usb.c b/drivers/staging/gdm72xx/gdm_usb.c
index e0cb2ff..7e4f698 100644
--- a/drivers/staging/gdm72xx/gdm_usb.c
+++ b/drivers/staging/gdm72xx/gdm_usb.c
@@ -61,13 +61,13 @@  static void hexdump(char *title, u8 *data, int len)
 {
 	int i;
 
-	printk(KERN_DEBUG "%s: length = %d\n", title, len);
+	netdev_dbg("%s: length = %d\n", title, len);
 	for (i = 0; i < len; i++) {
-		printk(KERN_DEBUG "%02x ", data[i]);
+		netdev_dbg("%02x ", data[i]);
 		if ((i & 0xf) == 0xf)
-			printk(KERN_DEBUG "\n");
+			netdev_dbg("\n");
 	}
-	printk(KERN_DEBUG "\n");
+	netdev_dbg("\n");
 }
 #endif
 
@@ -457,13 +457,13 @@  static void gdm_usb_rcv_complete(struct urb *urb)
 		if (cmd_evt == WIMAX_SDU_TX_FLOW) {
 			if (r->buf[4] == 0) {
 #ifdef DEBUG
-				printk(KERN_DEBUG "WIMAX ==> STOP SDU TX\n");
+				netdev_dbg("WIMAX ==> STOP SDU TX\n");
 #endif
 				list_for_each_entry(t, &tx->sdu_list, list)
 					usb_unlink_urb(t->urb);
 			} else if (r->buf[4] == 1) {
 #ifdef DEBUG
-				printk(KERN_DEBUG "WIMAX ==> START SDU TX\n");
+				netdev_dbg("WIMAX ==> START SDU TX\n");
 #endif
 				list_for_each_entry(t, &tx->sdu_list, list) {
 					usb_submit_urb(t->urb, GFP_ATOMIC);
diff --git a/drivers/staging/gdm72xx/gdm_wimax.c b/drivers/staging/gdm72xx/gdm_wimax.c
index dd85497..d411fa3 100644
--- a/drivers/staging/gdm72xx/gdm_wimax.c
+++ b/drivers/staging/gdm72xx/gdm_wimax.c
@@ -69,12 +69,12 @@  static void printk_hex(u8 *buf, u32 size)
 
 	for (i = 0; i < size; i++) {
 		if (i && i % 16 == 0)
-			printk(KERN_DEBUG "\n%02x ", *buf++);
+			netdev_dbg("\n%02x ", *buf++);
 		else
-			printk(KERN_DEBUG "%02x ", *buf++);
+			netdev_dbg("%02x ", *buf++);
 	}
 
-	printk(KERN_DEBUG "\n");
+	netdev_dbg("\n");
 }
 
 static const char *get_protocol_name(u16 protocol)
@@ -162,7 +162,7 @@  static void dump_eth_packet(const char *title, u8 *data, int len)
 		port = ntohs(uh->dest);
 	}
 
-	printk(KERN_DEBUG "[%s] len=%d, %s, %s, %s\n",
+	netdev_dbg("[%s] len=%d, %s, %s, %s\n",
 		title, len,
 		get_protocol_name(protocol),
 		get_ip_protocol_name(ip_protocol),
@@ -170,9 +170,9 @@  static void dump_eth_packet(const char *title, u8 *data, int len)
 
 	if (!(data[0] == 0xff && data[1] == 0xff)) {
 		if (protocol == ETH_P_IP) {
-			printk(KERN_DEBUG "     src=%pI4\n", &ih->saddr);
+			netdev_dbg("     src=%pI4\n", &ih->saddr);
 		} else if (protocol == ETH_P_IPV6) {
-			printk(KERN_DEBUG "     src=%pI6\n", &ih->saddr);
+			netdev_dbg("     src=%pI6\n", &ih->saddr);
 		}
 	}
 
@@ -241,7 +241,7 @@  static void gdm_wimax_event_rcv(struct net_device *dev, u16 type, void *msg,
 	u8 *buf = (u8 *) msg;
 	u16 hci_cmd =  (buf[0]<<8) | buf[1];
 	u16 hci_len = (buf[2]<<8) | buf[3];
-	printk(KERN_DEBUG "H=>D: 0x%04x(%d)\n", hci_cmd, hci_len);
+	netdev_dbg("H=>D: 0x%04x(%d)\n", hci_cmd, hci_len);
 	#endif
 
 	gdm_wimax_send(nic, msg, len);
@@ -354,7 +354,7 @@  static int gdm_wimax_event_send(struct net_device *dev, char *buf, int size)
 	#if defined(DEBUG_HCI)
 	u16 hci_cmd =  ((u8)buf[0]<<8) | (u8)buf[1];
 	u16 hci_len = ((u8)buf[2]<<8) | (u8)buf[3];
-	printk(KERN_DEBUG "D=>H: 0x%04x(%d)\n", hci_cmd, hci_len);
+	netdev_dbg("D=>H: 0x%04x(%d)\n", hci_cmd, hci_len);
 	#endif
 
 	spin_lock_irqsave(&wm_event.evt_lock, flags);
diff --git a/drivers/staging/gdm72xx/gdm_wimax.h b/drivers/staging/gdm72xx/gdm_wimax.h
index 6ec0ab4..eff8b4f 100644
--- a/drivers/staging/gdm72xx/gdm_wimax.h
+++ b/drivers/staging/gdm72xx/gdm_wimax.h
@@ -64,7 +64,7 @@  struct nic {
 
 
 #if 0
-#define dprintk(fmt, args ...)	printk(KERN_DEBUG " [GDM] " fmt, ## args)
+#define dprintk(fmt, args ...)	netdev_dbg(" [GDM] " fmt, ## args)
 #else
 #define dprintk(...)
 #endif