[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2023-12-18 (ice)

[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2023-12-18 (ice)

[Tony Nguyen]

This series contains updates to ice driver only.

Jakes stops clearing of needed aggregator information.

Dave adds a check for LAG device support before initializing the
associated event handler.

Larysa restores accounting of XDP queues in TC configurations.

The following are changes since commit 979e90173af8d2f52f671d988189aab98c6d1be6:
  Merge branch 'mptcp-misc-fixes'
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 100GbE

Dave Ertman (1):
  ice: alter feature support check for SRIOV and LAG

Jacob Keller (1):
  ice: stop trashing VF VSI aggregator node ID information

Larysa Zaremba (1):
  ice: Fix PF with enabled XDP going no-carrier after reset

 drivers/net/ethernet/intel/ice/ice_lag.c | 2 ++
 drivers/net/ethernet/intel/ice/ice_lib.c | 7 +++----
 2 files changed, 5 insertions(+), 4 deletions(-)

-- 
2.41.0

[PATCH net 1/3] ice: stop trashing VF VSI aggregator node ID information

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

When creating new VSIs, they are assigned into an aggregator node in the
scheduler tree. Information about which aggregator node a VSI is assigned
into is maintained by the vsi->agg_node structure. In ice_vsi_decfg(), this
information is being destroyed, by overwriting the valid flag and the
agg_id field to zero.

For VF VSIs, this breaks the aggregator node configuration replay, which
depends on this information. This results in VFs being inserted into the
default aggregator node. The resulting configuration will have unexpected
Tx bandwidth sharing behavior.

This was broken by commit 6624e780a577 ("ice: split ice_vsi_setup into
smaller functions"), which added the block to reset the agg_node data.

The vsi->agg_node structure is not managed by the scheduler code, but is
instead a wrapper around an aggregator node ID that is tracked at the VSI
layer. Its been around for a long time, and its primary purpose was for
handling VFs. The SR-IOV VF reset flow does not make use of the standard VSI
rebuild/replay logic, and uses vsi->agg_node as part of its handling to
rebuild the aggregator node configuration.

The logic for aggregator nodes stretches  back to early ice driver code from
commit b126bd6bcd67 ("ice: create scheduler aggregator node config and move
VSIs")

The logic in ice_vsi_decfg() which trashes the ice_agg_node data is clearly
wrong. It destroys information that is necessary for handling VF reset,. It
is also not the correct way to actually remove a VSI from an aggregator
node. For that, we need to implement logic in the scheduler code. Further,
non-VF VSIs properly replay their aggregator configuration using existing
scheduler replay logic.

To fix the VF replay logic, remove this broken aggregator node cleanup
logic. This is the simplest way to immediately fix this.

This ensures that VFs will have proper aggregate configuration after a
reset. This is especially important since VFs often perform resets as part
of their reconfiguration flows. Without fixing this, VFs will be placed in
the default aggregator node and Tx bandwidth will not be shared in the
expected and configured manner.

Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lib.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 4b1e56396293..de7ba87af45d 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -2620,10 +2620,6 @@ void ice_vsi_decfg(struct ice_vsi *vsi)
 	if (vsi->type == ICE_VSI_VF &&
 	    vsi->agg_node && vsi->agg_node->valid)
 		vsi->agg_node->num_vsis--;
-	if (vsi->agg_node) {
-		vsi->agg_node->valid = false;
-		vsi->agg_node->agg_id = 0;
-	}
 }
 
 /**
-- 
2.41.0

[PATCH net 2/3] ice: alter feature support check for SRIOV and LAG

[Tony Nguyen]

From: Dave Ertman <david.m.ertman@intel.com>

Previously, the ice driver had support for using a handler for bonding
netdev events to ensure that conflicting features were not allowed to be
activated at the same time.  While this was still in place, additional
support was added to specifically support SRIOV and LAG together.  These
both utilized the netdev event handler, but the SRIOV and LAG feature was
behind a capabilities feature check to make sure the current NVM has
support.

The exclusion part of the event handler should be removed since there are
users who have custom made solutions that depend on the non-exclusion of
features.

Wrap the creation/registration and cleanup of the event handler and
associated structs in the probe flow with a feature check so that the
only systems that support the full implementation of LAG features will
initialize support.  This will leave other systems unhindered with
functionality as it existed before any LAG code was added.

Fixes: bb52f42acef6 ("ice: Add driver support for firmware changes for LAG")
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lag.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c
index 280994ee5933..b47cd43ae871 100644
--- a/drivers/net/ethernet/intel/ice/ice_lag.c
+++ b/drivers/net/ethernet/intel/ice/ice_lag.c
@@ -1981,6 +1981,8 @@ int ice_init_lag(struct ice_pf *pf)
 	int n, err;
 
 	ice_lag_init_feature_support_flag(pf);
+	if (!ice_is_feature_supported(pf, ICE_F_SRIOV_LAG))
+		return 0;
 
 	pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL);
 	if (!pf->lag)
-- 
2.41.0

[PATCH net 3/3] ice: Fix PF with enabled XDP going no-carrier after reset

[Tony Nguyen]

From: Larysa Zaremba <larysa.zaremba@intel.com>

Commit 6624e780a577fc596788 ("ice: split ice_vsi_setup into smaller
functions") has refactored a bunch of code involved in PFR. In this
process, TC queue number adjustment for XDP was lost. Bring it back.

Lack of such adjustment causes interface to go into no-carrier after a
reset, if XDP program is attached, with the following message:

ice 0000:b1:00.0: Failed to set LAN Tx queue context, error: -22
ice 0000:b1:00.0 ens801f0np0: Failed to open VSI 0x0006 on switch 0x0001
ice 0000:b1:00.0: enable VSI failed, err -22, VSI index 0, type ICE_VSI_PF
ice 0000:b1:00.0: PF VSI rebuild failed: -22
ice 0000:b1:00.0: Rebuild failed, unload and reload driver

Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index de7ba87af45d..1bad6e17f9be 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -2371,6 +2371,9 @@ static int ice_vsi_cfg_tc_lan(struct ice_pf *pf, struct ice_vsi *vsi)
 		} else {
 			max_txqs[i] = vsi->alloc_txq;
 		}
+
+		if (vsi->type == ICE_VSI_PF)
+			max_txqs[i] += vsi->num_xdp_txq;
 	}
 
 	dev_dbg(dev, "vsi->tc_cfg.ena_tc = %d\n", vsi->tc_cfg.ena_tc);
-- 
2.41.0

Re: [PATCH net 3/3] ice: Fix PF with enabled XDP going no-carrier after reset

[Maciej Fijalkowski]

On Mon, Dec 18, 2023 at 11:27:06AM -0800, Tony Nguyen wrote:
> From: Larysa Zaremba <larysa.zaremba@intel.com>
> 
> Commit 6624e780a577fc596788 ("ice: split ice_vsi_setup into smaller
> functions") has refactored a bunch of code involved in PFR. In this
> process, TC queue number adjustment for XDP was lost. Bring it back.
> 
> Lack of such adjustment causes interface to go into no-carrier after a
> reset, if XDP program is attached, with the following message:
> 
> ice 0000:b1:00.0: Failed to set LAN Tx queue context, error: -22
> ice 0000:b1:00.0 ens801f0np0: Failed to open VSI 0x0006 on switch 0x0001
> ice 0000:b1:00.0: enable VSI failed, err -22, VSI index 0, type ICE_VSI_PF
> ice 0000:b1:00.0: PF VSI rebuild failed: -22
> ice 0000:b1:00.0: Rebuild failed, unload and reload driver
> 
> Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_lib.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index de7ba87af45d..1bad6e17f9be 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -2371,6 +2371,9 @@ static int ice_vsi_cfg_tc_lan(struct ice_pf *pf, struct ice_vsi *vsi)
>  		} else {
>  			max_txqs[i] = vsi->alloc_txq;
>  		}
> +
> +		if (vsi->type == ICE_VSI_PF)
> +			max_txqs[i] += vsi->num_xdp_txq;

Nit: typically we always preceded this with ice_is_xdp_ena_vsi() call.
However, in this case I suppose it doesn't matter much, as if XDP prog is
not present then this value will just be 0.

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

>  	}
>  
>  	dev_dbg(dev, "vsi->tc_cfg.ena_tc = %d\n", vsi->tc_cfg.ena_tc);
> -- 
> 2.41.0
> 

Re: [PATCH net 3/3] ice: Fix PF with enabled XDP going no-carrier after reset

[Nelson, Shannon]

On 12/18/2023 11:27 AM, Tony Nguyen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> From: Larysa Zaremba <larysa.zaremba@intel.com>
> 
> Commit 6624e780a577fc596788 ("ice: split ice_vsi_setup into smaller
> functions") has refactored a bunch of code involved in PFR. In this
> process, TC queue number adjustment for XDP was lost. Bring it back.
> 
> Lack of such adjustment causes interface to go into no-carrier after a
> reset, if XDP program is attached, with the following message:
> 
> ice 0000:b1:00.0: Failed to set LAN Tx queue context, error: -22
> ice 0000:b1:00.0 ens801f0np0: Failed to open VSI 0x0006 on switch 0x0001
> ice 0000:b1:00.0: enable VSI failed, err -22, VSI index 0, type ICE_VSI_PF
> ice 0000:b1:00.0: PF VSI rebuild failed: -22
> ice 0000:b1:00.0: Rebuild failed, unload and reload driver
> 
> Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_lib.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index de7ba87af45d..1bad6e17f9be 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -2371,6 +2371,9 @@ static int ice_vsi_cfg_tc_lan(struct ice_pf *pf, struct ice_vsi *vsi)
>                  } else {
>                          max_txqs[i] = vsi->alloc_txq;
>                  }
> +
> +               if (vsi->type == ICE_VSI_PF)
> +                       max_txqs[i] += vsi->num_xdp_txq;

Since this new code is coming right after an existing
		if (vsi->type == ICE_VSI_CHNL)
it looks like it would make sense to make it an 'else if' in that last 
block, e.g.:

		if (vsi->type == ICE_VSI_CHNL) {
			if (!vsi->alloc_txq && vsi->num_txq)
				max_txqs[i] = vsi->num_txq;
			else
				max_txqs[i] = pf->num_lan_tx;
		} else if (vsi->type == ICE_VSI_PF) {
			max_txqs[i] += vsi->num_xdp_txq;
		} else {
			max_txqs[i] = vsi->alloc_txq;
		}

Of course this begins to verge on the switch/case/default format.

sln


>          }
> 
>          dev_dbg(dev, "vsi->tc_cfg.ena_tc = %d\n", vsi->tc_cfg.ena_tc);
> --
> 2.41.0
> 
> 

Re: [PATCH net 3/3] ice: Fix PF with enabled XDP going no-carrier after reset

[Larysa Zaremba]

On Tue, Dec 19, 2023 at 04:09:09PM -0800, Nelson, Shannon wrote:
> On 12/18/2023 11:27 AM, Tony Nguyen wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > 
> > 
> > From: Larysa Zaremba <larysa.zaremba@intel.com>
> > 
> > Commit 6624e780a577fc596788 ("ice: split ice_vsi_setup into smaller
> > functions") has refactored a bunch of code involved in PFR. In this
> > process, TC queue number adjustment for XDP was lost. Bring it back.
> > 
> > Lack of such adjustment causes interface to go into no-carrier after a
> > reset, if XDP program is attached, with the following message:
> > 
> > ice 0000:b1:00.0: Failed to set LAN Tx queue context, error: -22
> > ice 0000:b1:00.0 ens801f0np0: Failed to open VSI 0x0006 on switch 0x0001
> > ice 0000:b1:00.0: enable VSI failed, err -22, VSI index 0, type ICE_VSI_PF
> > ice 0000:b1:00.0: PF VSI rebuild failed: -22
> > ice 0000:b1:00.0: Rebuild failed, unload and reload driver
> > 
> > Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > Reviewed-by: Simon Horman <horms@kernel.org>
> > Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > ---
> >   drivers/net/ethernet/intel/ice/ice_lib.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> > index de7ba87af45d..1bad6e17f9be 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> > @@ -2371,6 +2371,9 @@ static int ice_vsi_cfg_tc_lan(struct ice_pf *pf, struct ice_vsi *vsi)
> >                  } else {
> >                          max_txqs[i] = vsi->alloc_txq;
> >                  }
> > +
> > +               if (vsi->type == ICE_VSI_PF)
> > +                       max_txqs[i] += vsi->num_xdp_txq;
> 
> Since this new code is coming right after an existing
> 		if (vsi->type == ICE_VSI_CHNL)
> it looks like it would make sense to make it an 'else if' in that last
> block, e.g.:
> 
> 		if (vsi->type == ICE_VSI_CHNL) {
> 			if (!vsi->alloc_txq && vsi->num_txq)
> 				max_txqs[i] = vsi->num_txq;
> 			else
> 				max_txqs[i] = pf->num_lan_tx;
> 		} else if (vsi->type == ICE_VSI_PF) {
> 			max_txqs[i] += vsi->num_xdp_txq;

Would need to be
	max_txqs[i] = vsi->alloc_txq + vsi->num_xdp_txq;

> 		} else {
> 			max_txqs[i] = vsi->alloc_txq;
> 		}
> 
> Of course this begins to verge on the switch/case/default format.
> 
> sln
> 

I was going for logic: assign default values first, adjust based on enabled 
features (well, a single feature) second. The thing that in my opinion would 
make it more clear would be replacing 'vsi->type == ICE_VSI_PF' with 
ice_is_xdp_ena_vsi(). Do you think this is worth doing?

> 
> >          }
> > 
> >          dev_dbg(dev, "vsi->tc_cfg.ena_tc = %d\n", vsi->tc_cfg.ena_tc);
> > --
> > 2.41.0
> > 
> > 
> 

Re: [PATCH net 3/3] ice: Fix PF with enabled XDP going no-carrier after reset

[Nelson, Shannon]

On 12/20/2023 1:23 AM, Larysa Zaremba wrote:
> 
> On Tue, Dec 19, 2023 at 04:09:09PM -0800, Nelson, Shannon wrote:
>> On 12/18/2023 11:27 AM, Tony Nguyen wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> From: Larysa Zaremba <larysa.zaremba@intel.com>
>>>
>>> Commit 6624e780a577fc596788 ("ice: split ice_vsi_setup into smaller
>>> functions") has refactored a bunch of code involved in PFR. In this
>>> process, TC queue number adjustment for XDP was lost. Bring it back.
>>>
>>> Lack of such adjustment causes interface to go into no-carrier after a
>>> reset, if XDP program is attached, with the following message:
>>>
>>> ice 0000:b1:00.0: Failed to set LAN Tx queue context, error: -22
>>> ice 0000:b1:00.0 ens801f0np0: Failed to open VSI 0x0006 on switch 0x0001
>>> ice 0000:b1:00.0: enable VSI failed, err -22, VSI index 0, type ICE_VSI_PF
>>> ice 0000:b1:00.0: PF VSI rebuild failed: -22
>>> ice 0000:b1:00.0: Rebuild failed, unload and reload driver
>>>
>>> Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
>>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
>>> Reviewed-by: Simon Horman <horms@kernel.org>
>>> Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/ice/ice_lib.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
>>> index de7ba87af45d..1bad6e17f9be 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
>>> @@ -2371,6 +2371,9 @@ static int ice_vsi_cfg_tc_lan(struct ice_pf *pf, struct ice_vsi *vsi)
>>>                   } else {
>>>                           max_txqs[i] = vsi->alloc_txq;
>>>                   }
>>> +
>>> +               if (vsi->type == ICE_VSI_PF)
>>> +                       max_txqs[i] += vsi->num_xdp_txq;
>>
>> Since this new code is coming right after an existing
>>                if (vsi->type == ICE_VSI_CHNL)
>> it looks like it would make sense to make it an 'else if' in that last
>> block, e.g.:
>>
>>                if (vsi->type == ICE_VSI_CHNL) {
>>                        if (!vsi->alloc_txq && vsi->num_txq)
>>                                max_txqs[i] = vsi->num_txq;
>>                        else
>>                                max_txqs[i] = pf->num_lan_tx;
>>                } else if (vsi->type == ICE_VSI_PF) {
>>                        max_txqs[i] += vsi->num_xdp_txq;
> 
> Would need to be
>          max_txqs[i] = vsi->alloc_txq + vsi->num_xdp_txq;
> 
>>                } else {
>>                        max_txqs[i] = vsi->alloc_txq;
>>                }
>>
>> Of course this begins to verge on the switch/case/default format.
>>
>> sln
>>
> 
> I was going for logic: assign default values first, adjust based on enabled
> features (well, a single feature) second. The thing that in my opinion would
> make it more clear would be replacing 'vsi->type == ICE_VSI_PF' with
> ice_is_xdp_ena_vsi(). Do you think this is worth doing?

Hmm... I made a dumb error in a quick read of the code.  This suggests 
that making the intent of the code more clear would be a good idea.  I 
think that the ice_is_xdp_ena_vsi() would definitely make it more clear 
as opposed to the bare ICE_VCSI_PF.

sln


> 
>>
>>>           }
>>>
>>>           dev_dbg(dev, "vsi->tc_cfg.ena_tc = %d\n", vsi->tc_cfg.ena_tc);
>>> --
>>> 2.41.0
>>>
>>>
>>

Re: [PATCH net 3/3] ice: Fix PF with enabled XDP going no-carrier after reset

[Paolo Abeni]

On Wed, 2023-12-20 at 09:04 -0800, Nelson, Shannon wrote:
> On 12/20/2023 1:23 AM, Larysa Zaremba wrote:
> > 
> > On Tue, Dec 19, 2023 at 04:09:09PM -0800, Nelson, Shannon wrote:
> > > On 12/18/2023 11:27 AM, Tony Nguyen wrote:
> > > > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > > > 
> > > > 
> > > > From: Larysa Zaremba <larysa.zaremba@intel.com>
> > > > 
> > > > Commit 6624e780a577fc596788 ("ice: split ice_vsi_setup into smaller
> > > > functions") has refactored a bunch of code involved in PFR. In this
> > > > process, TC queue number adjustment for XDP was lost. Bring it back.
> > > > 
> > > > Lack of such adjustment causes interface to go into no-carrier after a
> > > > reset, if XDP program is attached, with the following message:
> > > > 
> > > > ice 0000:b1:00.0: Failed to set LAN Tx queue context, error: -22
> > > > ice 0000:b1:00.0 ens801f0np0: Failed to open VSI 0x0006 on switch 0x0001
> > > > ice 0000:b1:00.0: enable VSI failed, err -22, VSI index 0, type ICE_VSI_PF
> > > > ice 0000:b1:00.0: PF VSI rebuild failed: -22
> > > > ice 0000:b1:00.0: Rebuild failed, unload and reload driver
> > > > 
> > > > Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
> > > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > > > Reviewed-by: Simon Horman <horms@kernel.org>
> > > > Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
> > > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > > > ---
> > > >    drivers/net/ethernet/intel/ice/ice_lib.c | 3 +++
> > > >    1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> > > > index de7ba87af45d..1bad6e17f9be 100644
> > > > --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> > > > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> > > > @@ -2371,6 +2371,9 @@ static int ice_vsi_cfg_tc_lan(struct ice_pf *pf, struct ice_vsi *vsi)
> > > >                   } else {
> > > >                           max_txqs[i] = vsi->alloc_txq;
> > > >                   }
> > > > +
> > > > +               if (vsi->type == ICE_VSI_PF)
> > > > +                       max_txqs[i] += vsi->num_xdp_txq;
> > > 
> > > Since this new code is coming right after an existing
> > >                if (vsi->type == ICE_VSI_CHNL)
> > > it looks like it would make sense to make it an 'else if' in that last
> > > block, e.g.:
> > > 
> > >                if (vsi->type == ICE_VSI_CHNL) {
> > >                        if (!vsi->alloc_txq && vsi->num_txq)
> > >                                max_txqs[i] = vsi->num_txq;
> > >                        else
> > >                                max_txqs[i] = pf->num_lan_tx;
> > >                } else if (vsi->type == ICE_VSI_PF) {
> > >                        max_txqs[i] += vsi->num_xdp_txq;
> > 
> > Would need to be
> >          max_txqs[i] = vsi->alloc_txq + vsi->num_xdp_txq;
> > 
> > >                } else {
> > >                        max_txqs[i] = vsi->alloc_txq;
> > >                }
> > > 
> > > Of course this begins to verge on the switch/case/default format.
> > > 
> > > sln
> > > 
> > 
> > I was going for logic: assign default values first, adjust based on enabled
> > features (well, a single feature) second. The thing that in my opinion would
> > make it more clear would be replacing 'vsi->type == ICE_VSI_PF' with
> > ice_is_xdp_ena_vsi(). Do you think this is worth doing?
> 
> Hmm... I made a dumb error in a quick read of the code.  This suggests 
> that making the intent of the code more clear would be a good idea.  I 
> think that the ice_is_xdp_ena_vsi() would definitely make it more clear 
> as opposed to the bare ICE_VCSI_PF.

I think that the current patch fits well for stable, and the issue
looks relevant enough that we should prefer have it fixed in this
cycle. Any refactoring/change would not allow such result due to the
timing.

I'll apply the series as-is, please follow-up on net-next as needed (no
rush).

Cheers,

Paolo

Re: [PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2023-12-18 (ice)

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Tony Nguyen <anthony.l.nguyen@intel.com>:

On Mon, 18 Dec 2023 11:27:03 -0800 you wrote:
> This series contains updates to ice driver only.
> 
> Jakes stops clearing of needed aggregator information.
> 
> Dave adds a check for LAG device support before initializing the
> associated event handler.
> 
> [...]

Here is the summary with links:
  - [net,1/3] ice: stop trashing VF VSI aggregator node ID information
    https://git.kernel.org/netdev/net/c/7d881346121a
  - [net,2/3] ice: alter feature support check for SRIOV and LAG
    https://git.kernel.org/netdev/net/c/4d50fcdc2476
  - [net,3/3] ice: Fix PF with enabled XDP going no-carrier after reset
    https://git.kernel.org/netdev/net/c/f5728a418945

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

Re: [PATCH net 3/3] ice: Fix PF with enabled XDP going no-carrier after reset

[Larysa Zaremba]

On Thu, Dec 21, 2023 at 08:23:39AM +0100, Paolo Abeni wrote:
> On Wed, 2023-12-20 at 09:04 -0800, Nelson, Shannon wrote:
> > On 12/20/2023 1:23 AM, Larysa Zaremba wrote:
> > > 
> > > On Tue, Dec 19, 2023 at 04:09:09PM -0800, Nelson, Shannon wrote:
> > > > On 12/18/2023 11:27 AM, Tony Nguyen wrote:
> > > > > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > > > > 
> > > > > 
> > > > > From: Larysa Zaremba <larysa.zaremba@intel.com>
> > > > > 
> > > > > Commit 6624e780a577fc596788 ("ice: split ice_vsi_setup into smaller
> > > > > functions") has refactored a bunch of code involved in PFR. In this
> > > > > process, TC queue number adjustment for XDP was lost. Bring it back.
> > > > > 
> > > > > Lack of such adjustment causes interface to go into no-carrier after a
> > > > > reset, if XDP program is attached, with the following message:
> > > > > 
> > > > > ice 0000:b1:00.0: Failed to set LAN Tx queue context, error: -22
> > > > > ice 0000:b1:00.0 ens801f0np0: Failed to open VSI 0x0006 on switch 0x0001
> > > > > ice 0000:b1:00.0: enable VSI failed, err -22, VSI index 0, type ICE_VSI_PF
> > > > > ice 0000:b1:00.0: PF VSI rebuild failed: -22
> > > > > ice 0000:b1:00.0: Rebuild failed, unload and reload driver
> > > > > 
> > > > > Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
> > > > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > > > > Reviewed-by: Simon Horman <horms@kernel.org>
> > > > > Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
> > > > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > > > > ---
> > > > >    drivers/net/ethernet/intel/ice/ice_lib.c | 3 +++
> > > > >    1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> > > > > index de7ba87af45d..1bad6e17f9be 100644
> > > > > --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> > > > > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> > > > > @@ -2371,6 +2371,9 @@ static int ice_vsi_cfg_tc_lan(struct ice_pf *pf, struct ice_vsi *vsi)
> > > > >                   } else {
> > > > >                           max_txqs[i] = vsi->alloc_txq;
> > > > >                   }
> > > > > +
> > > > > +               if (vsi->type == ICE_VSI_PF)
> > > > > +                       max_txqs[i] += vsi->num_xdp_txq;
> > > > 
> > > > Since this new code is coming right after an existing
> > > >                if (vsi->type == ICE_VSI_CHNL)
> > > > it looks like it would make sense to make it an 'else if' in that last
> > > > block, e.g.:
> > > > 
> > > >                if (vsi->type == ICE_VSI_CHNL) {
> > > >                        if (!vsi->alloc_txq && vsi->num_txq)
> > > >                                max_txqs[i] = vsi->num_txq;
> > > >                        else
> > > >                                max_txqs[i] = pf->num_lan_tx;
> > > >                } else if (vsi->type == ICE_VSI_PF) {
> > > >                        max_txqs[i] += vsi->num_xdp_txq;
> > > 
> > > Would need to be
> > >          max_txqs[i] = vsi->alloc_txq + vsi->num_xdp_txq;
> > > 
> > > >                } else {
> > > >                        max_txqs[i] = vsi->alloc_txq;
> > > >                }
> > > > 
> > > > Of course this begins to verge on the switch/case/default format.
> > > > 
> > > > sln
> > > > 
> > > 
> > > I was going for logic: assign default values first, adjust based on enabled
> > > features (well, a single feature) second. The thing that in my opinion would
> > > make it more clear would be replacing 'vsi->type == ICE_VSI_PF' with
> > > ice_is_xdp_ena_vsi(). Do you think this is worth doing?
> > 
> > Hmm... I made a dumb error in a quick read of the code.  This suggests 
> > that making the intent of the code more clear would be a good idea.  I 
> > think that the ice_is_xdp_ena_vsi() would definitely make it more clear 
> > as opposed to the bare ICE_VCSI_PF.
> 
> I think that the current patch fits well for stable, and the issue
> looks relevant enough that we should prefer have it fixed in this
> cycle. Any refactoring/change would not allow such result due to the
> timing.
> 
> I'll apply the series as-is, please follow-up on net-next as needed (no
> rush).

Ok, thanks a lot.

> 
> Cheers,
> 
> Paolo
>