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.)
So if I understand what you're describing correctly, you're worried about
the DL_NOTE_PHYS_ADDR that will be generated by the underlying driver
when softmac itself does a DL_PHYS_ADDR_REQ in softmac_m_unicst(), right?
>
> 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.
>
> 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.
Yes, I think that's a viable solution.
>
> 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?)
It looks like mi_addr should be protected by mi_data_lock (from looking
at mac_unicst_{get,set}()). mac_header() would also need to be changed
to hold that lock.
-Seb
