[Intel-wired-lan] [PATCH] iavf: do not track VLAN 0 filters

[Ahmed Zaki]

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 at intel.com>
>> Reviewed-by: Michal Kubiak <michal.kubiak at 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.