[PATCH net-next 0/2] i40e: clean-up and fix for the i40e debug code

[PATCH net-next 0/2] i40e: clean-up and fix for the i40e debug code

[Stefan Assmann]

Stefan Assmann (2):
  i40e: remove superfluous I40E_DEBUG_USER statement
  i40e: fix setting debug parameter early

 drivers/net/ethernet/intel/i40e/i40e_common.c  |  3 --
 drivers/net/ethernet/intel/i40e/i40e_debugfs.c |  6 ----
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  2 --
 drivers/net/ethernet/intel/i40e/i40e_main.c    | 43 ++++++++++++--------------
 drivers/net/ethernet/intel/i40e/i40e_type.h    |  2 --
 5 files changed, 20 insertions(+), 36 deletions(-)

-- 
2.7.4

[Intel-wired-lan] [PATCH net-next 0/2] i40e: clean-up and fix for the i40e debug code

[Stefan Assmann]

Stefan Assmann (2):
  i40e: remove superfluous I40E_DEBUG_USER statement
  i40e: fix setting debug parameter early

 drivers/net/ethernet/intel/i40e/i40e_common.c  |  3 --
 drivers/net/ethernet/intel/i40e/i40e_debugfs.c |  6 ----
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  2 --
 drivers/net/ethernet/intel/i40e/i40e_main.c    | 43 ++++++++++++--------------
 drivers/net/ethernet/intel/i40e/i40e_type.h    |  2 --
 5 files changed, 20 insertions(+), 36 deletions(-)

-- 
2.7.4

[PATCH net-next 1/2] i40e: remove superfluous I40E_DEBUG_USER statement

[Stefan Assmann]

This debug statement is confusing and never set in the code. Any debug
output should be guarded by the proper I40E_DEBUG_* statement which can
be enabled via the debug module parameter.
Remove or convert the I40E_DEBUG_USER cases to I40E_DEBUG_INIT.

Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
---
 drivers/net/ethernet/intel/i40e/i40e_common.c  |  3 ---
 drivers/net/ethernet/intel/i40e/i40e_debugfs.c |  6 -----
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  2 --
 drivers/net/ethernet/intel/i40e/i40e_main.c    | 35 +++++++++++++-------------
 drivers/net/ethernet/intel/i40e/i40e_type.h    |  2 --
 5 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index 2154a34..8ccb09c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -3207,9 +3207,6 @@ static void i40e_parse_discover_capabilities(struct i40e_hw *hw, void *buff,
 			break;
 		case I40E_AQ_CAP_ID_MSIX:
 			p->num_msix_vectors = number;
-			i40e_debug(hw, I40E_DEBUG_INIT,
-				   "HW Capability: MSIX vector count = %d\n",
-				   p->num_msix_vectors);
 			break;
 		case I40E_AQ_CAP_ID_VF_MSIX:
 			p->num_msix_vectors_vf = number;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
index 05cf9a7..e9c6f1c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
@@ -1210,12 +1210,6 @@ static ssize_t i40e_dbg_command_write(struct file *filp,
 		u32 level;
 		cnt = sscanf(&cmd_buf[10], "%i", &level);
 		if (cnt) {
-			if (I40E_DEBUG_USER & level) {
-				pf->hw.debug_mask = level;
-				dev_info(&pf->pdev->dev,
-					 "set hw.debug_mask = 0x%08x\n",
-					 pf->hw.debug_mask);
-			}
 			pf->msg_enable = level;
 			dev_info(&pf->pdev->dev, "set msg_enable = 0x%08x\n",
 				 pf->msg_enable);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 1835186..c56877c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -987,8 +987,6 @@ static void i40e_set_msglevel(struct net_device *netdev, u32 data)
 	struct i40e_netdev_priv *np = netdev_priv(netdev);
 	struct i40e_pf *pf = np->vsi->back;
 
-	if (I40E_DEBUG_USER & data)
-		pf->hw.debug_mask = data;
 	pf->msg_enable = data;
 }
 
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 61b0fc4..56369761 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -6665,16 +6665,19 @@ static int i40e_get_capabilities(struct i40e_pf *pf)
 		}
 	} while (err);
 
-	if (pf->hw.debug_mask & I40E_DEBUG_USER)
-		dev_info(&pf->pdev->dev,
-			 "pf=%d, num_vfs=%d, msix_pf=%d, msix_vf=%d, fd_g=%d, fd_b=%d, pf_max_q=%d num_vsi=%d\n",
-			 pf->hw.pf_id, pf->hw.func_caps.num_vfs,
-			 pf->hw.func_caps.num_msix_vectors,
-			 pf->hw.func_caps.num_msix_vectors_vf,
-			 pf->hw.func_caps.fd_filters_guaranteed,
-			 pf->hw.func_caps.fd_filters_best_effort,
-			 pf->hw.func_caps.num_tx_qp,
-			 pf->hw.func_caps.num_vsis);
+	i40e_debug(&pf->hw, I40E_DEBUG_INIT,
+		   "HW Capabilities: PF-id[%d] num_vfs=%d, msix_pf=%d, msix_vf=%d\n",
+		   pf->hw.pf_id,
+		   pf->hw.func_caps.num_vfs,
+		   pf->hw.func_caps.num_msix_vectors,
+		   pf->hw.func_caps.num_msix_vectors_vf);
+	i40e_debug(&pf->hw, I40E_DEBUG_INIT,
+		   "HW Capabilities: PF-id[%d] fd_g=%d, fd_b=%d, pf_max_qp=%d num_vsis=%d\n",
+		   pf->hw.pf_id,
+		   pf->hw.func_caps.fd_filters_guaranteed,
+		   pf->hw.func_caps.fd_filters_best_effort,
+		   pf->hw.func_caps.num_tx_qp,
+		   pf->hw.func_caps.num_vsis);
 
 #define DEF_NUM_VSI (1 + (pf->hw.func_caps.fcoe ? 1 : 0) \
 		       + pf->hw.func_caps.num_vfs)
@@ -8495,14 +8498,10 @@ static int i40e_sw_init(struct i40e_pf *pf)
 	int err = 0;
 	int size;
 
-	pf->msg_enable = netif_msg_init(I40E_DEFAULT_MSG_ENABLE,
-				(NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK));
-	if (debug != -1 && debug != I40E_DEFAULT_MSG_ENABLE) {
-		if (I40E_DEBUG_USER & debug)
-			pf->hw.debug_mask = debug;
-		pf->msg_enable = netif_msg_init((debug & ~I40E_DEBUG_USER),
-						I40E_DEFAULT_MSG_ENABLE);
-	}
+	pf->msg_enable = netif_msg_init(debug,
+					NETIF_MSG_DRV    |
+					NETIF_MSG_PROBE  |
+					NETIF_MSG_LINK);
 
 	/* Set default capability flags */
 	pf->flags = I40E_FLAG_RX_CSUM_ENABLED |
diff --git a/drivers/net/ethernet/intel/i40e/i40e_type.h b/drivers/net/ethernet/intel/i40e/i40e_type.h
index bd5f13b..7e88e35 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_type.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
@@ -85,8 +85,6 @@ enum i40e_debug_mask {
 	I40E_DEBUG_AQ_COMMAND		= 0x06000000,
 	I40E_DEBUG_AQ			= 0x0F000000,
 
-	I40E_DEBUG_USER			= 0xF0000000,
-
 	I40E_DEBUG_ALL			= 0xFFFFFFFF
 };
 
-- 
2.7.4

[Intel-wired-lan] [PATCH net-next 1/2] i40e: remove superfluous I40E_DEBUG_USER statement

[Stefan Assmann]

This debug statement is confusing and never set in the code. Any debug
output should be guarded by the proper I40E_DEBUG_* statement which can
be enabled via the debug module parameter.
Remove or convert the I40E_DEBUG_USER cases to I40E_DEBUG_INIT.

Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
---
 drivers/net/ethernet/intel/i40e/i40e_common.c  |  3 ---
 drivers/net/ethernet/intel/i40e/i40e_debugfs.c |  6 -----
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  2 --
 drivers/net/ethernet/intel/i40e/i40e_main.c    | 35 +++++++++++++-------------
 drivers/net/ethernet/intel/i40e/i40e_type.h    |  2 --
 5 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index 2154a34..8ccb09c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -3207,9 +3207,6 @@ static void i40e_parse_discover_capabilities(struct i40e_hw *hw, void *buff,
 			break;
 		case I40E_AQ_CAP_ID_MSIX:
 			p->num_msix_vectors = number;
-			i40e_debug(hw, I40E_DEBUG_INIT,
-				   "HW Capability: MSIX vector count = %d\n",
-				   p->num_msix_vectors);
 			break;
 		case I40E_AQ_CAP_ID_VF_MSIX:
 			p->num_msix_vectors_vf = number;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
index 05cf9a7..e9c6f1c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
@@ -1210,12 +1210,6 @@ static ssize_t i40e_dbg_command_write(struct file *filp,
 		u32 level;
 		cnt = sscanf(&cmd_buf[10], "%i", &level);
 		if (cnt) {
-			if (I40E_DEBUG_USER & level) {
-				pf->hw.debug_mask = level;
-				dev_info(&pf->pdev->dev,
-					 "set hw.debug_mask = 0x%08x\n",
-					 pf->hw.debug_mask);
-			}
 			pf->msg_enable = level;
 			dev_info(&pf->pdev->dev, "set msg_enable = 0x%08x\n",
 				 pf->msg_enable);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 1835186..c56877c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -987,8 +987,6 @@ static void i40e_set_msglevel(struct net_device *netdev, u32 data)
 	struct i40e_netdev_priv *np = netdev_priv(netdev);
 	struct i40e_pf *pf = np->vsi->back;
 
-	if (I40E_DEBUG_USER & data)
-		pf->hw.debug_mask = data;
 	pf->msg_enable = data;
 }
 
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 61b0fc4..56369761 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -6665,16 +6665,19 @@ static int i40e_get_capabilities(struct i40e_pf *pf)
 		}
 	} while (err);
 
-	if (pf->hw.debug_mask & I40E_DEBUG_USER)
-		dev_info(&pf->pdev->dev,
-			 "pf=%d, num_vfs=%d, msix_pf=%d, msix_vf=%d, fd_g=%d, fd_b=%d, pf_max_q=%d num_vsi=%d\n",
-			 pf->hw.pf_id, pf->hw.func_caps.num_vfs,
-			 pf->hw.func_caps.num_msix_vectors,
-			 pf->hw.func_caps.num_msix_vectors_vf,
-			 pf->hw.func_caps.fd_filters_guaranteed,
-			 pf->hw.func_caps.fd_filters_best_effort,
-			 pf->hw.func_caps.num_tx_qp,
-			 pf->hw.func_caps.num_vsis);
+	i40e_debug(&pf->hw, I40E_DEBUG_INIT,
+		   "HW Capabilities: PF-id[%d] num_vfs=%d, msix_pf=%d, msix_vf=%d\n",
+		   pf->hw.pf_id,
+		   pf->hw.func_caps.num_vfs,
+		   pf->hw.func_caps.num_msix_vectors,
+		   pf->hw.func_caps.num_msix_vectors_vf);
+	i40e_debug(&pf->hw, I40E_DEBUG_INIT,
+		   "HW Capabilities: PF-id[%d] fd_g=%d, fd_b=%d, pf_max_qp=%d num_vsis=%d\n",
+		   pf->hw.pf_id,
+		   pf->hw.func_caps.fd_filters_guaranteed,
+		   pf->hw.func_caps.fd_filters_best_effort,
+		   pf->hw.func_caps.num_tx_qp,
+		   pf->hw.func_caps.num_vsis);
 
 #define DEF_NUM_VSI (1 + (pf->hw.func_caps.fcoe ? 1 : 0) \
 		       + pf->hw.func_caps.num_vfs)
@@ -8495,14 +8498,10 @@ static int i40e_sw_init(struct i40e_pf *pf)
 	int err = 0;
 	int size;
 
-	pf->msg_enable = netif_msg_init(I40E_DEFAULT_MSG_ENABLE,
-				(NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK));
-	if (debug != -1 && debug != I40E_DEFAULT_MSG_ENABLE) {
-		if (I40E_DEBUG_USER & debug)
-			pf->hw.debug_mask = debug;
-		pf->msg_enable = netif_msg_init((debug & ~I40E_DEBUG_USER),
-						I40E_DEFAULT_MSG_ENABLE);
-	}
+	pf->msg_enable = netif_msg_init(debug,
+					NETIF_MSG_DRV    |
+					NETIF_MSG_PROBE  |
+					NETIF_MSG_LINK);
 
 	/* Set default capability flags */
 	pf->flags = I40E_FLAG_RX_CSUM_ENABLED |
diff --git a/drivers/net/ethernet/intel/i40e/i40e_type.h b/drivers/net/ethernet/intel/i40e/i40e_type.h
index bd5f13b..7e88e35 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_type.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
@@ -85,8 +85,6 @@ enum i40e_debug_mask {
 	I40E_DEBUG_AQ_COMMAND		= 0x06000000,
 	I40E_DEBUG_AQ			= 0x0F000000,
 
-	I40E_DEBUG_USER			= 0xF0000000,
-
 	I40E_DEBUG_ALL			= 0xFFFFFFFF
 };
 
-- 
2.7.4

[PATCH net-next 2/2] i40e: fix setting debug parameter early

[Stefan Assmann]

pf->msg_enable is a bitmask, therefore assigning the value of the
"debug" parameter is wrong. It is initialized again later in
i40e_sw_init() so it didn't cause any problem, except that we missed
early debug messages. Moved the initialization and assigned
pf->hw.debug_mask the bitmask as that's what the driver actually uses
in i40e_debug(). Otherwise the debug parameter is just a noop.

Fixes: 5b5faa4 ("i40e: enable debug earlier")

Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 56369761..f972f0d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -8498,11 +8498,6 @@ static int i40e_sw_init(struct i40e_pf *pf)
 	int err = 0;
 	int size;
 
-	pf->msg_enable = netif_msg_init(debug,
-					NETIF_MSG_DRV    |
-					NETIF_MSG_PROBE  |
-					NETIF_MSG_LINK);
-
 	/* Set default capability flags */
 	pf->flags = I40E_FLAG_RX_CSUM_ENABLED |
 		    I40E_FLAG_MSI_ENABLED     |
@@ -10812,10 +10807,13 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	mutex_init(&hw->aq.asq_mutex);
 	mutex_init(&hw->aq.arq_mutex);
 
-	if (debug != -1) {
-		pf->msg_enable = pf->hw.debug_mask;
-		pf->msg_enable = debug;
-	}
+	/* enable debug prints if requested */
+	pf->msg_enable = netif_msg_init(debug,
+					NETIF_MSG_DRV   |
+					NETIF_MSG_PROBE |
+					NETIF_MSG_LINK);
+	if (debug != -1)
+		pf->hw.debug_mask = pf->msg_enable;
 
 	/* do a special CORER for clearing PXE mode once at init */
 	if (hw->revision_id == 0 &&
-- 
2.7.4

[Intel-wired-lan] [PATCH net-next 2/2] i40e: fix setting debug parameter early

[Stefan Assmann]

pf->msg_enable is a bitmask, therefore assigning the value of the
"debug" parameter is wrong. It is initialized again later in
i40e_sw_init() so it didn't cause any problem, except that we missed
early debug messages. Moved the initialization and assigned
pf->hw.debug_mask the bitmask as that's what the driver actually uses
in i40e_debug(). Otherwise the debug parameter is just a noop.

Fixes: 5b5faa4 ("i40e: enable debug earlier")

Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 56369761..f972f0d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -8498,11 +8498,6 @@ static int i40e_sw_init(struct i40e_pf *pf)
 	int err = 0;
 	int size;
 
-	pf->msg_enable = netif_msg_init(debug,
-					NETIF_MSG_DRV    |
-					NETIF_MSG_PROBE  |
-					NETIF_MSG_LINK);
-
 	/* Set default capability flags */
 	pf->flags = I40E_FLAG_RX_CSUM_ENABLED |
 		    I40E_FLAG_MSI_ENABLED     |
@@ -10812,10 +10807,13 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	mutex_init(&hw->aq.asq_mutex);
 	mutex_init(&hw->aq.arq_mutex);
 
-	if (debug != -1) {
-		pf->msg_enable = pf->hw.debug_mask;
-		pf->msg_enable = debug;
-	}
+	/* enable debug prints if requested */
+	pf->msg_enable = netif_msg_init(debug,
+					NETIF_MSG_DRV   |
+					NETIF_MSG_PROBE |
+					NETIF_MSG_LINK);
+	if (debug != -1)
+		pf->hw.debug_mask = pf->msg_enable;
 
 	/* do a special CORER for clearing PXE mode once at init */
 	if (hw->revision_id == 0 &&
-- 
2.7.4

[Intel-wired-lan] [PATCH net-next 2/2] i40e: fix setting debug parameter early

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Stefan Assmann
> Sent: Thursday, September 15, 2016 6:38 AM
> To: intel-wired-lan at lists.osuosl.org
> Cc: netdev at vger.kernel.org; davem at davemloft.net; sassmann at kpanic.de
> Subject: [Intel-wired-lan] [PATCH net-next 2/2] i40e: fix setting debug
> parameter early
> 
> pf->msg_enable is a bitmask, therefore assigning the value of the
> "debug" parameter is wrong. It is initialized again later in
> i40e_sw_init() so it didn't cause any problem, except that we missed early
> debug messages. Moved the initialization and assigned
> pf->hw.debug_mask the bitmask as that's what the driver actually uses
> in i40e_debug(). Otherwise the debug parameter is just a noop.
> 
> Fixes: 5b5faa4 ("i40e: enable debug earlier")
> 
> Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [PATCH net-next 1/2] i40e: remove superfluous I40E_DEBUG_USER statement

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Stefan Assmann
> Sent: Thursday, September 15, 2016 6:38 AM
> To: intel-wired-lan at lists.osuosl.org
> Cc: netdev at vger.kernel.org; davem at davemloft.net; sassmann at kpanic.de
> Subject: [Intel-wired-lan] [PATCH net-next 1/2] i40e: remove superfluous
> I40E_DEBUG_USER statement
> 
> This debug statement is confusing and never set in the code. Any debug
> output should be guarded by the proper I40E_DEBUG_* statement which
> can be enabled via the debug module parameter.
> Remove or convert the I40E_DEBUG_USER cases to I40E_DEBUG_INIT.
> 
> Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_common.c  |  3 ---
> drivers/net/ethernet/intel/i40e/i40e_debugfs.c |  6 -----
> drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  2 --
>  drivers/net/ethernet/intel/i40e/i40e_main.c    | 35 +++++++++++++----------
> ---
>  drivers/net/ethernet/intel/i40e/i40e_type.h    |  2 --
>  5 files changed, 17 insertions(+), 31 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [PATCH net-next 0/2] i40e: clean-up and fix for the i40e debug code

[Wyborny, Carolyn]

> -----Original Message-----
> From: netdev-owner at vger.kernel.org [mailto:netdev-owner at vger.kernel.org]
> On Behalf Of Stefan Assmann
> Sent: Thursday, September 15, 2016 6:38 AM
> To: intel-wired-lan at lists.osuosl.org
> Cc: netdev at vger.kernel.org; davem at davemloft.net; Kirsher, Jeffrey T
> <jeffrey.t.kirsher@intel.com>; sassmann at kpanic.de
> Subject: [PATCH net-next 0/2] i40e: clean-up and fix for the i40e debug code
> 
> Stefan Assmann (2):
>   i40e: remove superfluous I40E_DEBUG_USER statement
>   i40e: fix setting debug parameter early
> 
>  drivers/net/ethernet/intel/i40e/i40e_common.c  |  3 --
>  drivers/net/ethernet/intel/i40e/i40e_debugfs.c |  6 ----
>  drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  2 --
>  drivers/net/ethernet/intel/i40e/i40e_main.c    | 43 ++++++++++++--------------
>  drivers/net/ethernet/intel/i40e/i40e_type.h    |  2 --
>  5 files changed, 20 insertions(+), 36 deletions(-)
> 
> --
> 2.7.4
NACK

I'm not sure what problem is intended to be solved here.  This is code that enables output by the user using ethtool's msglvl feature.  I40E_DEBUG_USER is not superfluous is a mask for user input.

[Intel-wired-lan] [PATCH net-next 0/2] i40e: clean-up and fix for the i40e debug code

[Wyborny, Carolyn]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Wyborny, Carolyn
> Sent: Wednesday, September 21, 2016 11:31 AM
> To: Stefan Assmann <sassmann@kpanic.de>; intel-wired-lan at lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH net-next 0/2] i40e: clean-up and fix for the
> i40e debug code
> 
> > -----Original Message-----
> > From: netdev-owner at vger.kernel.org [mailto:netdev-
> owner at vger.kernel.org]
> > On Behalf Of Stefan Assmann
> > Sent: Thursday, September 15, 2016 6:38 AM
> > To: intel-wired-lan at lists.osuosl.org
> > Cc: netdev at vger.kernel.org; davem at davemloft.net; Kirsher, Jeffrey T
> > <jeffrey.t.kirsher@intel.com>; sassmann at kpanic.de
> > Subject: [PATCH net-next 0/2] i40e: clean-up and fix for the i40e debug code
> >
> > Stefan Assmann (2):
> >   i40e: remove superfluous I40E_DEBUG_USER statement
> >   i40e: fix setting debug parameter early
> >
> >  drivers/net/ethernet/intel/i40e/i40e_common.c  |  3 --
> >  drivers/net/ethernet/intel/i40e/i40e_debugfs.c |  6 ----
> >  drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  2 --
> >  drivers/net/ethernet/intel/i40e/i40e_main.c    | 43 ++++++++++++-------------
> -
> >  drivers/net/ethernet/intel/i40e/i40e_type.h    |  2 --
> >  5 files changed, 20 insertions(+), 36 deletions(-)
> >
> > --
> > 2.7.4
> NACK
> 
> I'm not sure what problem is intended to be solved here.  This is code that
> enables output by the user using ethtool's msglvl feature.  I40E_DEBUG_USER is
> not superfluous is a mask for user input.

To provide more info on this.  Yes, the debug output in this driver was implemented in a non-ideal way.  However, we are working on refactoring this in a way that is consistent with our newer development and in a way that is consistent with how this kind of debugging is expected to work generically.

Thanks,

Carolyn

[Intel-wired-lan] [PATCH net-next 0/2] i40e: clean-up and fix for the i40e debug code

[Stefan Assmann]

On 21.09.2016 23:38, Wyborny, Carolyn wrote:
>> -----Original Message-----
>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
>> Behalf Of Wyborny, Carolyn
>> Sent: Wednesday, September 21, 2016 11:31 AM
>> To: Stefan Assmann <sassmann@kpanic.de>; intel-wired-lan at lists.osuosl.org
>> Subject: Re: [Intel-wired-lan] [PATCH net-next 0/2] i40e: clean-up and fix for the
>> i40e debug code
>>
>>> -----Original Message-----
>>> From: netdev-owner at vger.kernel.org [mailto:netdev-
>> owner at vger.kernel.org]
>>> On Behalf Of Stefan Assmann
>>> Sent: Thursday, September 15, 2016 6:38 AM
>>> To: intel-wired-lan at lists.osuosl.org
>>> Cc: netdev at vger.kernel.org; davem at davemloft.net; Kirsher, Jeffrey T
>>> <jeffrey.t.kirsher@intel.com>; sassmann at kpanic.de
>>> Subject: [PATCH net-next 0/2] i40e: clean-up and fix for the i40e debug code
>>>
>>> Stefan Assmann (2):
>>>   i40e: remove superfluous I40E_DEBUG_USER statement
>>>   i40e: fix setting debug parameter early
>>>
>>>  drivers/net/ethernet/intel/i40e/i40e_common.c  |  3 --
>>>  drivers/net/ethernet/intel/i40e/i40e_debugfs.c |  6 ----
>>>  drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  2 --
>>>  drivers/net/ethernet/intel/i40e/i40e_main.c    | 43 ++++++++++++-------------
>> -
>>>  drivers/net/ethernet/intel/i40e/i40e_type.h    |  2 --
>>>  5 files changed, 20 insertions(+), 36 deletions(-)
>>>
>>> --
>>> 2.7.4
>> NACK
>>
>> I'm not sure what problem is intended to be solved here.  This is code that
>> enables output by the user using ethtool's msglvl feature.  I40E_DEBUG_USER is
>> not superfluous is a mask for user input.

Well, then please enlighten me what's the value of this flag? The
intention of this set is to improve and fix the debug code. I'd
appreciate it if your feedback would be more constructive.

> To provide more info on this.  Yes, the debug output in this driver was implemented in a non-ideal way.  However, we are working on refactoring this in a way that is consistent with our newer development and in a way that is consistent with how this kind of debugging is expected to work generically.

In retrospective, the only flaw I see in the set is that I broke setting
the debug_mask in i40e_set_msglevel(). So unless you plan to revamp the
debug code the next few days or provide a good reason to keep
I40E_DEBUG_USER I'll send a v2 with i40e_set_msglevel() fixed.

  Stefan