Page MenuHomeFreeBSD

e1000: Correct promiscuous multicast filter handling
ClosedPublic

Authored by kbowling on Apr 16 2021, 1:28 AM.

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

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
1715

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
1715

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

sys/dev/e1000/if_em.c
1677–1678

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
1673

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.

1677–1678

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
1717

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

sys/dev/e1000/if_em.c
1717

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
1717

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
1717

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