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

v2 fixes setting the debug_mask in i40e_set_msglevel().

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 |  3 +-
 drivers/net/ethernet/intel/i40e/i40e_main.c    | 43 ++++++++++++--------------
 drivers/net/ethernet/intel/i40e/i40e_type.h    |  2 --
 5 files changed, 21 insertions(+), 36 deletions(-)

-- 
2.7.4

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 or ethtool.
Remove or convert the I40E_DEBUG_USER cases to I40E_DEBUG_INIT.

v2: re-add setting the debug_mask in i40e_set_msglevel() so that the
debug level can still be altered via ethtool msglvl.

Signed-off-by: Stefan Assmann <redacted>
---
 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 |  3 +--
 drivers/net/ethernet/intel/i40e/i40e_main.c    | 35 +++++++++++++-------------
 drivers/net/ethernet/intel/i40e/i40e_type.h    |  2 --
 5 files changed, 18 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..02f55ab 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c

@@ -987,8 +987,7 @@ 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->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

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 <redacted>
---
 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

On Fri, Sep 23, 2016 at 6:30 AM, Stefan Assmann [off-list ref] wrote:

I'm assuming this is dropped because you considered it redundant with
the dump in i40e_get_capabilities.  If so it would have been nice to
see this called out in your patch description somewhere as it doesn't
jive with the rest of the patch since you are stripping something that
is using I40E_DEBUG_INIT.

From what I can tell the interface is completely redundant as ethtool

can already do this.  I'd say it is okay to just remove this command
and section entirely from the debugfs interface.

So the way I view this is that I40E_DEBUG_USER appears to be a flag
that is being used to differentiate between some proprietary flags and
the standard msg level.  The problem is that msg_enable and debug_mask
are playing off of two completely different bit definitions.  For
example how much sense does it make for NETIF_F_MSG_TX_DONE to map to
I40E_DEBUG_DCB.  If anything what should probably happen here is
instead of dropping the if there probably needs to be an else.

This is one of many things on my list of items to fix since I have
come back to Intel.  It is just a matter of finding the time.
Basically what I would really prefer to see here is us move all of the
flags in i40e_debug_mask so that we didn't have any overlap with the
NETIF_F_MSG_* flags unless there is a relation between the two.

I'd say don't bother with this.  There isn't any point.

Okay so I think I now see why there is confusion about how debug is
used.  The documentation in the driver is wrong for how it worked.  It
wasn't being passed as a 0-16, somebody implemented this as a 32 bit
bitmask.  So the question becomes how to fix it.  The problem is with
the patch as it is so far we end up with pf->msg_enable being
populated but pf->hw.debug_mask never being populated.  The values you
are passing as the default don't make any sense either since they
don't really map to the same functionality in I40e.  They map to
DEBUG_INIT, DEBUG_RELEASE, and an unused bit.

This end piece is where the problem really lies.  The problem
statement for this would essentially be that the i40e driver uses the
debug module parameter in a non-standard way.  It is using a tg3 style
bitmask to populate the fields, but then documenting it and coding
part of it like it is expecting the default debug usage.  To top it
off it is doing the same kind of nonsense with the ethtool msg level
interface.

The one piece we probably need with all this in order to really "fix"
the issue and still maintain some sense of functionality would be to
look at adding something that would populate pf->hw.debug_mask.  I'm
half tempted to say that we could try adding another module parameter
named i40e_debug that we could use like tg3 does with tg3_debug and
change the debugfs interface to only modify that instead of messing
with the msg level, but the fact is that would probably just be more
confusing.  For now what I would suggest doing is just splitting
msg_enable and pf->hw.debug_mask and for now just default the value of
pf->hw.debug_mask to I40E_DEFAULT_MSG_ENABLE.  That way in a week or
two after netdev/netconf we will hopefully had a chance to hash this
all out and can find a better way to solve this.

- Alex

On Fri, Sep 23, 2016 at 6:30 AM, Stefan Assmann [off-list ref] wrote:

The patch is broken, mainly because the code was already broken.  The
flags in pf->hw.debug_mask are in no way related to pf->msg_enable.
For now just use the default mask provided to populate
pf->hw.debug_mask and then the msg_enable portion is fine.

- Alex

On 24.09.2016 04:48, Alexander Duyck wrote:

On Fri, Sep 23, 2016 at 6:30 AM, Stefan Assmann [off-list ref] wrote:

quoted

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 or ethtool.
Remove or convert the I40E_DEBUG_USER cases to I40E_DEBUG_INIT.

v2: re-add setting the debug_mask in i40e_set_msglevel() so that the
debug level can still be altered via ethtool msglvl.

Signed-off-by: Stefan Assmann <redacted>
---
 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 |  3 +--
 drivers/net/ethernet/intel/i40e/i40e_main.c    | 35 +++++++++++++-------------
 drivers/net/ethernet/intel/i40e/i40e_type.h    |  2 --
 5 files changed, 18 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;

I'm assuming this is dropped because you considered it redundant with
the dump in i40e_get_capabilities.  If so it would have been nice to
see this called out in your patch description somewhere as it doesn't
jive with the rest of the patch since you are stripping something that
is using I40E_DEBUG_INIT.

Hi Alex,

agreed, it seemed redundant. I'll make a note about it in the next
version when we have decided how to proceed.

quoted

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);

From what I can tell the interface is completely redundant as ethtool
can already do this.  I'd say it is okay to just remove this command
and section entirely from the debugfs interface.

Yes, I didn't want to stray too far from what the description said and
just removed the I40E_DEBUG_USER related code.

quoted

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 1835186..02f55ab 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c

@@ -987,8 +987,7 @@ 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->hw.debug_mask = data;
        pf->msg_enable = data;
 }

So the way I view this is that I40E_DEBUG_USER appears to be a flag
that is being used to differentiate between some proprietary flags and
the standard msg level.  The problem is that msg_enable and debug_mask
are playing off of two completely different bit definitions.  For
example how much sense does it make for NETIF_F_MSG_TX_DONE to map to
I40E_DEBUG_DCB.  If anything what should probably happen here is
instead of dropping the if there probably needs to be an else.

As you said the flags don't match, which is part of the problem. What
tipped me of starting to work on this is, that the debug module
parameter doesn't do a thing atm and I had to debug some stuff during
driver MSI-X initialization. So my main pain point here is to get the
debug parameter in a sane state.

This is one of many things on my list of items to fix since I have
come back to Intel.  It is just a matter of finding the time.
Basically what I would really prefer to see here is us move all of the
flags in i40e_debug_mask so that we didn't have any overlap with the
NETIF_F_MSG_* flags unless there is a relation between the two.

That sounds like a good idea and I'm happy to join in. So for now, I
could drop the I40E_DEBUG_USER changes and just focus on making the
debug parameter usable. All the non-standard debug output could be
handled by I40E_DEBUG_USER or whatever better name we could for the
flag. The current name doesn't really explain what it's meant for.

quoted

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)

I'd say don't bother with this.  There isn't any point.

OK, I thought the same thing but wasn't sure if anybody relies on this
info.

quoted

@@ -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 |

Okay so I think I now see why there is confusion about how debug is
used.  The documentation in the driver is wrong for how it worked.  It
wasn't being passed as a 0-16, somebody implemented this as a 32 bit
bitmask.  So the question becomes how to fix it.  The problem is with
the patch as it is so far we end up with pf->msg_enable being
populated but pf->hw.debug_mask never being populated.  The values you
are passing as the default don't make any sense either since they
don't really map to the same functionality in I40e.  They map to
DEBUG_INIT, DEBUG_RELEASE, and an unused bit.

quoted

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
 };

This end piece is where the problem really lies.  The problem
statement for this would essentially be that the i40e driver uses the
debug module parameter in a non-standard way.  It is using a tg3 style
bitmask to populate the fields, but then documenting it and coding
part of it like it is expecting the default debug usage.  To top it
off it is doing the same kind of nonsense with the ethtool msg level
interface.

The one piece we probably need with all this in order to really "fix"
the issue and still maintain some sense of functionality would be to
look at adding something that would populate pf->hw.debug_mask.  I'm
half tempted to say that we could try adding another module parameter
named i40e_debug that we could use like tg3 does with tg3_debug and
change the debugfs interface to only modify that instead of messing
with the msg level, but the fact is that would probably just be more
confusing.  For now what I would suggest doing is just splitting
msg_enable and pf->hw.debug_mask and for now just default the value of
pf->hw.debug_mask to I40E_DEFAULT_MSG_ENABLE.  That way in a week or
two after netdev/netconf we will hopefully had a chance to hash this
all out and can find a better way to solve this.

I really appreciate your feedback. What you're suggesting makes sense.
Let's split the generic (msg_enable) debug information provided by the
debug parameter from driver specific debug information. Not sure I'd
like to see another module parameter, but that doesn't have to be
decided right away.

If you agree, I'll rewrite the patches to make a clear separation
between debug_mask and msg_enable, making the debug parameter actually
usable.
And we'll sort out the rest along the way.

  Stefan

On Sat, Sep 24, 2016 at 4:13 AM, Stefan Assmann [off-list ref] wrote:

On 24.09.2016 04:48, Alexander Duyck wrote:

quoted

On Fri, Sep 23, 2016 at 6:30 AM, Stefan Assmann [off-list ref] wrote:

quoted

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 or ethtool.
Remove or convert the I40E_DEBUG_USER cases to I40E_DEBUG_INIT.

v2: re-add setting the debug_mask in i40e_set_msglevel() so that the
debug level can still be altered via ethtool msglvl.

Signed-off-by: Stefan Assmann <redacted>
---
 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 |  3 +--
 drivers/net/ethernet/intel/i40e/i40e_main.c    | 35 +++++++++++++-------------
 drivers/net/ethernet/intel/i40e/i40e_type.h    |  2 --
 5 files changed, 18 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;

I'm assuming this is dropped because you considered it redundant with
the dump in i40e_get_capabilities.  If so it would have been nice to
see this called out in your patch description somewhere as it doesn't
jive with the rest of the patch since you are stripping something that
is using I40E_DEBUG_INIT.

Hi Alex,

agreed, it seemed redundant. I'll make a note about it in the next
version when we have decided how to proceed.

quoted

quoted

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);

From what I can tell the interface is completely redundant as ethtool
can already do this.  I'd say it is okay to just remove this command
and section entirely from the debugfs interface.

Yes, I didn't want to stray too far from what the description said and
just removed the I40E_DEBUG_USER related code.

quoted

quoted

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 1835186..02f55ab 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c

@@ -987,8 +987,7 @@ 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->hw.debug_mask = data;
        pf->msg_enable = data;
 }

So the way I view this is that I40E_DEBUG_USER appears to be a flag
that is being used to differentiate between some proprietary flags and
the standard msg level.  The problem is that msg_enable and debug_mask
are playing off of two completely different bit definitions.  For
example how much sense does it make for NETIF_F_MSG_TX_DONE to map to
I40E_DEBUG_DCB.  If anything what should probably happen here is
instead of dropping the if there probably needs to be an else.

As you said the flags don't match, which is part of the problem. What
tipped me of starting to work on this is, that the debug module
parameter doesn't do a thing atm and I had to debug some stuff during
driver MSI-X initialization. So my main pain point here is to get the
debug parameter in a sane state.

quoted

This is one of many things on my list of items to fix since I have
come back to Intel.  It is just a matter of finding the time.
Basically what I would really prefer to see here is us move all of the
flags in i40e_debug_mask so that we didn't have any overlap with the
NETIF_F_MSG_* flags unless there is a relation between the two.

That sounds like a good idea and I'm happy to join in. So for now, I
could drop the I40E_DEBUG_USER changes and just focus on making the
debug parameter usable. All the non-standard debug output could be
handled by I40E_DEBUG_USER or whatever better name we could for the
flag. The current name doesn't really explain what it's meant for.

quoted

quoted

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)

I'd say don't bother with this.  There isn't any point.

OK, I thought the same thing but wasn't sure if anybody relies on this
info.

quoted

quoted

@@ -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 |

Okay so I think I now see why there is confusion about how debug is
used.  The documentation in the driver is wrong for how it worked.  It
wasn't being passed as a 0-16, somebody implemented this as a 32 bit
bitmask.  So the question becomes how to fix it.  The problem is with
the patch as it is so far we end up with pf->msg_enable being
populated but pf->hw.debug_mask never being populated.  The values you
are passing as the default don't make any sense either since they
don't really map to the same functionality in I40e.  They map to
DEBUG_INIT, DEBUG_RELEASE, and an unused bit.

quoted

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
 };

This end piece is where the problem really lies.  The problem
statement for this would essentially be that the i40e driver uses the
debug module parameter in a non-standard way.  It is using a tg3 style
bitmask to populate the fields, but then documenting it and coding
part of it like it is expecting the default debug usage.  To top it
off it is doing the same kind of nonsense with the ethtool msg level
interface.

The one piece we probably need with all this in order to really "fix"
the issue and still maintain some sense of functionality would be to
look at adding something that would populate pf->hw.debug_mask.  I'm
half tempted to say that we could try adding another module parameter
named i40e_debug that we could use like tg3 does with tg3_debug and
change the debugfs interface to only modify that instead of messing
with the msg level, but the fact is that would probably just be more
confusing.  For now what I would suggest doing is just splitting
msg_enable and pf->hw.debug_mask and for now just default the value of
pf->hw.debug_mask to I40E_DEFAULT_MSG_ENABLE.  That way in a week or
two after netdev/netconf we will hopefully had a chance to hash this
all out and can find a better way to solve this.

I really appreciate your feedback. What you're suggesting makes sense.
Let's split the generic (msg_enable) debug information provided by the
debug parameter from driver specific debug information. Not sure I'd
like to see another module parameter, but that doesn't have to be
decided right away.

If you agree, I'll rewrite the patches to make a clear separation
between debug_mask and msg_enable, making the debug parameter actually
usable.
And we'll sort out the rest along the way.

  Stefan

I think that works for me.  It will be easier for us to sort out the
I40E_DEBUG_* flags since I think the out-of-tree driver might be
hiding some additional debug info.  So if we can split msg_enable and
hw.debug_mask for now that would probably work out best.

One thought I had is if we wanted to overload the debug value what we
could do is make it so that we use bit 31 as the "I40E_DEBUG_USER"
value.  Then we could go through and essentially do an if/else setup
throughout the code where if debug is negative it is assumed to be
either -1 or meant to represent hw.debug_mask, and if they are
positive the value is assumed to be setting msg enable.  With negative
values used to identify the hw.debug_mask values we have the added
advantage that it then automatically forces netif_msg_init into
setting the msg_enable to the value passed in default_msg_enable_bits.

- Alex