[PATCH] e1000e: Use pr_<level> and netdev_<level>

[PATCH] e1000e: Use pr_<level> and netdev_<level>

[Jeff Kirsher]

From: Bruce Allan <bruce.w.allan@intel.com>

As an alternative to a quite large patch previously submitted by Joe
Perches to make use of kernel logging API, this patch is much less
intrusive.

Convert e_<level> to netdev_<level>
Use #define pr_fmt
Convert a few printks to pr_<level>

Cc: Joe Perches <joe@perches.com>
Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/e1000e/e1000.h  |   19 +++++--------------
 drivers/net/e1000e/netdev.c |    9 +++++----
 2 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
index 118bdf4..23ab67c 100644
--- a/drivers/net/e1000e/e1000.h
+++ b/drivers/net/e1000e/e1000.h
@@ -42,25 +42,16 @@
 
 struct e1000_info;
 
-#define e_printk(level, adapter, format, arg...) \
-	printk(level "%s: %s: " format, pci_name(adapter->pdev), \
-	       adapter->netdev->name, ## arg)
-
-#ifdef DEBUG
 #define e_dbg(format, arg...) \
-	e_printk(KERN_DEBUG , hw->adapter, format, ## arg)
-#else
-#define e_dbg(format, arg...) do { (void)(hw); } while (0)
-#endif
-
+	netdev_dbg(hw->adapter->netdev, format, ## arg)
 #define e_err(format, arg...) \
-	e_printk(KERN_ERR, adapter, format, ## arg)
+	netdev_err(adapter->netdev, format, ## arg)
 #define e_info(format, arg...) \
-	e_printk(KERN_INFO, adapter, format, ## arg)
+	netdev_info(adapter->netdev, format, ## arg)
 #define e_warn(format, arg...) \
-	e_printk(KERN_WARNING, adapter, format, ## arg)
+	netdev_warn(adapter->netdev, format, ## arg)
 #define e_notice(format, arg...) \
-	e_printk(KERN_NOTICE, adapter, format, ## arg)
+	netdev_notice(adapter->netdev, format, ## arg)
 
 
 /* Interrupt modes, as used by the IntMode parameter */
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index e1cceb6..ad4546e 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -26,6 +26,8 @@
 
 *******************************************************************************/
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/init.h>
@@ -5394,10 +5396,9 @@ static struct pci_driver e1000_driver = {
 static int __init e1000_init_module(void)
 {
 	int ret;
-	printk(KERN_INFO "%s: Intel(R) PRO/1000 Network Driver - %s\n",
-	       e1000e_driver_name, e1000e_driver_version);
-	printk(KERN_INFO "%s: Copyright (c) 1999 - 2009 Intel Corporation.\n",
-	       e1000e_driver_name);
+	pr_info("Intel(R) PRO/1000 Network Driver - %s\n",
+		e1000e_driver_version);
+	pr_info("Copyright (c) 1999 - 2009 Intel Corporation.\n");
 	ret = pci_register_driver(&e1000_driver);
 
 	return ret;

Re: [PATCH] e1000e: Use pr_<level> and netdev_<level>

[Joe Perches]

On Wed, 2010-03-24 at 15:55 -0700, Jeff Kirsher wrote:
> From: Bruce Allan <bruce.w.allan@intel.com>
> As an alternative to a quite large patch previously submitted by Joe
> Perches to make use of kernel logging API, this patch is much less
> intrusive.
[]
> diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
> index 118bdf4..23ab67c 100644
> --- a/drivers/net/e1000e/e1000.h
> +++ b/drivers/net/e1000e/e1000.h
> @@ -42,25 +42,16 @@
>  
>  struct e1000_info;
>  
> -#define e_printk(level, adapter, format, arg...) \
> -	printk(level "%s: %s: " format, pci_name(adapter->pdev), \
> -	       adapter->netdev->name, ## arg)
> -
> -#ifdef DEBUG
>  #define e_dbg(format, arg...) \
> -	e_printk(KERN_DEBUG , hw->adapter, format, ## arg)
> -#else
> -#define e_dbg(format, arg...) do { (void)(hw); } while (0)
> -#endif
> -
> +	netdev_dbg(hw->adapter->netdev, format, ## arg)
>  #define e_err(format, arg...) \
> -	e_printk(KERN_ERR, adapter, format, ## arg)
> +	netdev_err(adapter->netdev, format, ## arg)
>  #define e_info(format, arg...) \
> -	e_printk(KERN_INFO, adapter, format, ## arg)
> +	netdev_info(adapter->netdev, format, ## arg)
>  #define e_warn(format, arg...) \
> -	e_printk(KERN_WARNING, adapter, format, ## arg)
> +	netdev_warn(adapter->netdev, format, ## arg)
>  #define e_notice(format, arg...) \
> -	e_printk(KERN_NOTICE, adapter, format, ## arg)
> +	netdev_notice(adapter->netdev, format, ## arg)

Macros that use global variables are undesirable.

I think there's value in using standard mechanisms.

cheers, Joe

RE: [PATCH] e1000e: Use pr_<level> and netdev_<level>

[Allan, Bruce W]

On Wednesday, March 24, 2010 4:43 PM, Joe Perches wrote:
> On Wed, 2010-03-24 at 15:55 -0700, Jeff Kirsher wrote:
>> From: Bruce Allan <bruce.w.allan@intel.com>
>> As an alternative to a quite large patch previously submitted by Joe
>> Perches to make use of kernel logging API, this patch is much less
>> intrusive.
> []
>> diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
>> index 118bdf4..23ab67c 100644 --- a/drivers/net/e1000e/e1000.h
>> +++ b/drivers/net/e1000e/e1000.h
>> @@ -42,25 +42,16 @@
>> 
>>  struct e1000_info;
>> 
>> -#define e_printk(level, adapter, format, arg...) \
>> -	printk(level "%s: %s: " format, pci_name(adapter->pdev), \
>> -	       adapter->netdev->name, ## arg)
>> -
>> -#ifdef DEBUG
>>  #define e_dbg(format, arg...) \
>> -	e_printk(KERN_DEBUG , hw->adapter, format, ## arg)
>> -#else
>> -#define e_dbg(format, arg...) do { (void)(hw); } while (0)
>> -#endif
>> -
>> +	netdev_dbg(hw->adapter->netdev, format, ## arg)
>>  #define e_err(format, arg...) \
>> -	e_printk(KERN_ERR, adapter, format, ## arg)
>> +	netdev_err(adapter->netdev, format, ## arg)
>>  #define e_info(format, arg...) \
>> -	e_printk(KERN_INFO, adapter, format, ## arg)
>> +	netdev_info(adapter->netdev, format, ## arg)
>>  #define e_warn(format, arg...) \
>> -	e_printk(KERN_WARNING, adapter, format, ## arg)
>> +	netdev_warn(adapter->netdev, format, ## arg)
>>  #define e_notice(format, arg...) \
>> -	e_printk(KERN_NOTICE, adapter, format, ## arg)
>> +	netdev_notice(adapter->netdev, format, ## arg)
> 
> Macros that use global variables are undesirable.
> 
> I think there's value in using standard mechanisms.
> 
> cheers, Joe

No global variable is used - both adapter and hw are either a local variable or a passed in parameter for all functions that use these macros.

RE: [PATCH] e1000e: Use pr_<level> and netdev_<level>

[Joe Perches]

On Wed, 2010-03-24 at 16:54 -0700, Allan, Bruce W wrote:
> On Wednesday, March 24, 2010 4:43 PM, Joe Perches wrote:
> > On Wed, 2010-03-24 at 15:55 -0700, Jeff Kirsher wrote:
> >> From: Bruce Allan <bruce.w.allan@intel.com>
> >> As an alternative to a quite large patch previously submitted by Joe
> >> Perches to make use of kernel logging API, this patch is much less
> >> intrusive.
> > []
> >> diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
> >> index 118bdf4..23ab67c 100644 --- a/drivers/net/e1000e/e1000.h
> >> +++ b/drivers/net/e1000e/e1000.h
> >> @@ -42,25 +42,16 @@
> >> 
> >>  struct e1000_info;
> >> 
> >> -#define e_printk(level, adapter, format, arg...) \
> >> -	printk(level "%s: %s: " format, pci_name(adapter->pdev), \
> >> -	       adapter->netdev->name, ## arg)
> >> -
> >> -#ifdef DEBUG
> >>  #define e_dbg(format, arg...) \
> >> -	e_printk(KERN_DEBUG , hw->adapter, format, ## arg)
> >> -#else
> >> -#define e_dbg(format, arg...) do { (void)(hw); } while (0)
> >> -#endif
> >> -
> >> +	netdev_dbg(hw->adapter->netdev, format, ## arg)
> >>  #define e_err(format, arg...) \
> >> -	e_printk(KERN_ERR, adapter, format, ## arg)
> >> +	netdev_err(adapter->netdev, format, ## arg)
> >>  #define e_info(format, arg...) \
> >> -	e_printk(KERN_INFO, adapter, format, ## arg)
> >> +	netdev_info(adapter->netdev, format, ## arg)
> >>  #define e_warn(format, arg...) \
> >> -	e_printk(KERN_WARNING, adapter, format, ## arg)
> >> +	netdev_warn(adapter->netdev, format, ## arg)
> >>  #define e_notice(format, arg...) \
> >> -	e_printk(KERN_NOTICE, adapter, format, ## arg)
> >> +	netdev_notice(adapter->netdev, format, ## arg)
> > 
> > Macros that use global variables are undesirable.
> > I think there's value in using standard mechanisms.
> > 
> No global variable is used - both adapter and hw are either a local
> variable or a passed in parameter for all functions that use these macros.

hw and adapter are locally scoped in each function here,
but are not local to the macro.  That can make conversions
of macros to functions difficult.

It's your code, not mine.
Proving the conversion correct is pretty trivial.
Do what's best.

cheers, Joe

Re: [PATCH] e1000e: Use pr_<level> and netdev_<level>

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Wed, 24 Mar 2010 17:04:34 -0700

> hw and adapter are locally scoped in each function here,
> but are not local to the macro.  That can make conversions
> of macros to functions difficult.

This is pretty common practice in drivers though.

Especially for register accessors and whatnot, and I see no problem
doing it here.

All specifying these args explicitly will do is make the fingers move
one step closer to carpel tunnel. :-)

Re: [PATCH] e1000e: Use pr_<level> and netdev_<level>

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Wed, 24 Mar 2010 15:55:30 -0700

> From: Bruce Allan <bruce.w.allan@intel.com>
> 
> As an alternative to a quite large patch previously submitted by Joe
> Perches to make use of kernel logging API, this patch is much less
> intrusive.
> 
> Convert e_<level> to netdev_<level>
> Use #define pr_fmt
> Convert a few printks to pr_<level>
> 
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied to net-next-2.6

[PATCH] e1000e: typo corrections

[Joe Perches]

Here are the other miscellaneous corrections
done by an earlier larger suggested patch now
made unnecessary by a less invasive change.

Correct a few missing newlines from logging
messages and a typo fix.  Fix speed/duplex
logging message.

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/net/e1000e/82571.c   |    2 +-
 drivers/net/e1000e/ich8lan.c |    6 +++---
 drivers/net/e1000e/lib.c     |   21 +++++++++------------
 3 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/net/e1000e/82571.c b/drivers/net/e1000e/82571.c
index 712ccc6..4b0016d 100644
--- a/drivers/net/e1000e/82571.c
+++ b/drivers/net/e1000e/82571.c
@@ -323,7 +323,7 @@ static s32 e1000_init_mac_params_82571(struct e1000_adapter *adapter)
 	}
 
 	/*
-	 * Initialze device specific counter of SMBI acquisition
+	 * Initialize device specific counter of SMBI acquisition
 	 * timeouts.
 	 */
 	 hw->dev_spec.e82571.smb_counter = 0;
diff --git a/drivers/net/e1000e/ich8lan.c b/drivers/net/e1000e/ich8lan.c
index 8b5e157..5059c22 100644
--- a/drivers/net/e1000e/ich8lan.c
+++ b/drivers/net/e1000e/ich8lan.c
@@ -1622,7 +1622,7 @@ static s32 e1000_flash_cycle_init_ich8lan(struct e1000_hw *hw)
 	/* Check if the flash descriptor is valid */
 	if (hsfsts.hsf_status.fldesvalid == 0) {
 		e_dbg("Flash descriptor invalid.  "
-			 "SW Sequencing must be used.");
+			 "SW Sequencing must be used.\n");
 		return -E1000_ERR_NVM;
 	}
 
@@ -1671,7 +1671,7 @@ static s32 e1000_flash_cycle_init_ich8lan(struct e1000_hw *hw)
 			hsfsts.hsf_status.flcdone = 1;
 			ew16flash(ICH_FLASH_HSFSTS, hsfsts.regval);
 		} else {
-			e_dbg("Flash controller busy, cannot get access");
+			e_dbg("Flash controller busy, cannot get access\n");
 		}
 	}
 
@@ -1822,7 +1822,7 @@ static s32 e1000_read_flash_data_ich8lan(struct e1000_hw *hw, u32 offset,
 				continue;
 			} else if (hsfsts.hsf_status.flcdone == 0) {
 				e_dbg("Timeout error - flash cycle "
-					 "did not complete.");
+					 "did not complete.\n");
 				break;
 			}
 		}
diff --git a/drivers/net/e1000e/lib.c b/drivers/net/e1000e/lib.c
index a8b2c0d..b0d2a60 100644
--- a/drivers/net/e1000e/lib.c
+++ b/drivers/net/e1000e/lib.c
@@ -1262,24 +1262,21 @@ s32 e1000e_get_speed_and_duplex_copper(struct e1000_hw *hw, u16 *speed, u16 *dup
 	u32 status;
 
 	status = er32(STATUS);
-	if (status & E1000_STATUS_SPEED_1000) {
+	if (status & E1000_STATUS_SPEED_1000)
 		*speed = SPEED_1000;
-		e_dbg("1000 Mbs, ");
-	} else if (status & E1000_STATUS_SPEED_100) {
+	else if (status & E1000_STATUS_SPEED_100)
 		*speed = SPEED_100;
-		e_dbg("100 Mbs, ");
-	} else {
+	else
 		*speed = SPEED_10;
-		e_dbg("10 Mbs, ");
-	}
 
-	if (status & E1000_STATUS_FD) {
+	if (status & E1000_STATUS_FD)
 		*duplex = FULL_DUPLEX;
-		e_dbg("Full Duplex\n");
-	} else {
+	else
 		*duplex = HALF_DUPLEX;
-		e_dbg("Half Duplex\n");
-	}
+
+	e_dbg("%u Mbps, %s Duplex\n",
+	      *speed == SPEED_1000 ? 1000 : *speed == SPEED_100 ? 100 : 10,
+	      *duplex == FULL_DUPLEX ? "Full" : "Half");
 
 	return 0;
 }

Re: [PATCH] e1000e: typo corrections

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Fri, 26 Mar 2010 23:16:59 -0700

> Here are the other miscellaneous corrections
> done by an earlier larger suggested patch now
> made unnecessary by a less invasive change.
> 
> Correct a few missing newlines from logging
> messages and a typo fix.  Fix speed/duplex
> logging message.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

I'll apply this to net-next-2.6, thanks.