Re: [PATCH net-next 2/3] net: add retry mechanism to ndo_set_rx_mode_async

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 26 May 2026 18:41:16 -0700 Stanislav Fomichev wrote:
> When ndo_set_rx_mode_async returns an error, schedule a retry with
> exponential backoff (1s, 2s, 4s, 8s — 15s total). Give up after the

Em-dash detected. Issuing a brown bag and initiating shaming procedures.

> 4th retry and log an error via netdev_err().
> 
> This moves retry logic from individual drivers into the core stack.


> @@ -2327,6 +2329,8 @@ struct net_device {
>  	struct list_head	rx_mode_node;
>  	netdevice_tracker	rx_mode_tracker;
>  	struct netdev_hw_addr_list	rx_mode_addr_cache;
> +	struct delayed_work	rx_mode_retry_work;

Either replace the existing work or only add a timer?
Having two work structs feels odd.

> +	unsigned long		rx_mode_retry_delay;

int should be plenty bits for this.
Actually for clarity maybe it's better to keep a counter here?
easy enough to (base << dev->bla_retry_counter) at scheduling
time and that way we don't have to remember what the init value
should be. Retry counter resetting to 0 is quite obvious, and
so is the cap value.

>  #ifdef CONFIG_LOCKDEP
>  	unsigned char		nested_level;
>  #endif
> @@ -5150,6 +5154,7 @@ static inline void __dev_mc_unsync(struct net_device *dev,
>  
>  /* Functions used for secondary unicast and multicast support */
>  void dev_set_rx_mode(struct net_device *dev);
> +void netif_rx_mode_schedule_retry(struct net_device *dev);
>  int netif_set_promiscuity(struct net_device *dev, int inc);
>  int dev_set_promiscuity(struct net_device *dev, int inc);
>  int netif_set_allmulti(struct net_device *dev, int inc, bool notify);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 26ac8eb9b259..751d6b5c4e23 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1775,6 +1775,8 @@ static void __dev_close_many(struct list_head *head)
>  		if (ops->ndo_stop)
>  			ops->ndo_stop(dev);
>  
> +		netif_rx_mode_cancel_retry(dev);
> +
>  		netif_set_up(dev, false);
>  		netpoll_poll_enable(dev);
>  	}
> @@ -11720,6 +11722,8 @@ void netdev_run_todo(void)
>  			continue;
>  		}
>  
> +		if (cancel_delayed_work_sync(&dev->rx_mode_retry_work))
> +			dev_put(dev);

trackers required these days
Perhaps you can avoid the reference if you use disable_.._sync() ?

>  		netdev_lock(dev);
>  		WRITE_ONCE(dev->reg_state, NETREG_UNREGISTERED);
>  		netdev_unlock(dev);
> @@ -12100,8 +12104,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>  #endif
>  
>  	mutex_init(&dev->lock);
> -	INIT_LIST_HEAD(&dev->rx_mode_node);
> -	__hw_addr_init(&dev->rx_mode_addr_cache);
> +	netif_rx_mode_init(dev);
>  
>  	dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
>  	setup(dev);
> diff --git a/net/core/dev.h b/net/core/dev.h
> index 0cf24b8f5008..4352f39c8dfe 100644
> --- a/net/core/dev.h
> +++ b/net/core/dev.h
> @@ -166,8 +166,10 @@ int dev_change_carrier(struct net_device *dev, bool new_carrier);
>  
>  void __dev_set_rx_mode(struct net_device *dev);
>  int __dev_set_promiscuity(struct net_device *dev, int inc, bool notify);
> +void netif_rx_mode_init(struct net_device *dev);
>  bool netif_rx_mode_clean(struct net_device *dev);
>  void netif_rx_mode_sync(struct net_device *dev);
> +void netif_rx_mode_cancel_retry(struct net_device *dev);
>  
>  void __dev_notify_flags(struct net_device *dev, unsigned int old_flags,
>  			unsigned int gchanges, u32 portid,
> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index d73fcb0c6785..c02aa05c9975 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
> @@ -23,6 +23,24 @@ static LIST_HEAD(rx_mode_list);
>  static DEFINE_SPINLOCK(rx_mode_lock);
>  static DECLARE_WORK(rx_mode_work, netdev_rx_mode_work);
>  
> +static void netif_rx_mode_queue(struct net_device *dev);

No fwd declarations please :/

> +static void netif_rx_mode_retry(struct work_struct *work)
> +{
> +	struct net_device *dev = container_of(work, struct net_device,
> +					      rx_mode_retry_work.work);
> +
> +	netif_rx_mode_queue(dev);
> +	dev_put(dev);
> +}
> +
> +void netif_rx_mode_init(struct net_device *dev)
> +{
> +	INIT_LIST_HEAD(&dev->rx_mode_node);
> +	__hw_addr_init(&dev->rx_mode_addr_cache);
> +	INIT_DELAYED_WORK(&dev->rx_mode_retry_work, netif_rx_mode_retry);
> +}
> +
>  /*
>   * General list handling functions
>   */
> @@ -1252,6 +1270,35 @@ static int netif_uc_promisc_update(struct net_device *dev)
>  	return 0;
>  }
>  
> +void netif_rx_mode_schedule_retry(struct net_device *dev)
> +{
> +	unsigned long delay;
> +
> +	/* Total retry budget: 1+2+4+8 = 15 seconds. */

What is the value of this comment..?