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

[Rob Herring]

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