Re: [net-next,v20,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 for the Alibaba Elastic Ethernet
Adapter driver. The eea_stats() function iterates over RX and TX queues
to collect packet and byte statistics using u64_stats_sync primitives.

> diff --git a/drivers/net/ethernet/alibaba/eea/eea_net.c b/drivers/net/ethernet/alibaba/eea/eea_net.c
> index 2eba43a82ebc..440f5f11e055 100644
> --- a/drivers/net/ethernet/alibaba/eea/eea_net.c
> +++ b/drivers/net/ethernet/alibaba/eea/eea_net.c
> @@ -269,6 +269,52 @@ 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;
> +
> +	/* This function is protected by RCU. Here uses enet->tx and enet->rx
> +	 * to check whether the TX and RX structures are safe to access. In
> +	 * eea_free_rxtx_q_mem, before freeing the TX and RX resources, enet->rx
> +	 * and enet->tx are set to NULL, and synchronize_net is called.
> +	 */
> +
> +	if (enet->rx) {
> +		for (i = 0; i < enet->cfg.rx_ring_num; i++) {
> +			struct eea_net_rx *rx = enet->rx[i];

The comment states this function is protected by RCU, but enet->rx is read
without READ_ONCE() or rcu_dereference(). Since eea_free_rxtx_q_mem() can
set enet->rx to NULL concurrently (before calling synchronize_net()), should
the pointer be loaded with READ_ONCE() to prevent the compiler from
re-reading it between the NULL check and the array access?

Similarly, enet->tx below has the same pattern. The RCU documentation
recommends using rcu_dereference() for pointers that are accessed under
RCU protection and may be concurrently modified.

> +
> +			do {
> +				start = u64_stats_fetch_begin(&rx->stats.syncp);
> +				packets = u64_stats_read(&rx->stats.packets);
> +				bytes = u64_stats_read(&rx->stats.bytes);
> +			} while (u64_stats_fetch_retry(&rx->stats.syncp,
> +						       start));
> +
> +			tot->rx_packets += packets;
> +			tot->rx_bytes   += bytes;
> +		}
> +	}
> +
> +	if (enet->tx) {
> +		for (i = 0; i < enet->cfg.tx_ring_num; i++) {
> +			struct eea_net_tx *tx = &enet->tx[i];

[ ... ]