Re: [PATCH net 08/11] net: au1000: restore receive filtering after MAC initialization

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In au1000_init() (drivers/net/ethernet/amd/au1000_eth.c), the
  control register is written with MAC_RX_ENABLE | MAC_TX_ENABLE and no…

--

net: au1000: restore receive filtering after MAC initialization

This patch reapplies the current netdev receive mode at the end of
au1000_init(). Promiscuous and multicast settings are then kept after a
watchdog reset. It also takes aup->lock around the control register
read-modify-write in au1000_multicast_list().

> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
> index f8976e324f0e7..6b817d18329b2 100644
> --- a/drivers/net/ethernet/amd/au1000_eth.c
> +++ b/drivers/net/ethernet/amd/au1000_eth.c

[ ... ]

> @@ -742,6 +744,9 @@ static int au1000_init(struct net_device *dev)
>  	wmb(); /* drain writebuffer */
>  
>  	spin_unlock_irqrestore(&aup->lock, flags);
> +	netif_addr_lock_bh(dev);
> +	au1000_multicast_list(dev);
> +	netif_addr_unlock_bh(dev);

[Severity: Low]
Is there a short window here where the receiver runs without the
requested filter?

A little earlier, au1000_init() enables RX and TX with none of the filter
bits set:

	if (!dev->phydev || dev->phydev->link)
		control |= MAC_RX_ENABLE | MAC_TX_ENABLE;

	writel(control, &aup->mac->control);

Then aup->lock is dropped. Only after that does au1000_multicast_list()
program multi_hash_high/low and set MAC_HASH_MODE, MAC_PROMISCUOUS or
MAC_PASS_ALL_MULTI, using a second read-modify-write.

This can happen on the au1000_tx_timeout()->au1000_reset_mac()->au1000_init()
path when the link is up, or on open when there is no PHY. In between the
two writes, frames for joined multicast groups, or non-local frames in
promiscuous mode, could be dropped.

The window is small next to the reset delays that come before it. On open
it is also shorter than before, since __dev_open() calls dev_set_rx_mode()
only after ndo_open. The window is still there at the end of the series in
"net: au1000: keep the MAC stopped until the PHY link is ready" when there
is no PHY or the link is up.

Would it be worth computing the filter bits and adding them to control
before the single writel()? Then enabling RX and programming the filter
would happen in one step.

>  	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027223140.3774570-1-f.fainelli%40gmail.com