[SRU][saucy][PATCH] net: calxedaxgmac: add mac address learning
Rob Herring
robherring2 at gmail.com
Tue Nov 19 16:11:49 UTC 2013
On 11/19/2013 08:42 AM, Tim Gardner wrote:
> 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() ?
Yes, "bridge fdb show" shows the entries in the list. I use pktgen to
generate packets with new source MAC addresses. Then checking with
"bridge fdb show" I can see they are added. Then with devmem2 I can
check the h/w registers directly. Also with devmem2, I can clear the AE
bit to trigger a removal manually and verify with "bridge fdb show".
Rob
More information about the kernel-team
mailing list