netdev - Re: [net-next.git 2/7] stmmac: review barriers

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: <20130403085106.GA2039@localhost.localdomain>
Date:	Wed, 3 Apr 2013 14:21:06 +0530
From:	Shiraz Hashim <shiraz.hashim@...com>
To:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Deepak Sikri <deepak.sikri@...com>
Subject: Re: [net-next.git 2/7] stmmac: review barriers

Hi Giuseppe,

On Wed, Apr 03, 2013 at 01:41:24PM +0800, Giuseppe CAVALLARO wrote:
> In all my tests performed on SH4 and ARM A9 platforms, I've never met problems
> that can be fixed by using memory barriers. In the past there was some issues
> on SMP ARM but fixed by reviewing xmit spinlock.

The problem which was addressed was not because of SMP IMO. It was
rather due to the fact that the write to the GMAC descriptor (which
happens to be in normal memory) has to be ordered with respect to GMAC
DMA as observer. Isn't it ?

> Further barriers have been added in the commits too: 8e83989106562326bf
> 
> This patch is to use the smp_wbm instead of wbm because the driver
                           ^^^^^^^^^^^^^^^^^^^^^^
Perhaps you meant smp_wmb and wmb

> runs on UP systems. Then, IMO it could make sense to only maintain the barriers
> just in places where we touch the dma owner bits (that is the
> only real critical path as we had seen and fixed in the commit:
> eb0dc4bb2e22c04964d).

Replacing wmb by smp_wmb may not be a good idea as we need to order
the store transaction to the descriptor with respect to GMAC DMA and
using smp_* version would just be compiler barrier in uniprocessor
systems.

> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@...com>
> Cc: Deepak Sikri <deepak.sikri@...com>
> Cc: Shiraz Hashim <shiraz.hashim@...com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8b69e3b..c92dcbc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1797,15 +1797,13 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  		priv->tx_skbuff[entry] = NULL;
>  		priv->hw->desc->prepare_tx_desc(desc, 0, len, csum_insertion,
>  						priv->mode);
> -		wmb();
> +		smp_wmb();
>  		priv->hw->desc->set_tx_owner(desc);
> -		wmb();

Since it is a loop, shouldn't we ensure that the ownership of a tx
descriptor is set before next descriptor in chain is programmed ?

>  	}
>  
>  	/* Finalize the latest segment. */
>  	priv->hw->desc->close_tx_desc(desc);
>  
> -	wmb();
>  	/* According to the coalesce parameter the IC bit for the latest
>  	 * segment could be reset and the timer re-started to invoke the
>  	 * stmmac_tx function. This approach takes care about the fragments.
> @@ -1821,9 +1819,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	} else
>  		priv->tx_count_frames = 0;
>  
> +	smp_wmb();

Please reconsider, may be keeping wmb is better.

>  	/* To avoid raise condition */
>  	priv->hw->desc->set_tx_owner(first);
> -	wmb();

Not sure about this, perhaps can be removed.

>  
>  	priv->cur_tx++;
>  
> @@ -1899,9 +1897,8 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv)
>  
>  			RX_DBG(KERN_INFO "\trefill entry #%d\n", entry);
>  		}
> -		wmb();
> +		smp_wmb();
>  		priv->hw->desc->set_rx_owner(p);
> -		wmb();

Similarly this is a part of a loop, we need to see if set rx owner
should be reflected before next descriptor program.

--
regards
Shiraz
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html