[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2023-04-25 (ice, iavf)

[PATCH net 0/3][pull request] Intel Wired LAN Driver Updates 2023-04-25 (ice, iavf)

[Tony Nguyen]

This series contains updates to ice and iavf drivers.

Ahmed adds setting of missed condition for statistics which caused
incorrect reporting of values for ice. For iavf, he removes a call to set
VLAN offloads during re-initialization which can cause incorrect values
to be set.

Dawid adds checks to ensure VF is ready to be reset before executing
commands that will require it to be reset on ice.

The following are changes since commit 50749f2dd6854a41830996ad302aef2ffaf011d8:
  tcp/udp: Fix memleaks of sk and zerocopy skbs with TX timestamp.
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 100GbE

Ahmed Zaki (2):
  ice: Fix stats after PF reset
  iavf: send VLAN offloading caps once after VFR

Dawid Wesierski (1):
  ice: Fix ice VF reset during iavf initialization

 .../net/ethernet/intel/iavf/iavf_virtchnl.c   |  5 -----
 drivers/net/ethernet/intel/ice/ice_lib.c      |  5 +++++
 drivers/net/ethernet/intel/ice/ice_sriov.c    |  8 ++++----
 drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 19 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_vf_lib.h   |  1 +
 drivers/net/ethernet/intel/ice/ice_virtchnl.c |  1 +
 6 files changed, 30 insertions(+), 9 deletions(-)

-- 
2.38.1

[PATCH net 1/3] ice: Fix stats after PF reset

[Tony Nguyen]

From: Ahmed Zaki <ahmed.zaki@intel.com>

After a core PF reset, the VFs were showing wrong Rx/Tx stats. This is a
regression in commit 6624e780a577 ("ice: split ice_vsi_setup into smaller
functions") caused by missing to set "stat_offsets_loaded = false" in the
ice_vsi_rebuild() path.

Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 450317dfcca7..11ae0e41f518 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -2745,6 +2745,8 @@ ice_vsi_cfg_def(struct ice_vsi *vsi, struct ice_vsi_cfg_params *params)
 			goto unroll_vector_base;
 
 		ice_vsi_map_rings_to_vectors(vsi);
+		vsi->stat_offsets_loaded = false;
+
 		if (ice_is_xdp_ena_vsi(vsi)) {
 			ret = ice_vsi_determine_xdp_res(vsi);
 			if (ret)
@@ -2793,6 +2795,9 @@ ice_vsi_cfg_def(struct ice_vsi *vsi, struct ice_vsi_cfg_params *params)
 		ret = ice_vsi_alloc_ring_stats(vsi);
 		if (ret)
 			goto unroll_vector_base;
+
+		vsi->stat_offsets_loaded = false;
+
 		/* Do not exit if configuring RSS had an issue, at least
 		 * receive traffic on first queue. Hence no need to capture
 		 * return value
-- 
2.38.1

Re: [PATCH net 1/3] ice: Fix stats after PF reset

[Leon Romanovsky]

On Tue, Apr 25, 2023 at 10:01:25AM -0700, Tony Nguyen wrote:
> From: Ahmed Zaki <ahmed.zaki@intel.com>
> 
> After a core PF reset, the VFs were showing wrong Rx/Tx stats. This is a
> regression in commit 6624e780a577 ("ice: split ice_vsi_setup into smaller
> functions") caused by missing to set "stat_offsets_loaded = false" in the
> ice_vsi_rebuild() path.
> 
> Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> 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 | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

[PATCH net 2/3] ice: Fix ice VF reset during iavf initialization

[Tony Nguyen]

From: Dawid Wesierski <dawidx.wesierski@intel.com>

Fix the current implementation that causes ice_trigger_vf_reset()
to start resetting the VF even when the VF is still resetting itself
and initializing adminq. This leads to a series of -53 errors
(failed to init adminq) from the IAVF.

Change the state of the vf_state field to be not active when the IAVF
asks for a reset. To avoid issues caused by the VF being reset too
early, make sure to wait until receiving the message on the message
box to know the exact state of the IAVF driver.

Fixes: c54d209c78b8 ("ice: Wait for VF to be reset/ready before configuration")
Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
Acked-by: Jacob Keller <Jacob.e.keller@intel.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_sriov.c    |  8 ++++----
 drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 19 +++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_vf_lib.h   |  1 +
 drivers/net/ethernet/intel/ice/ice_virtchnl.c |  1 +
 4 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index 0cc05e54a781..d4206db7d6d5 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -1181,7 +1181,7 @@ int ice_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool ena)
 	if (!vf)
 		return -EINVAL;
 
-	ret = ice_check_vf_ready_for_cfg(vf);
+	ret = ice_check_vf_ready_for_reset(vf);
 	if (ret)
 		goto out_put_vf;
 
@@ -1296,7 +1296,7 @@ int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac)
 		goto out_put_vf;
 	}
 
-	ret = ice_check_vf_ready_for_cfg(vf);
+	ret = ice_check_vf_ready_for_reset(vf);
 	if (ret)
 		goto out_put_vf;
 
@@ -1350,7 +1350,7 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted)
 		return -EOPNOTSUPP;
 	}
 
-	ret = ice_check_vf_ready_for_cfg(vf);
+	ret = ice_check_vf_ready_for_reset(vf);
 	if (ret)
 		goto out_put_vf;
 
@@ -1663,7 +1663,7 @@ ice_set_vf_port_vlan(struct net_device *netdev, int vf_id, u16 vlan_id, u8 qos,
 	if (!vf)
 		return -EINVAL;
 
-	ret = ice_check_vf_ready_for_cfg(vf);
+	ret = ice_check_vf_ready_for_reset(vf);
 	if (ret)
 		goto out_put_vf;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index 0e57bd1b85fd..59524a7c88c5 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -185,6 +185,25 @@ int ice_check_vf_ready_for_cfg(struct ice_vf *vf)
 	return 0;
 }
 
+/**
+ * ice_check_vf_ready_for_reset - check if VF is ready to be reset
+ * @vf: VF to check if it's ready to be reset
+ *
+ * The purpose of this function is to ensure that the VF is not in reset,
+ * disabled, and is both initialized and active, thus enabling us to safely
+ * initialize another reset.
+ */
+int ice_check_vf_ready_for_reset(struct ice_vf *vf)
+{
+	int ret;
+
+	ret = ice_check_vf_ready_for_cfg(vf);
+	if (!ret && !test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states))
+		ret = -EAGAIN;
+
+	return ret;
+}
+
 /**
  * ice_trigger_vf_reset - Reset a VF on HW
  * @vf: pointer to the VF structure
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
index ef30f05b5d02..3fc6a0a8d955 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
@@ -215,6 +215,7 @@ u16 ice_get_num_vfs(struct ice_pf *pf);
 struct ice_vsi *ice_get_vf_vsi(struct ice_vf *vf);
 bool ice_is_vf_disabled(struct ice_vf *vf);
 int ice_check_vf_ready_for_cfg(struct ice_vf *vf);
+int ice_check_vf_ready_for_reset(struct ice_vf *vf);
 void ice_set_vf_state_dis(struct ice_vf *vf);
 bool ice_is_any_vf_in_unicast_promisc(struct ice_pf *pf);
 void
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index e24e3f5017ca..d8c66baf4eb4 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -3908,6 +3908,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event)
 		ice_vc_notify_vf_link_state(vf);
 		break;
 	case VIRTCHNL_OP_RESET_VF:
+		clear_bit(ICE_VF_STATE_ACTIVE, vf->vf_states);
 		ops->reset_vf(vf);
 		break;
 	case VIRTCHNL_OP_ADD_ETH_ADDR:
-- 
2.38.1

Re: [PATCH net 2/3] ice: Fix ice VF reset during iavf initialization

[Leon Romanovsky]

On Tue, Apr 25, 2023 at 10:01:26AM -0700, Tony Nguyen wrote:
> From: Dawid Wesierski <dawidx.wesierski@intel.com>
> 
> Fix the current implementation that causes ice_trigger_vf_reset()
> to start resetting the VF even when the VF is still resetting itself
> and initializing adminq. This leads to a series of -53 errors
> (failed to init adminq) from the IAVF.
> 
> Change the state of the vf_state field to be not active when the IAVF
> asks for a reset. To avoid issues caused by the VF being reset too
> early, make sure to wait until receiving the message on the message
> box to know the exact state of the IAVF driver.
> 
> Fixes: c54d209c78b8 ("ice: Wait for VF to be reset/ready before configuration")
> Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
> Acked-by: Jacob Keller <Jacob.e.keller@intel.com>
> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_sriov.c    |  8 ++++----
>  drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 19 +++++++++++++++++++
>  drivers/net/ethernet/intel/ice/ice_vf_lib.h   |  1 +
>  drivers/net/ethernet/intel/ice/ice_virtchnl.c |  1 +
>  4 files changed, 25 insertions(+), 4 deletions(-)

<...>

> -	ret = ice_check_vf_ready_for_cfg(vf);
> +	ret = ice_check_vf_ready_for_reset(vf);
>  	if (ret)
>  		goto out_put_vf;

<...>

> +/**
> + * ice_check_vf_ready_for_reset - check if VF is ready to be reset
> + * @vf: VF to check if it's ready to be reset
> + *
> + * The purpose of this function is to ensure that the VF is not in reset,
> + * disabled, and is both initialized and active, thus enabling us to safely
> + * initialize another reset.
> + */
> +int ice_check_vf_ready_for_reset(struct ice_vf *vf)
> +{
> +	int ret;
> +
> +	ret = ice_check_vf_ready_for_cfg(vf);
> +	if (!ret && !test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states))
> +		ret = -EAGAIN;

I don't know your driver enough to say how it is it possible to find VF
"resetting itself" and PF trying to reset VF at the same time.

But what I see is that ICE_VF_STATE_ACTIVE bit check is racy and you
don't really fix the root cause of calling to reset without proper locking.

Thanks

> +
> +	return ret;
> +}

<...>

>  	case VIRTCHNL_OP_RESET_VF:
> +		clear_bit(ICE_VF_STATE_ACTIVE, vf->vf_states);
>  		ops->reset_vf(vf);
>  		break;
>  	case VIRTCHNL_OP_ADD_ETH_ADDR:
> -- 
> 2.38.1
> 

RE: [PATCH net 2/3] ice: Fix ice VF reset during iavf initialization

[Keller, Jacob E]

> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Tuesday, April 25, 2023 11:50 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> edumazet@google.com; netdev@vger.kernel.org; Wesierski, DawidX
> <dawidx.wesierski@intel.com>; Maziarz, Kamil <kamil.maziarz@intel.com>;
> Keller, Jacob E <jacob.e.keller@intel.com>; Romanowski, Rafal
> <rafal.romanowski@intel.com>
> Subject: Re: [PATCH net 2/3] ice: Fix ice VF reset during iavf initialization
> 
> On Tue, Apr 25, 2023 at 10:01:26AM -0700, Tony Nguyen wrote:
> > From: Dawid Wesierski <dawidx.wesierski@intel.com>
> >
> > Fix the current implementation that causes ice_trigger_vf_reset()
> > to start resetting the VF even when the VF is still resetting itself
> > and initializing adminq. This leads to a series of -53 errors
> > (failed to init adminq) from the IAVF.
> >
> > Change the state of the vf_state field to be not active when the IAVF
> > asks for a reset. To avoid issues caused by the VF being reset too
> > early, make sure to wait until receiving the message on the message
> > box to know the exact state of the IAVF driver.
> >
> > Fixes: c54d209c78b8 ("ice: Wait for VF to be reset/ready before configuration")
> > Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> > Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
> > Acked-by: Jacob Keller <Jacob.e.keller@intel.com>
> > Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_sriov.c    |  8 ++++----
> >  drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 19 +++++++++++++++++++
> >  drivers/net/ethernet/intel/ice/ice_vf_lib.h   |  1 +
> >  drivers/net/ethernet/intel/ice/ice_virtchnl.c |  1 +
> >  4 files changed, 25 insertions(+), 4 deletions(-)
> 
> <...>
> 
> > -	ret = ice_check_vf_ready_for_cfg(vf);
> > +	ret = ice_check_vf_ready_for_reset(vf);
> >  	if (ret)
> >  		goto out_put_vf;
> 
> <...>
> 
> > +/**
> > + * ice_check_vf_ready_for_reset - check if VF is ready to be reset
> > + * @vf: VF to check if it's ready to be reset
> > + *
> > + * The purpose of this function is to ensure that the VF is not in reset,
> > + * disabled, and is both initialized and active, thus enabling us to safely
> > + * initialize another reset.
> > + */
> > +int ice_check_vf_ready_for_reset(struct ice_vf *vf)
> > +{
> > +	int ret;
> > +
> > +	ret = ice_check_vf_ready_for_cfg(vf);
> > +	if (!ret && !test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states))
> > +		ret = -EAGAIN;
> 
> I don't know your driver enough to say how it is it possible to find VF
> "resetting itself" and PF trying to reset VF at the same time.
> 


VF can request a reset via virtchnl, and the PF can request a reset due to system administration activity such as changing a configuration.

> But what I see is that ICE_VF_STATE_ACTIVE bit check is racy and you
> don't really fix the root cause of calling to reset without proper locking.
> 

I think there's some confusing re-use of words going on in the commit message. It describes what the VF does while recovering and re-initializing from a reset. I think the goal is to prevent starting another reset until the first one has recovered. I am not sure we can use a standard lock here because we likely do want to be able to recover if the VF driver doesn't respond in a sufficient time.

I don't know exactly what problem this commit claims to fix.

> Thanks
> 
> > +
> > +	return ret;
> > +}
> 
> <...>
> 
> >  	case VIRTCHNL_OP_RESET_VF:
> > +		clear_bit(ICE_VF_STATE_ACTIVE, vf->vf_states);
> >  		ops->reset_vf(vf);
> >  		break;
> >  	case VIRTCHNL_OP_ADD_ETH_ADDR:
> > --
> > 2.38.1
> >

Re: [PATCH net 2/3] ice: Fix ice VF reset during iavf initialization

[Paolo Abeni]

On Wed, 2023-04-26 at 16:22 +0000, Keller, Jacob E wrote:
> > From: Leon Romanovsky <leon@kernel.org>
> 
> > But what I see is that ICE_VF_STATE_ACTIVE bit check is racy and
> > you
> > don't really fix the root cause of calling to reset without proper
> > locking.
> > 
> 
> I think there's some confusing re-use of words going on in the commit
> message. It describes what the VF does while recovering and re-
> initializing from a reset. I think the goal is to prevent starting
> another reset until the first one has recovered. 

Uhmm... it looks like the current patch does not prevent two concurrent
resets, I think the goal of this patch is let other vf related ndo
restart gracefully when a VF reset is running.

> I am not sure we can use a standard lock here because we likely do
> want to be able to recover if the VF driver doesn't respond in a
> sufficient time.
> 
> I don't know exactly what problem this commit claims to fix.

I think this patch could benefit from at least a more
descriptive/clearer commit message.

Thanks,

Paolo

RE: [PATCH net 2/3] ice: Fix ice VF reset during iavf initialization

[Maziarz, Kamil]

[-- Attachment #1: Type: text/plain, Size: 9529 bytes --]

On Wed, 2023-04-26 at 16:22 +0000, Keller, Jacob E wrote:
> > > From: Leon Romanovsky <leon@kernel.org>
> >
> > > But what I see is that ICE_VF_STATE_ACTIVE bit check is racy and you 
> > > don't really fix the root cause of calling to reset without proper 
> > > locking.
> > > 
>  > 
> > I think there's some confusing re-use of words going on in the commit 
> > message. It describes what the VF does while recovering and re- 
> > initializing from a reset. I think the goal is to prevent starting 
> > another reset until the first one has recovered.

> Uhmm... it looks like the current patch does not prevent two concurrent resets, I think the goal of this patch is let other vf related ndo restart gracefully when a VF reset is running.

> > I am not sure we can use a standard lock here because we likely do 
> > want to be able to recover if the VF driver doesn't respond in a 
> > sufficient time.
> > 
> > I don't know exactly what problem this commit claims to fix.

> I think this patch could benefit from at least a more descriptive/clearer commit message.

> Thanks,

> Paolo

Dear All,

Forwarding message from the author:

"Hi, this patch addresses an issue where during concurrent resets from the pf-side iavf driver was unable to finish the reset because the pf started the reset procedure again unaware that the iavf reset completion is not done. 

the pf is setting the hw registers responsible for adminq length among other things which causes the iavf flow to throw errors when the iavf is unable to setup adminq's. 

this issue looks something like this

you can see the iavf reset flow on the 28 core 

28) ! 550.242 us  |      iavf_free_rx_resources [iavf]();
 34)               |  /* WE TAKE RTNL_LOCK HERE */
 34)   1.385 us    |  i40e_get_phys_port_id [i40e]();
 34)               |  i40e_get_netdev_stats_struct [i40e]() {
 34)   0.410 us    |    __rcu_read_lock();
 34)   0.346 us    |    __rcu_read_unlock();
 34)   3.821 us    |  }
 34)               |  i40e_ndo_get_vf_config [i40e]() {
 34)               |    i40e_validate_vf [i40e]() {
 34)   0.738 us    |      i40e_find_vsi_from_id [i40e]();
 34)   1.654 us    |    }
 34)   2.755 us    |  }
 34)               |  i40e_get_vf_stats [i40e]() {
 34)               |    i40e_validate_vf [i40e]() {
 34)   0.546 us    |      i40e_find_vsi_from_id [i40e]();
 34)   1.194 us    |    }
 34)               |    i40e_update_eth_stats [i40e]() {
 34)   1.456 us    |      i40e_stat_update48 [i40e]();
 34)   1.238 us    |      i40e_stat_update48 [i40e]();
 34)   1.231 us    |      i40e_stat_update48 [i40e]();
 34)   1.157 us    |      i40e_stat_update48 [i40e]();
 34)   1.375 us    |      i40e_stat_update48 [i40e]();
 34)   1.311 us    |      i40e_stat_update48 [i40e]();
 34)   1.167 us    |      i40e_stat_update48 [i40e]();
 34)   1.139 us    |      i40e_stat_update48 [i40e]();
 34) + 19.299 us   |    }
 34) + 22.060 us   |  }
 34)               |  /* WE UNLOCK RTNL_UNLOCK HERE */
 34)               |  /* WE TAKE RTNL_LOCK HERE */
 34)               |  i40e_ndo_set_vf_trust [i40e]() {
 34)               |    i40e_vc_reset_vf [i40e]() {
 34)               |      i40e_vc_notify_vf_reset [i40e]() {
 34)               |        /* i40e_vc_notify_vf_reset PASS 
test  &vf->vf_states == 1  */
 34) ! 104.964 us  |      }
 34)               |      /* we are i40e_vc_reset_vf with state== 0 */
 34)               |      i40e_reset_vf [i40e]() {
 34)               |        /* i40e_reset_vf -- test_and_set_bit(I40E_VF_STATE_RESETTING, &vf->vf_states)  == false */
 34)               |        /* i40e_trigger_vf_reset unsets I40E_VF_STATE_ACTIVE flag */
 ------------------------------------------
 33) kworker-22304  =>    <idle>-0   
 ------------------------------------------

 33)               |  i40e_intr [i40e]() {
 33)               |    i40e_service_event_schedule [i40e]() {
 33) + 14.017 us   |      queue_work_on();
 33) + 15.544 us   |    }
 33) + 22.424 us   |  }
 ------------------------------------------
 33)    <idle>-0    => kworker-22304 
 ------------------------------------------

 33)               |  i40e_service_task [i40e]() {
 33)   5.570 us    |    i40e_detect_recover_hung [i40e]();
 33)               |    i40e_sync_filters_subtask [i40e]() {
 33) + 11.219 us   |      i40e_sync_vsi_filters [i40e]();
 33) + 13.328 us   |    }
 33)   0.616 us    |    i40e_reset_subtask [i40e]();
 33)               |    i40e_vc_process_vflr_event [i40e]() {
 33)               |      /* i40e_vc_process_vflr_event -> writes into hw reg 0x64f90000 */
 33)   4.683 us    |    }
 33)   1.522 us    |    i40e_fdir_check_and_reenable [i40e]();
 33)   0.516 us    |    i40e_client_subtask [i40e]();
 33)   0.511 us    |    i40e_sync_filters_subtask [i40e]();
 33)               |    kmalloc_trace() {
 33)   1.564 us    |      __kmem_cache_alloc_node();
 28) ! 750.262 us  |      iavf_free_rx_resources [iavf]();
 33)   2.913 us    |    }
 33)               |    i40e_clean_arq_element [i40e]() {
 33)   0.644 us    |      mutex_lock();
 33)   0.353 us    |      mutex_unlock();
 33)   2.914 us    |    }
 33)               |    kfree() {
 33)   0.571 us    |      __kmem_cache_free();
 33)   3.267 us    |    }
 33) + 45.828 us   |  }
 28) ! 628.357 us  |      iavf_free_rx_resources [iavf]();
 28) ! 618.235 us  |      iavf_free_rx_resources [iavf]();
 28) # 2549.539 us |    }
 28)               |    iavf_free_all_tx_resources.part.49 [iavf]() {
 28) + 60.257 us   |      iavf_free_tx_resources [iavf]();
 28) + 59.489 us   |      iavf_free_tx_resources [iavf]();
 28) + 59.960 us   |      iavf_free_tx_resources [iavf]();
 28) + 60.755 us   |      iavf_free_tx_resources [iavf]();
 28) ! 242.568 us  |    }
 28)               |    iavf_shutdown_adminq [iavf]() {
 28)   1.450 us    |      iavf_check_asq_alive [iavf]();
 28)   2.275 us    |      iavf_aq_queue_shutdown [iavf]();
 28) ! 137.843 us  |      iavf_shutdown_asq [iavf]();
 28)   0.597 us    |      mutex_lock();
 28)   4.232 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.562 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.553 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.521 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.498 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.514 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.602 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.455 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.564 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.543 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.574 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.701 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.998 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.482 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.542 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.522 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.472 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.580 us    |      iavf_free_dma_mem_d [iavf]();
 28)   3.143 us    |      iavf_free_dma_mem_d [iavf]();
 28)   3.015 us    |      iavf_free_dma_mem_d [iavf]();
 28)   3.072 us    |      iavf_free_dma_mem_d [iavf]();
 28)   3.104 us    |      iavf_free_dma_mem_d [iavf]();
 28)   3.033 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.458 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.486 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.518 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.482 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.564 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.906 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.843 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.828 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.911 us    |      iavf_free_dma_mem_d [iavf]();
 28)   2.879 us    |      iavf_free_dma_mem_d [iavf]();
 28)   0.637 us    |      iavf_free_virt_mem_d [iavf]();
 28)   0.393 us    |      mutex_unlock();
 28) ! 250.148 us  |    }
 28)               |    iavf_init_adminq [iavf]() {
 28)               |      /* iavf_init_adminq(hw->hw->aq.asq.bal == 0xdeadbeef */
 28) * 30059.69 us |      usleep_range_state();
 34)               |        /* i40e_reset_vf we read the VFRSTAT and got 0x1  */
 28)               |      /* iavf_init_asq(hw->hw->aq.asq.bal == 0xdeadbeef */
 28)   6.043 us    |      iavf_allocate_dma_mem_d [iavf]();
 28) + 37.481 us   |      iavf_allocate_virt_mem_d [iavf]();
 28)               |      /* allocate the ring memory(hw->hw->aq.asq.bal == 0xdeadbeef */
 28) + 11.787 us   |      _printk();
 28)               |      /* iavf_init_asq_1(hw->hw->aq.asq.bal == 0xdeadbeef */
 28)   1.243 us    |      iavf_allocate_virt_mem_d [iavf]();

thread 28 - is doing the iavf restet
while the 33 thread is cutting in and starts another pf triggerd vf reset 

As you can see this means that the pf isn't able to recognize when the vf reset is done it thinks that the vf reset already finished initialization. 
Im using existing messaging architecture to address that.

the whole thought process behind addressing this could be seen inside the attached pdf.
The flows are quite complicated and there are 2 major challenges here.

Resets induced by the iavf as well as i40e driver.

As I understand the wording of the patch is confusing and should be corrected ?

Thanks for your input "

Thanks,
Kamil

[-- Attachment #2: reset_pf_reset_change_and_bug.pdf --]
[-- Type: application/pdf, Size: 166749 bytes --]

[PATCH net 3/3] iavf: send VLAN offloading caps once after VFR

[Tony Nguyen]

From: Ahmed Zaki <ahmed.zaki@intel.com>

When the user disables rxvlan offloading and then changes the number of
channels, all VLAN ports are unable to receive traffic.

Changing the number of channels triggers a VFR reset. During re-init, when
VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS is received, we do:
1 - set the IAVF_FLAG_SETUP_NETDEV_FEATURES flag
2 - call
    iavf_set_vlan_offload_features(adapter, 0, netdev->features);

The second step sends to the PF the __default__ features, in this case
aq_required |= IAVF_FLAG_AQ_ENABLE_CTAG_VLAN_STRIPPING

While the first step forces the watchdog task to call
netdev_update_features() ->  iavf_set_features() ->
iavf_set_vlan_offload_features(adapter, netdev->features, features).
Since the user disabled the "rxvlan", this sets:
aq_required |= IAVF_FLAG_AQ_DISABLE_CTAG_VLAN_STRIPPING

When we start processing the AQ commands, both flags are enabled. Since we
process DISABLE_XTAG first then ENABLE_XTAG, this results in the PF
enabling the rxvlan offload. This breaks all communications on the VLAN
net devices.

Fix by removing the call to iavf_set_vlan_offload_features() (second
step). Calling netdev_update_features() from watchdog task is enough for
both init and reset paths.

Fixes: 7598f4b40bd6 ("iavf: Move netdev_update_features() into watchdog task")
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 9afbbdac3590..7c0578b5457b 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -2238,11 +2238,6 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 		iavf_process_config(adapter);
 		adapter->flags |= IAVF_FLAG_SETUP_NETDEV_FEATURES;
 
-		/* Request VLAN offload settings */
-		if (VLAN_V2_ALLOWED(adapter))
-			iavf_set_vlan_offload_features(adapter, 0,
-						       netdev->features);
-
 		iavf_set_queue_vlan_tag_loc(adapter);
 
 		was_mac_changed = !ether_addr_equal(netdev->dev_addr,
-- 
2.38.1

Re: [PATCH net 3/3] iavf: send VLAN offloading caps once after VFR

[Leon Romanovsky]

On Tue, Apr 25, 2023 at 10:01:27AM -0700, Tony Nguyen wrote:
> From: Ahmed Zaki <ahmed.zaki@intel.com>
> 
> When the user disables rxvlan offloading and then changes the number of
> channels, all VLAN ports are unable to receive traffic.
> 
> Changing the number of channels triggers a VFR reset. During re-init, when
> VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS is received, we do:
> 1 - set the IAVF_FLAG_SETUP_NETDEV_FEATURES flag
> 2 - call
>     iavf_set_vlan_offload_features(adapter, 0, netdev->features);
> 
> The second step sends to the PF the __default__ features, in this case
> aq_required |= IAVF_FLAG_AQ_ENABLE_CTAG_VLAN_STRIPPING
> 
> While the first step forces the watchdog task to call
> netdev_update_features() ->  iavf_set_features() ->
> iavf_set_vlan_offload_features(adapter, netdev->features, features).
> Since the user disabled the "rxvlan", this sets:
> aq_required |= IAVF_FLAG_AQ_DISABLE_CTAG_VLAN_STRIPPING
> 
> When we start processing the AQ commands, both flags are enabled. Since we
> process DISABLE_XTAG first then ENABLE_XTAG, this results in the PF
> enabling the rxvlan offload. This breaks all communications on the VLAN
> net devices.
> 
> Fix by removing the call to iavf_set_vlan_offload_features() (second
> step). Calling netdev_update_features() from watchdog task is enough for
> both init and reset paths.
> 
> Fixes: 7598f4b40bd6 ("iavf: Move netdev_update_features() into watchdog task")
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 5 -----
>  1 file changed, 5 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>