Handling set_mac_address in set_rx_mode

Handling set_mac_address in set_rx_mode

[Patrick McHardy]

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.

Re: Handling set_mac_address in set_rx_mode

[Jeff Garzik]

Patrick McHardy wrote:
> 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:

[...]

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

Re: Handling set_mac_address in set_rx_mode

[Patrick McHardy]

Jeff Garzik wrote:
> Patrick McHardy wrote:
> 
>> 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:
> 
> 
> [...]
> 
> Something that is not entirely clear to me...  This has zero impact on
> existing drivers, right?


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.