[PATCH net-next v6 1/2] net: axienet: Be more careful about updating tx_bd_tail

[Jakub Kicinski]

On Wed, 11 May 2022 12:44:31 -0600 Robert Hancock wrote:
> The axienet_start_xmit function was updating the tx_bd_tail variable
> multiple times, with potential rollbacks on error or invalid
> intermediate positions, even though this variable is also used in the
> TX completion path. Use READ_ONCE and WRITE_ONCE to make this update
> more atomic, and move the write before the MMIO write to start the
> transfer, so it is protected by that implicit write barrier.
> 
> Signed-off-by: Robert Hancock <robert.hancock at calian.com>
> ---
>  .../net/ethernet/xilinx/xilinx_axienet_main.c | 23 +++++++++++--------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index d6fc3f7acdf0..2f39eb4de249 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -807,12 +807,15 @@ axienet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	u32 csum_index_off;
>  	skb_frag_t *frag;
>  	dma_addr_t tail_p, phys;
> +	u32 orig_tail_ptr, new_tail_ptr;
>  	struct axienet_local *lp = netdev_priv(ndev);
>  	struct axidma_bd *cur_p;
> -	u32 orig_tail_ptr = lp->tx_bd_tail;
> +
> +	orig_tail_ptr = READ_ONCE(lp->tx_bd_tail);

This one does not need READ_ONCE().

We only need to protect reads and writes which may race with each other.
This read can't race with any write. We need WRITE_ONCE() in
axienet_start_xmit() and READ_ONCE() in xienet_check_tx_bd_space().

BTW I'm slightly murky on what the rmb() in xienet_check_tx_bd_space()
does. Memory barrier is a fence, not a flush, I don't see what two
accesses that rmb() is separating.

> +	new_tail_ptr = orig_tail_ptr;
>  
>  	num_frag = skb_shinfo(skb)->nr_frags;
> -	cur_p = &lp->tx_bd_v[lp->tx_bd_tail];
> +	cur_p = &lp->tx_bd_v[orig_tail_ptr];
>  
>  	if (axienet_check_tx_bd_space(lp, num_frag + 1)) {
>  		/* Should not happen as last start_xmit call should have