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

[Robert Hancock]

On Wed, 2022-05-11 at 13:00 -0700, Jakub Kicinski wrote:
> 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().

Makes sense, can fix that up.

> 
> 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.

I believe the idea is to ensure that we're seeing a complete descriptor update
from the hardware (i.e. what dma_rmb does), and also that the last write to
tx_bd_tail will be visible (basically pairing with the implicit write barrier
from the IO write in axienet_start_xmit)?

> 
> > +	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
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com