Page MenuHomeFreeBSD

e1000: Correct promiscuous multicast filter handling
ClosedPublic

Authored by kbowling on Apr 16 2021, 1:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, May 18, 3:37 PM
Unknown Object (File)
Mon, May 13, 9:39 PM
Unknown Object (File)
Mon, May 13, 9:39 PM
Unknown Object (File)
Mon, May 13, 9:39 PM
Unknown Object (File)
Mon, May 13, 9:39 PM
Unknown Object (File)
Mon, May 13, 9:39 PM
Unknown Object (File)
Mon, May 13, 9:39 PM
Unknown Object (File)
Mon, May 13, 9:21 PM
Subscribers
None

Details

Summary

I saw jtl's report in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=140647

It looked like the promiscuous set had been updated since then, but there were still some of the mentioned issues so I went and stole the algorithm from ixgbe(4) which looks correct to me.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kbowling created this revision.
kbowling retitled this revision from e1000: Correct multicast filter handling to e1000: Correct promiscuous multicast filter handling .Apr 16 2021, 1:43 AM
sys/dev/e1000/if_em.c
1697

Starting from here, this doesn't match the version of em_if_multi_set() in the main branch. Is there an intermediate diff that's missing?

kbowling added inline comments.
sys/dev/e1000/if_em.c
1697

Your are right, I had stable/12 checked out for an MFC. Let me rebase onto main

sys/dev/e1000/if_em.c
1666–1667

This is a boundary condition, it is only called in em_if_multi_set, and I think with this new logic this maintains the correct count. Eyes around this would be appreciated.

sys/dev/e1000/if_em.c
1666–1667

It looks like it would be less confusing to rename "cnt" to something like "idx", since that's what it's used for in this function.

1666–1667

Then, if you think of it like an index, then this checks out -- it'd be over the bounds of mta, and so the address can't be copied.

sys/dev/e1000/if_em.c
1700

Why set these flags here? All three of the cases below will set or clear them.

sys/dev/e1000/if_em.c
1700

My read is this strips away anything unrelated from the four bytes of E1000_RCTL we get from E1000_READ_REG above. Then as we do the following &= and |= we are doing so against the previous state of just these registers. Then finally we write just these UPE and/or MPE registers. Is that initial bit strip, and frobbing only UPE and MPE at the end sensible or unnecessary?

sys/dev/e1000/if_em.c
1700

Ok I've taken one more look down to digesting what all the relevant macros are and agree with you. I'll prepare a similar change for ixgbe.

Remove unnecessary bitwise statement noted by @markj

markj added inline comments.
sys/dev/e1000/if_em.c
1700

I see, I didn't catch that this was ported from ixgbe. It was just a minor nit but made me wonder if there was a misunderstanding somewhere.

This revision is now accepted and ready to land.Apr 19 2021, 12:59 PM