| Message ID | 1474637458-5255-2-git-send-email-sassmann@kpanic.de |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Sep 23, 2016 at 6:30 AM, Stefan Assmann <sassmann@kpanic.de> wrote: > 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 <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 | 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. > 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. > 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. 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. > 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. > @@ -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. > 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. - Alex
On 24.09.2016 04:48, Alexander Duyck wrote: > On Fri, Sep 23, 2016 at 6:30 AM, Stefan Assmann <sassmann@kpanic.de> wrote: >> 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 <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 | 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. >> 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. >> 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. >> 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. >> @@ -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. > >> 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 <sassmann@kpanic.de> wrote: > On 24.09.2016 04:48, Alexander Duyck wrote: >> On Fri, Sep 23, 2016 at 6:30 AM, Stefan Assmann <sassmann@kpanic.de> wrote: >>> 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 <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 | 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. > >>> 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. > >>> 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. > >>> 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. > >>> @@ -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. >> >>> 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
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 };
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 <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 | 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(-)