[PATCH] RFC: ptp: add comment about register access race
From: Arnd Bergmann <arnd@kernel.org>
Date: 2025-02-27 14:17:58
Also in:
linux-fpga, lkml
Subsystem:
intel ptp dfl tod driver, networking drivers, opencompute ptp clock driver, ptp hardware clock support, the rest · Maintainers:
Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Vadim Fedorenko, Richard Cochran, Linus Torvalds
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/ (local) 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(®s->systime_lo); hi = __raw_readl(®s->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, ®s->systime_lo); __raw_writel(hi, ®s->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(®s->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, ®s->systime_lo); }
--
2.39.5