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
