[PATCH net 0/4][pull request] Intel Wired LAN Driver Updates 2021-03-25

[PATCH net 0/4][pull request] Intel Wired LAN Driver Updates 2021-03-25

[Tony Nguyen]

This series contains updates to virtchnl header file and i40e driver.

Norbert removes added padding from virtchnl RSS structures as this
causes issues when iterating over the arrays.

Mateusz adds Asym_Pause as supported to allow these settings to be set
as the hardware supports it.

Eryk fixes an issue where encountering a VF reset alongside releasing
VFs could cause a call trace.

Arkadiusz moves TC setup before resource setup as previously it was
possible to enter with a null q_vector causing a kernel oops.

The following are changes since commit e43accba9b071dcd106b5e7643b1b106a158cbb1:
  psample: Fix user API breakage
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 40GbE

Arkadiusz Kubalewski (1):
  i40e: Fix oops at i40e_rebuild()

Eryk Rybak (1):
  i40e: Fix kernel oops when i40e driver removes VF's

Mateusz Palczewski (1):
  i40e: Added Asym_Pause to supported link modes

Norbert Ciosek (1):
  virtchnl: Fix layout of RSS structures

 drivers/net/ethernet/intel/i40e/i40e.h             |  1 +
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c     |  1 +
 drivers/net/ethernet/intel/i40e/i40e_main.c        | 11 +++++------
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |  9 +++++++++
 include/linux/avf/virtchnl.h                       |  2 --
 5 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.26.2

[PATCH net 1/4] virtchnl: Fix layout of RSS structures

[Tony Nguyen]

From: Norbert Ciosek <norbertx.ciosek@intel.com>

Remove padding from RSS structures. Previous layout
could lead to unwanted compiler optimizations
in loops when iterating over key and lut arrays.

Fixes: 65ece6de0114 ("virtchnl: Add missing explicit padding to structures")
Signed-off-by: Norbert Ciosek <norbertx.ciosek@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 include/linux/avf/virtchnl.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index 40bad71865ea..532bcbfc4716 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -476,7 +476,6 @@ struct virtchnl_rss_key {
 	u16 vsi_id;
 	u16 key_len;
 	u8 key[1];         /* RSS hash key, packed bytes */
-	u8 pad[1];
 };
 
 VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_key);
@@ -485,7 +484,6 @@ struct virtchnl_rss_lut {
 	u16 vsi_id;
 	u16 lut_entries;
 	u8 lut[1];        /* RSS lookup table */
-	u8 pad[1];
 };
 
 VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_lut);
-- 
2.26.2

Re: [PATCH net 1/4] virtchnl: Fix layout of RSS structures

[Geert Uytterhoeven]

Hi Anthony, Norbert,

Thanks for your patch!

On Thu, Mar 25, 2021 at 11:29 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
> From: Norbert Ciosek <norbertx.ciosek@intel.com>
>
> Remove padding from RSS structures. Previous layout
> could lead to unwanted compiler optimizations
> in loops when iterating over key and lut arrays.

From an earlier private conversation with Mateusz, I understand the real
explanation is that key[] and lut[] must be at the end of the
structures, because they are used as flexible array members?

> Fixes: 65ece6de0114 ("virtchnl: Add missing explicit padding to structures")
> Signed-off-by: Norbert Ciosek <norbertx.ciosek@intel.com>
> Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

> --- a/include/linux/avf/virtchnl.h
> +++ b/include/linux/avf/virtchnl.h
> @@ -476,7 +476,6 @@ struct virtchnl_rss_key {
>         u16 vsi_id;
>         u16 key_len;
>         u8 key[1];         /* RSS hash key, packed bytes */
> -       u8 pad[1];
>  };
>
>  VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_key);
> @@ -485,7 +484,6 @@ struct virtchnl_rss_lut {
>         u16 vsi_id;
>         u16 lut_entries;
>         u8 lut[1];        /* RSS lookup table */
> -       u8 pad[1];
>  };

If you use a flexible array member, it should be declared without a size,
i.e.

    u8 key[];

Everything else is (trying to) fool the compiler, and leading to undefined
behavior, and people (re)adding explicit padding.



--
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH net 1/4] virtchnl: Fix layout of RSS structures

[Geert Uytterhoeven]

Hi Samudrala,

On Fri, Mar 26, 2021 at 11:45 PM Samudrala, Sridhar
<sridhar.samudrala@intel.com> wrote:
> On 3/26/2021 1:06 AM, Geert Uytterhoeven wrote:
> > On Thu, Mar 25, 2021 at 11:29 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
> > From: Norbert Ciosek <norbertx.ciosek@intel.com>
> >
> > Remove padding from RSS structures. Previous layout
> > could lead to unwanted compiler optimizations
> > in loops when iterating over key and lut arrays.
> >
> > From an earlier private conversation with Mateusz, I understand the real
> > explanation is that key[] and lut[] must be at the end of the
> > structures, because they are used as flexible array members?
> >
> > Fixes: 65ece6de0114 ("virtchnl: Add missing explicit padding to structures")
> > Signed-off-by: Norbert Ciosek <norbertx.ciosek@intel.com>
> > Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> >
> > --- a/include/linux/avf/virtchnl.h
> > +++ b/include/linux/avf/virtchnl.h
> > @@ -476,7 +476,6 @@ struct virtchnl_rss_key {
> >         u16 vsi_id;
> >         u16 key_len;
> >         u8 key[1];         /* RSS hash key, packed bytes */
> > -       u8 pad[1];
> >  };
> >
> >  VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_key);
> > @@ -485,7 +484,6 @@ struct virtchnl_rss_lut {
> >         u16 vsi_id;
> >         u16 lut_entries;
> >         u8 lut[1];        /* RSS lookup table */
> > -       u8 pad[1];
> >  };
> >
> > If you use a flexible array member, it should be declared without a size,
> > i.e.
> >
> >     u8 key[];
> >
> > Everything else is (trying to) fool the compiler, and leading to undefined
> > behavior, and people (re)adding explicit padding.
>
> This header file is shared across other OSes that use C++ that doesn't support
> flexible arrays. So the structures in this file use an array of size 1 as a last
> element to enable variable sized arrays.

I don't think it is accepted practice to have non-Linux-isms in
include/*linux*/avf/virtchnl.h header files.  Moreover, using a size
of 1 is counter-intuitive for people used to Linux kernel development,
and may lead to off-by-one errors in calculation of sizes.

If you insist on ignoring the above, this definitely deserves a
comment next to the member's declaration.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH net 1/4] virtchnl: Fix layout of RSS structures

[Samudrala, Sridhar]

On 3/27/2021 2:53 AM, Geert Uytterhoeven wrote:
> Hi Samudrala,
>
> On Fri, Mar 26, 2021 at 11:45 PM Samudrala, Sridhar
> <sridhar.samudrala@intel.com> wrote:
>> On 3/26/2021 1:06 AM, Geert Uytterhoeven wrote:
>>> On Thu, Mar 25, 2021 at 11:29 PM Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
>>> From: Norbert Ciosek <norbertx.ciosek@intel.com>
>>>
>>> Remove padding from RSS structures. Previous layout
>>> could lead to unwanted compiler optimizations
>>> in loops when iterating over key and lut arrays.
>>>
>>>  From an earlier private conversation with Mateusz, I understand the real
>>> explanation is that key[] and lut[] must be at the end of the
>>> structures, because they are used as flexible array members?
>>>
>>> Fixes: 65ece6de0114 ("virtchnl: Add missing explicit padding to structures")
>>> Signed-off-by: Norbert Ciosek <norbertx.ciosek@intel.com>
>>> Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>>
>>> --- a/include/linux/avf/virtchnl.h
>>> +++ b/include/linux/avf/virtchnl.h
>>> @@ -476,7 +476,6 @@ struct virtchnl_rss_key {
>>>          u16 vsi_id;
>>>          u16 key_len;
>>>          u8 key[1];         /* RSS hash key, packed bytes */
>>> -       u8 pad[1];
>>>   };
>>>
>>>   VIRTCHNL_CHECK_STRUCT_LEN(6, virtchnl_rss_key);
>>> @@ -485,7 +484,6 @@ struct virtchnl_rss_lut {
>>>          u16 vsi_id;
>>>          u16 lut_entries;
>>>          u8 lut[1];        /* RSS lookup table */
>>> -       u8 pad[1];
>>>   };
>>>
>>> If you use a flexible array member, it should be declared without a size,
>>> i.e.
>>>
>>>      u8 key[];
>>>
>>> Everything else is (trying to) fool the compiler, and leading to undefined
>>> behavior, and people (re)adding explicit padding.
>> This header file is shared across other OSes that use C++ that doesn't support
>> flexible arrays. So the structures in this file use an array of size 1 as a last
>> element to enable variable sized arrays.
> I don't think it is accepted practice to have non-Linux-isms in
> include/*linux*/avf/virtchnl.h header files.  Moreover, using a size
> of 1 is counter-intuitive for people used to Linux kernel development,
> and may lead to off-by-one errors in calculation of sizes.
>
> If you insist on ignoring the above, this definitely deserves a
> comment next to the member's declaration.
Sure. We can add a comment indicating that these fields are used 
variable sized arrays.

Thanks
Sridhar

[PATCH net 2/4] i40e: Added Asym_Pause to supported link modes

[Tony Nguyen]

From: Mateusz Palczewski <mateusz.palczewski@intel.com>

Add Asym_Pause to supported link modes (it is supported by HW).
Lack of Asym_Pause in supported modes can cause several problems,
i.e. it won't be possible to turn the autonegotiation on
with asymmetric pause settings (i.e. Tx on, Rx off).

Fixes: 4e91bcd5d47a ("i40e: Finish implementation of ethtool get settings")
Signed-off-by: Dawid Lukwinski <dawid.lukwinski@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index c70dec65a572..2c637a5678b3 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -1101,6 +1101,7 @@ static int i40e_get_link_ksettings(struct net_device *netdev,
 
 	/* Set flow control settings */
 	ethtool_link_ksettings_add_link_mode(ks, supported, Pause);
+	ethtool_link_ksettings_add_link_mode(ks, supported, Asym_Pause);
 
 	switch (hw->fc.requested_mode) {
 	case I40E_FC_FULL:
-- 
2.26.2

[PATCH net 3/4] i40e: Fix kernel oops when i40e driver removes VF's

[Tony Nguyen]

From: Eryk Rybak <eryk.roch.rybak@intel.com>

Fix the reason of kernel oops when i40e driver removed VFs.
Added new __I40E_VFS_RELEASING state to signalize releasing
process by PF, that it makes possible to exit of reset VF procedure.
Without this patch, it is possible to suspend the VFs reset by
releasing VFs resources procedure. Retrying the reset after the
timeout works on the freed VF memory causing a kernel oops.

Fixes: d43d60e5eb95("i40e: ensure reset occurs when disabling VF")
Signed-off-by: Eryk Rybak <eryk.roch.rybak@intel.com>
Signed-off-by: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e.h             | 1 +
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index cd53981fa5e0..15f93b355099 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -142,6 +142,7 @@ enum i40e_state_t {
 	__I40E_VIRTCHNL_OP_PENDING,
 	__I40E_RECOVERY_MODE,
 	__I40E_VF_RESETS_DISABLED,	/* disable resets during i40e_remove */
+	__I40E_VFS_RELEASING,
 	/* This must be last as it determines the size of the BITMAP */
 	__I40E_STATE_SIZE__,
 };
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 1b6ec9be155a..5d301a466f5c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -137,6 +137,7 @@ void i40e_vc_notify_vf_reset(struct i40e_vf *vf)
  **/
 static inline void i40e_vc_disable_vf(struct i40e_vf *vf)
 {
+	struct i40e_pf *pf = vf->pf;
 	int i;
 
 	i40e_vc_notify_vf_reset(vf);
@@ -147,6 +148,11 @@ static inline void i40e_vc_disable_vf(struct i40e_vf *vf)
 	 * ensure a reset.
 	 */
 	for (i = 0; i < 20; i++) {
+		/* If PF is in VFs releasing state reset VF is impossible,
+		 * so leave it.
+		 */
+		if (test_bit(__I40E_VFS_RELEASING, pf->state))
+			return;
 		if (i40e_reset_vf(vf, false))
 			return;
 		usleep_range(10000, 20000);
@@ -1574,6 +1580,8 @@ void i40e_free_vfs(struct i40e_pf *pf)
 
 	if (!pf->vf)
 		return;
+
+	set_bit(__I40E_VFS_RELEASING, pf->state);
 	while (test_and_set_bit(__I40E_VF_DISABLE, pf->state))
 		usleep_range(1000, 2000);
 
@@ -1631,6 +1639,7 @@ void i40e_free_vfs(struct i40e_pf *pf)
 		}
 	}
 	clear_bit(__I40E_VF_DISABLE, pf->state);
+	clear_bit(__I40E_VFS_RELEASING, pf->state);
 }
 
 #ifdef CONFIG_PCI_IOV
-- 
2.26.2

[PATCH net 4/4] i40e: Fix oops at i40e_rebuild()

[Tony Nguyen]

From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

Setup TC before the i40e_setup_pf_switch() call.
Memory must be initialized for all the queues
before using its resources.

Previously it could be possible that a call:
xdp_rxq_info_reg(&rx_ring->xdp_rxq, rx_ring->netdev,
rx_ring->queue_index, rx_ring->q_vector->napi.napi_id);
was made with q_vector being null.

Oops could show up with the following sequence:
- no driver loaded
- FW LLDP agent is on (flag disable-fw-lldp:off)
- link is up
- DCB configured with number of Traffic Classes that will not divide
  completely the default number of queues (usually cpu cores)
- driver load
- set private flag: disable-fw-lldp:on

Fixes: 4b208eaa8078 ("i40e: Add init and default config of software based DCB")
Fixes: b02e5a0ebb17 ("xsk: Propagate napi_id to XDP socket Rx path")
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Tested-by: Tony Brelinski <tonyx.brelinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 17f3b800640e..f67f0cc9dadf 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -10573,12 +10573,6 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
 		goto end_core_reset;
 	}
 
-	if (!lock_acquired)
-		rtnl_lock();
-	ret = i40e_setup_pf_switch(pf, reinit);
-	if (ret)
-		goto end_unlock;
-
 #ifdef CONFIG_I40E_DCB
 	/* Enable FW to write a default DCB config on link-up
 	 * unless I40E_FLAG_TC_MQPRIO was enabled or DCB
@@ -10607,6 +10601,11 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
 	}
 
 #endif /* CONFIG_I40E_DCB */
+	if (!lock_acquired)
+		rtnl_lock();
+	ret = i40e_setup_pf_switch(pf, reinit);
+	if (ret)
+		goto end_unlock;
 
 	/* The driver only wants link up/down and module qualification
 	 * reports from firmware.  Note the negative logic.
-- 
2.26.2

Re: [PATCH net 0/4][pull request] Intel Wired LAN Driver Updates 2021-03-25

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Thu, 25 Mar 2021 15:31:15 -0700 you wrote:
> This series contains updates to virtchnl header file and i40e driver.
> 
> Norbert removes added padding from virtchnl RSS structures as this
> causes issues when iterating over the arrays.
> 
> Mateusz adds Asym_Pause as supported to allow these settings to be set
> as the hardware supports it.
> 
> [...]

Here is the summary with links:
  - [net,1/4] virtchnl: Fix layout of RSS structures
    https://git.kernel.org/netdev/net/c/22f8b5df881e
  - [net,2/4] i40e: Added Asym_Pause to supported link modes
    https://git.kernel.org/netdev/net/c/90449e98c265
  - [net,3/4] i40e: Fix kernel oops when i40e driver removes VF's
    https://git.kernel.org/netdev/net/c/347b5650cd15
  - [net,4/4] i40e: Fix oops at i40e_rebuild()
    https://git.kernel.org/netdev/net/c/f2916ae9a1bc

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html