linux-kernel - Re: [RFC PATCH 13/20] powerpc/io: Remove unnecessary indirection

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: <e34e2353-5818-408f-ab04-ce289bf418af@app.fastmail.com>
Date: Thu, 14 Nov 2024 16:24:16 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "Michael Ellerman" <mpe@...erman.id.au>, linuxppc-dev@...ts.ozlabs.org
Cc: linux-kernel@...r.kernel.org, "Jeremy Kerr" <jk@...abs.org>,
 "Geoff Levand" <geoff@...radead.org>
Subject: Re: [RFC PATCH 13/20] powerpc/io: Remove unnecessary indirection

On Thu, Nov 14, 2024, at 13:51, Michael Ellerman wrote:
> Some of the __do_xxx() defines do nothing useful, they just existed to
> make the previous hooking macros work. So remove them.
>
> Signed-off-by: Michael Ellerman <mpe@...erman.id.au>

Reviewed-by: Arnd Bergmann <arnd@...db.de>

> @@ -607,27 +600,27 @@ static inline u32 readl_be(const PCI_IO_ADDR addr)
> 
>  static inline void writeb(u8 val, PCI_IO_ADDR addr)
>  {
> -	__do_writeb(val, addr);
> +	out_8(addr, val);
>  }

I would open-code PCI_IO_ADDR here.

Also, at this point the writeb() etc functions are close enough
to the asm-generic/io.h version that we could try to go the next
few steps. One problem doing this is the definition of the
*_relaxed() accessors. Ideally the inline asm would go
into the __raw_*() helpers, with the "sync" and "twi;isync"
going into __io_br(), __io_bw() and __io_aw(), at which point
these can all use the generic versions, and out_*/in_*() can
be defined on top of those.

What I'm not sure about here is the __io_br() (sync before
readl()), since most other architectures don't do this.
Any idea why powerpc does it, and if the relaxed() version
needs it as well? Is this for spinlocks serialization?

I think the sync before write and twi;isync after read() is
needed mainly for synchronizing against concurrent DMA, so
it should be fine to leave those out of the relaxed
versions, but it's possible that there is something
more going on than I remember.

      Arnd