linux-kernel - Re: [PATCH 3/3] reset: socfpga: use readl/writel

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:	Tue, 05 Jul 2016 15:20:38 +0200
From:	Philipp Zabel <p.zabel@...gutronix.de>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Chen-Yu Tsai <wens@...e.org>,
	Dinh Nguyen <dinguyen@...nsource.altera.com>,
	Steffen Trumtrar <s.trumtrar@...gutronix.de>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>
Subject: Re: [PATCH 3/3] reset: socfpga: use readl/writel_relaxed

Am Dienstag, den 05.07.2016, 14:59 +0200 schrieb Arnd Bergmann:
> On Tuesday, July 5, 2016 1:40:16 PM CEST Philipp Zabel wrote:
> > Am Dienstag, den 05.07.2016, 13:20 +0200 schrieb Arnd Bergmann:
> > > On Tuesday, July 5, 2016 12:17:52 PM CEST Philipp Zabel wrote:
> > > > This just removes the rmb()/wmb() pair between register read and
> > > > write. Since no relevant reads follow the rmb and no relevant writes
> > > > precede the wmb, they should be safe to remove.
> > > > 
> > > > Signed-off-by: Philipp Zabel <p.zabel@...gutronix.de>
> > > 
> > > We should only do this if you are fixing a bug (which you don't mention
> > > in the changelog), or if you can show a relevant performance
> > > improvement. Is this code ever used in a fast path? If it is,
> > > wouldn't that indicate a problem in some driver?
> > 
> > It does not fix a bug, and it's not about performance either. I'd like
> > to align code with the recently posted stm32 driver, to unify them in a
> > future patch.
> > Of course we can change the stm32 driver to use readl/writel instead of
> > the relaxed variants, it just seemed useless to have those barriers
> > between the read and write.
> 
> On stm32, there is no barrier because ARM_DMA_MEM_BUFFERABLE is not set.
> 
> I'd really prefer to just have readl/writel everywhere except in the
> few places that are performance critical and have a comment explaining
> why it's safe there, mainly to avoid having new developers blindly
> add the relaxed accessors in drivers because they think it's the
> normal coding style.

I get your point. I'll ask the stm32 developers to use non-relaxed
readl/writel then.

> > If anything, we'd need to try to make sure that the writel in assert
> > hits the hardware before the function returns, so that a
> > assert-delay-deassert doesn't accidentally spend half its delay with the
> > writel still in the store buffer, and we'd need a full barrier after the
> > writel in deassert so that there can be no successive reads from still
> > disabled IP cores.
> 
> In general, I think you need a readl() following the writel() to guarantee
> that it has actually hit the hardware.
>
> On ARM you often have just the CPU write buffer that needs to be flushed,
> but if you have a PCI device or a more complex SoC, then a barriers doesn't
> wait for a write to arrive at the device, it only ensures that a subsequent
> write cannot arrive any earlier.

Yes, exactly. Until now I have not considered this at all.

regards
Philipp