[SRU][saucy][PATCH] net: calxedaxgmac: add mac address learning

[Tim Gardner]

On 11/15/2013 02:32 PM, Rob Herring wrote:
> On 11/15/2013 01:54 PM, Tim Gardner wrote:
>> More comments inline.
>>
>> On 11/14/2013 02:01 PM, Rob Herring wrote:
>>> From: Rob Herring <rob.herring at calxeda.com>
>>>
>>> The Calxeda xgmac must learn secondary mac addresses such as those
>>> behind a bridge in order to properly receive packets with those mac
>>> addresses. Add mac address learning on transmit with timestamping of
>>> entries. The mac addresses are added to the driver's unicast address
>>> list, so they are visible to user via "bridge fdb show" command.
>>>
>>> Addresses can get disabled when the AE bit is cleared, so address
>>> registers must be checked for possible removal and update the unicast
>>> list.
>>>
>>> Signed-off-by: Rob Herring <rob.herring at calxeda.com>
>>> ---
>>>  drivers/net/ethernet/calxeda/xgmac.c | 128 ++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 127 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/calxeda/xgmac.c b/drivers/net/ethernet/calxeda/xgmac.c
>>> index 48f5288..584aca8 100644
>>> --- a/drivers/net/ethernet/calxeda/xgmac.c
>>> +++ b/drivers/net/ethernet/calxeda/xgmac.c
>>> @@ -360,6 +360,13 @@ struct xgmac_extra_stats {
>>>  	unsigned long rx_process_stopped;
>>>  	unsigned long tx_early;
>>>  	unsigned long fatal_bus_error;
>>> +	unsigned long learning_drop;
>>> +};
>>> +
>>> +struct xgmac_mac {
>>> +	char mac_addr[ETH_ALEN];
>>> +	unsigned long time;
>>> +	bool rm_pending;
>>>  };
>>>  
>>>  struct xgmac_priv {
>>> @@ -384,6 +391,8 @@ struct xgmac_priv {
>>>  	struct napi_struct napi;
>>>  
>>>  	int max_macs;
>>> +	struct xgmac_mac mac_list[32];
>>> +
>>>  	struct xgmac_extra_stats xstats;
>>>  
>>>  	spinlock_t stats_lock;
>>> @@ -392,6 +401,7 @@ struct xgmac_priv {
>>>  	char tx_pause;
>>>  	int wolopts;
>>>  	struct work_struct tx_timeout_work;
>>> +	struct delayed_work mac_aging_work;
>>>  };
>>>  
>>>  /* XGMAC Configuration Settings */
>>> @@ -634,7 +644,7 @@ static void xgmac_set_mac_addr(void __iomem *ioaddr, unsigned char *addr,
>>>  	}
>>>  }
>>>  
>>> -static void xgmac_get_mac_addr(void __iomem *ioaddr, unsigned char *addr,
>>> +static bool xgmac_get_mac_addr(void __iomem *ioaddr, unsigned char *addr,
>>>  			       int num)
>>>  {
>>>  	u32 hi_addr, lo_addr;
>>> @@ -650,6 +660,8 @@ static void xgmac_get_mac_addr(void __iomem *ioaddr, unsigned char *addr,
>>>  	addr[3] = (lo_addr >> 24) & 0xff;
>>>  	addr[4] = hi_addr & 0xff;
>>>  	addr[5] = (hi_addr >> 8) & 0xff;
>>> +
>>> +	return (hi_addr & XGMAC_ADDR_AE) ? true : false;
>>>  }
>>>  
>>>  static int xgmac_set_flow_ctrl(struct xgmac_priv *priv, int rx, int tx)
>>> @@ -1047,6 +1059,8 @@ static int xgmac_open(struct net_device *dev)
>>>  	writel(DMA_INTR_DEFAULT_MASK, ioaddr + XGMAC_DMA_STATUS);
>>>  	writel(DMA_INTR_DEFAULT_MASK, ioaddr + XGMAC_DMA_INTR_ENA);
>>>  
>>> +	schedule_delayed_work(&priv->mac_aging_work, 5 * HZ);
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> @@ -1059,6 +1073,7 @@ static int xgmac_open(struct net_device *dev)
>>>  static int xgmac_stop(struct net_device *dev)
>>>  {
>>>  	struct xgmac_priv *priv = netdev_priv(dev);
>>> +	int i;
>>>  
>>>  	netif_stop_queue(dev);
>>>  
>>> @@ -1073,9 +1088,111 @@ static int xgmac_stop(struct net_device *dev)
>>>  	/* Release and free the Rx/Tx resources */
>>>  	xgmac_free_dma_desc_rings(priv);
>>>  
>>> +	cancel_delayed_work_sync(&priv->mac_aging_work);
>>> +	for (i = 0; i < priv->max_macs; i++) {
>>> +		if (!is_valid_ether_addr(priv->mac_list[i].mac_addr))
>>> +			continue;
>>> +		dev_uc_del(dev, priv->mac_list[i].mac_addr);
>>> +		memset(&priv->mac_list[i], 0, sizeof(priv->mac_list[i]));
>>> +	}
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> +static void xgmac_check_mac_addrs(struct xgmac_priv *priv)
>>> +{
>>> +	int i, j;
>>> +	char addr[ETH_ALEN];
>>> +
>>> +	for (i = 1; i < priv->max_macs; i++) {
>>> +		if (xgmac_get_mac_addr(priv->base, addr, i))
>>> +			continue;
>>> +
>>> +		if (!is_valid_ether_addr(addr))
>>> +			break;
>>> +
>>> +		for (j = 1; j < priv->max_macs; j++) {
>>> +			if (!ether_addr_equal(addr, priv->mac_list[j].mac_addr))
>>> +				continue;
>>> +			priv->mac_list[j].rm_pending = true;
>>> +			break;
>>> +		}
>>> +	}
>>> +}
>>
>> Shouldn't there be a test for how old a MAC address is before you mark
>> it for removal ? Otherwise every address in priv->mac_list that is also
>> in the HW table gets marked for removal each time this function is
>> called. Are entries getting removed from the HW table somewhere else ?
> 
> Yes. There is a valid bit which can be cleared by the firmware and we
> have to check this before changing the MAC registers. This would only
> happen if the same address shows up on another node which is the case
> for VM migration (which isn't really fully baked on arm yet).
> 
>> xgmac_check_mac_addrs() is also called from xgmac_set_rx_mode(). I just
>> can't see how that is going to work when addition of an address to the
>> HW table is predicated on xgmac_mac_rm_pending() being false.
> 
> The flow here is a bit weird because I can't call dev_uc_del within
> xgmac_set_rx_mode as the list lock is already held. So we have this
> intermediate state where the address needs to be removed but is still in
> the unicast list. The check is simply to avoid re-adding an address.
> 
> Rob
> 

Well, I still think this doesn't do you intend. How about adding some
debug so we can see when these MAC addresses come and go ? If debug is
flooding the log then we'll know something is wrong 'cause it should
only happen when you migrate a VM.

I wonder if there is already a general way of viewing MAC addresses that
have been entered into the dev MAC table via  dev_uc_add()/dev_uc_del() ?

rtg
-- 
Tim Gardner tim.gardner at canonical.com