| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 24 Sep 2016 13:13:45 +0200
From: Stefan Assmann <sassmann@...nic.de>
To: Alexander Duyck <alexander.duyck@...il.com>,
David Miller <davem@...emloft.net>
Cc: intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
Netdev <netdev@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH net-next v2 1/2] i40e: remove
superfluous I40E_DEBUG_USER statement
On 24.09.2016 04:48, Alexander Duyck wrote:
> On Fri, Sep 23, 2016 at 6:30 AM, Stefan Assmann <sassmann@...nic.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@...nic.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
Powered by blists - more mailing lists