Page MenuHomeFreeBSD

iflib:correct vlan register/unregister
Needs RevisionPublic

Authored by kaho_elam.kais.kyoto-u.ac.jp on May 26 2021, 2:35 AM.

Details

Reviewers
shurd
erj
kbowling
Group Reviewers
iflib
Summary

if_vlan.c calls the function of vlan register/unregister with (struct ifnet *) value,
but iflib.c registers with (struct iflib_ctx *) .
iflib can not catch a vlan creation or destroy event by this mismatch.

see Bug #230996

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

kbowling added a subscriber: kbowling.

I see it clearly now, and agree. Thanks for the fix!

This revision is now accepted and ready to land.May 28 2021, 6:37 AM

I don't understand this change. If you look at EVENTHANDLER_INVOKE, it traverses the list of registered handlers and calls each one with

_t->eh_func(_ep->ee_arg , ## __VA_ARGS__);

so the argument passed with EVENTHANDLER_REGISTER is the first parameter, followed by the parent ifnet and VID. iflib specifies the ctx (stored in ifp->if_softc) as the argument, and in iflib_vlan_register() we compare the argument with ifp->if_softc.

I tried adding some prints around this code and configured a vlan on an igb interface, and iflib indeed handled the event. So I can't see what problem this diff is fixing.

erj requested changes to this revision.May 28 2021, 10:55 PM
erj added a subscriber: erj.

I also don't really understand what this is supposed to fix; we haven't had problems registering VLANs at all.

This revision now requires changes to proceed.May 28 2021, 10:55 PM
In D30463#685828, @erj wrote:

I also don't really understand what this is supposed to fix; we haven't had problems registering VLANs at all.

I'm sorry for you to be less information. The vlanhwtag flag on a network interface requires to reproduce a problem.
A network driver don't require vlan tags if vlan hardware offload is disabled.

I don't understand this change. If you look at EVENTHANDLER_INVOKE, it traverses the list of registered handlers and calls each one with

_t->eh_func(_ep->ee_arg , ## __VA_ARGS__);

so the argument passed with EVENTHANDLER_REGISTER is the first parameter, followed by the parent ifnet and VID. iflib specifies the ctx (stored in ifp->if_softc) as the argument, and in iflib_vlan_register() we compare the argument with ifp->if_softc.

I tried adding some prints around this code and configured a vlan on an igb interface, and iflib indeed handled the event. So I can't see what problem this diff is fixing.

Yes, I look at EVENTHANDLER_INVOKE. I can understand relation between EVENTHANDLER_REGISTER and comparison of arguments, and your comment seems to be correct. The patch is not broke anything but is not fixed anything.

I am tracking igb vlan problem of Bug #230996, and I could not observe em_if_vlan_register() execution. Perhaps my debug printf code was corrupted. I'm sorry this noise.

Yes I apologize for my own misunderstanding, I saw this patch in BZ and was confused previously and didn't see it helping the related e1000 issues. Upon seeing it here again, it seemed like the if statement WRT ctx and arg would be incorrect if receiving an event from the vlan handler, but I see that we control the argument with our EVENTHANDLER_REGISTER here so it is consistent. @kaho_elam.kais.kyoto-u.ac.jp I believe the problems you are pursuing are a combination of filter management in e1000 and a couple issues in the em_txrx.c. I have today off so I'll see if I can make some progress, I've found some bugs in testing so nothing is ready for commit yet but here's my works in progress: D30002 and D30072