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

[PATCH net-next 0/4] 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, turn off one-step mode for TX_OFF, and clear SRTSM when
switching away from a PTPv2 filter. Each change is kept in its own patch.

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 switched from one-step TX and PTPv2 RX to other settings and back.
Only the relevant bits changed, and restoring the original settings
restored NCR.

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 the series with ARM64 allyesconfig and allmodconfig,
both with W=1. Sparse reported no diagnostics in macb_ptp.c.

Assisted-by: GPT-6 Astra
Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>
---
Kim Wooseok (4):
      net: macb: Preserve timestamp settings on rejected requests
      net: macb: Enable RX timestamping for specific PTPv1 filters
      net: macb: Disable one-step mode when TX timestamping is off
      net: macb: Clear SRTSM outside PTPv2 receive filters

 drivers/net/ethernet/cadence/macb_ptp.c | 63 ++++++++++++---------------------
 1 file changed, 23 insertions(+), 40 deletions(-)
---
base-commit: 8830e65ed46de41f849eefb8ba227d4852c460f6
change-id: 20260922-codex-macb-hwtstamp-submit-25ab4a2e6b94

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

[PATCH net-next 1/4] 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")
Assisted-by: GPT-6 Astra
Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>
---
 drivers/net/ethernet/cadence/macb_ptp.c | 61 ++++++++++++++-------------------
 1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index 14ae57fa0..4dbb6daa6 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)
 {
+	u32 ncr_mask = 0;
 	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_bits = 0;
 	u32 regval;
 
 	if (!macb_dma_ptp(bp))
@@ -424,18 +406,17 @@ 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);
-		tx_bd_control = TSTAMP_ALL_FRAMES;
-		break;
+		ncr_bits |= MACB_BIT(OSSMODE);
+		fallthrough;
 	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 +432,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

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

[Nicolai Buchwitz]

On 22.9.2026 11:10, Kim Wooseok via B4 Relay wrote:
> 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")
> Assisted-by: GPT-6 Astra
> Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>
> ---
>  drivers/net/ethernet/cadence/macb_ptp.c | 61 
> ++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c 
> b/drivers/net/ethernet/cadence/macb_ptp.c
> index 14ae57fa0..4dbb6daa6 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c

> [...]

>  int gem_set_hwtst(struct net_device *netdev,
>  		  struct kernel_hwtstamp_config *tstamp_config,
>  		  struct netlink_ext_ack *extack)
>  {
> +	u32 ncr_mask = 0;
>  	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_bits = 0;
>  	u32 regval;

Please re-order to keep RCS [1]

> [...]

[1] 
https://www.kernel.org/doc/html/v6.2/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs

With the above fixed:

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

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

[Théo Lebrun]

Hello Kim,

On Tue Sep 22, 2026 at 11:10 AM CEST, Kim Wooseok via B4 Relay wrote:
> 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")
> Assisted-by: GPT-6 Astra
> Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>
> ---
[...]
>  int gem_set_hwtst(struct net_device *netdev,
>  		  struct kernel_hwtstamp_config *tstamp_config,
>  		  struct netlink_ext_ack *extack)
>  {
> +	u32 ncr_mask = 0;
>  	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_bits = 0;
>  	u32 regval;

Same remark as Nicolai (no surprise there). With that

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Théo Lebrun]

Hello Théo,

On Tue Sep 22, 2026 at 9:01 PM CEST, Théo Lebrun wrote:
> Hello Kim,
>
> On Tue Sep 22, 2026 at 11:10 AM CEST, Kim Wooseok via B4 Relay wrote:
>> 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")
>> Assisted-by: GPT-6 Astra
>> Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>
>> ---
> [...]
>>  int gem_set_hwtst(struct net_device *netdev,
>>  		  struct kernel_hwtstamp_config *tstamp_config,
>>  		  struct netlink_ext_ack *extack)
>>  {
>> +	u32 ncr_mask = 0;
>>  	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_bits = 0;
>>  	u32 regval;
>
> Same remark as Nicolai (no surprise there). With that
>
> Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Sorry for the two stage review, I had forgotten that aspect on first
review. I would prefer simpler code:

> @@ -424,18 +406,17 @@ 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);
> -      tx_bd_control = TSTAMP_ALL_FRAMES;
> -      break;
> +      ncr_bits |= MACB_BIT(OSSMODE);
> +      fallthrough;
>     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;
>     }

Let's avoid the fallthrough. Simple & easy to read:

	case HWTSTAMP_TX_ONESTEP_SYNC:
		ncr_bits |= MACB_BIT(OSSMODE);
		ncr_mask |= MACB_BIT(OSSMODE);
		tx_bd_control = TSTAMP_ALL_FRAMES;
		break;
	case HWTSTAMP_TX_ON:
		ncr_mask |= MACB_BIT(OSSMODE);
		tx_bd_control = TSTAMP_ALL_FRAMES;
		break;

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 1/4] 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 · 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] Incomplete NCR lock coverage: gem_set_hwtst()
  (drivers/net/ethernet/cadence/macb_ptp.c:449-461) now performs its NCR…

Pre-existing issues:
- [Low] (Validated as FIXED LATER IN THIS SERIES — baseline decision
  false.) At this commit gem_set_hwtst() only OR-s MACB_BIT(OSSMODE)…
- [Low] (Validated as having no functional consequence — baseline
  decision false.) bp->tstamp_config is published under bp->lock in…

--

net: macb: Preserve timestamp settings on rejected requests

gem_set_hwtst() used to program the TX one-step-sync bit before validating
the RX filter, so a request rejected with -ERANGE could leave the hardware
out of sync with the cached configuration. The patch validates both the TX
type and the RX filter first, keeps the adjusted RX filter in a local, then
applies TXBDCTRL/RXBDCTRL and the NCR update under bp->lock, dropping the
gem_ptp_set_one_step_sync() and gem_ptp_set_ts_mode() helpers.

(No Link: tags in the commit header.)

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

[ ... ]

> @@ -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)
>  {
> +	u32 ncr_mask = 0;
>  	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_bits = 0;
>  	u32 regval;
>  
>  	if (!macb_dma_ptp(bp))
> @@ -424,18 +406,17 @@ 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);
> -		tx_bd_control = TSTAMP_ALL_FRAMES;
> -		break;
> +		ncr_bits |= MACB_BIT(OSSMODE);
> +		fallthrough;
>  	case HWTSTAMP_TX_ON:
> -		gem_ptp_set_one_step_sync(bp, 0);
> +		ncr_mask |= MACB_BIT(OSSMODE);
>  		tx_bd_control = TSTAMP_ALL_FRAMES;
>  		break;

[Severity: Low]

This isn't a bug introduced by this patch, but a note on which NCR bits
end up in ncr_mask.

MACB_BIT(OSSMODE) is only added to ncr_mask in the HWTSTAMP_TX_ON arm, and
HWTSTAMP_TX_OFF breaks out without touching ncr_mask, so is a previously
programmed NCR.OSSMODE ever cleared when timestamping is turned off?
MACB_BIT(SRTSM) is likewise only added in the PTPv2 group below, so the
FILTER_NONE, FILTER_ALL and PTPv1 arms leave NCR.SRTSM as it was.

When both switches leave ncr_mask == 0 (tx_type HWTSTAMP_TX_OFF plus
rx_filter HWTSTAMP_FILTER_NONE), the if (ncr_mask) guard further down skips
the NCR write entirely, while TXBDCTRL/RXBDCTRL and bp->tstamp_config are
still updated to the disabled state.

The behaviour matches the pre-patch code, since gem_ptp_set_one_step_sync()
was not called for HWTSTAMP_TX_OFF either and SRTSM was only ever set, so
this is not a new regression. It is also addressed later in the series,
where ncr_mask becomes an unconditional
MACB_BIT(OSSMODE) | MACB_BIT(SRTSM) and the read-modify-write is no longer
guarded, in "net: macb: Disable one-step mode when TX timestamping is off"
and "net: macb: Clear SRTSM outside PTPv2 receive filters".

>  	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 +432,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);
> +	}

[Severity: Medium]

The changelog says "Protect the NCR read-modify-write with bp->lock". Do
the other NCR read-modify-write sites in this driver take bp->lock too?

macb_mac_link_up() in macb_main.c does its NCR update after releasing
bp->lock:

	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_mac_link_down() does the same unlocked:

	/* Disable Rx and Tx */
	ctrl = macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE));
	macb_writel(bp, NCR, ctrl);

and macb_hresp_error_task() carries an NCR image across the ring and buffer
re-initialisation before writing it back, also without bp->lock.

macb_mac_link_up() and macb_mac_link_down() are reached from
phylink_resolve(), which holds pl->phydev_mutex, phy->lock and
pl->state_mutex, but neither rtnl nor bp->lock:

	phylink_resolve()
	  -> phylink_link_up() -> macb_mac_link_up()
	  -> phylink_link_down() -> macb_mac_link_down()

So can these run concurrently with the rtnl-serialized ndo_hwtstamp_set()
path that ends up here?

If they can, is either of these lost-update outcomes possible?

- the OSSMODE/SRTSM bits just programmed here are erased by the link
  callback writing back its older NCR image, while bp->tstamp_config (what
  gem_get_hwtst() returns) still reports one-step-sync or PTPv2 RX
  filtering as enabled

- MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(PTPUNI) from macb_mac_link_up()
  are erased by the write-back here, leaving the MAC receiver and
  transmitter disabled while the carrier stays up until the next link
  transition

macb_hresp_error_task() seems to give the widest window, since the NCR
snapshot is taken before mog_init_rings()/macb_init_buffers() and written
back afterwards. Should these NCR read-modify-writes be brought under
bp->lock as well for the new critical section to actually be exclusive?

>  
> -	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);

[Severity: Low]

This is a pre-existing property rather than something this patch
introduces, but does taking bp->lock around the bp->tstamp_config
assignment buy anything with respect to the fast-path readers?

gem_ptp_do_txstamp() and gem_ptp_do_rxstamp() in macb.h read the same
fields from NAPI/softirq context with no lock:

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

so a poll running during the update can still observe a new tx_type with
an old rx_filter. Both fields are naturally aligned int members of
struct kernel_hwtstamp_config, so no torn value is possible, and
gem_ptp_rxstamp()/gem_ptp_txstamp() re-check DMA_RXVALID/DMA_TXVALID in the
descriptor, so the worst case looks like one in-flight frame evaluated
against a mixed configuration during reconfiguration. The unlocked
publication predates this patch, which did the assignment with no lock at
all, so this is only a note that the new section is not atomic for those
readers.

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-codex-macb-hwtstamp-submit-v1-0-9d1abaa53296%40khu.ac.kr

[PATCH net-next 2/4] 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")
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 4dbb6daa6..b6d17fef4 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -419,10 +419,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:
@@ -437,6 +433,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

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

[Nicolai Buchwitz]

On 22.9.2026 11:10, Kim Wooseok via B4 Relay wrote:
> 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")
> 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 4dbb6daa6..b6d17fef4 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -419,10 +419,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:
> @@ -437,6 +433,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;

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

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

[Théo Lebrun]

Hello Kim,

On Tue Sep 22, 2026 at 11:10 AM CEST, Kim Wooseok via B4 Relay wrote:
> 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")
> Assisted-by: GPT-6 Astra
> Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next 3/4] net: macb: Disable one-step mode when TX timestamping is off

[Kim Wooseok via B4 Relay]

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

Switching from HWTSTAMP_TX_ONESTEP_SYNC to HWTSTAMP_TX_OFF turns off
descriptor timestamping, but leaves NCR.OSSMODE set, so one-step mode
remains enabled.

Update OSSMODE for every accepted TX mode and set it only for
HWTSTAMP_TX_ONESTEP_SYNC. This also clears the previous one-step setting
when switching to OFF. Since the update mask now always includes
OSSMODE, drop the check for an empty mask.

Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
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 b6d17fef4..6376b6631 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -390,7 +390,7 @@ int gem_set_hwtst(struct net_device *netdev,
 		  struct kernel_hwtstamp_config *tstamp_config,
 		  struct netlink_ext_ack *extack)
 {
-	u32 ncr_mask = 0;
+	u32 ncr_mask = MACB_BIT(OSSMODE);
 	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;
@@ -409,7 +409,6 @@ int gem_set_hwtst(struct net_device *netdev,
 		ncr_bits |= MACB_BIT(OSSMODE);
 		fallthrough;
 	case HWTSTAMP_TX_ON:
-		ncr_mask |= MACB_BIT(OSSMODE);
 		tx_bd_control = TSTAMP_ALL_FRAMES;
 		break;
 	default:
@@ -445,11 +444,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 3/4] net: macb: Disable one-step mode when TX timestamping is off

[Nicolai Buchwitz]

On 22.9.2026 11:10, Kim Wooseok via B4 Relay wrote:
> From: Kim Wooseok <5mghybrid@khu.ac.kr>
> 
> Switching from HWTSTAMP_TX_ONESTEP_SYNC to HWTSTAMP_TX_OFF turns off
> descriptor timestamping, but leaves NCR.OSSMODE set, so one-step mode
> remains enabled.
> 
> Update OSSMODE for every accepted TX mode and set it only for
> HWTSTAMP_TX_ONESTEP_SYNC. This also clears the previous one-step 
> setting
> when switching to OFF. Since the update mask now always includes
> OSSMODE, drop the check for an empty mask.
> 
> Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
> 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 b6d17fef4..6376b6631 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -390,7 +390,7 @@ int gem_set_hwtst(struct net_device *netdev,
>  		  struct kernel_hwtstamp_config *tstamp_config,
>  		  struct netlink_ext_ack *extack)
>  {
> -	u32 ncr_mask = 0;
> +	u32 ncr_mask = MACB_BIT(OSSMODE);
>  	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;
> @@ -409,7 +409,6 @@ int gem_set_hwtst(struct net_device *netdev,
>  		ncr_bits |= MACB_BIT(OSSMODE);
>  		fallthrough;
>  	case HWTSTAMP_TX_ON:
> -		ncr_mask |= MACB_BIT(OSSMODE);
>  		tx_bd_control = TSTAMP_ALL_FRAMES;
>  		break;
>  	default:
> @@ -445,11 +444,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));

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net-next 3/4] net: macb: Disable one-step mode when TX timestamping is off

[Théo Lebrun]

Hello Kim,

On Tue Sep 22, 2026 at 11:10 AM CEST, Kim Wooseok via B4 Relay wrote:
> From: Kim Wooseok <5mghybrid@khu.ac.kr>
>
> Switching from HWTSTAMP_TX_ONESTEP_SYNC to HWTSTAMP_TX_OFF turns off
> descriptor timestamping, but leaves NCR.OSSMODE set, so one-step mode
> remains enabled.
>
> Update OSSMODE for every accepted TX mode and set it only for
> HWTSTAMP_TX_ONESTEP_SYNC. This also clears the previous one-step setting
> when switching to OFF. Since the update mask now always includes
> OSSMODE, drop the check for an empty mask.
>
> Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
> Assisted-by: GPT-6 Astra
> Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>

I don't see the point in this commit. NCR.OSSMODE defines what value we
put in TX Sync messages. This register can be left alone when
HWTSTAMP_TX_OFF.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 3/4] net: macb: Disable one-step mode when TX timestamping is off

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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() now performs an unconditional read-modify-
  write of NCR under bp->lock (macb_ptp.c:446-448), including for…

--

net: macb: Disable one-step mode when TX timestamping is off

gem_set_hwtst() left NCR.OSSMODE set when switching from
HWTSTAMP_TX_ONESTEP_SYNC to HWTSTAMP_TX_OFF.  The patch always includes
OSSMODE in the NCR update mask and sets the bit only for
HWTSTAMP_TX_ONESTEP_SYNC, so the empty-mask check around the NCR
read-modify-write is dropped.

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index b6d17fef429f6..6376b663130d8 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c

[ ... ]

> @@ -445,11 +444,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);

[Severity: Medium]
Is bp->lock enough to make this NCR read-modify-write atomic against the
other NCR writers in the driver?

macb_tx_lpi_set() in macb_main.c documents bp->lock as the intended
serialisation for NCR:

	lockdep_assert_held(&bp->lock);

	ncr = macb_readl(bp, NCR);

but several other NCR read-modify-write sites do not take it.
macb_mac_link_down():

	/* Disable Rx and Tx */
	ctrl = macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE));
	macb_writel(bp, NCR, ctrl);

macb_mac_link_up() drops bp->lock first and only then touches NCR:

	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_interrupt_misc() does the same thing from hardirq context, and
macb_hresp_error_task() from a work item, neither under bp->lock.

gem_set_hwtst() runs from ndo_hwtstamp_set under rtnl, while
mac_link_up/mac_link_down run from phylink_resolve() under
pl->state_mutex, so can these two run concurrently on different CPUs?

If so, can the OSSMODE clear be lost like this?

CPU0 phylink_resolve()->macb_mac_link_up()
	ctrl = macb_readl(bp, NCR);		/* OSSMODE still set */

CPU1 gem_set_hwtst(HWTSTAMP_TX_OFF)
	regval = (regval & ~MACB_BIT(OSSMODE));
	macb_writel(bp, NCR, regval);
	bp->tstamp_config = *tstamp_config;	/* records TX_OFF */

CPU0
	macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));
						/* OSSMODE back on */

That leaves one-step insertion enabled in hardware while the cached
config reports HWTSTAMP_TX_OFF, which is the divergence the commit
message describes removing.

The reverse direction looks newly reachable too, since a
HWTSTAMP_TX_OFF plus HWTSTAMP_FILTER_NONE request previously skipped the
register access entirely and now always writes NCR:

CPU0 gem_set_hwtst()
	regval = macb_readl(bp, NCR);		/* RE|TE set */

CPU1 macb_mac_link_down()
	macb_writel(bp, NCR, ctrl & ~(RE|TE));

CPU0
	macb_writel(bp, NCR, regval & ~ncr_mask);	/* RE|TE restored */

Would it make sense to convert the remaining NCR read-modify-write sites
to take bp->lock, so the assumption this patch relies on actually holds?
Note spin_lock_irqsave() here does not exclude the macb_interrupt_misc()
NCR update running on another CPU.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-codex-macb-hwtstamp-submit-v1-0-9d1abaa53296%40khu.ac.kr

[PATCH net-next 4/4] 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.

Assisted-by: GPT-6 Astra
Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>
---
 drivers/net/ethernet/cadence/macb_ptp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index 6376b6631..637d0da32 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -390,7 +390,7 @@ int gem_set_hwtst(struct net_device *netdev,
 		  struct kernel_hwtstamp_config *tstamp_config,
 		  struct netlink_ext_ack *extack)
 {
-	u32 ncr_mask = MACB_BIT(OSSMODE);
+	u32 ncr_mask = MACB_BIT(OSSMODE) | MACB_BIT(SRTSM);
 	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;
@@ -429,7 +429,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:

-- 
2.53.0

Re: [PATCH net-next 4/4] net: macb: Clear SRTSM outside PTPv2 receive filters

[Nicolai Buchwitz]

On 22.9.2026 11:10, Kim Wooseok via B4 Relay wrote:
> 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.
> 
> Assisted-by: GPT-6 Astra
> Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>
> ---
>  drivers/net/ethernet/cadence/macb_ptp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c 
> b/drivers/net/ethernet/cadence/macb_ptp.c
> index 6376b6631..637d0da32 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -390,7 +390,7 @@ int gem_set_hwtst(struct net_device *netdev,
>  		  struct kernel_hwtstamp_config *tstamp_config,
>  		  struct netlink_ext_ack *extack)
>  {
> -	u32 ncr_mask = MACB_BIT(OSSMODE);
> +	u32 ncr_mask = MACB_BIT(OSSMODE) | MACB_BIT(SRTSM);
>  	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;
> @@ -429,7 +429,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:

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net-next 4/4] net: macb: Clear SRTSM outside PTPv2 receive filters

[Théo Lebrun]

Hello Kim,

On Tue Sep 22, 2026 at 11:10 AM CEST, Kim Wooseok via B4 Relay wrote:
> 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.
>
> Assisted-by: GPT-6 Astra
> Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>

This one sounds more relevant to keep (compared to patch 3). We have
NCFGR.DRFCS always set so the FCS is always discarded so I don't think
we fix any bug here. But we avoid a footgun that could hit later if we
add Rx FCS support.

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 4/4] net: macb: Clear SRTSM outside PTPv2 receive filters

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Missing `Fixes:` tag and no stated user-visible impact.

Pre-existing issues:
- [Medium] ABI advertisement vs. implementation mismatch:
  gem_get_ts_info() (drivers/net/ethernet/cadence/macb_main.c:3772-3774)…
- [Medium] PTP timestamping hardware state is lost across system
  suspend/resume while the cached configuration is kept.
- [Medium] Lost-update race on the NCR MMIO register: gem_set_hwtst()
  performs its read-modify-write of NCR under…

--

net: macb: Clear SRTSM outside PTPv2 receive filters

The patch moves MACB_BIT(SRTSM) into the unconditional NCR read-modify-write
mask in gem_set_hwtst(), so the bit is now programmed from the currently
selected RX filter instead of only ever being set. Switching away from a
PTPv2 filter therefore clears it.

A few questions below, mostly about pre-existing behaviour in the same
function.

>     net: macb: Clear SRTSM outside PTPv2 receive filters
>
>     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.

[Severity: Low]
Should this carry a Fixes: tag?  git blame at the parent commit points the
sticky bit at ab91f0a9b5f4 ("net: macb: Add hardware PTP support"), which
introduced:

	regval = macb_readl(bp, NCR);
	macb_writel(bp, NCR, (regval | MACB_BIT(SRTSM)));

and nothing in the tree ever cleared it since.  The three sibling patches
in this series that touch the same function ("net: macb: Preserve timestamp
settings on rejected requests", "net: macb: Enable RX timestamping for
specific PTPv1 filters", "net: macb: Disable one-step mode when TX
timestamping is off") all carry Fixes: ab91f0a9b5f4.

This patch also depends on the ncr_mask read-modify-write added by
"net: macb: Preserve timestamp settings on rejected requests", so without
a tag stable trees would get patches 1-3 and not this one, leaving the
SRTSM half of the same defect in place.

The changelog stops at "produce a different register setting" and never
names the user-visible effect.  Can the consequence of leaving SRTSM set
for a NONE/ALL/PTPv1 filter be spelled out, so it is clear whether this is
a functional fix or a consistency cleanup?

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index 6376b663130d8..637d0da3278c8 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -390,7 +390,7 @@ int gem_set_hwtst(struct net_device *netdev,
>  		  struct kernel_hwtstamp_config *tstamp_config,
>  		  struct netlink_ext_ack *extack)
>  {
> -	u32 ncr_mask = MACB_BIT(OSSMODE);
> +	u32 ncr_mask = MACB_BIT(OSSMODE) | MACB_BIT(SRTSM);

[Severity: Medium]
This is a pre-existing issue, but can the NCR update programmed here be
lost?  gem_set_hwtst() does its read-modify-write under bp->lock:

	spin_lock_irqsave(&bp->lock, flags);
	regval = macb_readl(bp, NCR);
	regval = (regval & ~ncr_mask) | ncr_bits;
	macb_writel(bp, NCR, regval);

while macb_mac_link_up() in macb_main.c drops bp->lock and then touches the
same register with no lock held:

	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));

gem_set_hwtst() runs from ndo_hwtstamp_set under rtnl, and
macb_mac_link_up() from the phylink resolve worker under phylink's
state_mutex, so the two are not serialised against each other:

CPU0 (phylink resolve)          CPU1 (SIOCSHWTSTAMP)
ctrl = macb_readl(bp, NCR);
                                spin_lock_irqsave(&bp->lock);
                                macb_writel(bp, NCR, ... SRTSM/OSSMODE ...);
                                bp->tstamp_config = *tstamp_config;
                                spin_unlock_irqrestore(&bp->lock);
macb_writel(bp, NCR, ctrl | RE | TE);

Does that leave the hardware bit disagreeing with bp->tstamp_config, i.e.
the same "the bit does not follow the currently selected filter" symptom
the changelog describes, now for SRTSM as well as OSSMODE?  Would moving
the RE/TE/PTPUNI update in macb_mac_link_up() inside bp->lock be the right
fix, so all NCR read-modify-writes share one lock?

[Severity: Medium]
This isn't a bug introduced by this patch either, but what happens to these
bits across suspend/resume?  macb_resume() in macb_main.c blanks the
register:

	macb_writel(bp, NCR, MACB_BIT(MPE));
	macb_init_hw(bp);

which clears OSSMODE and SRTSM, and macb_init_hw() only programs NCFGR and
the DMA registers.  Grepping the driver, the only writer of GEM_TXBDCTRL
and GEM_RXBDCTRL is gem_set_hwtst():

	gem_writel(bp, TXBDCTRL, GEM_BF(TXTSMODE, tx_bd_control));
	gem_writel(bp, RXBDCTRL, GEM_BF(RXTSMODE, rx_bd_control));

and the re-invoked gem_ptp_init() only restores the timer/TSU and registers
the PTP clock.

bp->tstamp_config survives suspend, so gem_get_hwtst() keeps returning the
old tx_type and rx_filter:

	*tstamp_config = bp->tstamp_config;

while the descriptors no longer carry timestamps, so RX frames get no
hwtstamp attached and every timestamped transmit ends in:

	if (!GEM_BFEXT(DMA_TXVALID, desc->ctrl)) {
		dev_warn_ratelimited(&bp->pdev->dev, "Timestamp not set in TX BD as expected\n");
		return;
	}

with skb_tstamp_tx() never called and no error reported to userspace.  Since
SRTSM is now derived strictly from the cached filter, does this patch extend
that cached-versus-hardware divergence to SRTSM too, and should the resume
path re-apply bp->tstamp_config?

>  	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;
> @@ -429,7 +429,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;

[Severity: Medium]
This is pre-existing and not introduced here, but are these PTPv2 filters
reachable through the advertised capabilities?  gem_get_ts_info() in
macb_main.c reports only:

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

yet this switch accepts all nine HWTSTAMP_FILTER_PTP_V2_* values, programs
distinct hardware state for them (rx_bd_control = TSTAMP_ALL_PTP_FRAMES plus
NCR.SRTSM), normalises rx_filter to HWTSTAMP_FILTER_PTP_V2_EVENT, caches it
in bp->tstamp_config and returns it to userspace.  gem_get_hwtst() then
reports PTP_V2_EVENT as active, a filter ethtool -T says is unsupported.

Should gem_get_ts_info() advertise the PTPv2 bits, or should gem_set_hwtst()
reject them with -ERANGE?  This matters for the patch because the newly
unconditional SRTSM clear is only observable on transitions to and from
those unadvertised PTPv2 filters.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-codex-macb-hwtstamp-submit-v1-0-9d1abaa53296%40khu.ac.kr

Re: [PATCH net-next 0/4] net: macb: Rework hardware timestamp configuration

[Nicolai Buchwitz]

Hi Kim

On 22.9.2026 11:10, Kim Wooseok via B4 Relay wrote:
> 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, turn off one-step mode for TX_OFF, and clear SRTSM when
> switching away from a PTPv2 filter. Each change is kept in its own 
> patch.
> 
> 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 switched from one-step TX and PTPv2 RX to other settings and back.
> Only the relevant bits changed, and restoring the original settings
> restored NCR.
> 
> 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 the series with ARM64 allyesconfig and 
> allmodconfig,
> both with W=1. Sparse reported no diagnostics in macb_ptp.c.
> 
> Assisted-by: GPT-6 Astra
> Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>
> ---
> Kim Wooseok (4):
>       net: macb: Preserve timestamp settings on rejected requests
>       net: macb: Enable RX timestamping for specific PTPv1 filters
>       net: macb: Disable one-step mode when TX timestamping is off
>       net: macb: Clear SRTSM outside PTPv2 receive filters
> 
>  drivers/net/ethernet/cadence/macb_ptp.c | 63 
> ++++++++++++---------------------
>  1 file changed, 23 insertions(+), 40 deletions(-)
> ---
> base-commit: 8830e65ed46de41f849eefb8ba227d4852c460f6
> change-id: 20260922-codex-macb-hwtstamp-submit-25ab4a2e6b94
> 
> Best regards,
> --
> Kim Wooseok <5mghybrid@khu.ac.kr>

For the series:

Tested-by: Nicolai Buchwitz <nb@tipi-net.de> # Raspberry Pi CM5 (macb 
rp1 with BCM54210PE)

Regards
Nicolai