Fix checks for vlan parameters

[mlguerrero12]

• Add check to not allow non default values of qos and proto when vlan is zero.
• Remove check when vlan is zero and qos and proto are not in config since default values are now set.

[mlguerrero12]

@adrianchiris, @SchSeba, @zeeke, could you please review this? The sriov network operator sets vlan and vlanQoS equal to zero by default which is a valid config. I'm removing the check that prevents that and adding back a check that I omitted in my last PR.

[zeeke]

LGTM

[adrianchiris]

while at it, what about vlan proto == 802.1ad and vlan==0 ?

[mlguerrero12]

nothing happens, it is ignored when vlan is set to 0 (checked with ice and mlx5 drivers). Problem with qos was that in mlx5, the qos was set even if vlan is zero (didn't happen with ice, there it is simply ignored).

Is this really a problem?
vf 0 link/ether 76:c9:0e:3f:be:95 brd ff:ff:ff:ff:ff:ff, qos 6, spoof checking off, link-state auto, trust on, query_rss off

It's a valid config. It doesn't return an error.

[adrianchiris]

nothing happens, it is ignored when vlan is set to 0 (checked with ice and mlx5 drivers).

so its invalid ? maybe we want to block it in cni ? currently 802.1ad and vlan=0 is allowed and as you say will work and be ignored.

[mlguerrero12]

It's a valid config. The driver doesn't return an error. If it was invalid, then the driver should be the one returning the error I think.

Same with the qos for mlx5 (as mentioned in the previous comment), if it was invalid as we are claiming, why would the driver allow it?

My point is, why do we want to introduce an extra layer here in the cni and what is the criteria to classify something as invalid?

[mlguerrero12]

Just to clarify, the drivers for all cases effectively set the vlan to 0. With ignore I mean the other parameters, i.e. if the qos is set to 5 and vlan to 0, the ice driver sets vlan 0 and qos 0, whereas the mlx5 sets vlan 0 and qos 5.

[mlguerrero12]

I also wonder, the fact that mlx5 allows to set vlan 0 and qos 5, doesn't it mean that it supports the vlan 0 priority tag support? When a device needs to send priority-tagged packets but it doesn't know in which vlan resides.

[SchSeba]

LGTM

[mlguerrero12]

To speed up the merge since ci is broken due to this, I will restrict the default values of qos and proto to their defaults when vlan is zero. We can discuss what I mentioned above in a following PR.

[adrianchiris]

+1 for restricting this for now until there is a use-case for it.

its not the first time we see different behavior between drivers around VF configuration such as this.