Re: [RFC/RFT PATCH net-next v3 1/2] net: Add ndo_write_rx_config and helper structs and functions:

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 28 Oct 2025 23:12:21 +0530 I Viswanath wrote:
> @@ -1421,6 +1426,7 @@ struct net_device_ops {
>  	void			(*ndo_change_rx_flags)(struct net_device *dev,
>  						       int flags);
>  	void			(*ndo_set_rx_mode)(struct net_device *dev);
> +	void			(*ndo_write_rx_config)(struct net_device *dev);
>  	int			(*ndo_set_mac_address)(struct net_device *dev,
>  						       void *addr);
>  	int			(*ndo_validate_addr)(struct net_device *dev);
> @@ -1767,6 +1773,12 @@ enum netdev_reg_state {
>  	NETREG_DUMMY,		/* dummy device for NAPI poll */
>  };
>  
> +struct rx_config_work {

pls make sure to prefix names of types and functions with netif,
netdev or net

> +	struct work_struct config_write;
> +	struct net_device *dev;
> +	spinlock_t config_lock;
> +};
> +

> +#define update_snapshot(config_ptr, type)						\
> +	do {										\
> +		typeof((config_ptr)) rx_config = ((type *)(dev->priv))->rx_config;	\
> +		unsigned long flags;							\
> +		spin_lock_irqsave(&((dev)->rx_work->config_lock), flags);		\
> +		*rx_config = *(config_ptr);						\
> +		spin_unlock_irqrestore(&((dev)->rx_work->config_lock), flags);		\
> +	} while (0)

The driver you picked is relatively trivial, advanced drivers need
to sync longer lists of mcast / ucast addresses. Bulk of the complexity
is in keeping those lists. Simple 

	*rx_config = *(config_ptr);

assignment is not enough. The driver needs to know old and new entries
and send ADD/DEL commands to FW. Converting virtio_net would be better,
but it does one huge dump which is also not representative of most
advanced NICs.

> @@ -11961,9 +11989,17 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>  	refcount_set(&dev->dev_refcnt, 1);
>  #endif
>  
> -	if (dev_addr_init(dev))
> +	dev->rx_work = kmalloc(sizeof(*dev->rx_work), GFP_KERNEL);

Let's only allocate any extra state if driver has the NDO

> +	if (!dev->rx_work)
>  		goto free_pcpu;
>  
> +	dev->rx_work->dev = dev;
> +	spin_lock_init(&dev->rx_work->config_lock);
> +	INIT_WORK(&dev->rx_work->config_write, execute_write_rx_config);
> +
> +	if (dev_addr_init(dev))
> +		goto free_rx_work;
> +
>  	dev_mc_init(dev);
>  	dev_uc_init(dev);
>  
> @@ -12045,6 +12081,10 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>  	free_netdev(dev);
>  	return NULL;
>  
> +free_rx_work:
> +	cancel_work_sync(&dev->rx_work->config_write);
> +	kfree(dev->rx_work);
> +
>  free_pcpu:
>  #ifdef CONFIG_PCPU_DEV_REFCNT
>  	free_percpu(dev->pcpu_refcnt);
> @@ -12130,6 +12170,9 @@ void free_netdev(struct net_device *dev)
>  		return;
>  	}
>  
> +	cancel_work_sync(&dev->rx_work->config_write);
> +	kfree(dev->rx_work);

We need to shut down sooner, some time between ndo_stop and ndo_uninit