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

[Arnd Bergmann]

On Mon, Feb 3, 2025, at 13:07, Andy Shevchenko wrote:
> On Mon, Feb 03, 2025 at 12:43:01PM +0100, Arnd Bergmann wrote:
>> 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.)

Ok, so we don't even know how that hardware block would read to
a 64-bit bus transaction, it might give a race-free result, might
have the same race as 32-bit or might just cause data corruption
(e.g. ignoring half the bits).

I think the usual way to access a timestamp in two registers works
like this

u64 read_double_reg(u32 __iomem *reg)
{
       u32 hi, lo;

       /* check for overflow race by re-reading upper bits */
       do {
               hi = readl(reg + 1);
               lo = readl(reg);
       } while (hi != readl(reg + 1);

       return (u64)hi << 32 | lo;
}

void write_double_reg(u32 __iomem *reg, u64 val)
{
       /* ensure the low bits don't overflow right now, assumes
          low word is ticking up */
       writel(reg, 0);

       writel(reg + 1, upper_32_bits(val));
       writel(reg,     lower_32_bits(val));
}

[If there might be concurrent read/write accesses, it gets
much more complicated than this.]

Do you know why the driver doesn't do it like that?

> 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.

Ok, I'll try to split it up into sensible patches then. For ptp
(both ixp46x and pch), these are the options I see:

- leave it unchanged since nobody cares any more
- add some comments about being racy and possibly broken on 64-bit
- revert your pch patch d09adf61002f/8664d49a815e3 to make it have 32-
  bit accesses again and fix the theoretical 64-bit issue but not the
  race
- use helper functions like the ones I showed above and test it
  properly

I also added Richard Cochran to cc, as he wrote the original
ixp46x driver and may know of other ptp drivers that have
this issue. One potential candidate I see is
https://elixir.bootlin.com/linux/v6.13.1/source/drivers/ptp/ptp_dfl_tod.c#L226
and other functions in that file.

       Arnd