[PATCH net RESEND 0/2] net: macb: Fix PTP timestamp configuration handling

[PATCH net RESEND 0/2] net: macb: Fix PTP timestamp configuration handling

[kimwooseok]

Resending the series as plain text to correct the previous multipart
webmail submission and the quoted and rewrapped patch bodies. There are
no code changes; the Assisted-by trailers now name the tool.

This series fixes two timestamp configuration problems in gem_set_hwtst().
A rejected RX-filter request can change the active TX one-step mode
while leaving the cached configuration unchanged. Patch 1 defers the
one-step mode update until both TX type and RX filter are validated.

The two specific PTPv1 RX filters currently succeed with RX timestamping
disabled. Patch 2 routes HWTSTAMP_FILTER_PTP_V1_L4_SYNC and
HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ through the existing all-frame
fallback and reports HWTSTAMP_FILTER_ALL to userspace. Both defects
originate in ab91f0a9b5f4 ("net: macb: Add hardware PTP support").

Both defects were reproduced on a Raspberry Pi 5 Model B Rev 1.1 with
macb eth0. Before the fixes, rejected requests changed OSSMODE in both
directions; each specific PTPv1 filter produced zero hardware timestamps
for 61 packets of its requested type. On 6.18.46-macb-ptp-functional-rt+,
both rejected requests preserved cached settings and the full NCR, and
each PTPv1 filter returned ALL and timestamped 60/60 Sync and 60/60
Delay_Req fixtures. ALL/NONE controls and all nine configuration cases
passed. The board was subsequently returned to its original kernel.

Additional validation:
- net 7f26a5e8040b: ARM64 allmodconfig and allyesconfig full targets
  passed with GCC 14.2.0, W=1 and CONFIG_WERROR=n. After applying the
  series, incremental builds of the same full targets passed with
  identical configurations and no new warnings. Both rebuilt macb_ptp.o.
- Pi board configuration: Image.gz, modules and dtbs built with W=1;
  the resulting kernel booted and passed the hardware tests above.
- Both final patches passed strict checkpatch including sign-off checks,
  and standalone/series application checks on the net base.

The RX tests check raw hardware timestamp presence, not absolute
timestamp accuracy or PHC synchronization.

An LLM assisted with source analysis, preparation of the fixes and test
helpers, and drafting the descriptions. Results are from the recorded
board tests and build logs.

Assisted-by: GPT-6 Astra

kimwooseok (2):
  net: macb: Preserve one-step mode on rejected timestamp requests
  net: macb: Use all-frame timestamping for PTPv1 RX filters

base-commit: 7f26a5e8040b4957ef4dbdfcde6cc7ba2db53937

[PATCH net RESEND 1/2] net: macb: Preserve one-step mode on rejected timestamp requests

[kimwooseok]

gem_set_hwtst() programs NCR.OSSMODE while processing tx_type, before
validating rx_filter. An unsupported receive filter can therefore cause
the operation to return -ERANGE after changing the active transmit mode.
The cached configuration is not updated, so a subsequent SIOCGHWTSTAMP
reports the previous transmit mode even though the hardware has changed.

For example, configure HWTSTAMP_TX_ON with HWTSTAMP_FILTER_ALL, then
request HWTSTAMP_TX_ONESTEP_SYNC with HWTSTAMP_FILTER_NTP_ALL. The latter
request fails but enables one-step synchronization. The reverse
transition can clear one-step mode despite returning the same error.

Defer programming the one-step mode until both the transmit type and
receive filter have been validated. Rejected receive filters then leave
the active transmit mode unchanged.

Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
Assisted-by: GPT-6 Astra
Signed-off-by: kimwooseok <5mghybrid@khu.ac.kr>
---
Resending as plain text because the previous webmail submission included
HTML and quoted and rewrapped the patch. No code changes; the Assisted-by
trailer now names the tool.

 drivers/net/ethernet/cadence/macb_ptp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index e5195d7..51659bb 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -418,11 +418,9 @@ 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;
 	case HWTSTAMP_TX_ON:
-		gem_ptp_set_one_step_sync(bp, 0);
 		tx_bd_control = TSTAMP_ALL_FRAMES;
 		break;
 	default:
@@ -460,6 +458,11 @@ int gem_set_hwtst(struct net_device *netdev,
 		return -ERANGE;
 	}
 
+	if (tstamp_config->tx_type == HWTSTAMP_TX_ONESTEP_SYNC)
+		gem_ptp_set_one_step_sync(bp, 1);
+	else if (tstamp_config->tx_type == HWTSTAMP_TX_ON)
+		gem_ptp_set_one_step_sync(bp, 0);
+
 	bp->tstamp_config = *tstamp_config;
 
 	if (gem_ptp_set_ts_mode(bp, tx_bd_control, rx_bd_control) != 0)
-- 
2.53.0.windows.3

Re: [PATCH net RESEND 1/2] net: macb: Preserve one-step mode on rejected timestamp requests

[Nicolai Buchwitz]

Hi Kimwoo

On 11.9.2026 10:04, kimwooseok wrote:
> gem_set_hwtst() programs NCR.OSSMODE while processing tx_type, before
> validating rx_filter. An unsupported receive filter can therefore cause
> the operation to return -ERANGE after changing the active transmit 
> mode.
> The cached configuration is not updated, so a subsequent SIOCGHWTSTAMP
> reports the previous transmit mode even though the hardware has 
> changed.
> 
> For example, configure HWTSTAMP_TX_ON with HWTSTAMP_FILTER_ALL, then
> request HWTSTAMP_TX_ONESTEP_SYNC with HWTSTAMP_FILTER_NTP_ALL. The 
> latter
> request fails but enables one-step synchronization. The reverse
> transition can clear one-step mode despite returning the same error.
> 
> Defer programming the one-step mode until both the transmit type and
> receive filter have been validated. Rejected receive filters then leave
> the active transmit mode unchanged.
> 
> Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
> Assisted-by: GPT-6 Astra
> Signed-off-by: kimwooseok <5mghybrid@khu.ac.kr>
> ---
> Resending as plain text because the previous webmail submission 
> included
> HTML and quoted and rewrapped the patch. No code changes; the 
> Assisted-by
> trailer now names the tool.
> 
>  drivers/net/ethernet/cadence/macb_ptp.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c 
> b/drivers/net/ethernet/cadence/macb_ptp.c
> index e5195d7..51659bb 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -418,11 +418,9 @@ 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;
>  	case HWTSTAMP_TX_ON:
> -		gem_ptp_set_one_step_sync(bp, 0);
>  		tx_bd_control = TSTAMP_ALL_FRAMES;
>  		break;
>  	default:
> @@ -460,6 +458,11 @@ int gem_set_hwtst(struct net_device *netdev,
>  		return -ERANGE;
>  	}
> 
> +	if (tstamp_config->tx_type == HWTSTAMP_TX_ONESTEP_SYNC)
> +		gem_ptp_set_one_step_sync(bp, 1);
> +	else if (tstamp_config->tx_type == HWTSTAMP_TX_ON)
> +		gem_ptp_set_one_step_sync(bp, 0);
> +
>  	bp->tstamp_config = *tstamp_config;
> 
>  	if (gem_ptp_set_ts_mode(bp, tx_bd_control, rx_bd_control) != 0)

Reviewed-by: Nicolai Buchwitz <nb@tip-net.de>
Tested-by: Nicolai Buchwitz <nb@tipi-net.de> # Raspberry Pi CM5

Thanks,
Nicolai

Re: [PATCH net RESEND 1/2] net: macb: Preserve one-step mode on rejected timestamp requests

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: macb: Preserve one-step mode on rejected timestamp requests

This moves the gem_ptp_set_one_step_sync() calls in gem_set_hwtst() out of
the tx_type switch and down past the rx_filter validation, so a request
rejected with -ERANGE no longer changes NCR.OSSMODE.  The cached
bp->tstamp_config and the TXBDCTRL/RXBDCTRL programming are unchanged.

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index e5195d7dac1d5..51659bbdfac9a 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -418,11 +418,9 @@ 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;
>  	case HWTSTAMP_TX_ON:
> -		gem_ptp_set_one_step_sync(bp, 0);
>  		tx_bd_control = TSTAMP_ALL_FRAMES;
>  		break;
>  	default:
> @@ -460,6 +458,11 @@ int gem_set_hwtst(struct net_device *netdev,
>  		return -ERANGE;
>  	}
>  
> +	if (tstamp_config->tx_type == HWTSTAMP_TX_ONESTEP_SYNC)
> +		gem_ptp_set_one_step_sync(bp, 1);
> +	else if (tstamp_config->tx_type == HWTSTAMP_TX_ON)
> +		gem_ptp_set_one_step_sync(bp, 0);
> +
>  	bp->tstamp_config = *tstamp_config;

[Severity: High]
Does this leave the same hardware/cache divergence reachable for the
HWTSTAMP_TX_OFF transition that the commit message sets out to remove?

Neither arm of the new conditional runs for HWTSTAMP_TX_OFF, but the next
statement caches the request unconditionally and the call after it programs
TXBDCTRL = TSTAMP_DISABLED:

	if (tstamp_config->tx_type == HWTSTAMP_TX_ONESTEP_SYNC)
		gem_ptp_set_one_step_sync(bp, 1);
	else if (tstamp_config->tx_type == HWTSTAMP_TX_ON)
		gem_ptp_set_one_step_sync(bp, 0);

	bp->tstamp_config = *tstamp_config;

	if (gem_ptp_set_ts_mode(bp, tx_bd_control, rx_bd_control) != 0)

So for the sequence

  SIOCSHWTSTAMP{tx_type = HWTSTAMP_TX_ONESTEP_SYNC}   -> OSSMODE = 1
  SIOCSHWTSTAMP{tx_type = HWTSTAMP_TX_OFF}            -> returns 0

gem_get_hwtst() then reports HWTSTAMP_TX_OFF via

	*tstamp_config = bp->tstamp_config;

while gem_ptp_set_one_step_sync() in macb_ptp.c is the only writer of
MACB_BIT(OSSMODE) in the driver, so One Step Synchro Mode stays enabled in
NCR.  Does the MAC then keep rewriting the originTimestamp of transmitted
PTP Sync frames?  The driver assumes it does elsewhere, since
ptp_one_step_sync() in macb_main.c is used to suppress software FCS/CRC
insertion for exactly those frames.

Would an unconditional call be preferable to the two-arm conditional, for
example:

	gem_ptp_set_one_step_sync(bp,
				  tstamp_config->tx_type == HWTSTAMP_TX_ONESTEP_SYNC);

[Severity: Medium]
This isn't a bug introduced by this patch, but while the call site is being
moved, is the read-modify-write inside gem_ptp_set_one_step_sync()
sufficiently serialized?

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

The SRTSM update in the rx_filter switch of gem_set_hwtst() does the same
thing:

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

Both run from process context with BHs enabled and without bp->lock, while
other NCR updaters take bp->lock, for example macb_tx_restart() from
NAPI/softirq context:

	spin_lock(&bp->lock);
	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
	spin_unlock(&bp->lock);

and macb_tx_lpi_set(), which asserts it:

	lockdep_assert_held(&bp->lock);

	ncr = macb_readl(bp, NCR);

Can a concurrent locked writer's bit be lost if it lands between the
macb_readl() and macb_writel() here?  In the worst case the stale write-back
would drop MACB_BIT(RE) | MACB_BIT(TE) just restored by macb_mac_link_up(),
leaving RX/TX disabled while the stack considers the link up; more commonly
a TSTART or TXLPIEN update is lost.  The same unlocked pattern already
exists in macb_mac_link_up() after it releases bp->lock and in the RXUBR
errata path of macb_interrupt(), so this predates the patch, but would
wrapping both NCR read-modify-writes in gem_set_hwtst() with
spin_lock_irqsave(&bp->lock, ...) be the right direction?

[Severity: Low]
This is a pre-existing inconsistency rather than something this patch
introduces, but since gem_set_hwtst() is being touched: gem_get_ts_info()
in macb_main.c advertises only

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

while the rx_filter switch in gem_set_hwtst() accepts the nine
HWTSTAMP_FILTER_PTP_V2_* values and reports back a filter the ts_info
bitmap says is unsupported:

		rx_bd_control =  TSTAMP_ALL_PTP_FRAMES;
		tstamp_config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;

Neither the legacy ioctl path via net_hwtstamp_validate() nor the ethtool
netlink tsconfig path checks the request against the driver's advertised
bitmap, so userspace can end up with an rx_filter value that is absent from
the supported set.  Should HWTSTAMP_FILTER_PTP_V2_EVENT be added to
info->rx_filters?

For reference, the related concern about HWTSTAMP_FILTER_PTP_V1_L4_SYNC and
HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ returning success while leaving
rx_bd_control at TSTAMP_DISABLED is addressed later in this series by "net:
macb: Use all-frame timestamping for PTPv1 RX filters", which folds both
cases into the TSTAMP_ALL_FRAMES arm.
-- 
This is an AI-generated review.

Re: [PATCH net RESEND 1/2] net: macb: Preserve one-step mode on rejected timestamp requests

[kimwooseok]

Hi Paolo,

Thanks for forwarding the review. Taking the items in turn:

TX_OFF / OSSMODE: this is pre-existing and independent of what this
patch fixes. The original code also left OSSMODE untouched on
HWTSTAMP_TX_OFF; this patch only addresses rejected (-ERANGE)
requests changing hardware state, while TX_OFF is an accepted request.

I propose handling TX_OFF in a separate follow-up patch, keeping this
series focused on the two reported bugs. It is a distinct defect and
should have its own changelog and Fixes: tag for stable backporting.
I will verify the TX_OFF fix on the Pi 5 and post it as a follow-up
once this series lands, since it touches the same lines.

NCR read-modify-write locking, the gem_get_ts_info() rx_filters bitmap,
and re-applying tstamp_config on resume: as the review itself notes,
these predate the series and are not made worse by it. I'd handle
them separately (net-next) rather than widen these Fixes patches.

SRTSM: NCR.SRTSM only makes the MAC replace the received FCS with the RX
timestamp nanoseconds in memory; with DRFCS set it has no effect on the
descriptor-based timestamps this driver uses, so leaving it set does not
affect timestamping. It is also unrelated to the two PTPv1 filters
remapped in patch 2.

Thanks,
Wooseok

[PATCH net RESEND 2/2] net: macb: Use all-frame timestamping for PTPv1 RX filters

[kimwooseok]

gem_set_hwtst() accepts HWTSTAMP_FILTER_PTP_V1_L4_SYNC and
HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ without changing rx_bd_control from
TSTAMP_DISABLED. A successful request thus disables receive timestamping
while reporting the requested nonempty filter to userspace.

Handle these two filters through the existing all-frame fallback used
for HWTSTAMP_FILTER_PTP_V1_L4_EVENT. This enables receive timestamping
for a superset of the requested packets and returns HWTSTAMP_FILTER_ALL
to describe the configuration actually selected.

Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
Assisted-by: GPT-6 Astra
Signed-off-by: kimwooseok <5mghybrid@khu.ac.kr>
---
Resending as plain text because the previous webmail submission included
HTML and quoted and rewrapped the patch. No code changes; the Assisted-by
trailer now names the tool.

 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 51659bb..2ffc46b 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -430,10 +430,6 @@ int gem_set_hwtst(struct net_device *netdev,
 	switch (tstamp_config->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:
@@ -448,6 +444,8 @@ int gem_set_hwtst(struct net_device *netdev,
 		regval = macb_readl(bp, NCR);
 		macb_writel(bp, NCR, (regval | 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.windows.3

Re: [PATCH net RESEND 2/2] net: macb: Use all-frame timestamping for PTPv1 RX filters

[Nicolai Buchwitz]

Hi Kimwoo

On 11.9.2026 10:04, kimwooseok wrote:
> gem_set_hwtst() accepts HWTSTAMP_FILTER_PTP_V1_L4_SYNC and
> HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ without changing rx_bd_control from
> TSTAMP_DISABLED. A successful request thus disables receive 
> timestamping
> while reporting the requested nonempty filter to userspace.
> 
> Handle these two filters through the existing all-frame fallback used
> for HWTSTAMP_FILTER_PTP_V1_L4_EVENT. This enables receive timestamping
> for a superset of the requested packets and returns HWTSTAMP_FILTER_ALL
> to describe the configuration actually selected.
> 
> Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
> Assisted-by: GPT-6 Astra
> Signed-off-by: kimwooseok <5mghybrid@khu.ac.kr>
> ---
> Resending as plain text because the previous webmail submission 
> included
> HTML and quoted and rewrapped the patch. No code changes; the 
> Assisted-by
> trailer now names the tool.
> 
>  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 51659bb..2ffc46b 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -430,10 +430,6 @@ int gem_set_hwtst(struct net_device *netdev,
>  	switch (tstamp_config->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:
> @@ -448,6 +444,8 @@ int gem_set_hwtst(struct net_device *netdev,
>  		regval = macb_readl(bp, NCR);
>  		macb_writel(bp, NCR, (regval | 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@tip-net.de>
Tested-by: Nicolai Buchwitz <nb@tipi-net.de> # Raspberry Pi CM5

Thanks,
Nicolai

Re: [PATCH net RESEND 2/2] net: macb: Use all-frame timestamping for PTPv1 RX filters

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: macb: Use all-frame timestamping for PTPv1 RX filters

The two PTPv1 L4 filters used to be accepted by gem_set_hwtst() while
leaving rx_bd_control at TSTAMP_DISABLED, so receive timestamping ended up
off even though a nonempty filter was reported back. They are now folded
into the existing all-frame arm alongside HWTSTAMP_FILTER_PTP_V1_L4_EVENT,
which reports HWTSTAMP_FILTER_ALL.

No Link: tags in the commit header.

A few observations on the surrounding code in gem_set_hwtst(), all of them
about behaviour that predates this change.

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index 51659bbdfac9a..2ffc46b81264e 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -430,10 +430,6 @@ int gem_set_hwtst(struct net_device *netdev,
>  	switch (tstamp_config->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:

[Severity: Medium]
This isn't a bug introduced by this patch, but while the new arm reports
HWTSTAMP_FILTER_ALL (which is advertised), the sibling PTPv2 arm right below
reports a filter value the driver never advertises:

drivers/net/ethernet/cadence/macb_main.c:gem_get_ts_info() {
	info->rx_filters =
		(1 << HWTSTAMP_FILTER_NONE) |
		(1 << HWTSTAMP_FILTER_ALL);
}

drivers/net/ethernet/cadence/macb_ptp.c:gem_set_hwtst() {
		rx_bd_control =  TSTAMP_ALL_PTP_FRAMES;
		tstamp_config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
}

That value is then stored in bp->tstamp_config and handed back by
gem_get_hwtst() on every later SIOCGHWTSTAMP, so ethtool -T and
SIOCGHWTSTAMP describe different capability sets. Should the PTPv2 arm
either report HWTSTAMP_FILTER_ALL too, or should gem_get_ts_info() advertise
HWTSTAMP_FILTER_PTP_V2_EVENT?

> @@ -448,6 +444,8 @@ int gem_set_hwtst(struct net_device *netdev,
>  		regval = macb_readl(bp, NCR);
>  		macb_writel(bp, NCR, (regval | MACB_BIT(SRTSM)));

[Severity: High]
This is a pre-existing issue and not something this patch introduces, but is
this read-modify-write of NCR safe without bp->lock? Every other NCR
read-modify-write in the driver is serialized, and macb_tx_lpi_set() even
states the rule:

drivers/net/ethernet/cadence/macb_main.c:macb_tx_lpi_set() {
	lockdep_assert_held(&bp->lock);

	ncr = macb_readl(bp, NCR);
	old = ncr;
	...
}

drivers/net/ethernet/cadence/macb_main.c:macb_start_xmit() {
	spin_lock(&bp->lock);
	macb_tx_lpi_wake(bp);
	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
	spin_unlock(&bp->lock);
}

gem_set_hwtst() runs under rtnl from ndo_hwtstamp_set, which does not
exclude the xmit path, the EEE LPI work, macb_mac_link_up(), or the hardirq
RXUBR workaround. Can the unlocked write back lose a bit another context
just set? For example the RXUBR errata path in macb_interrupt() toggles RE:

drivers/net/ethernet/cadence/macb_main.c:macb_interrupt() {
		ctrl = macb_readl(bp, NCR);
		macb_writel(bp, NCR, ctrl & ~MACB_BIT(RE));
		wmb();
		macb_writel(bp, NCR, ctrl | MACB_BIT(RE));
}

If gem_set_hwtst() samples NCR inside that window and then writes back with
RE cleared, does receive stay off until the next reconfiguration? The same
question applies to gem_ptp_set_one_step_sync(), which this function calls
for TX_ON and ONESTEP_SYNC, including for the two filters added here:

drivers/net/ethernet/cadence/macb_ptp.c:gem_ptp_set_one_step_sync() {
	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));
}

Since the RXUBR handler runs in hardirq context, would the irqsave variant
of the lock be needed on the configuration side?

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

Two more items in the same function and driver, outside the hunks:

[Severity: Medium]
This is a pre-existing issue that this patch does not touch, but in
gem_set_hwtst() the HWTSTAMP_TX_OFF arm is a bare break:

	switch (tstamp_config->tx_type) {
	case HWTSTAMP_TX_OFF:
		break;
	case HWTSTAMP_TX_ONESTEP_SYNC:
		tx_bd_control = TSTAMP_ALL_FRAMES;
		break;

and OSSMODE is only touched for the other two values:

	if (tstamp_config->tx_type == HWTSTAMP_TX_ONESTEP_SYNC)
		gem_ptp_set_one_step_sync(bp, 1);
	else if (tstamp_config->tx_type == HWTSTAMP_TX_ON)
		gem_ptp_set_one_step_sync(bp, 0);

After HWTSTAMP_TX_ONESTEP_SYNC followed by HWTSTAMP_TX_OFF, does NCR.OSSMODE
stay set so the MAC keeps rewriting originTimestamp in transmitted Sync
frames while SIOCGHWTSTAMP reports transmit timestamping as off? Should the
TX_OFF case call gem_ptp_set_one_step_sync(bp, 0) as well? The symmetric
case, NCR.SRTSM never being cleared when moving away from the PTPv2 arm,
looks like the same shape of problem.

[Severity: Medium]
Also pre-existing rather than something this patch changes: is the
timestamping configuration lost across suspend/resume? macb_resume()
rewrites NCR from scratch:

drivers/net/ethernet/cadence/macb_main.c:macb_resume() {
	macb_writel(bp, NCR, MACB_BIT(MPE));
	macb_init_hw(bp);
	macb_set_rx_mode(netdev);
	macb_restore_features(bp);
	...
}

which clears SRTSM and OSSMODE, and none of the callees reprogram the
descriptor capture registers. gem_ptp_set_ts_mode() is the only writer of
GEM_TXBDCTRL and GEM_RXBDCTRL:

drivers/net/ethernet/cadence/macb_ptp.c:gem_ptp_set_ts_mode() {
	gem_writel(bp, TXBDCTRL, GEM_BF(TXTSMODE, tx_bd_control));
	gem_writel(bp, RXBDCTRL, GEM_BF(RXTSMODE, rx_bd_control));
}

and it is reachable only from gem_set_hwtst(). Does that leave the hardware
at its reset default after resume while gem_get_hwtst() still reports the
pre-suspend filter, now including HWTSTAMP_FILTER_ALL for the two PTPv1
filters remapped here? Should the resume path replay bp->tstamp_config
through gem_ptp_set_ts_mode()?
-- 
This is an AI-generated review.

Re: [PATCH net RESEND 2/2] net: macb: Use all-frame timestamping for PTPv1 RX filters

[kimwooseok]

Replied on 1/2; the SRTSM and resume points are covered there.