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: <515BDA1C.8010503@st.com>
Date:	Wed, 03 Apr 2013 09:28:28 +0200
From:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [net-next.git 2/7] stmmac: review barriers

On 4/3/2013 9:18 AM, Eric Dumazet wrote:
> On Wed, 2013-04-03 at 07:41 +0200, 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.
>>
>> Further barriers have been added in the commits too: 8e83989106562326bf
>>
>> This patch is to use the smp_wbm instead of wbm because the driver
>> 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).
>>
>> 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);
>
> This looks pretty bogus to me.
>
> If arch is UP, smp_wmb() is empty.
>
> So why are you using it ?
>
> Barrier here is not to protect the interaction with another cpu, but the
> NIC itself.
>
> If there is no need for barrier as you claim in changelog, don't add a
> fake smp_wmb().
>

ok I'll do it because I do not need any barrier when test on my UP and
SMP. Barriers were added for SPEAr but I have never seen problems
although I have not done too many tests on these platforms. I usually
run on other ST ARM box but, as rule of thumb, similar to some SPEAr
for CPU and MAC.
At any rate, if somebody aims to have them we will discuss in this
mailing list to understand why/where these have to be placed in the code

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