linux-kernel - Re: [PATCH net-next] net: stmmac: remove superfluous wmb() memory 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] [day] [month] [year] [list]

Message-Id: <20180312.212010.215262612578386966.davem@davemloft.net>
Date:   Mon, 12 Mar 2018 21:20:10 -0400 (EDT)
From:   David Miller <davem@...emloft.net>
To:     niklas.cassel@...s.com
Cc:     Jose.Abreu@...opsys.com, peppe.cavallaro@...com,
        alexandre.torgue@...com, pavel@....cz, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: stmmac: remove superfluous wmb() memory
 barriers

From: Niklas Cassel <niklas.cassel@...s.com>
Date: Mon, 12 Mar 2018 09:55:42 +0100

> Jose is simply responding to the commit message description of this patch.
> 
> You explained that there is an implicit memory barrier between physical memory
> writes and those to MMIO register space, as long as you used writel().
> 
> I assumed that you meant writel() vs writel_relaxed(), where there latter
> does not do an implicit barrier.
> 
> I also found this from you:
> https://lwn.net/Articles/198995/
> 
> If my assumption was incorrect, please correct me.
> 
> As you seem to possess knowledge regarding this, you are probably the most
> suited person to know if this patch simply needs a commit message rewrite,
> or if it should be dropped completely.

Yes, I have always argued that the non-relaxed {read,write}{b,w,l}()
interfaces should imply barriers wrt. physical memory accesses.

Without that, drivers are harder to write.  Specifically, drivers that
work properly on all architectures will be very hard to write.

But looking at some drivers, probably this isn't fully the case right
now.  Which is unfortunate, but we must code to reality.

For example, looking at drivers/net/ethernet/broadcom/tg3.c, we have
tg3_start_xmit() going:

	write descriptors
	...
	/* Sync BD data before updating mailbox */
	wmb();
	...
		/* Packets are ready, update Tx producer idx on card. */
		tw32_tx_mbox(tnapi->prodmbox, entry);

so it really seems to be necessary.

So this stmmac revert is not valid.

Sorry for all the confusion.  I guess it's a lot of wishful thinking on
my part :-)