[PATCH net-next v2 0/3] net: macb: Rework hardware timestamp configuration

[PATCH net-next v2 0/3] net: macb: Rework hardware timestamp configuration

[Kim Wooseok via B4 Relay]

This started with two problems I found while using PTP hardware
timestamping on a Raspberry Pi 5. A rejected RX filter could still
change the TX one-step setting, and the PTPv1 Sync and Delay_Req filters
returned success without enabling RX timestamps.

This series reworks gem_set_hwtst() to validate the whole request before
programming the hardware. It calculates the settings locally, then
updates the registers and saved configuration under bp->lock. With the
register writes now in the setter, the two helpers are no longer needed.

The remaining patches use the existing ALL fallback for the specific
PTPv1 filters and clear SRTSM when switching away from a PTPv2 filter.
Each change is kept in its own patch.

For v1, I compared the old and new behavior on the Pi 5. When I
requested a TX mode change together with an invalid RX filter, the
patched driver rejected the request without changing the saved settings
or NCR. I also checked transitions between PTPv2 RX and the other
receive filters. SRTSM followed the selected RX filter without changing
unrelated NCR bits.

For the PTPv1 filters, I checked the effect by sending Sync and Delay_Req
packets over Ethernet. Before the change, the packets arrived without
hardware timestamps. With the patches, the driver returned ALL and I
could read the hardware timestamps from the received packets. PTPv2
multicast reception continued to provide hardware timestamps, and
selecting NONE disabled timestamping. I ran these tests with the changes
backported to the Pi's existing Linux 6.18.46 RT kernel.

On net-next, I built v1 with ARM64 allyesconfig and allmodconfig,
both with W=1. Sparse reported no diagnostics in macb_ptp.c.
For v2, I rebuilt macb_ptp.o with W=1.

Assisted-by: GPT-6 Astra
Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>
---
Changes in v2:
- Drop v1 patch 3/4 and retain the existing TX_OFF handling.
- Rebase the SRTSM cleanup onto the remaining patches, preserving
  its RX behavior.
- Reorder local variables to follow reverse Christmas tree ordering
  and handle each TX mode explicitly instead of using fallthrough.
- Add Reviewed-by and Tested-by tags.
- No other functional changes.
- Link to v1: https://patch.msgid.link/20260922-codex-macb-hwtstamp-submit-v1-0-9d1abaa53296@khu.ac.kr

---
Kim Wooseok (3):
      net: macb: Preserve timestamp settings on rejected requests
      net: macb: Enable RX timestamping for specific PTPv1 filters
      net: macb: Clear SRTSM outside PTPv2 receive filters

 drivers/net/ethernet/cadence/macb_ptp.c | 62 +++++++++++++--------------------
 1 file changed, 24 insertions(+), 38 deletions(-)
---
base-commit: 8830e65ed46de41f849eefb8ba227d4852c460f6
change-id: 20260922-macb-hwtstamp-rework-25ab4a2e6b94

Best regards,
--  
Kim Wooseok <5mghybrid@khu.ac.kr>

[PATCH net-next v2 1/3] net: macb: Preserve timestamp settings on rejected requests

[Kim Wooseok via B4 Relay]

From: Kim Wooseok <5mghybrid@khu.ac.kr>

gem_set_hwtst() can reject a request after changing the TX one-step
setting, because it programs the TX mode before checking the RX
filter. The call returns -ERANGE, but the hardware may no longer match
the cached configuration.

Validate both settings first and keep the adjusted RX filter local
until validation succeeds. Then apply the register settings and update
the configuration. A rejected request now leaves the hardware, the
caller's settings and the cached configuration unchanged.

Protect the NCR read-modify-write with bp->lock, keeping the descriptor
writes and cache update in the same section. With the register writes
now in the setter, remove gem_ptp_set_one_step_sync() and
gem_ptp_set_ts_mode().

Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>
Tested-by: Nicolai Buchwitz <nb@tipi-net.de>
Assisted-by: GPT-6 Astra
Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>
---
 drivers/net/ethernet/cadence/macb_ptp.c | 59 ++++++++++++++-------------------
 1 file changed, 25 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index 14ae57fa0..02face93a 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -374,16 +374,6 @@ void gem_ptp_remove(struct net_device *netdev)
 		 GEM_PTP_TIMER_NAME);
 }
 
-static int gem_ptp_set_ts_mode(struct macb *bp,
-			       enum macb_bd_control tx_bd_control,
-			       enum macb_bd_control rx_bd_control)
-{
-	gem_writel(bp, TXBDCTRL, GEM_BF(TXTSMODE, tx_bd_control));
-	gem_writel(bp, RXBDCTRL, GEM_BF(RXTSMODE, rx_bd_control));
-
-	return 0;
-}
-
 int gem_get_hwtst(struct net_device *netdev,
 		  struct kernel_hwtstamp_config *tstamp_config)
 {
@@ -396,25 +386,17 @@ int gem_get_hwtst(struct net_device *netdev,
 	return 0;
 }
 
-static void gem_ptp_set_one_step_sync(struct macb *bp, u8 enable)
-{
-	u32 reg_val;
-
-	reg_val = macb_readl(bp, NCR);
-
-	if (enable)
-		macb_writel(bp, NCR, reg_val | MACB_BIT(OSSMODE));
-	else
-		macb_writel(bp, NCR, reg_val & ~MACB_BIT(OSSMODE));
-}
-
 int gem_set_hwtst(struct net_device *netdev,
 		  struct kernel_hwtstamp_config *tstamp_config,
 		  struct netlink_ext_ack *extack)
 {
 	enum macb_bd_control tx_bd_control = TSTAMP_DISABLED;
 	enum macb_bd_control rx_bd_control = TSTAMP_DISABLED;
+	int rx_filter = tstamp_config->rx_filter;
 	struct macb *bp = netdev_priv(netdev);
+	unsigned long flags;
+	u32 ncr_mask = 0;
+	u32 ncr_bits = 0;
 	u32 regval;
 
 	if (!macb_dma_ptp(bp))
@@ -424,18 +406,19 @@ int gem_set_hwtst(struct net_device *netdev,
 	case HWTSTAMP_TX_OFF:
 		break;
 	case HWTSTAMP_TX_ONESTEP_SYNC:
-		gem_ptp_set_one_step_sync(bp, 1);
+		ncr_bits |= MACB_BIT(OSSMODE);
+		ncr_mask |= MACB_BIT(OSSMODE);
 		tx_bd_control = TSTAMP_ALL_FRAMES;
 		break;
 	case HWTSTAMP_TX_ON:
-		gem_ptp_set_one_step_sync(bp, 0);
+		ncr_mask |= MACB_BIT(OSSMODE);
 		tx_bd_control = TSTAMP_ALL_FRAMES;
 		break;
 	default:
 		return -ERANGE;
 	}
 
-	switch (tstamp_config->rx_filter) {
+	switch (rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
@@ -451,25 +434,33 @@ int gem_set_hwtst(struct net_device *netdev,
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
 	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
 	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
-		rx_bd_control =  TSTAMP_ALL_PTP_FRAMES;
-		tstamp_config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
-		regval = macb_readl(bp, NCR);
-		macb_writel(bp, NCR, (regval | MACB_BIT(SRTSM)));
+		rx_bd_control = TSTAMP_ALL_PTP_FRAMES;
+		rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		ncr_mask |= MACB_BIT(SRTSM);
+		ncr_bits |= MACB_BIT(SRTSM);
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
 	case HWTSTAMP_FILTER_ALL:
 		rx_bd_control = TSTAMP_ALL_FRAMES;
-		tstamp_config->rx_filter = HWTSTAMP_FILTER_ALL;
+		rx_filter = HWTSTAMP_FILTER_ALL;
 		break;
 	default:
-		tstamp_config->rx_filter = HWTSTAMP_FILTER_NONE;
 		return -ERANGE;
 	}
 
-	bp->tstamp_config = *tstamp_config;
+	spin_lock_irqsave(&bp->lock, flags);
+	if (ncr_mask) {
+		regval = macb_readl(bp, NCR);
+		regval = (regval & ~ncr_mask) | ncr_bits;
+		macb_writel(bp, NCR, regval);
+	}
 
-	if (gem_ptp_set_ts_mode(bp, tx_bd_control, rx_bd_control) != 0)
-		return -ERANGE;
+	gem_writel(bp, TXBDCTRL, GEM_BF(TXTSMODE, tx_bd_control));
+	gem_writel(bp, RXBDCTRL, GEM_BF(RXTSMODE, rx_bd_control));
+
+	tstamp_config->rx_filter = rx_filter;
+	bp->tstamp_config = *tstamp_config;
+	spin_unlock_irqrestore(&bp->lock, flags);
 
 	return 0;
 }

-- 
2.53.0

[PATCH net-next v2 2/3] net: macb: Enable RX timestamping for specific PTPv1 filters

[Kim Wooseok via B4 Relay]

From: Kim Wooseok <5mghybrid@khu.ac.kr>

Selecting a PTPv1 Sync or Delay_Req filter in gem_set_hwtst() returns
success, but leaves rx_bd_control at its initial value of
TSTAMP_DISABLED. The requested filter therefore appears to have been
applied, even though received packets have no hardware timestamps.

The PTPv1 event case already enables timestamping for all frames, so
use that path for Sync and Delay_Req as well. This enables timestamping
for both requests and returns HWTSTAMP_FILTER_ALL to tell the caller
which filter was actually applied.

Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>
Tested-by: Nicolai Buchwitz <nb@tipi-net.de>
Assisted-by: GPT-6 Astra
Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>
---
 drivers/net/ethernet/cadence/macb_ptp.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index 02face93a..af2e36f81 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -421,10 +421,6 @@ int gem_set_hwtst(struct net_device *netdev,
 	switch (rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		break;
-	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
-		break;
-	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
-		break;
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
@@ -439,6 +435,8 @@ int gem_set_hwtst(struct net_device *netdev,
 		ncr_mask |= MACB_BIT(SRTSM);
 		ncr_bits |= MACB_BIT(SRTSM);
 		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
 	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
 	case HWTSTAMP_FILTER_ALL:
 		rx_bd_control = TSTAMP_ALL_FRAMES;

-- 
2.53.0

[PATCH net-next v2 3/3] net: macb: Clear SRTSM outside PTPv2 receive filters

[Kim Wooseok via B4 Relay]

From: Kim Wooseok <5mghybrid@khu.ac.kr>

A PTPv2 receive filter sets NCR.SRTSM, but switching to NONE, ALL or a
PTPv1 filter leaves it set. As a result, selecting the same filter can
produce a different register setting depending on whether PTPv2 was
used before.

Update SRTSM whenever the RX filter changes, setting it only for PTPv2.
Switching away from PTPv2 then clears the old setting, so the bit follows
the currently selected filter.

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>
Tested-by: Nicolai Buchwitz <nb@tipi-net.de>
Assisted-by: GPT-6 Astra
Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>
---
 drivers/net/ethernet/cadence/macb_ptp.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index af2e36f81..2fd063394 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -394,8 +394,8 @@ int gem_set_hwtst(struct net_device *netdev,
 	enum macb_bd_control rx_bd_control = TSTAMP_DISABLED;
 	int rx_filter = tstamp_config->rx_filter;
 	struct macb *bp = netdev_priv(netdev);
+	u32 ncr_mask = MACB_BIT(SRTSM);
 	unsigned long flags;
-	u32 ncr_mask = 0;
 	u32 ncr_bits = 0;
 	u32 regval;
 
@@ -432,7 +432,6 @@ int gem_set_hwtst(struct net_device *netdev,
 	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
 		rx_bd_control = TSTAMP_ALL_PTP_FRAMES;
 		rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
-		ncr_mask |= MACB_BIT(SRTSM);
 		ncr_bits |= MACB_BIT(SRTSM);
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
@@ -447,11 +446,9 @@ int gem_set_hwtst(struct net_device *netdev,
 	}
 
 	spin_lock_irqsave(&bp->lock, flags);
-	if (ncr_mask) {
-		regval = macb_readl(bp, NCR);
-		regval = (regval & ~ncr_mask) | ncr_bits;
-		macb_writel(bp, NCR, regval);
-	}
+	regval = macb_readl(bp, NCR);
+	regval = (regval & ~ncr_mask) | ncr_bits;
+	macb_writel(bp, NCR, regval);
 
 	gem_writel(bp, TXBDCTRL, GEM_BF(TXTSMODE, tx_bd_control));
 	gem_writel(bp, RXBDCTRL, GEM_BF(RXTSMODE, rx_bd_control));

-- 
2.53.0

Re: [PATCH net-next v2 1/3] net: macb: Preserve timestamp settings on rejected requests

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (3) · Low: 0 (2)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] gem_set_hwtst() in drivers/net/ethernet/cadence/macb_ptp.c now
  claims to protect the NCR read-modify-write (the OSSMODE/SRTSM update)…

Pre-existing issues:
- [Medium] NCR PTP bits stay set on accepted requests in gem_set_hwtst()
  (drivers/net/ethernet/cadence/macb_ptp.c).
- [Medium] In this patch, gem_set_hwtst() accepts
  HWTSTAMP_FILTER_PTP_V1_L4_SYNC and HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ…
- [Medium] macb_resume() in drivers/net/ethernet/cadence/macb_main.c
  overwrites NCR with `macb_writel(bp, NCR, MACB_BIT(MPE))`, which…
- [Low] gem_get_ts_info() in drivers/net/ethernet/cadence/macb_main.c
  advertises only `(1 << HWTSTAMP_FILTER_NONE) | (1 <<…
- [Low] gem_set_hwtst() now updates bp->tstamp_config with a whole-struct
  copy under bp->lock.

--

net: macb: Preserve timestamp settings on rejected requests

This commit reworks gem_set_hwtst() to validate both the TX type and the
RX filter before touching any register. The adjusted RX filter is kept
local until validation succeeds. The NCR read-modify-write, the
TXBDCTRL/RXBDCTRL writes and the cache update are then done together
under bp->lock.

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index 14ae57fa00cba..02face93a53a1 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c

[ ... ]

> @@ -424,18 +406,19 @@ int gem_set_hwtst(struct net_device *netdev,
>  	case HWTSTAMP_TX_OFF:
>  		break;

[Severity: Medium]
This isn't a bug introduced by this patch, but should HWTSTAMP_TX_OFF also
add OSSMODE to ncr_mask?

Only HWTSTAMP_TX_ON clears OSSMODE here. If a ONESTEP_SYNC request is
followed by a TX_OFF request, NCR.OSSMODE stays set. At the same time,
TXBDCTRL is programmed as TSTAMP_DISABLED and bp->tstamp_config.tx_type
reports HWTSTAMP_TX_OFF.

In this patch, SRTSM has the same history dependence. ncr_mask starts at
0 and SRTSM is only added in the PTP_V2 branch. The if (ncr_mask) guard
also skips the NCR write entirely for TX_OFF with a non-V2 filter. So a
V2_EVENT -> NONE/ALL transition leaves SRTSM set.

The SRTSM half is fixed later in the series by "net: macb: Clear SRTSM
outside PTPv2 receive filters". That patch starts ncr_mask at
MACB_BIT(SRTSM) and drops the guard. The OSSMODE half still appears to be
there at the end of the series.

This patch already rewrites the logic into the mask/bits scheme. Would an
unconditional ncr_mask |= MACB_BIT(OSSMODE) cover it?

>  	case HWTSTAMP_TX_ONESTEP_SYNC:
> -		gem_ptp_set_one_step_sync(bp, 1);
> +		ncr_bits |= MACB_BIT(OSSMODE);
> +		ncr_mask |= MACB_BIT(OSSMODE);
>  		tx_bd_control = TSTAMP_ALL_FRAMES;
>  		break;
>  	case HWTSTAMP_TX_ON:
> -		gem_ptp_set_one_step_sync(bp, 0);
> +		ncr_mask |= MACB_BIT(OSSMODE);
>  		tx_bd_control = TSTAMP_ALL_FRAMES;
>  		break;
>  	default:
>  		return -ERANGE;
>  	}
>  
> -	switch (tstamp_config->rx_filter) {
> +	switch (rx_filter) {
>  	case HWTSTAMP_FILTER_NONE:
>  		break;
>  	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:

[Severity: Medium]
This is a pre-existing issue, but the two V1 L4 filters below are accepted
with a bare break:

	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
		break;
	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
		break;

rx_bd_control stays TSTAMP_DISABLED, so RXBDCTRL is programmed as
disabled. The non-zero filter is still returned to the caller unchanged
and stored in bp->tstamp_config.

Won't gem_ptp_do_rxstamp() then treat RX stamping as enabled, while no RX
timestamps are actually delivered?

This looks to be resolved later in the series by "net: macb: Enable RX
timestamping for specific PTPv1 filters". There, both cases fall through
to the HWTSTAMP_FILTER_ALL branch.

> @@ -451,25 +434,33 @@ int gem_set_hwtst(struct net_device *netdev,
>  	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
>  	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
>  	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> -		rx_bd_control =  TSTAMP_ALL_PTP_FRAMES;
> -		tstamp_config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> -		regval = macb_readl(bp, NCR);
> -		macb_writel(bp, NCR, (regval | MACB_BIT(SRTSM)));
> +		rx_bd_control = TSTAMP_ALL_PTP_FRAMES;
> +		rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;

[Severity: Low]
This isn't a bug introduced by this patch, but gem_get_ts_info() in
macb_main.c only advertises these filters:

	info->rx_filters =
		(1 << HWTSTAMP_FILTER_NONE) |
		(1 << HWTSTAMP_FILTER_ALL);

gem_set_hwtst() accepts nine PTP V2 filters. For them it programs
TSTAMP_ALL_PTP_FRAMES plus SRTSM and reports HWTSTAMP_FILTER_PTP_V2_EVENT
back to the caller.

Should HWTSTAMP_FILTER_PTP_V2_EVENT be listed in rx_filters, so that
ethtool -T matches what the setter applies?

> +		ncr_mask |= MACB_BIT(SRTSM);
> +		ncr_bits |= MACB_BIT(SRTSM);
>  		break;

[ ... ]

> -	bp->tstamp_config = *tstamp_config;
> +	spin_lock_irqsave(&bp->lock, flags);
> +	if (ncr_mask) {
> +		regval = macb_readl(bp, NCR);
> +		regval = (regval & ~ncr_mask) | ncr_bits;
> +		macb_writel(bp, NCR, regval);
> +	}

[Severity: Medium]
The commit message says "Protect the NCR read-modify-write with bp->lock".
Does bp->lock actually serialise this against the other NCR writers?

Some NCR read-modify-write paths in macb_main.c don't take bp->lock. They
could write back a stale NCR value and drop the OSSMODE/SRTSM bits set
here.

macb_mac_link_up() releases bp->lock before it updates NCR. It runs from
phylink's resolve work, not under RTNL:

	spin_unlock_irqrestore(&bp->lock, flags);
	...
	/* Enable Rx and Tx; Enable PTP unicast */
	ctrl = macb_readl(bp, NCR);
	if (gem_has_ptp(bp))
		ctrl |= MACB_BIT(PTPUNI);

	macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));

macb_hresp_error_task() is queued on system_bh_wq from
macb_interrupt_misc() on HRESP. It saves NCR, reinitialises the rings and
buffers, and then writes the saved value back, all without bp->lock:

	ctrl = macb_readl(bp, NCR);
	ctrl &= ~(MACB_BIT(RE) | MACB_BIT(TE));
	macb_writel(bp, NCR, ctrl);
	...
	ctrl |= MACB_BIT(RE) | MACB_BIT(TE);
	macb_writel(bp, NCR, ctrl);

One possible interleaving:

CPU1 (macb_mac_link_up() or macb_hresp_error_task())
  ctrl = macb_readl(bp, NCR);          /* OSSMODE == 0 */

CPU2 (gem_set_hwtst(), HWTSTAMP_TX_ONESTEP_SYNC)
  spin_lock_irqsave(&bp->lock, flags);
  macb_writel(bp, NCR, regval);        /* OSSMODE == 1 */
  bp->tstamp_config = *tstamp_config;
  spin_unlock_irqrestore(&bp->lock, flags);

CPU1
  macb_writel(bp, NCR, ctrl | ...);    /* OSSMODE == 0 again */

At this point the setter has returned 0 and bp->tstamp_config says
one-step is active. The TX completion path skips TX timestamp reporting
for one-step Sync frames via ptp_one_step_sync(), but the MAC no longer
inserts originTimestamp.

spin_lock_irqsave() only disables interrupts on the local CPU, so it does
not exclude these paths when they run on another CPU.

Do these other NCR read-modify-write sites also need bp->lock, or some
other shared serialisation, for this protection to hold?

>  
> -	if (gem_ptp_set_ts_mode(bp, tx_bd_control, rx_bd_control) != 0)
> -		return -ERANGE;
> +	gem_writel(bp, TXBDCTRL, GEM_BF(TXTSMODE, tx_bd_control));
> +	gem_writel(bp, RXBDCTRL, GEM_BF(RXTSMODE, rx_bd_control));
> +
> +	tstamp_config->rx_filter = rx_filter;
> +	bp->tstamp_config = *tstamp_config;

[Severity: Low]
This is a pre-existing issue, but the packet-path readers of this cache
don't take bp->lock. gem_ptp_do_txstamp() is called from
macb_tx_complete() under queue->tx_ptr_lock, and gem_ptp_do_rxstamp() is
called from gem_rx(). Both are in macb.h and use plain loads:

	if (bp->tstamp_config.tx_type == TSTAMP_DISABLED)
		return;
	...
	if (bp->tstamp_config.rx_filter == TSTAMP_DISABLED)
		return;

The writer and the readers share no lock. Is this a data race that KCSAN
would report?

During a reconfiguration, readers may also see the new cached mode before
or after the register writes take effect. Before this patch the store had
no lock at all.

> +	spin_unlock_irqrestore(&bp->lock, flags);
>  
>  	return 0;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but does the cached config
stay in sync with the hardware across suspend/resume?

macb_resume() in macb_main.c overwrites NCR, which clears OSSMODE and
SRTSM:

	macb_writel(bp, NCR, MACB_BIT(MPE));
	macb_init_hw(bp);
	...
	if (bp->ptp_info)
		bp->ptp_info->ptp_init(netdev);

gem_ptp_init() sets up the clock info, timer and TSU and registers the
PTP clock. It never reapplies NCR, TXBDCTRL or RXBDCTRL from
bp->tstamp_config.

After resume, gem_get_hwtst() can report a mode the hardware no longer
has, such as HWTSTAMP_TX_ONESTEP_SYNC or HWTSTAMP_FILTER_PTP_V2_EVENT.

With one-step PTP, won't the MAC stop inserting originTimestamp until
the configuration is set again?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-macb-hwtstamp-rework-v2-0-75c909b2676d%40khu.ac.kr