netdev - Re: [PATCH v2 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

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]

Date: Fri, 28 Jul 2023 12:29:46 -0400
From: Frank Li <Frank.li@....com>
To: Will Deacon <will@...nel.org>
Cc: "Russell King (Oracle)" <linux@...linux.org.uk>,
	Andrew Halaney <ahalaney@...hat.com>,
	Shenwei Wang <shenwei.wang@....com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Kevin Hilman <khilman@...libre.com>, Vinod Koul <vkoul@...nel.org>,
	Chen-Yu Tsai <wens@...e.org>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Samuel Holland <samuel@...lland.org>,
	Giuseppe Cavallaro <peppe.cavallaro@...com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	NXP Linux Team <linux-imx@....com>,
	Jerome Brunet <jbrunet@...libre.com>,
	Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
	Bhupesh Sharma <bhupesh.sharma@...aro.org>,
	Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.co.jp>,
	Simon Horman <simon.horman@...igine.com>,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
	Wong Vee Khee <veekhee@...le.com>,
	Revanth Kumar Uppala <ruppala@...dia.com>,
	Jochen Henneberg <jh@...neberg-systemdesign.com>,
	netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-amlogic@...ts.infradead.org, imx@...ts.linux.dev
Subject: Re: [PATCH v2 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock
 in fixed-link

On Fri, Jul 28, 2023 at 04:36:12PM +0100, Will Deacon wrote:
> On Fri, Jul 28, 2023 at 04:22:19PM +0100, Russell King (Oracle) wrote:
> > On Thu, Jul 27, 2023 at 01:36:45PM -0500, Andrew Halaney wrote:
> > > I don't have any documentation for the registers here, and as you can
> > > see I'm an amateur with respect to memory ordering based on my prior
> > > comment.
> > > 
> > > But you:
> > > 
> > >     1. Read intf_reg_off into variable iface
> > >     2. Write the RESET_SPEED for the appropriate mode to MAC_CTRL_REG
> > >     3. wmb() to ensure that write goes through
> > 
> > I wonder about whether that wmb() is required. If the mapping is
> > device-like rather than memory-like, the write should be committed
> > before the read that regmap_update_bits() does according to the ARM
> > memory model. Maybe a bit of information about where this barrier
> > has come from would be good, and maybe getting it reviewed by the
> > arm64 barrier specialist, Will Deacon. :)
> > 
> > wmb() is normally required to be paired with a rmb(), but we're not
> > talking about system memory here, so I also wonder whether wmb() is
> > the correct barrier to use.
> 
> Yes, I don't think wmb() is the right thing here. If you need to ensure
> that the write to MAC_CTRL_REG has taken effect, then you'll need to go
> through some device-specific sequence which probably involves reading
> something back. If you just need things to arrive in order eventually,
> the memory type already gives you that.
> 
> It's also worth pointing out that udelay() isn't necessarily ordered wrt
> MMIO writes, so that usleep_range() might need some help as well.

Hi Deacon:

Does it means below pattern will be problem?

1.writel()
2.udelay()
3.writel()

It may not wait enough time between 1 and 3. I think the above pattern
is quite common in driver code.  I am not sure if usleep_range involve
MMIO to get current counter, ARM may use cp15 to get local timer counter.

In our system, readl() is quite slow because cross some bus bridge.
even readl() can work, we don't know it is because delay by readl() itself.
Or it works logically. Suppose readl() and writel() just guarantee memory
access order. 

Frank

> Non-relaxed MMIO reads, however, _are_ ordered against a subsequent
> udelay(), so if you add the readback then this might all work out.
> 
> I gave a (slightly dated) talk about some of this at ELC a while back:
> 
> https://www.youtube.com/watch?v=i6DayghhA8Q
> 
> which might help.
> 
> Will