Details

Reviewers

jtl
markj
marius
erj

Group Reviewers

Intel Networking

Commits

rGab177386aa87: e1000: Correct promisc multicast filter handling
rG938b01476a3c: e1000: Correct promisc multicast filter handling
rG4b38eed76da9: e1000: Correct promisc multicast filter handling

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 requested review of this revision.Apr 16 2021, 1:28 AM

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

markj added inline comments.Apr 16 2021, 1:32 PM

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 planned changes to this revision.Apr 16 2021, 5:17 PM

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

Rebased onto main

kbowling added inline comments.Apr 16 2021, 6:06 PM

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.

erj added inline comments.Apr 16 2021, 7:01 PM

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.

Address @erj feedback

kbowling marked 2 inline comments as done.Apr 16 2021, 8:43 PM

kbowling mentioned this in D29798: ixgbe: Clarify variable index usage in ixgbe_mc_filter_apply.Apr 16 2021, 8:50 PM

markj added inline comments.Apr 18 2021, 8:23 PM

sys/dev/e1000/if_em.c
1700	Why set these flags here? All three of the cases below will set or clear them.

kbowling added inline comments.Apr 18 2021, 10:28 PM

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?