[patch] e1000e, igbvf: fix default message level

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [patch] e1000e, igbvf: fix default message level
@ 2012-03-10  8:49 Dan Carpenter
  2012-03-10  9:02 ` Jeff Kirsher
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Dan Carpenter @ 2012-03-10  8:49 UTC (permalink / raw)
  To: Jeff Kirsher
  Cc: Jesse Brandeburg, Bruce Allan, Carolyn Wyborny, Don Skidmore,
	Greg Rose, Peter P Waskiewicz Jr, Alex Duyck, John Ronciak,
	David S. Miller, e1000-devel, netdev, kernel-janitors

The intent here was to enable both NETIF_MSG_DRV and NETIF_MSG_PROBE
messages, but in the original code only the NETIF_MSG_DRV bit was set.

NETIF_MSG_DRV and NETIF_MSG_PROBE are bits 0 and 1, they are not
supposed to be used to do a shift.  I think the confusion is because
the msg_enable can also be controlled through ethtool which passes a bit
number that is used to do a bit shift.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index 217c143..e50c2ec 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -2649,7 +2649,7 @@ static int __devinit igbvf_probe(struct pci_dev *pdev,
 	adapter->flags = ei->flags;
 	adapter->hw.back = adapter;
 	adapter->hw.mac.type = ei->mac;
-	adapter->msg_enable = (1 << NETIF_MSG_DRV | NETIF_MSG_PROBE) - 1;
+	adapter->msg_enable = NETIF_MSG_DRV | NETIF_MSG_PROBE;
 
 	/* PCI config space info */
 
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 7152eb1..0e2de0b 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6172,7 +6172,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
 	adapter->hw.adapter = adapter;
 	adapter->hw.mac.type = ei->mac;
 	adapter->max_hw_frame_size = ei->max_hw_frame_size;
-	adapter->msg_enable = (1 << NETIF_MSG_DRV | NETIF_MSG_PROBE) - 1;
+	adapter->msg_enable = NETIF_MSG_DRV | NETIF_MSG_PROBE;
 
 	mmio_start = pci_resource_start(pdev, 0);
 	mmio_len = pci_resource_len(pdev, 0);

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [patch] e1000e, igbvf: fix default message level
  2012-03-10  8:49 [patch] e1000e, igbvf: fix default message level Dan Carpenter
@ 2012-03-10  9:02 ` Jeff Kirsher
  2012-03-10 23:06 ` Ben Hutchings
  2012-03-11  0:01 ` [PATCH] intel: make wired ethernet driver message level consistent Stephen Hemminger
  2 siblings, 0 replies; 8+ messages in thread
From: Jeff Kirsher @ 2012-03-10  9:02 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jesse Brandeburg, Bruce Allan, Carolyn Wyborny, Don Skidmore,
	Greg Rose, Peter P Waskiewicz Jr, Alex Duyck, John Ronciak,
	David S. Miller, e1000-devel, netdev, kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

On Sat, 2012-03-10 at 11:49 +0300, Dan Carpenter wrote:
> The intent here was to enable both NETIF_MSG_DRV and NETIF_MSG_PROBE
> messages, but in the original code only the NETIF_MSG_DRV bit was set.
> 
> NETIF_MSG_DRV and NETIF_MSG_PROBE are bits 0 and 1, they are not
> supposed to be used to do a shift.  I think the confusion is because
> the msg_enable can also be controlled through ethtool which passes a
> bit
> number that is used to do a bit shift.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> 

Thanks Dan!  I will add the patch to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [patch] e1000e, igbvf: fix default message level
  2012-03-10  8:49 [patch] e1000e, igbvf: fix default message level Dan Carpenter
  2012-03-10  9:02 ` Jeff Kirsher
@ 2012-03-10 23:06 ` Ben Hutchings
  2012-03-11  0:01 ` [PATCH] intel: make wired ethernet driver message level consistent Stephen Hemminger
  2 siblings, 0 replies; 8+ messages in thread
From: Ben Hutchings @ 2012-03-10 23:06 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jeff Kirsher, Jesse Brandeburg, Bruce Allan, Carolyn Wyborny,
	Don Skidmore, Greg Rose, Peter P Waskiewicz Jr, Alex Duyck,
	John Ronciak, David S. Miller, e1000-devel, netdev,
	kernel-janitors

On Sat, 2012-03-10 at 11:49 +0300, Dan Carpenter wrote:
> The intent here was to enable both NETIF_MSG_DRV and NETIF_MSG_PROBE
> messages, but in the original code only the NETIF_MSG_DRV bit was set.
> 
> NETIF_MSG_DRV and NETIF_MSG_PROBE are bits 0 and 1, they are not
> supposed to be used to do a shift.  I think the confusion is because
> the msg_enable can also be controlled through ethtool which passes a bit
> number that is used to do a bit shift.
[...]

No, the ethtool interface also uses a mask.

However netif_msg_init() is commonly used to initialise the msg_enable
mask based on a module parameter that's a bit number.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] intel: make wired ethernet driver message level consistent
  2012-03-10  8:49 [patch] e1000e, igbvf: fix default message level Dan Carpenter
  2012-03-10  9:02 ` Jeff Kirsher
  2012-03-10 23:06 ` Ben Hutchings
@ 2012-03-11  0:01 ` Stephen Hemminger
  2012-03-11  0:38   ` Ben Hutchings
  2 siblings, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2012-03-11  0:01 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jeff Kirsher, Jesse Brandeburg, Bruce Allan, Carolyn Wyborny,
	Don Skidmore, Greg Rose, Peter P Waskiewicz Jr, Alex Duyck,
	John Ronciak, David S. Miller, e1000-devel, netdev,
	kernel-janitors

Dan Carpenter noticed that ixgbevf initial default was different than
the rest. But the problem is broader than that, only one Intel driver (ixgb)
was doing it right.

The convention for default debug level should be consistent among
Intel drivers and follow established convention.

Compile tested only.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
Supersedes patch from Dan Carpenter

 drivers/net/ethernet/intel/e1000/e1000_main.c     |    5 +++--
 drivers/net/ethernet/intel/e1000e/netdev.c        |    7 ++++++-
 drivers/net/ethernet/intel/igb/igb_main.c         |    7 ++++++-
 drivers/net/ethernet/intel/igbvf/netdev.c         |    7 ++++++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    7 ++++++-
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    8 +++++++-
 6 files changed, 34 insertions(+), 7 deletions(-)

--- a/drivers/net/ethernet/intel/e1000/e1000_main.c	2012-02-27 08:43:02.348936997 -0800
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c	2012-03-10 15:50:57.199452804 -0800
@@ -215,7 +215,8 @@ MODULE_DESCRIPTION("Intel(R) PRO/1000 Ne
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
-static int debug = NETIF_MSG_DRV | NETIF_MSG_PROBE;
+#define DEFAULT_DEBUG_LEVEL_SHIFT 3
+static int debug = DEFAULT_DEBUG_LEVEL_SHIFT;
 module_param(debug, int, 0);
 MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 
@@ -979,7 +980,7 @@ static int __devinit e1000_probe(struct
 	adapter = netdev_priv(netdev);
 	adapter->netdev = netdev;
 	adapter->pdev = pdev;
-	adapter->msg_enable = (1 << debug) - 1;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_DEBUG_LEVEL_SHIFT);
 	adapter->bars = bars;
 	adapter->need_ioport = need_ioport;
 
--- a/drivers/net/ethernet/intel/e1000e/netdev.c	2012-03-04 21:12:14.059108802 -0800
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c	2012-03-10 15:51:03.759522110 -0800
@@ -60,6 +60,11 @@
 char e1000e_driver_name[] = "e1000e";
 const char e1000e_driver_version[] = DRV_VERSION;
 
+#define DEFAULT_DEBUG_LEVEL_SHIFT 3
+static int debug = DEFAULT_DEBUG_LEVEL_SHIFT;
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
+
 static void e1000e_disable_aspm(struct pci_dev *pdev, u16 state);
 
 static const struct e1000_info *e1000_info_tbl[] = {
@@ -6175,7 +6180,7 @@ static int __devinit e1000_probe(struct
 	adapter->hw.adapter = adapter;
 	adapter->hw.mac.type = ei->mac;
 	adapter->max_hw_frame_size = ei->max_hw_frame_size;
-	adapter->msg_enable = (1 << NETIF_MSG_DRV | NETIF_MSG_PROBE) - 1;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_DEBUG_LEVEL_SHIFT);
 
 	mmio_start = pci_resource_start(pdev, 0);
 	mmio_len = pci_resource_len(pdev, 0);
--- a/drivers/net/ethernet/intel/igb/igb_main.c	2012-02-13 09:23:53.622447001 -0800
+++ b/drivers/net/ethernet/intel/igb/igb_main.c	2012-03-10 15:51:07.295559468 -0800
@@ -238,6 +238,11 @@ MODULE_DESCRIPTION("Intel(R) Gigabit Eth
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
+#define DEFAULT_DEBUG_LEVEL_SHIFT 3
+static int debug = DEFAULT_DEBUG_LEVEL_SHIFT;
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
+
 struct igb_reg_info {
 	u32 ofs;
 	char *name;
@@ -1882,7 +1887,7 @@ static int __devinit igb_probe(struct pc
 	adapter->pdev = pdev;
 	hw = &adapter->hw;
 	hw->back = adapter;
-	adapter->msg_enable = NETIF_MSG_DRV | NETIF_MSG_PROBE;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_DEBUG_LEVEL_SHIFT);
 
 	mmio_start = pci_resource_start(pdev, 0);
 	mmio_len = pci_resource_len(pdev, 0);
--- a/drivers/net/ethernet/intel/igbvf/netdev.c	2012-02-24 10:04:00.123237028 -0800
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c	2012-03-10 15:53:41.949194426 -0800
@@ -55,6 +55,11 @@ static const char igbvf_driver_string[]
 static const char igbvf_copyright[] =
 		  "Copyright (c) 2009 - 2012 Intel Corporation.";
 
+#define DEFAULT_DEBUG_LEVEL_SHIFT 3
+static int debug = DEFAULT_DEBUG_LEVEL_SHIFT;
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
+
 static int igbvf_poll(struct napi_struct *napi, int budget);
 static void igbvf_reset(struct igbvf_adapter *);
 static void igbvf_set_interrupt_capability(struct igbvf_adapter *);
@@ -2649,7 +2654,7 @@ static int __devinit igbvf_probe(struct
 	adapter->flags = ei->flags;
 	adapter->hw.back = adapter;
 	adapter->hw.mac.type = ei->mac;
-	adapter->msg_enable = (1 << NETIF_MSG_DRV | NETIF_MSG_PROBE) - 1;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_DEBUG_LEVEL_SHIFT);
 
 	/* PCI config space info */
 
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c	2012-03-04 21:12:14.063108911 -0800
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c	2012-03-10 15:51:16.119652706 -0800
@@ -136,6 +136,11 @@ module_param(allow_unsupported_sfp, uint
 MODULE_PARM_DESC(allow_unsupported_sfp,
 		 "Allow unsupported and untested SFP+ modules on 82599-based adapters");
 
+#define DEFAULT_DEBUG_LEVEL_SHIFT 3
+static int debug = DEFAULT_DEBUG_LEVEL_SHIFT;
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
+
 MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>");
 MODULE_DESCRIPTION("Intel(R) 10 Gigabit PCI Express Network Driver");
 MODULE_LICENSE("GPL");
@@ -7638,7 +7643,7 @@ static int __devinit ixgbe_probe(struct
 	adapter->pdev = pdev;
 	hw = &adapter->hw;
 	hw->back = adapter;
-	adapter->msg_enable = (1 << DEFAULT_DEBUG_LEVEL_SHIFT) - 1;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_DEBUG_LEVEL_SHIFT);
 
 	hw->hw_addr = ioremap(pci_resource_start(pdev, 0),
 			      pci_resource_len(pdev, 0));
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c	2012-02-27 08:43:02.356936810 -0800
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c	2012-03-10 15:50:39.319263934 -0800
@@ -92,6 +92,12 @@ MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
 #define DEFAULT_DEBUG_LEVEL_SHIFT 3
+static int debug = DEFAULT_DEBUG_LEVEL_SHIFT;
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
+
+
+#define DEFAULT_DEBUG_LEVEL_SHIFT 3
 
 /* forward decls */
 static void ixgbevf_set_itr_msix(struct ixgbevf_q_vector *q_vector);
@@ -3367,7 +3373,7 @@ static int __devinit ixgbevf_probe(struc
 	adapter->pdev = pdev;
 	hw = &adapter->hw;
 	hw->back = adapter;
-	adapter->msg_enable = (1 << DEFAULT_DEBUG_LEVEL_SHIFT) - 1;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_DEBUG_LEVEL_SHIFT);
 
 	/*
 	 * call save state here in standalone driver because it relies on

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] intel: make wired ethernet driver message level consistent
  2012-03-11  0:01 ` [PATCH] intel: make wired ethernet driver message level consistent Stephen Hemminger
@ 2012-03-11  0:38   ` Ben Hutchings
  2012-03-11  0:44     ` Stephen Hemminger
  0 siblings, 1 reply; 8+ messages in thread
From: Ben Hutchings @ 2012-03-11  0:38 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: kernel-janitors, e1000-devel, Bruce Allan, Jesse Brandeburg,
	John Ronciak, Greg, netdev, David S. Miller, Dan Carpenter

On Sat, 2012-03-10 at 16:01 -0800, Stephen Hemminger wrote:
> Dan Carpenter noticed that ixgbevf initial default was different than
> the rest. But the problem is broader than that, only one Intel driver (ixgb)
> was doing it right.
> 
> The convention for default debug level should be consistent among
> Intel drivers and follow established convention.
[...]
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c	2012-02-27 08:43:02.348936997 -0800
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c	2012-03-10 15:50:57.199452804 -0800
> @@ -215,7 +215,8 @@ MODULE_DESCRIPTION("Intel(R) PRO/1000 Ne
>  MODULE_LICENSE("GPL");
>  MODULE_VERSION(DRV_VERSION);
>  
> -static int debug = NETIF_MSG_DRV | NETIF_MSG_PROBE;
> +#define DEFAULT_DEBUG_LEVEL_SHIFT 3

Wonder if this should really be 2 (enable DRV and PROBE) or 3 (enable
DRV, PROBE and LINK; equivalent to current behaviour)?

> +static int debug = DEFAULT_DEBUG_LEVEL_SHIFT;
>  module_param(debug, int, 0);
>  MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
>  
> @@ -979,7 +980,7 @@ static int __devinit e1000_probe(struct
>  	adapter = netdev_priv(netdev);
>  	adapter->netdev = netdev;
>  	adapter->pdev = pdev;
> -	adapter->msg_enable = (1 << debug) - 1;
> +	adapter->msg_enable = netif_msg_init(debug, DEFAULT_DEBUG_LEVEL_SHIFT);
[...]

This works, but not the way you intended.  The first parameter is
supposed to be a module parameter with a default of -1.  The second
parameter is supposed to be the bitmask to use when that default is not
overridden.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


------------------------------------------------------------------------------
Virtualization & Cloud Management Using Capacity Planning
Cloud computing makes use of virtualization - but cloud computing 
also focuses on allowing computing to be delivered as a service.
http://www.accelacomm.com/jaw/sfnl/114/51521223/
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] intel: make wired ethernet driver message level consistent
  2012-03-11  0:38   ` Ben Hutchings
@ 2012-03-11  0:44     ` Stephen Hemminger
  2012-03-11  9:57       ` Jeff Kirsher
  0 siblings, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2012-03-11  0:44 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: kernel-janitors, e1000-devel, Bruce Allan, Jesse Brandeburg,
	John Ronciak, netdev, Peter, David S. Miller, Dan Carpenter

On Sun, 11 Mar 2012 00:38:57 +0000
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Sat, 2012-03-10 at 16:01 -0800, Stephen Hemminger wrote:
> > Dan Carpenter noticed that ixgbevf initial default was different than
> > the rest. But the problem is broader than that, only one Intel driver (ixgb)
> > was doing it right.
> > 
> > The convention for default debug level should be consistent among
> > Intel drivers and follow established convention.
> [...]
> > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c	2012-02-27 08:43:02.348936997 -0800
> > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c	2012-03-10 15:50:57.199452804 -0800
> > @@ -215,7 +215,8 @@ MODULE_DESCRIPTION("Intel(R) PRO/1000 Ne
> >  MODULE_LICENSE("GPL");
> >  MODULE_VERSION(DRV_VERSION);
> >  
> > -static int debug = NETIF_MSG_DRV | NETIF_MSG_PROBE;
> > +#define DEFAULT_DEBUG_LEVEL_SHIFT 3
> 
> Wonder if this should really be 2 (enable DRV and PROBE) or 3 (enable
> DRV, PROBE and LINK; equivalent to current behaviour)?

That is really up to intel, the link up/down is useful, but nuisance
with lots of devices.

> > +static int debug = DEFAULT_DEBUG_LEVEL_SHIFT;
> >  module_param(debug, int, 0);
> >  MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
> >  
> > @@ -979,7 +980,7 @@ static int __devinit e1000_probe(struct
> >  	adapter = netdev_priv(netdev);
> >  	adapter->netdev = netdev;
> >  	adapter->pdev = pdev;
> > -	adapter->msg_enable = (1 << debug) - 1;
> > +	adapter->msg_enable = netif_msg_init(debug, DEFAULT_DEBUG_LEVEL_SHIFT);
> [...]
> 
> This works, but not the way you intended.  The first parameter is
> supposed to be a module parameter with a default of -1.  The second
> parameter is supposed to be the bitmask to use when that default is not
> overridden.

I'll fix that.



------------------------------------------------------------------------------
Virtualization & Cloud Management Using Capacity Planning
Cloud computing makes use of virtualization - but cloud computing 
also focuses on allowing computing to be delivered as a service.
http://www.accelacomm.com/jaw/sfnl/114/51521223/
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] intel: make wired ethernet driver message level consistent
  2012-03-11  0:44     ` Stephen Hemminger
@ 2012-03-11  9:57       ` Jeff Kirsher
  2012-03-11 22:12         ` [PATCH] intel: make wired ethernet driver message level consistent (rev2) Stephen Hemminger
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff Kirsher @ 2012-03-11  9:57 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Ben Hutchings, Dan Carpenter, Jesse Brandeburg, Bruce Allan,
	Carolyn Wyborny, Don Skidmore, Greg Rose, Peter P Waskiewicz Jr,
	Alex Duyck, John Ronciak, David S. Miller, e1000-devel, netdev,
	kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 2057 bytes --]

On Sat, 2012-03-10 at 16:44 -0800, Stephen Hemminger wrote:
> On Sun, 11 Mar 2012 00:38:57 +0000
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > On Sat, 2012-03-10 at 16:01 -0800, Stephen Hemminger wrote:
> > > Dan Carpenter noticed that ixgbevf initial default was different than
> > > the rest. But the problem is broader than that, only one Intel driver (ixgb)
> > > was doing it right.
> > > 
> > > The convention for default debug level should be consistent among
> > > Intel drivers and follow established convention.
> > [...]
> > > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c	2012-02-27 08:43:02.348936997 -0800
> > > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c	2012-03-10 15:50:57.199452804 -0800
> > > @@ -215,7 +215,8 @@ MODULE_DESCRIPTION("Intel(R) PRO/1000 Ne
> > >  MODULE_LICENSE("GPL");
> > >  MODULE_VERSION(DRV_VERSION);
> > >  
> > > -static int debug = NETIF_MSG_DRV | NETIF_MSG_PROBE;
> > > +#define DEFAULT_DEBUG_LEVEL_SHIFT 3
> > 
> > Wonder if this should really be 2 (enable DRV and PROBE) or 3 (enable
> > DRV, PROBE and LINK; equivalent to current behaviour)?
> 
> That is really up to intel, the link up/down is useful, but nuisance
> with lots of devices.
> 
> > > +static int debug = DEFAULT_DEBUG_LEVEL_SHIFT;
> > >  module_param(debug, int, 0);
> > >  MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
> > >  
> > > @@ -979,7 +980,7 @@ static int __devinit e1000_probe(struct
> > >  	adapter = netdev_priv(netdev);
> > >  	adapter->netdev = netdev;
> > >  	adapter->pdev = pdev;
> > > -	adapter->msg_enable = (1 << debug) - 1;
> > > +	adapter->msg_enable = netif_msg_init(debug, DEFAULT_DEBUG_LEVEL_SHIFT);
> > [...]
> > 
> > This works, but not the way you intended.  The first parameter is
> > supposed to be a module parameter with a default of -1.  The second
> > parameter is supposed to be the bitmask to use when that default is not
> > overridden.
> 
> I'll fix that.
> 
> 

Stephen, I await you updated patch with the fix, thanks!

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] intel: make wired ethernet driver message level consistent (rev2)
  2012-03-11  9:57       ` Jeff Kirsher
@ 2012-03-11 22:12         ` Stephen Hemminger
  0 siblings, 0 replies; 8+ messages in thread
From: Stephen Hemminger @ 2012-03-11 22:12 UTC (permalink / raw)
  To: jeffrey.t.kirsher
  Cc: Ben Hutchings, Dan Carpenter, Jesse Brandeburg, Bruce Allan,
	Carolyn Wyborny, Don Skidmore, Greg Rose, Peter P Waskiewicz Jr,
	Alex Duyck, John Ronciak, David S. Miller, e1000-devel, netdev,
	kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 8024 bytes --]

Dan Carpenter noticed that ixgbevf initial default was different than
the rest. But the problem is broader than that, only one Intel driver (ixgb)
was doing it almost right.

The convention for default debug level should be consistent among
Intel drivers and follow established convention.

Compile tested only.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
rev2 - use msg mask for default as expected rather than shift

 drivers/net/ethernet/intel/e1000/e1000_main.c     |    5 +++--
 drivers/net/ethernet/intel/e1000e/netdev.c        |    7 ++++++-
 drivers/net/ethernet/intel/igb/igb_main.c         |    7 ++++++-
 drivers/net/ethernet/intel/igbvf/netdev.c         |    7 ++++++-
 drivers/net/ethernet/intel/ixgb/ixgb_main.c       |    6 +++---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c     |    9 ++++++---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |    7 +++++--
 7 files changed, 35 insertions(+), 13 deletions(-)

--- a/drivers/net/ethernet/intel/e1000/e1000_main.c	2012-02-27 08:43:02.348936997 -0800
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c	2012-03-11 15:10:04.754698066 -0700
@@ -215,7 +215,8 @@ MODULE_DESCRIPTION("Intel(R) PRO/1000 Ne
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
-static int debug = NETIF_MSG_DRV | NETIF_MSG_PROBE;
+#define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK)
+static int debug = -1;
 module_param(debug, int, 0);
 MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 
@@ -979,7 +980,7 @@ static int __devinit e1000_probe(struct
 	adapter = netdev_priv(netdev);
 	adapter->netdev = netdev;
 	adapter->pdev = pdev;
-	adapter->msg_enable = (1 << debug) - 1;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
 	adapter->bars = bars;
 	adapter->need_ioport = need_ioport;
 
--- a/drivers/net/ethernet/intel/e1000e/netdev.c	2012-03-04 21:12:14.059108802 -0800
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c	2012-03-11 15:01:15.765998170 -0700
@@ -60,6 +60,11 @@
 char e1000e_driver_name[] = "e1000e";
 const char e1000e_driver_version[] = DRV_VERSION;
 
+#define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK)
+static int debug = -1;
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
+
 static void e1000e_disable_aspm(struct pci_dev *pdev, u16 state);
 
 static const struct e1000_info *e1000_info_tbl[] = {
@@ -6175,7 +6180,7 @@ static int __devinit e1000_probe(struct
 	adapter->hw.adapter = adapter;
 	adapter->hw.mac.type = ei->mac;
 	adapter->max_hw_frame_size = ei->max_hw_frame_size;
-	adapter->msg_enable = (1 << NETIF_MSG_DRV | NETIF_MSG_PROBE) - 1;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
 
 	mmio_start = pci_resource_start(pdev, 0);
 	mmio_len = pci_resource_len(pdev, 0);
--- a/drivers/net/ethernet/intel/igb/igb_main.c	2012-02-13 09:23:53.622447001 -0800
+++ b/drivers/net/ethernet/intel/igb/igb_main.c	2012-03-11 15:01:50.950305707 -0700
@@ -238,6 +238,11 @@ MODULE_DESCRIPTION("Intel(R) Gigabit Eth
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
+#define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK)
+static int debug = -1;
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
+
 struct igb_reg_info {
 	u32 ofs;
 	char *name;
@@ -1882,7 +1887,7 @@ static int __devinit igb_probe(struct pc
 	adapter->pdev = pdev;
 	hw = &adapter->hw;
 	hw->back = adapter;
-	adapter->msg_enable = NETIF_MSG_DRV | NETIF_MSG_PROBE;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
 
 	mmio_start = pci_resource_start(pdev, 0);
 	mmio_len = pci_resource_len(pdev, 0);
--- a/drivers/net/ethernet/intel/igbvf/netdev.c	2012-02-24 10:04:00.123237028 -0800
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c	2012-03-11 15:02:55.246869631 -0700
@@ -55,6 +55,11 @@ static const char igbvf_driver_string[]
 static const char igbvf_copyright[] =
 		  "Copyright (c) 2009 - 2012 Intel Corporation.";
 
+#define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK)
+static int debug = -1;
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
+
 static int igbvf_poll(struct napi_struct *napi, int budget);
 static void igbvf_reset(struct igbvf_adapter *);
 static void igbvf_set_interrupt_capability(struct igbvf_adapter *);
@@ -2649,7 +2654,7 @@ static int __devinit igbvf_probe(struct
 	adapter->flags = ei->flags;
 	adapter->hw.back = adapter;
 	adapter->hw.mac.type = ei->mac;
-	adapter->msg_enable = (1 << NETIF_MSG_DRV | NETIF_MSG_PROBE) - 1;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
 
 	/* PCI config space info */
 
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c	2012-03-04 21:12:14.063108911 -0800
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c	2012-03-11 15:09:36.874446437 -0700
@@ -136,13 +136,16 @@ module_param(allow_unsupported_sfp, uint
 MODULE_PARM_DESC(allow_unsupported_sfp,
 		 "Allow unsupported and untested SFP+ modules on 82599-based adapters");
 
+#define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK)
+static int debug = -1;
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
+
 MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>");
 MODULE_DESCRIPTION("Intel(R) 10 Gigabit PCI Express Network Driver");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
-#define DEFAULT_DEBUG_LEVEL_SHIFT 3
-
 static void ixgbe_service_event_schedule(struct ixgbe_adapter *adapter)
 {
 	if (!test_bit(__IXGBE_DOWN, &adapter->state) &&
@@ -7638,7 +7641,7 @@ static int __devinit ixgbe_probe(struct
 	adapter->pdev = pdev;
 	hw = &adapter->hw;
 	hw->back = adapter;
-	adapter->msg_enable = (1 << DEFAULT_DEBUG_LEVEL_SHIFT) - 1;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
 
 	hw->hw_addr = ioremap(pci_resource_start(pdev, 0),
 			      pci_resource_len(pdev, 0));
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c	2012-02-27 08:43:02.356936810 -0800
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c	2012-03-11 15:09:29.186377123 -0700
@@ -91,7 +91,10 @@ MODULE_DESCRIPTION("Intel(R) 82599 Virtu
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
-#define DEFAULT_DEBUG_LEVEL_SHIFT 3
+#define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK)
+static int debug = -1;
+module_param(debug, int, 0);
+MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 
 /* forward decls */
 static void ixgbevf_set_itr_msix(struct ixgbevf_q_vector *q_vector);
@@ -3367,7 +3370,7 @@ static int __devinit ixgbevf_probe(struc
 	adapter->pdev = pdev;
 	hw = &adapter->hw;
 	hw->back = adapter;
-	adapter->msg_enable = (1 << DEFAULT_DEBUG_LEVEL_SHIFT) - 1;
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
 
 	/*
 	 * call save state here in standalone driver because it relies on
--- a/drivers/net/ethernet/intel/ixgb/ixgb_main.c	2012-02-13 09:23:53.626447048 -0800
+++ b/drivers/net/ethernet/intel/ixgb/ixgb_main.c	2012-03-11 15:04:00.263442371 -0700
@@ -134,8 +134,8 @@ MODULE_DESCRIPTION("Intel(R) PRO/10GbE N
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 
-#define DEFAULT_DEBUG_LEVEL_SHIFT 3
-static int debug = DEFAULT_DEBUG_LEVEL_SHIFT;
+#define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK)
+static int debug = -1;
 module_param(debug, int, 0);
 MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 
@@ -442,7 +442,7 @@ ixgb_probe(struct pci_dev *pdev, const s
 	adapter->netdev = netdev;
 	adapter->pdev = pdev;
 	adapter->hw.back = adapter;
-	adapter->msg_enable = netif_msg_init(debug, DEFAULT_DEBUG_LEVEL_SHIFT);
+	adapter->msg_enable = netif_msg_init(debug, DEFAULT_MSG_ENABLE);
 
 	adapter->hw.hw_addr = pci_ioremap_bar(pdev, BAR_0);
 	if (!adapter->hw.hw_addr) {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2012-03-11 22:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-10  8:49 [patch] e1000e, igbvf: fix default message level Dan Carpenter
2012-03-10  9:02 ` Jeff Kirsher
2012-03-10 23:06 ` Ben Hutchings
2012-03-11  0:01 ` [PATCH] intel: make wired ethernet driver message level consistent Stephen Hemminger
2012-03-11  0:38   ` Ben Hutchings
2012-03-11  0:44     ` Stephen Hemminger
2012-03-11  9:57       ` Jeff Kirsher
2012-03-11 22:12         ` [PATCH] intel: make wired ethernet driver message level consistent (rev2) Stephen Hemminger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).