Re: [net-next,v25,6/6] eea: introduce callback for ndo_get_stats64

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
eea: introduce callback for ndo_get_stats64

This commit adds ndo_get_stats64 support to the eea driver, implementing
statistics collection for rx/tx packets and bytes. A spinlock is introduced
to protect access to the rx and tx queue pointers during stats collection.

> diff --git a/drivers/net/ethernet/alibaba/eea/eea_net.c b/drivers/net/ethernet/alibaba/eea/eea_net.c
> --- a/drivers/net/ethernet/alibaba/eea/eea_net.c
> +++ b/drivers/net/ethernet/alibaba/eea/eea_net.c
> @@ -27,8 +27,10 @@ static int enet_bind_new_q_and_cfg(struct eea_net *enet,
>
>  	enet->cfg = ctx->cfg;
>
> +	spin_lock(&enet->stats_lock);
>  	enet->rx = ctx->rx;
>  	enet->tx = ctx->tx;
> +	spin_unlock(&enet->stats_lock);

[ ... ]

> @@ -57,11 +59,13 @@ static void eea_free_rxtx_q_mem(struct eea_net *enet)
>  	struct eea_net_tx *tx, *tx_array;
>  	int i;
>
> +	spin_lock(&enet->stats_lock);
>  	rx_array = enet->rx;
>  	tx_array = enet->tx;
>
>  	enet->rx = NULL;
>  	enet->tx = NULL;
> +	spin_unlock(&enet->stats_lock);

[ ... ]

> @@ -260,6 +264,50 @@ static int eea_netdev_open(struct net_device *netdev)
>  	return err;
>  }
>
> +static void eea_stats(struct net_device *netdev, struct rtnl_link_stats64 *tot)
> +{
> +	struct eea_net *enet = netdev_priv(netdev);
> +	u64 packets, bytes;
> +	u32 start;
> +	int i;
> +
> +	spin_lock_bh(&enet->stats_lock);

Can this cause a deadlock? The enet_bind_new_q_and_cfg() and
eea_free_rxtx_q_mem() functions use spin_lock(&enet->stats_lock), but
eea_stats() uses spin_lock_bh(&enet->stats_lock).

If a thread holds stats_lock via spin_lock() (softirqs still enabled) and a
softirq fires that calls eea_stats() through ndo_get_stats64, the
spin_lock_bh() will attempt to acquire the already-held lock.

Should all acquisition sites use spin_lock_bh()/spin_unlock_bh() for
consistency?

> +
> +	if (enet->rx) {
> +		for (i = 0; i < enet->cfg.rx_ring_num; i++) {
> +			struct eea_net_rx *rx = enet->rx[i];

[ ... ]