| Message ID | 20230307150433.2441989-1-ahmed.zaki@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iavf: do not track VLAN 0 filters | expand |
Dear Ahmed, Thank you for your patch. Am 07.03.23 um 16:04 schrieb Ahmed Zaki: > When an interface with the maximum number of VLAN filters is brought up, > a spurious error is logged: > > [ 257.483082] 8021q: adding VLAN 0 to HW filter on device enp0s3 > [ 257.483094] iavf 0000:00:03.0 enp0s3: Max allowed VLAN filters 8. Remove existing VLANs or disable filtering via Ethtool if supported. (You might want to indent “code” blocks by four spaces.) > The VF driver complains that it cannot add the VLAN 0 filter. > > On the other hand, the PF driver always adds VLAN 0 filter on VF > initialization. The VF does not need to ask the PF for that filter at > all. > > Fix the error by not tracking VLAN 0 filters altogether. With that, the > check dded by commit 0e710a3ffd0c ("iavf: Fix VF driver counting VLAN 0 *a*dded > filters") in iavf_virtchnl.c is useless and might be confusing if left as > it suggests that we track VLAN 0. > > Fixes: 0e710a3ffd0c ("iavf: Fix VF driver counting VLAN 0 filters") > Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com> > Reviewed-by: Michal Kubiak <michal.kubiak@intel.com> > --- > drivers/net/ethernet/intel/iavf/iavf_main.c | 4 ++++ > drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 2 -- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c > index 3273aeb8fa67..d4e50f6ed14b 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c > @@ -893,6 +893,10 @@ static int iavf_vlan_rx_add_vid(struct net_device *netdev, > { > struct iavf_adapter *adapter = netdev_priv(netdev); > > + /* The PF always adds VLAN 0 filters on VF init */ > + if (!vid) > + return 0; > + > if (!VLAN_FILTERING_ALLOWED(adapter)) > return -EIO; > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > index 6d23338604bb..4e17d006c52d 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > @@ -2446,8 +2446,6 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter, > list_for_each_entry(f, &adapter->vlan_filter_list, list) { > if (f->is_new_vlan) { > f->is_new_vlan = false; > - if (!f->vlan.vid) > - continue; Is a comment warranted, that VLAN 0 is not tracked? > if (f->vlan.tpid == ETH_P_8021Q) > set_bit(f->vlan.vid, > adapter->vsi.active_cvlans); Kind regards, Paul
On 2023-03-07 08:18, Paul Menzel wrote: > Dear Ahmed, > > > Thank you for your patch. > > Am 07.03.23 um 16:04 schrieb Ahmed Zaki: >> When an interface with the maximum number of VLAN filters is brought up, >> a spurious error is logged: >> >> [ 257.483082] 8021q: adding VLAN 0 to HW filter on device enp0s3 >> [ 257.483094] iavf 0000:00:03.0 enp0s3: Max allowed VLAN filters 8. >> Remove existing VLANs or disable filtering via Ethtool if supported. > > (You might want to indent “code” blocks by four spaces.) Thanks, will do. > >> The VF driver complains that it cannot add the VLAN 0 filter. >> >> On the other hand, the PF driver always adds VLAN 0 filter on VF >> initialization. The VF does not need to ask the PF for that filter at >> all. >> >> Fix the error by not tracking VLAN 0 filters altogether. With that, the >> check dded by commit 0e710a3ffd0c ("iavf: Fix VF driver counting VLAN 0 > > *a*dded fixed in v2. > >> filters") in iavf_virtchnl.c is useless and might be confusing if >> left as >> it suggests that we track VLAN 0. >> >> Fixes: 0e710a3ffd0c ("iavf: Fix VF driver counting VLAN 0 filters") >> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com> >> Reviewed-by: Michal Kubiak <michal.kubiak@intel.com> >> --- >> drivers/net/ethernet/intel/iavf/iavf_main.c | 4 ++++ >> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 2 -- >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c >> b/drivers/net/ethernet/intel/iavf/iavf_main.c >> index 3273aeb8fa67..d4e50f6ed14b 100644 >> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c >> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c >> @@ -893,6 +893,10 @@ static int iavf_vlan_rx_add_vid(struct >> net_device *netdev, >> { >> struct iavf_adapter *adapter = netdev_priv(netdev); >> + /* The PF always adds VLAN 0 filters on VF init */ >> + if (!vid) >> + return 0; >> + >> if (!VLAN_FILTERING_ALLOWED(adapter)) >> return -EIO; >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c >> b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c >> index 6d23338604bb..4e17d006c52d 100644 >> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c >> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c >> @@ -2446,8 +2446,6 @@ void iavf_virtchnl_completion(struct >> iavf_adapter *adapter, >> list_for_each_entry(f, &adapter->vlan_filter_list, list) { >> if (f->is_new_vlan) { >> f->is_new_vlan = false; >> - if (!f->vlan.vid) >> - continue; > > Is a comment warranted, that VLAN 0 is not tracked? The vlan_filter_list is checked in other places too. I don't think it makes sense to add a comment just here. Instead, I will modify the comment in iavf_vlan_rx_add_vid() above to explicitly mention that we do not track VLAN 0 filter. > >> if (f->vlan.tpid == ETH_P_8021Q) >> set_bit(f->vlan.vid, >> adapter->vsi.active_cvlans); > > > Kind regards, > > Paul Thank you for the review.
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c index 3273aeb8fa67..d4e50f6ed14b 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_main.c +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c @@ -893,6 +893,10 @@ static int iavf_vlan_rx_add_vid(struct net_device *netdev, { struct iavf_adapter *adapter = netdev_priv(netdev); + /* The PF always adds VLAN 0 filters on VF init */ + if (!vid) + return 0; + if (!VLAN_FILTERING_ALLOWED(adapter)) return -EIO; diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c index 6d23338604bb..4e17d006c52d 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c @@ -2446,8 +2446,6 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter, list_for_each_entry(f, &adapter->vlan_filter_list, list) { if (f->is_new_vlan) { f->is_new_vlan = false; - if (!f->vlan.vid) - continue; if (f->vlan.tpid == ETH_P_8021Q) set_bit(f->vlan.vid, adapter->vsi.active_cvlans);
When an interface with the maximum number of VLAN filters is brought up, a spurious error is logged: [ 257.483082] 8021q: adding VLAN 0 to HW filter on device enp0s3 [ 257.483094] iavf 0000:00:03.0 enp0s3: Max allowed VLAN filters 8. Remove existing VLANs or disable filtering via Ethtool if supported. The VF driver complains that it cannot add the VLAN 0 filter. On the other hand, the PF driver always adds VLAN 0 filter on VF initialization. The VF does not need to ask the PF for that filter at all. Fix the error by not tracking VLAN 0 filters altogether. With that, the check dded by commit 0e710a3ffd0c ("iavf: Fix VF driver counting VLAN 0 filters") in iavf_virtchnl.c is useless and might be confusing if left as it suggests that we track VLAN 0. Fixes: 0e710a3ffd0c ("iavf: Fix VF driver counting VLAN 0 filters") Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com> Reviewed-by: Michal Kubiak <michal.kubiak@intel.com> --- drivers/net/ethernet/intel/iavf/iavf_main.c | 4 ++++ drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-)