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

[Arnd Bergmann]

From: Arnd Bergmann <arnd@xxxxxxxx>

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@xxxxxxxxxxxxxxxx/
Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
---
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