[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-Id: <20250227141749.3767032-1-arnd@kernel.org>
Date: Thu, 27 Feb 2025 15:17:27 +0100
From: Arnd Bergmann <arnd@...nel.org>
To: Richard Cochran <richardcochran@...il.com>
Cc: Thomas Weißschuh <thomas.weissschuh@...utronix.de>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Arnd Bergmann <arnd@...db.de>,
Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Tianfei Zhang <tianfei.zhang@...el.com>,
Jonathan Lemon <jonathan.lemon@...il.com>,
Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Thomas Weißschuh <linux@...ssschuh.net>,
Calvin Owens <calvin@...nvd.org>,
Philipp Stanner <pstanner@...hat.com>,
netdev@...r.kernel.org,
linux-kernel@...r.kernel.org,
linux-fpga@...r.kernel.org
Subject: [PATCH] RFC: ptp: add comment about register access race
From: Arnd Bergmann <arnd@...db.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@...db.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
Powered by blists - more mailing lists