Peter Memishian wrote: > While tying up some loose ends in softmac, we stumbled on a case that > seemed worthy of some discussion. Specifically, usually MAC address > changes will be done via DL_SET_PHYS_ADDR_REQ messages on the /dev/net > (aka GLDv3) node. Since those updates are done via the framework, > softmac doesn't need to do anything special. > > However, there is also a chance that an application will directly open the > legacy /dev node and issue a DL_SET_PHYS_ADDR_REQ. Assuming the legacy > /dev node supports DL_NOTE_PHYS_ADDR (and most do), softmac will then > receive a DLPI notification, and then needs to do a mac_unicst_update() to > notify the GLDv3 framework of the new hardware address. Our concern here > is that there is no clean way for softmac to tell the two cases apart; > both the GLDv3 framework and the underlying driver generate a DLPI > notification and in both cases softmac will find that its MAC address has > changed. Further, in the common (GLDv3) case, if softmac just blindly > calls mac_unicst_update(), it will lead to the framework generating > *another* DL_NOTE_PHYS_ADDR -- even though the framework already had that > address. (This won't lead to an infinite loop since softmac tracks the > MAC address -- but the extra notification still seems sloppy. > In the case of DL-SET_PHYS_ADDR_REQ on the /dev/net node, won't the path be something like
proto_setphysaddr_req -> mac_unicst_set(mac of the softmac endpoint) -> softmac driver -> DL_SET_PHYS_ADDR_REQ (ce DLPI driver) If the softmac driver changes the hardware address in its 'softmac' structure before sending the DL_SET_PHYS_ADDR_REQ request down to the ce driver, then the softmac won't see any address change when it receives the DL_NOTE_PHYS_ADDR notification from the ce driver. Only in the /dev case the DL_SET_PHYS_ADDR_REQ bypasses softmac. In this case the softmac will find that the address has changed and it needs to do mac_unicst_update based on the notification ? > One possible solution would be to tweak mac_unicst_update() to compare the > "new" address and against the address stored in the framework, and return > immediately if they match. With that change, softmac could also be > simplified to just always call mac_unicst_update() and not bother tracking > the current MAC address, which would be nice. > Looking at aggr, there seem to some complex cases, but it looks like the aggr driver handles the cases and keeps track of when the address really changes and calls mac_unicst_update only when the address has really changed. > Is this viable? If so, it seems like it would make sense to do this for > other "update" routines where the requested state matches the current > state known to the framework. > I guess it won't harm to have the extra check in mac_unicst_update, but I am not sure. Nicolas/Eric may know the history here. > On a related note, it seems like some locking is missing from the update > routines. For instance, mac_unicst_update() currently does a bcopy() of > the new address into mi_addr without holding any locks. This issue > becomes magnified if we add the proposed checks. (Maybe Thiru has some > thoughts on this? > yes, we need a lock at a minimum in the mac layer. Thirumalai
