netdev - Re: [PATCH] net: Google gve: Remove dma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b9172cbc-de9d-5926-0021-7a276b9f304f@gmail.com>
Date:   Fri, 3 Jan 2020 05:36:16 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Liran Alon <liran.alon@...cle.com>, csully@...gle.com,
        davem@...emloft.net, netdev@...r.kernel.org
Cc:     sagis@...gle.com, jonolson@...gle.com, yangchun@...gle.com,
        lrizzo@...gle.com, adisuresh@...gle.com,
        Si-Wei Liu <si-wei.liu@...cle.com>
Subject: Re: [PATCH] net: Google gve: Remove dma_wmb() before ringing doorbell



On 1/3/20 5:01 AM, Liran Alon wrote:
> Current code use dma_wmb() to ensure Tx descriptors are visible
> to device before writing to doorbell.
> 
> However, these dma_wmb() are wrong and unnecessary. Therefore,
> they should be removed.
> 
> iowrite32be() called from gve_tx_put_doorbell() internally executes
> dma_wmb()/wmb() on relevant architectures. E.g. On ARM, iowrite32be()
> calls __iowmb() which translates to wmb() and only then executes
> __raw_writel(). However on x86, iowrite32be() will call writel()
> which just writes to memory because writes to UC memory is guaranteed
> to be globally visible only after previous writes to WB memory are
> globally visible.

This part about x86 is confusing.

writel() has the needed barrier (compared to writel_relaxed() which has no such barrier)

The UC vs WB memory sounds irrelevant.

> 
> Reviewed-by: Si-Wei Liu <si-wei.liu@...cle.com>
> Signed-off-by: Liran Alon <liran.alon@...cle.com>
> ---
>  drivers/net/ethernet/google/gve/gve_tx.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve_tx.c b/drivers/net/ethernet/google/gve/gve_tx.c
> index f4889431f9b7..d0244feb0301 100644
> --- a/drivers/net/ethernet/google/gve/gve_tx.c
> +++ b/drivers/net/ethernet/google/gve/gve_tx.c
> @@ -487,10 +487,6 @@ netdev_tx_t gve_tx(struct sk_buff *skb, struct net_device *dev)
>  		 * may have added descriptors without ringing the doorbell.
>  		 */
>  
> -		/* Ensure tx descs from a prior gve_tx are visible before
> -		 * ringing doorbell.
> -		 */
> -		dma_wmb();
>  		gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
>  		return NETDEV_TX_BUSY;
>  	}
> @@ -505,8 +501,6 @@ netdev_tx_t gve_tx(struct sk_buff *skb, struct net_device *dev)
>  	if (!netif_xmit_stopped(tx->netdev_txq) && netdev_xmit_more())
>  		return NETDEV_TX_OK;
>  
> -	/* Ensure tx descs are visible before ringing doorbell */
> -	dma_wmb();
>  	gve_tx_put_doorbell(priv, tx->q_resources, tx->req);
>  	return NETDEV_TX_OK;
>  }
> 

This seems strange to care about TX but not RX ?

gve_rx_write_doorbell() also uses iowrite32be()