From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtpout-03.galae.net (smtpout-03.galae.net [185.246.85.4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id AE77C3ACEE2 for ; Thu, 10 Sep 2026 07:13:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.246.85.4 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789024401; cv=none; b=SY1rWrg89VthllpetHbVBhVRuBbYpsuMpA96Y9n8DXncI8ic2bNQn8ZwsmQKFy4q0C6maqlyWyPuDqUdVVGdafA2EDJ8KmFbxFnfkroDV/lFMuehpOicJm6lroBZVFF+Jx3mXQcZvuaBaZqMcHcV0U8Y3pnFwgHX009eCgyEWLg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789024401; c=relaxed/simple; bh=fffekK7AapVIVynDE9YUbHpgRADon2nGwP8qdjLXKSE=; h=Mime-Version:Content-Type:Date:Message-Id:Subject:Cc:To:From: References:In-Reply-To; b=m4S24tR1/LIU5LsD3ni3+vhvsjo0vW1JLEc1Ax7WJ/fJHHn7xWctpQ5SvPmG64h0EVbaXFghD56o5UcwoN0k6vSJRuWBPdWI2mRcUy/toLEs9t7DKCZVKFXwnlGjCDQxZCoLZMMdeZHwom/BxUMbAacLuzE6JPcAFDLeFGPKMYg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com; spf=pass smtp.mailfrom=bootlin.com; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b=P0DUM8I8; arc=none smtp.client-ip=185.246.85.4 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bootlin.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b="P0DUM8I8" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-03.galae.net (Postfix) with ESMTPS id E65054E415BC; Thu, 10 Sep 2026 07:13:16 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id 9CD07602B8; Thu, 10 Sep 2026 07:13:16 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id 0BEA411C7AF5D; Thu, 10 Sep 2026 09:13:10 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1789024395; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:references; bh=9WU12DE4FC9F3p5LnbxXD4loTgoWcD6hopLD4rZm3ME=; b=P0DUM8I8olU8qUDSJoUZoms3xMfWsySLLNWY5RjffqgIREOJvJAmRO5G6ZiatSunyCSo7A 2/3ryUpWPFZNVwQB1ARVzc8jpoWjkAAgJcD4htkY7k+tBR5DSsw75WTHms/tGQkzqajdBV ERJe2l+k3VKM4tfWNgRmiaNNX3Hf0V87r5+sKyOtJ2U7OWRRTB8IZXmfQcTrrYhVQo7L5t cTFsYXgfwdBtt0kMCv1UeNu0rWhgS5MU5erYTz1Fia7wlB6y1UGg2LcbPBEFestLyJR1SL xWAmyn/cO2MGnlHRhh96Uy+WcpDvYOHo8Z2MOG6NEK2VhPaBaNa0M2pTe6CRwQ== Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 10 Sep 2026 09:13:09 +0200 Message-Id: Subject: Re: [PATCH net v2] net: macb: fix ordering around PTP timestamp read Cc: "Richard Cochran" , "Conor Dooley" , "Andrew Lunn" , "David S . Miller" , "Eric Dumazet" , "Jakub Kicinski" , "Paolo Abeni" , , "Nicolai Buchwitz" To: "James Clark" , From: =?utf-8?q?Th=C3=A9o_Lebrun?= X-Mailer: aerc 0.21.0-0-g5549850facc2 References: <20260910040658.18359-1-jjc@jclark.com> In-Reply-To: <20260910040658.18359-1-jjc@jclark.com> X-Last-TLS-Session-Version: TLSv1.3 Hello James, On Thu Sep 10, 2026 at 6:06 AM CEST, James Clark wrote: > PTP_SYS_OFFSET_EXTENDED returns system timestamps that do not correctly > bracket the PHC register read on MACB/GEM. On a Raspberry Pi 5, the > returned interval can be as short as 37 ns, while an ordered register > read takes approximately 1 us. This biases the midpoint used by phc2sys, > causing CLOCK_REALTIME to run approximately 0.5 us ahead when synchronize= d > to the PHC. > > gem_tsu_get_time() reads the nanoseconds register using the driver's > relaxed MMIO accessor. On weakly ordered systems, the subsequent system > timestamp can be taken before the register read completes. The internal > smp_rmb() in the pre-timestamp path also does not guarantee ordering > against the subsequent MMIO read. > > Add rmb() before and after the bracketed nanoseconds read in both the > normal and seconds rollover paths so the system timestamps bracket the > PHC read. Adding the post-read barrier increases the minimum interval on > the same Raspberry Pi 5 to approximately 1 us. > > Fixes: e51bb5c2784c ("net: macb: ptp: Switch to gettimex64() interface") > Tested-by: Nicolai Buchwitz # Raspberry Pi CM5, min brac= ket 37 ns -> 981 ns > Reviewed-by: Nicolai Buchwitz > Signed-off-by: James Clark > --- > Changes in v2: > - Add rmb() before the PHC read in both paths, following Th=C3=A9o Lebrun= 's > feedback. Explain why the pre-timestamp path's smp_rmb() is insufficien= t > for MMIO ordering. > - Wrap the post-read barrier comments. > - Add Nicolai Buchwitz's Tested-by and Reviewed-by tags. > - Drop RFC. > > v1: https://lore.kernel.org/netdev/20260908053150.28694-1-jjc@jclark.com/ > > Nicolai's review and testing were on v1. The additional pre-read barriers > in v2 address Th=C3=A9o's feedback. [...] > drivers/net/ethernet/cadence/macb_ptp.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethern= et/cadence/macb_ptp.c > index e5195d7da..4fb287608 100644 > --- a/drivers/net/ethernet/cadence/macb_ptp.c > +++ b/drivers/net/ethernet/cadence/macb_ptp.c > @@ -50,7 +50,15 @@ static int gem_tsu_get_time(struct ptp_clock_info *ptp= , struct timespec64 *ts, > =20 > spin_lock_irqsave(&bp->tsu_clk_lock, flags); > ptp_read_system_prets(sts); > + /* ptp_read_system_prets() uses smp_rmb() internally, > + * which does not guarantee ordering against MMIO reads. > + */ > + rmb(); > first =3D gem_readl(bp, TN); > + /* Ensure the PHC read completes before taking > + * the post timestamp. > + */ > + rmb(); > ptp_read_system_postts(sts); > secl =3D gem_readl(bp, TSL); > sech =3D gem_readl(bp, TSH); > @@ -62,7 +70,15 @@ static int gem_tsu_get_time(struct ptp_clock_info *ptp= , struct timespec64 *ts, > * (assume all done within 1s) > */ > ptp_read_system_prets(sts); > + /* ptp_read_system_prets() uses smp_rmb() internally, > + * which does not guarantee ordering against MMIO reads. > + */ > + rmb(); > ts->tv_nsec =3D gem_readl(bp, TN); > + /* Ensure the PHC read completes before taking > + * the post timestamp. > + */ > + rmb(); > ptp_read_system_postts(sts); > secl =3D gem_readl(bp, TSL); > sech =3D gem_readl(bp, TSH); Honestly I wouldn't be surprised to see this code without comments. Especially as the rmb() were added in a separate commit so git blame/log will point to your commit message which is plentiful. It would have been different if the rmb were part of the commit introducing gettimex64 support, with a commit message which would probably not talk about why rmb are required. Don't bother sending a new revision just for that though! With or without the code comments: Reviewed-by: Th=C3=A9o Lebrun Thanks James, -- Th=C3=A9o Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com