Message-ID: <Z6DDQlhmzvNj-B7o@smile.fi.intel.com>
Date: Mon, 3 Feb 2025 15:23:14 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: Thomas Weißschuh <thomas.weissschuh@...utronix.de>,
	Kees Cook <kees@...nel.org>, Palmer Dabbelt <palmer@...osinc.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux-Arch <linux-arch@...r.kernel.org>,
	linux-kernel@...r.kernel.org,
	Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH] iomap: Fix -Wmissing-prototypes on UM

On Mon, Feb 03, 2025 at 02:08:35PM +0100, Arnd Bergmann wrote:
> 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?

No idea.

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

Any combination of these two I would prefer.

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

Definitely not this. I assume that _hi_lo and _lo_hi semantics of IO
APIs implies non-atomicity access and hence always splits the IO in
64-bit case. These helpers make code much less verbose and actually
(due to naming) clearer about the sequence of the reads or writes.
I prefer to have them stay (in the drivers).

> - use helper functions like the ones I showed above and test it
>   properly

If so, these helper functions should be available to wider audience.
But I think it will be a premature effort.

> I also added Richard Cochran to cc, as he wrote the original
> ixp46x driver and may know of other ptp drivers that have

I also suggest to ping Linus W. He seems to have an IXP4xx hardware,

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

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists