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

[Florian Fainelli <f.fainelli@gmail.com>]

On 10/29/26 09:15, netdev-bot+sashiko@kernel.org wrote:
> 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.

Agreed. I’ll share the receive-mode programming logic with 
initialization, program the hash registers before enabling RX, and 
include the filter bits in the same control-register write as RX_ENABLE.

pw-bot: cr
-- 
Florian