Re: [PATCH net-next] net: wangxun: protect ring accesses with READ- and WRITE_ONCE

[Simon Horman <horms@kernel.org>]

On Thu, Aug 13, 2026 at 05:51:41PM +0800, Mengyuan Lou wrote:
> In wx_update_stats(), the ring pointers wx->rx_ring[i] and wx->tx_ring[i]
> can become NULL during queue teardown or reconfiguration (e.g., via
> wx_free_q_vector()).
> 
> READ_ONCE should be used when reading rings prior to accessing the
> statistics pointer to ensure protected access. As well as the
> corresponding WRITE_ONCE usage when allocating and freeing the rings.

Hi Mengyuan,

As observed in an AI-generated review of this patch [1], this patch is
doing a bit more than adding READ_ONCE/WRITE_ONCE.  It also adds an RCU
read-side critical section (the most important part of the change, IMHO),
and some NULL checks. It might be nice to extend this description somehow.

[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260813095141.88227-1-mengyuanlou%40net-swift.com

> 
> Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
> ---
>  drivers/net/ethernet/wangxun/libwx/wx_hw.c  | 20 ++++++++++++++++----
>  drivers/net/ethernet/wangxun/libwx/wx_lib.c |  8 ++++----
>  2 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> index 122c4952d203..096fe7f3d84f 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> @@ -2893,10 +2893,13 @@ void wx_update_stats(struct wx *wx)
>  
>  	spin_lock(&wx->hw_stats_lock);
>  
> +	rcu_read_lock();
>  	/* gather some stats to the wx struct that are per queue */
>  	for (i = 0; i < wx->num_rx_queues; i++) {
> -		struct wx_ring *rx_ring = wx->rx_ring[i];
> +		struct wx_ring *rx_ring = READ_ONCE(wx->rx_ring[i]);
>  
> +		if (!rx_ring)
> +			continue;
>  		non_eop_descs += rx_ring->rx_stats.non_eop_descs;
>  		alloc_rx_buff_failed += rx_ring->rx_stats.alloc_rx_buff_failed;
>  		hw_csum_rx_good += rx_ring->rx_stats.csum_good_cnt;
> @@ -2912,19 +2915,28 @@ void wx_update_stats(struct wx *wx)
>  		u64 rsc_flush = 0;
>  
>  		for (i = 0; i < wx->num_rx_queues; i++) {
> -			rsc_count += wx->rx_ring[i]->rx_stats.rsc_count;
> -			rsc_flush += wx->rx_ring[i]->rx_stats.rsc_flush;
> +			struct wx_ring *rx_ring = READ_ONCE(wx->rx_ring[i]);
> +
> +			if (!rx_ring)
> +				continue;
> +
> +			rsc_count += rx_ring->rx_stats.rsc_count;
> +			rsc_flush += rx_ring->rx_stats.rsc_flush;
>  		}
>  		wx->rsc_count = rsc_count;
>  		wx->rsc_flush = rsc_flush;
>  	}
>  
>  	for (i = 0; i < wx->num_tx_queues; i++) {
> -		struct wx_ring *tx_ring = wx->tx_ring[i];
> +		struct wx_ring *tx_ring = READ_ONCE(wx->tx_ring[i]);
> +
> +		if (!tx_ring)
> +			continue;
>  
>  		restart_queue += tx_ring->tx_stats.restart_queue;
>  		tx_busy += tx_ring->tx_stats.tx_busy;
>  	}
> +	rcu_read_unlock();
>  	wx->restart_queue = restart_queue;
>  	wx->tx_busy = tx_busy;
>  

A littler further down in this function there is a call to
wx_update_xoff_rx_lfc().

And since commit 22d95e93c05b ("net: wangxun: add Tx timeout process")
that function includes the following loop:


	for (i = 0; i < wx->num_tx_queues; i++)
		clear_bit(WX_HANG_CHECK_ARMED, wx->tx_ring[i]->state);

So I am wondering of this access to wx->tx_ring should be protected.
E.g. by extending the RCU read-side critical section to also cover
the call to wx_update_xoff_rx_lfc(), and by using READ_ONCE in
the loop above.

Observed with some assistance from Goose:gemini-3.1-flash-lite

...