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
