diff mbox series

[iwl-net,v1] iavf: allow changing VLAN state without calling PF

Message ID 20240904120052.24561-1-michal.swiatkowski@linux.intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [iwl-net,v1] iavf: allow changing VLAN state without calling PF | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 2 blamed authors not CCed: anthony.l.nguyen@intel.com ahmed.zaki@intel.com; 6 maintainers not CCed: przemyslaw.kitszel@intel.com pabeni@redhat.com ahmed.zaki@intel.com kuba@kernel.org edumazet@google.com anthony.l.nguyen@intel.com
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 16 this patch: 16
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 30 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 70 this patch: 70
netdev/source_inline success Was 0 now: 0

Commit Message

Michal Swiatkowski Sept. 4, 2024, noon UTC 
First case:
> ip l a l $VF name vlanx type vlan id 100
> ip l d vlanx
> ip l a l $VF name vlanx type vlan id 100

As workqueue can be execute after sometime, there is a window to have
call trace like that:
- iavf_del_vlan
- iavf_add_vlan
- iavf_del_vlans (wq)

It means that our VLAN 100 will change the state from IAVF_VLAN_ACTIVE
to IAVF_VLAN_REMOVE (iavf_del_vlan). After that in iavf_add_vlan state
won't be changed because VLAN 100 is on the filter list. The final
result is that the VLAN 100 filter isn't added in hardware (no
iavf_add_vlans call).

To fix that change the state if the filter wasn't removed yet directly
to active. It is save as IAVF_VLAN_REMOVE means that virtchnl message
wasn't sent yet.

Second case:
> ip l a l $VF name vlanx type vlan id 100
Any type of VF reset ex. change trust
> ip l s $PF vf $VF_NUM trust on
> ip l d vlanx
> ip l a l $VF name vlanx type vlan id 100

In case of reset iavf driver is responsible for readding all filters
that are being used. To do that all VLAN filters state are changed to
IAVF_VLAN_ADD. Here is even longer window for changing VLAN state from
kernel side, as workqueue isn't called immediately. We can have call
trace like that:

- changing to IAVF_VLAN_ADD (after reset)
- iavf_del_vlan (called from kernel ops)
- iavf_del_vlans (wq)

Not exsisitng VLAN filters will be removed from hardware. It isn't a
bug, ice driver will handle it fine. However, we can have call trace
like that:

- changing to IAVF_VLAN_ADD (after reset)
- iavf_del_vlan (called from kernel ops)
- iavf_add_vlan (called from kernel ops)
- iavf_del_vlans (wq)

With fix for previous case we end up with no VLAN filters in hardware.
We have to remove VLAN filters if the state is IAVF_VLAN_ADD and delete
VLAN was called. It is save as IAVF_VLAN_ADD means that virtchnl message
wasn't sent yet.

Fixes: 0c0da0e95105 ("iavf: refactor VLAN filter states")
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Przemek Kitszel Sept. 5, 2024, 6:19 a.m. UTC | #1 
On 9/4/24 14:00, Michal Swiatkowski wrote:
> First case:

[...]

> Second case:

[...]

> With fix for previous case we end up with no VLAN filters in hardware.
> We have to remove VLAN filters if the state is IAVF_VLAN_ADD and delete
> VLAN was called. It is save as IAVF_VLAN_ADD means that virtchnl message
> wasn't sent yet.

I'm fine with combining the two cases into one commit as that is related

> 
> Fixes: 0c0da0e95105 ("iavf: refactor VLAN filter states")
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> ---
>   drivers/net/ethernet/intel/iavf/iavf_main.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)

[...]

> @@ -793,8 +798,17 @@ static void iavf_del_vlan(struct iavf_adapter *adapter, struct iavf_vlan vlan)
>   
>   	f = iavf_find_vlan(adapter, vlan);
>   	if (f) {
> -		f->state = IAVF_VLAN_REMOVE;

you forgot to put this line in else case below

> -		iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_DEL_VLAN_FILTER);
> +		/* IAVF_ADD_VLAN means that VLAN wasn't even added yet.
> +		 * Remove it from the list.
> +		 */
> +		if (f->state == IAVF_VLAN_ADD) {
> +			list_del(&f->list);
> +			kfree(f);
> +			adapter->num_vlan_filters--;
> +		} else {
> +			iavf_schedule_aq_request(adapter,
> +						 IAVF_FLAG_AQ_DEL_VLAN_FILTER);
> +		}
>   	}
>   
>   	spin_unlock_bh(&adapter->mac_vlan_list_lock);
Michal Swiatkowski Sept. 5, 2024, 9:04 a.m. UTC | #2 
On Thu, Sep 05, 2024 at 08:19:58AM +0200, Przemek Kitszel wrote:
> On 9/4/24 14:00, Michal Swiatkowski wrote:
> > First case:
> 
> [...]
> 
> > Second case:
> 
> [...]
> 
> > With fix for previous case we end up with no VLAN filters in hardware.
> > We have to remove VLAN filters if the state is IAVF_VLAN_ADD and delete
> > VLAN was called. It is save as IAVF_VLAN_ADD means that virtchnl message
> > wasn't sent yet.
> 
> I'm fine with combining the two cases into one commit as that is related
> 
> > 
> > Fixes: 0c0da0e95105 ("iavf: refactor VLAN filter states")
> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > ---
> >   drivers/net/ethernet/intel/iavf/iavf_main.c | 18 ++++++++++++++++--
> >   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> [...]
> 
> > @@ -793,8 +798,17 @@ static void iavf_del_vlan(struct iavf_adapter *adapter, struct iavf_vlan vlan)
> >   	f = iavf_find_vlan(adapter, vlan);
> >   	if (f) {
> > -		f->state = IAVF_VLAN_REMOVE;
> 
> you forgot to put this line in else case below
> 

Oh, sorry, thanks for finding that. Will send v2.

> > -		iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_DEL_VLAN_FILTER);
> > +		/* IAVF_ADD_VLAN means that VLAN wasn't even added yet.
> > +		 * Remove it from the list.
> > +		 */
> > +		if (f->state == IAVF_VLAN_ADD) {
> > +			list_del(&f->list);
> > +			kfree(f);
> > +			adapter->num_vlan_filters--;
> > +		} else {
> > +			iavf_schedule_aq_request(adapter,
> > +						 IAVF_FLAG_AQ_DEL_VLAN_FILTER);
> > +		}
> >   	}
> >   	spin_unlock_bh(&adapter->mac_vlan_list_lock);
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index ff11bafb3b4f..ae5e37eac761 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -773,6 +773,11 @@  iavf_vlan_filter *iavf_add_vlan(struct iavf_adapter *adapter,
 		f->state = IAVF_VLAN_ADD;
 		adapter->num_vlan_filters++;
 		iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_ADD_VLAN_FILTER);
+	} else if (f->state == IAVF_VLAN_REMOVE) {
+		/* IAVF_VLAN_REMOVE means that VLAN wasn't yet removed.
+		 * We can safely only change the state here.
+		 */
+		f->state = IAVF_VLAN_ACTIVE;
 	}
 
 clearout:
@@ -793,8 +798,17 @@  static void iavf_del_vlan(struct iavf_adapter *adapter, struct iavf_vlan vlan)
 
 	f = iavf_find_vlan(adapter, vlan);
 	if (f) {
-		f->state = IAVF_VLAN_REMOVE;
-		iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_DEL_VLAN_FILTER);
+		/* IAVF_ADD_VLAN means that VLAN wasn't even added yet.
+		 * Remove it from the list.
+		 */
+		if (f->state == IAVF_VLAN_ADD) {
+			list_del(&f->list);
+			kfree(f);
+			adapter->num_vlan_filters--;
+		} else {
+			iavf_schedule_aq_request(adapter,
+						 IAVF_FLAG_AQ_DEL_VLAN_FILTER);
+		}
 	}
 
 	spin_unlock_bh(&adapter->mac_vlan_list_lock);