[PATCH] RFC: ptp: add comment about register access race

[PATCH] RFC: ptp: add comment about register access race

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

While reviewing a patch to the ioread64_hi_lo() helpers, I noticed
that there are several PTP drivers that use multiple register reads
to access a 64-bit hardware register in a racy way.

There are usually safe ways of doing this, but at least these four
drivers do that.  A third register read obviously makes the hardware
access 50% slower. If the low word counds nanoseconds and a single
register read takes on the order of 1µs, the resulting value is
wrong in one of 4 million cases, which is pretty rare but common
enough that it would be observed in practice.

Link: https://lore.kernel.org/all/96829b49-62a9-435b-9e35-fe3ef01d1c67@app.fastmail.com/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Sorry I hadn't sent this out as a proper patch so far. Any ideas
what we should do here?
---
 drivers/net/ethernet/xscale/ptp_ixp46x.c | 8 ++++++++
 drivers/ptp/ptp_dfl_tod.c                | 4 ++++
 drivers/ptp/ptp_ocp.c                    | 4 ++++
 drivers/ptp/ptp_pch.c                    | 8 ++++++++
 4 files changed, 24 insertions(+)

diff --git a/drivers/net/ethernet/xscale/ptp_ixp46x.c b/drivers/net/ethernet/xscale/ptp_ixp46x.c
index 94203eb46e6b..e9c8589fdef0 100644
--- a/drivers/net/ethernet/xscale/ptp_ixp46x.c
+++ b/drivers/net/ethernet/xscale/ptp_ixp46x.c
@@ -43,6 +43,10 @@ static u64 ixp_systime_read(struct ixp46x_ts_regs *regs)
 	u64 ns;
 	u32 lo, hi;
 
+	/*
+	 * Caution: Split access lo/hi, which has a small race
+	 * when the low half overflows while we read it.
+	 */
 	lo = __raw_readl(&regs->systime_lo);
 	hi = __raw_readl(&regs->systime_hi);
 
@@ -61,6 +65,10 @@ static void ixp_systime_write(struct ixp46x_ts_regs *regs, u64 ns)
 	hi = ns >> 32;
 	lo = ns & 0xffffffff;
 
+	/*
+	 * This can equally overflow when the low half of the new
+	 * nanosecond value is close to 0xffffffff.
+	 */
 	__raw_writel(lo, &regs->systime_lo);
 	__raw_writel(hi, &regs->systime_hi);
 }
diff --git a/drivers/ptp/ptp_dfl_tod.c b/drivers/ptp/ptp_dfl_tod.c
index f699d541b360..1eed76c3a256 100644
--- a/drivers/ptp/ptp_dfl_tod.c
+++ b/drivers/ptp/ptp_dfl_tod.c
@@ -104,6 +104,10 @@ static int coarse_adjust_tod_clock(struct dfl_tod *dt, s64 delta)
 	if (delta == 0)
 		return 0;
 
+	/*
+	 * Caution: Split access lo/hi, which has a small race
+	 * when the low half overflows while we read it.
+	 */
 	nanosec = readl(base + TOD_NANOSEC);
 	seconds_lsb = readl(base + TOD_SECONDSL);
 	seconds_msb = readl(base + TOD_SECONDSH);
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index b651087f426f..cdf2ebe7c9bc 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -1229,6 +1229,10 @@ __ptp_ocp_gettime_locked(struct ptp_ocp *bp, struct timespec64 *ts,
 		sts->post_ts = ns_to_timespec64(ns - bp->ts_window_adjust);
 	}
 
+	/*
+	 * Caution: Split access to nanoseconds/seconds, which has a small
+	 * race when the low half overflows while we read it.
+	 */
 	time_ns = ioread32(&bp->reg->time_ns);
 	time_sec = ioread32(&bp->reg->time_sec);
 
diff --git a/drivers/ptp/ptp_pch.c b/drivers/ptp/ptp_pch.c
index b8a9a54a176c..a90c206e320b 100644
--- a/drivers/ptp/ptp_pch.c
+++ b/drivers/ptp/ptp_pch.c
@@ -147,6 +147,10 @@ static u64 pch_systime_read(struct pch_ts_regs __iomem *regs)
 {
 	u64 ns;
 
+	/*
+	 * Caution: Split access lo/hi, which has a small race
+	 * when the low half overflows while we read it.
+	 */
 	ns = ioread64_lo_hi(&regs->systime_lo);
 
 	return ns << TICKS_NS_SHIFT;
@@ -154,6 +158,10 @@ static u64 pch_systime_read(struct pch_ts_regs __iomem *regs)
 
 static void pch_systime_write(struct pch_ts_regs __iomem *regs, u64 ns)
 {
+	/*
+	 * This can equally overflow when the low half of the new
+	 * nanosecond value is close to 0xffffffff.
+	 */
 	iowrite64_lo_hi(ns >> TICKS_NS_SHIFT, &regs->systime_lo);
 }
 
-- 
2.39.5

Re: [PATCH] RFC: ptp: add comment about register access race

[Andy Shevchenko]

On Thu, Feb 27, 2025 at 03:17:27PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> While reviewing a patch to the ioread64_hi_lo() helpers, I noticed
> that there are several PTP drivers that use multiple register reads
> to access a 64-bit hardware register in a racy way.
> 
> There are usually safe ways of doing this, but at least these four
> drivers do that.  A third register read obviously makes the hardware
> access 50% slower. If the low word counds nanoseconds and a single
> register read takes on the order of 1µs, the resulting value is
> wrong in one of 4 million cases, which is pretty rare but common
> enough that it would be observed in practice.

...

> Sorry I hadn't sent this out as a proper patch so far. Any ideas
> what we should do here?

Actually this reminds me one of the discussion where it was some interesting
HW design that latches the value on the first read of _low_ part (IIRC), but
I might be mistaken with the details.

That said, it's from HW to HW, it might be race-less in some cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] RFC: ptp: add comment about register access race

[Richard Cochran]

On Thu, Feb 27, 2025 at 05:23:48PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 27, 2025 at 03:17:27PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > While reviewing a patch to the ioread64_hi_lo() helpers, I noticed
> > that there are several PTP drivers that use multiple register reads
> > to access a 64-bit hardware register in a racy way.
> > 
> > There are usually safe ways of doing this, but at least these four
> > drivers do that.  A third register read obviously makes the hardware
> > access 50% slower. If the low word counds nanoseconds and a single
> > register read takes on the order of 1µs, the resulting value is
> > wrong in one of 4 million cases, which is pretty rare but common
> > enough that it would be observed in practice.

If the hardware does NOT latch the registers together, then the driver must do:

  1. hi1 = read hi
  2. low = read lo
  3. hi2 = read h1
  4. if (hi2 == hi1 return (hi1 << 32) | low;
  5. goto step 1.

This for correctness, and correctness > performance.

> > Sorry I hadn't sent this out as a proper patch so far. Any ideas
> > what we should do here?

Need to have driver authors check the data sheet because ...

> Actually this reminds me one of the discussion where it was some interesting
> HW design that latches the value on the first read of _low_ part (IIRC), but
> I might be mistaken with the details.
> 
> That said, it's from HW to HW, it might be race-less in some cases.

... of this.

Thanks,
Richard

Re: [PATCH] RFC: ptp: add comment about register access race

[Andy Shevchenko]

On Sun, Mar 02, 2025 at 12:55:08PM -0800, Richard Cochran wrote:
> On Thu, Feb 27, 2025 at 05:23:48PM +0200, Andy Shevchenko wrote:
> > On Thu, Feb 27, 2025 at 03:17:27PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > > 
> > > While reviewing a patch to the ioread64_hi_lo() helpers, I noticed
> > > that there are several PTP drivers that use multiple register reads
> > > to access a 64-bit hardware register in a racy way.
> > > 
> > > There are usually safe ways of doing this, but at least these four
> > > drivers do that.  A third register read obviously makes the hardware
> > > access 50% slower. If the low word counds nanoseconds and a single
> > > register read takes on the order of 1µs, the resulting value is
> > > wrong in one of 4 million cases, which is pretty rare but common
> > > enough that it would be observed in practice.
> 
> If the hardware does NOT latch the registers together, then the driver must do:
> 
>   1. hi1 = read hi
>   2. low = read lo
>   3. hi2 = read h1
>   4. if (hi2 == hi1 return (hi1 << 32) | low;
>   5. goto step 1.
> 
> This for correctness, and correctness > performance.

Right.

> > > Sorry I hadn't sent this out as a proper patch so far. Any ideas
> > > what we should do here?
> 
> Need to have driver authors check the data sheet because ...
> 
> > Actually this reminds me one of the discussion where it was some interesting
> > HW design that latches the value on the first read of _low_ part (IIRC), but
> > I might be mistaken with the details.
> > 
> > That said, it's from HW to HW, it might be race-less in some cases.
> 
> ... of this.

Perhaps it's still good to have a comment, but rephrase it that the code is
questionable depending on the HW behaviour that needs to be checked.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] RFC: ptp: add comment about register access race

[Richard Cochran]

On Mon, Mar 03, 2025 at 09:50:01AM +0200, Andy Shevchenko wrote:
> Perhaps it's still good to have a comment, but rephrase it that the code is
> questionable depending on the HW behaviour that needs to be checked.

IIRC both ixp4xx and the PCH are the same design and latch high reg on
read of low.

Thanks,
Richard

Re: [PATCH] RFC: ptp: add comment about register access race

[Arnd Bergmann]

On Tue, Mar 4, 2025, at 05:09, Richard Cochran wrote:
> On Mon, Mar 03, 2025 at 09:50:01AM +0200, Andy Shevchenko wrote:
>> Perhaps it's still good to have a comment, but rephrase it that the code is
>> questionable depending on the HW behaviour that needs to be checked.
>
> IIRC both ixp4xx and the PCH are the same design and latch high reg on
> read of low.

Ok, if that's a common thing, let's assume that the drivers are
all correct for the respective hardware and discard my patch.

With the patch I have queued up in the asm-generic tree, the
behavior changes so ioread64_lo_hi() no longer gets turned
into a single 64-bit access, and that is then more likely to
be correct for both 32-bit and 64-bit architectures in case the
device doesn't actually implement 64-bit transactions but does
correctly latch the contents.

      Arnd