Re: [PATCH] iomap: Fix -Wmissing-prototypes on UM

[Andy Shevchenko]

On Mon, Feb 03, 2025 at 12:43:01PM +0100, Arnd Bergmann wrote:
> On Mon, Feb 3, 2025, at 09:25, Thomas Weißschuh wrote:
> > On Mon, Feb 03, 2025 at 09:18:33AM +0100, Arnd Bergmann wrote:
> >> I have not tried it yet, but I suspect this is not the correct
> >> fix here. Unfortunately the indirect header inclusions in this
> >> file are way too complicated with corner cases in various
> >> architectures. How much testing have you given your patch
> >> across other targets? I think the last time we tried to address
> >> it, we broke mips or parisc.
> >
> > It was build-tested on 0day.
> > I also gave it some light boot testing on kunit/qemu.
> > (Neither on mips or parisc)
> 
> Ok, I see. I checked the usage of these functions again and
> now remembered the reason we didn't fix it last time, which is
> that the semantics are inconsistent across architectures
> and I think this should be addressed first, so we can untangle
> the definitions:
> 
> There is one driver that is specific to ARM processors
> (drivers/firmware/arm_scmi) using  ioread64_hi_lo/iowrite64_hi_lo
> and this uses the documented semantics from
> Documentation/driver-api/device-io.rst, which says that the
> helpers always do separate 32-bit accesses (even on 64-bit
> machines), but that it's possible to just call
> ioread64()/iowrite64() after including linux/io-64-nonatomic-hi-lo.h
> in order to always get 64-bit accesses on 64-bit architectures
> but get 32-bit accesses on 32-bit architectures. There are
> another dozen or so drivers that do this.
> 
> There are two other drivers that were written for x86 that
> use other semantics (drivers/net/wwan/t7xx/ and
> drivers/ptp/ptp_pch.c): Here, the definition from lib/iomap.c
> means that even on 64-bit architectures calling
> ioread64_hi_lo/iowrite64_hi_lo on an MMIO space always
> results in a 64-bit access,

Interesting... I believe both cases mentioned in this paragraph
were written with only the include/linux/io-64*.h in mind.

> and only the PIO version is split
> up. On 32-bit architectures, this part is not provided, so
> it should fall back to split access (I think this is where
> we had bugs in the past).
> 
> Another complication is that alpha, parisc and sh (not mips
> any more) explicitly include asm-generic/iomap.h but don't
> select CONFIG_GENERIC_IOMAP. At this time, GENERIC_IOMAP
> is selected at least in some configurations on m68k, mips,
> powerpc, sh, um and x86, but most of these should not do that
> (mips, m68k and sh have no PIO instructions, powerpc had
> a hack that I think was just removed but needs more cleanup).
> 
> I'm testing with the patch below now, which separates the
> CONFIG_GENERIC_IOMAP implementation (with the 32-bit PIO
> access) from the normal version, and picks an appropriate
> one in linux/io-64-nonatomic-*.h based on the architecture
> to avoid some of the more confusing nested 
> "#ifdef ioread64" etc checks.
> 
> I checked that none of the three drivers ever actually uses
> PIO registers instead of MMIO, and since none of them use the
> big-endian accessors, this all turns into readq/writeq
> in practice anyway.
> 
> The ptp_pch.c driver still needs more work as I noticed two
> issues there: the driver has a mix of lo_hi and hi_lo
> variants, but it is unclear whether is is actually required
> on 32-bit or if the hardware works the same either way.

PTP is special, some registers are related to a read timestamp IIRC and
hence hi_lo is a must there.

> In addition, these seem to be timer registers that may overrun
> from the lo into the hi field between the two accesses, so
> technically a 32-bit host needs to do an extra read to
> check for overflow and possibly repeat the access.

Yes, precisely why hi_lo is used to minimize the error when it races like this.

But IIRC *_pch drivers are for 32-bit platform, the code, if so, was made
to be compiled on 64-bit but never used IRL, just for test coverage.

(I believe the PCH stands for EG20 PCH, I have [32-bit] boards with it.)

...

I like the lib/* and include/* cleanup but PTP probably should stay as is.
OTOH WWAN case most likely had been tested on 64-bit platforms only and
proves that readq()/writeq() works there, so it's okay to unify.

-- 
With Best Regards,
Andy Shevchenko