RE: [PATCH net v2 4/9] net: axienet: add missing memory barriers

From: Radhey Shyam Pandey <radheys@xilinx.com>
To: Robert Hancock <robert.hancock@calian.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	Michal Simek <michals@xilinx.com>,
	"ariane.keller@tik.ee.ethz.ch" <ariane.keller@tik.ee.ethz.ch>,
	"daniel@iogearbox.net" <daniel@iogearbox.net>
Subject: RE: [PATCH net v2 4/9] net: axienet: add missing memory barriers
Date: Thu, 13 Jan 2022 12:09:41 +0000	[thread overview]
Message-ID: <SA1PR02MB85604DE700BA8511C604B632C7539@SA1PR02MB8560.namprd02.prod.outlook.com> (raw)
In-Reply-To: <20220112173700.873002-5-robert.hancock@calian.com>

> -----Original Message-----
> From: Robert Hancock <robert.hancock@calian.com>
> Sent: Wednesday, January 12, 2022 11:07 PM
> To: netdev@vger.kernel.org
> Cc: Radhey Shyam Pandey <radheys@xilinx.com>; davem@davemloft.net;
> kuba@kernel.org; linux-arm-kernel@lists.infradead.org; Michal Simek
> <michals@xilinx.com>; ariane.keller@tik.ee.ethz.ch; daniel@iogearbox.net;
> Robert Hancock <robert.hancock@calian.com>
> Subject: [PATCH net v2 4/9] net: axienet: add missing memory barriers
> 
> This driver was missing some required memory barriers:
> 
> Use dma_rmb to ensure we see all updates to the descriptor after we see that
> an entry has been completed.
> 
> Use wmb and rmb to avoid stale descriptor status between the TX path and TX
> complete IRQ path.
> 
> Fixes: 8a3b7a252dca9 ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet
> driver")
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index f4ae035bed35..de8f85175a6c 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -632,6 +632,8 @@ static int axienet_free_tx_chain(struct net_device
> *ndev, u32 first_bd,
>  		if (nr_bds == -1 && !(status &
> XAXIDMA_BD_STS_COMPLETE_MASK))
>  			break;
> 
> +		/* Ensure we see complete descriptor update */
> +		dma_rmb();
>  		phys = desc_get_phys_addr(lp, cur_p);
>  		dma_unmap_single(ndev->dev.parent, phys,
>  				 (cur_p->cntrl &
> XAXIDMA_BD_CTRL_LENGTH_MASK), @@ -645,8 +647,10 @@ static int
> axienet_free_tx_chain(struct net_device *ndev, u32 first_bd,
>  		cur_p->app1 = 0;
>  		cur_p->app2 = 0;
>  		cur_p->app4 = 0;
> -		cur_p->status = 0;
>  		cur_p->skb = NULL;
> +		/* ensure our transmit path and device don't prematurely see
> status cleared */
> +		wmb();
> +		cur_p->status = 0;

Any reason for moving status initialization down?

> 
>  		if (sizep)
>  			*sizep += status &
> XAXIDMA_BD_STS_ACTUAL_LEN_MASK; @@ -704,6 +708,9 @@ static inline
> int axienet_check_tx_bd_space(struct axienet_local *lp,
>  					    int num_frag)
>  {
>  	struct axidma_bd *cur_p;
> +
> +	/* Ensure we see all descriptor updates from device or TX IRQ path */
> +	rmb();
>  	cur_p = &lp->tx_bd_v[(lp->tx_bd_tail + num_frag) % lp->tx_bd_num];
>  	if (cur_p->status & XAXIDMA_BD_STS_ALL_MASK)
>  		return NETDEV_TX_BUSY;
> @@ -843,6 +850,8 @@ static void axienet_recv(struct net_device *ndev)
> 
>  		tail_p = lp->rx_bd_p + sizeof(*lp->rx_bd_v) * lp->rx_bd_ci;
> 
> +		/* Ensure we see complete descriptor update */
> +		dma_rmb();
>  		phys = desc_get_phys_addr(lp, cur_p);
>  		dma_unmap_single(ndev->dev.parent, phys, lp-
> >max_frm_size,
>  				 DMA_FROM_DEVICE);

Ideally we would also need a write barrier in xmit function just before 
updating tail descriptor.

> --
> 2.31.1