Handling set_mac_address in set_rx_mode

While adding support for secondary unicast addresses to 8021q and macvlan, I've tried keeping dev->dev_addr as global address on dev->uc_list and have drivers skip them to avoid having all dev_unicast_add users implement a state machine like this:

open(struct net_device *dev)

{
    struct net_device *lowerdev = ...;

    /* add our address in case it differs */
    if (memcmp(addr, lowerdev->dev_addr, dev->addr_len)
       dev_unicast_add(lowerdev, dev->dev_addr, dev->addr_len);
}

close(struct net_device *dev)

{
    struct net_device *lowerdev = ...;

    /* delete our address in case we've added it */
    if (memcmp(dev->dev_addr, lowerdev->dev_addr, dev->addr_len))
       dev_unicast_delete(lowerdev, dev->dev_addr, dev->addr_len);
}

set_mac_address(struct net_device *dev, void *addr)

{
    struct net_device *lowerdev = ...;

    if (!dev->flags & IFF_UP)
       return 0;

    /* delete our old address in case we've added it */
    if (memcmp(dev->dev_addr, lowerdev->dev_addr, dev->addr_len))
       dev_unicast_delete(lowerdev, dev->dev_addr, dev->addr_len);

    memcpy(dev->dev_addr, addr, dev->addr_len)

    /* add our new address in case it differs */
    if (memcmp(addr, lowerdev->dev_addr, dev->addr_len)
       dev_unicast_add(lowerdev, dev->dev_addr, dev->addr_len);

    store lowerdev->dev_addr somewhere
}

netdev_notifier(struct notifier_block, unsigned long event, void *ptr)

{
    struct net_device *lowerdev = ptr;
    struct net_device *dev = ...;

    switch (event) {
    case NETDEV_CHANGEADDR:
        /* was different before, is equal now: remove our address */
       if (memcmp(dev->dev_addr, last_lowerdev_addr, dev->addr_len) &&
           !memcmp(dev->dev_addr, lowerdev->dev_addr, dev->addr_len))
            dev_unicast_delete(lowerdev, dev->dev_addr, dev->addr_len);

       /* is different now, was equal before: add our address */
       if (memcmp(dev->dev_addr, lowerdev->dev_addr, dev->addr_len) &&
           !memcmp(dev->dev_addr, last_lowerdev_addr, dev->addr_len))
          dev_unicast_add(lowerdev, dev->dev_addr, dev->addr_len);
       break;
    }  
}

Not that its terribly complicated, but its not exactly pretty and the existing examples of address and flag synchronization all get *something* wrong, so its better to keep requirements simple.

Having the primary address on dev->uc_list would have the advantage that the existing refcounting would take care of keeping addresses around while they're still in use and drivers could just unconditionally add and remove their own addresses without worrying about needlessly putting the underlying device in promiscous mode.

Adding dev_addr to the uc_list in dev_set_mac_address turned out to be more complicated than expected though because rolling back to the previous state on error always includes an operation that might fail again (dev->set_mac_address and dev->set_rx_mode).

The easiest way to handle this would be to program the MAC address in set_rx_mode as well since then we'd only have a single operation that could fail. For e1000 this would even simplify the code because set_mac_address and set_rx_mode both program the same filters and handle the same quirks. I would expect the same to be true for other drivers as well.

If there are no objections to this I'll code something up tommorrow.

[...]

Something that is not entirely clear to me... This has zero impact on existing drivers, right?

I would not fancy updating all drivers for a new MAC addr scheme...

Jeff

Yes, since the set_rx_mode hook is new anyway this would only affect drivers offering it (which is none so far). But this idea turned not to be so good anyways, without the set_mac_address callback the driver can't validate the new address.