e1000: Fix up HW vlan ops

[kbowling]

Summary

1. Call down to the hw support routine on vlan register and unregister events
2. Don't muck with the vfta if we aren't doing HW vlan filtering
3. On the I350 there is a specification update (2.4.20) in which the suggested workaround is to write to the vfta 10 times (if at first you don't succeed, try, try again). Our shared code has the goods, so use it.
4. Increase a VF's frame receive size in the case of vlans

Inspired by PR 230996 but unspecific and not sure what will happen
Tested by: Özkan KIRIK <[e-mail]>
MFC: 2 weeks

Test Plan

See if folks in PR 230996 have any luck

[kbowling]

• Don't flap the link on vlan changes
• Disable interrupts when programming a lem-class filter

[markj]

sys/dev/e1000/if_em.c

3490–3503

When an interface is first brought up, I believe we'll have adapter->num_vlans == 0 and thus won't set CTRL.VME. This means that the NIC will discard VLAN-tagged packets - is that the expected behaviour when vlan(4) isn't in use? I guess it matches the behaviour we had before if VLAN_HWTAGGING is set, but I'm not sure what's correct here.

3518

This check is unreachable, we will return above if this is a VF.

[kbowling]

sys/dev/e1000/if_em.c

3490–3503

Other platforms seem to set CTRL.VME unconditionally as long as HW Tagging capability is active (which we handle correctly lower in this patch). I guess we should match that.

I think VME is "safe" to have off without VLANs, unexpected 802.1Q packets should come up into the software stack as long as they pass any other filters whether VME is on or off (7.4.3.2 in the I210 datasheet).

I have a WIP to manage the VFE when in promiscuous mode so I'm leaning toward removing this condition now that you bring it up. I /think/ VFE is the one we need to be more careful with.

Thoughts on this?

3518

I copied this dead code just like it is over from the out of tree igb-2.5.18. I think the early return above was done because the rest of this filter management isn't ready for VFs. I was mulling over leaving this as a place holder if to try and implement more of the igb VF driver, or if that is just too much work with too little reward it can be nixed. What should we do with WIP/dead code versus the OOT driver?

[kbowling]

sys/dev/e1000/if_em.c

3518

@markj I've done a bit more research and there is evidence of people trying to use the igb vf driver https://bugs.freebsd.org/bugzilla/buglist.cgi?quicksearch=igb vf . Can I leave this in as a placeholder for some follow on work after I've made progress on some of the other basic functionality bugs people are having?

[kbowling]

Enable the VME bit if IFCAP_VLAN_HWTAGGING. Some better logic around the VFE bit, disable it when promiscuous. Add some helper functions for filter stuff.

[kbowling]

Some improvements to SRRCTL related settings pointed out by Kaho Toshikazu <[e-mail]> in PR 230996

[kbowling]

bugfix for em_if_vlan_filter_capable

[kbowling]

Spotted one more error after the last

[kbowling]

This seems ok on I210 but I am having a link stability issue on I219 after I add a vlan. I will need to check out the txrx code.

[markj]

sys/dev/e1000/if_em.c

3450

Indentation's off here.

3518

Ok, I would suggest adding a /* XXX incomplete VF support */ comment or so.

4160

Why this change? The out-of-tree driver appears to re-initialize the driver when registering or unregistering VLANs.

[kbowling]

sys/dev/e1000/if_em.c

4160

We don't want to flap the link when doing this, it impacts the user experience. So far it looks like it was just a heavy hitting way to get the filter to reprogram but I think I've addressed it, I'll need to watch closely for unexpected change.

[markj]

sys/dev/e1000/if_em.c

3323

Is this right for large MTUs? isc_max_frame_size can be larger than the receive buffers allocated by iflib. I believe iflib will allocate at most 4KB for a contiguous buffer, based on iflib_fl_setup().

I see that we supposedly handle adapter->rx_mbuf_sz > 4096 here but I think it's effectively dead code, see iflib_get_mbuf_size_for().

[kbowling]

sys/dev/e1000/if_em.c

3323

Ahh I do see the issues, I'll fix this.

[kbowling]

Remove unrelated changes, rebase on main

[jason_tubnor.net]

When patched against stable-20210923 snapshot, an undeclared identifier error occurred during build of buildkernel. This could be due to typo of psize.

sys/dev/e1000/if_em.c

3329

pszie is an undeclared identifier. This caused an abort during build. Changing it to psize allowed for a clean build. CI may throw errors during runs.

[kbowling]

sys/dev/e1000/if_em.c

3329

Thanks, yeah that hit in the commit to main :(. I MFCed this change to stable/12 and stable/13 with the typo correction squashed, so if you update you should get this fix and some other necessary ones for e1000.