[PATCH net] net: macb: initialize PTP lock before registering clock

[PATCH net] net: macb: initialize PTP lock before registering clock

[Runyu Xiao]

gem_ptp_init() registers the PTP clock before initializing
bp->tsu_clk_lock. ptp_clock_register() publishes the PTP character
device, so a userspace operation can enter a PTP callback as soon as
registration completes. Those callbacks use tsu_clk_lock and can therefore
observe the lock before it has been initialized.

Initialize the lock before registering the PTP clock so all published
callbacks see valid synchronization state.

Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
Cc: stable@vger.kernel.org
Assisted-by: Codex:GPT-5
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/net/ethernet/cadence/macb_ptp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index e5195d7da..055160c42 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -330,6 +330,8 @@ void gem_ptp_init(struct net_device *netdev)
 
 	bp->ptp_clock_info = gem_ptp_caps_template;
 
+	spin_lock_init(&bp->tsu_clk_lock);
+
 	/* nominal frequency and maximum adjustment in ppb */
 	bp->tsu_rate = bp->ptp_info->get_tsu_rate(bp);
 	bp->ptp_clock_info.max_adj = bp->ptp_info->get_ptp_max_adj();
@@ -345,8 +347,6 @@ void gem_ptp_init(struct net_device *netdev)
 		return;
 	}
 
-	spin_lock_init(&bp->tsu_clk_lock);
-
 	gem_ptp_init_tsu(bp);
 
 	dev_info(&bp->pdev->dev, "%s ptp clock registered.\n",
@@ -467,4 +467,3 @@ int gem_set_hwtst(struct net_device *netdev,
 
 	return 0;
 }
-
-- 
2.34.1

Re: [PATCH net] net: macb: initialize PTP lock before registering clock

[Vadim Fedorenko]

On 30/08/2026 16:35, Runyu Xiao wrote:
> gem_ptp_init() registers the PTP clock before initializing
> bp->tsu_clk_lock. ptp_clock_register() publishes the PTP character
> device, so a userspace operation can enter a PTP callback as soon as
> registration completes. Those callbacks use tsu_clk_lock and can therefore
> observe the lock before it has been initialized.
> 
> Initialize the lock before registering the PTP clock so all published
> callbacks see valid synchronization state.
> 
> Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:GPT-5
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
> ---
>   drivers/net/ethernet/cadence/macb_ptp.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Re: [net] net: macb: initialize PTP lock before registering clock

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 0 · Low: 0

- [High] Incomplete fix: gem_ptp_init()
  (drivers/net/ethernet/cadence/macb_ptp.c:327) still calls…
- [High] bp->tsu_clk_lock is initialized in exactly one place,
  gem_ptp_init() (drivers/net/ethernet/cadence/macb_ptp.c:333 after this…

--

net: macb: initialize PTP lock before registering clock

The patch moves spin_lock_init(&bp->tsu_clk_lock) in gem_ptp_init() from
after ptp_clock_register() to before it, on the grounds that PTP callbacks
become reachable from userspace as soon as the clock is registered. The
lock is otherwise not initialized anywhere else in the driver.

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index e5195d7dac1d..055160c42db3 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -330,6 +330,8 @@ void gem_ptp_init(struct net_device *netdev)
>  
>  	bp->ptp_clock_info = gem_ptp_caps_template;
>  
> +	spin_lock_init(&bp->tsu_clk_lock);
> +

[Severity: High]
This is the only spin_lock_init() for bp->tsu_clk_lock in
drivers/net/ethernet/cadence/, and gem_ptp_init() runs on every interface
open and every resume rather than once at probe. Can that re-initialize the
lock while a softirq on another CPU holds it?

Both call sites start the datapath before calling ptp_init():

drivers/net/ethernet/cadence/macb_main.c:macb_open() {
	...
	err = macb_phylink_connect(bp);
	if (err)
		goto phy_off;

	netif_tx_start_all_queues(netdev);

	if (bp->ptp_info)
		bp->ptp_info->ptp_init(netdev);
	...
}

drivers/net/ethernet/cadence/macb_main.c:macb_resume() {
	...
	phylink_start(bp->phylink);
	rtnl_unlock();

	netif_device_attach(netdev);
	if (bp->ptp_info)
		bp->ptp_info->ptp_init(netdev);
	...
}

The receive and transmit completion paths take the same lock, gated only on
bp->tstamp_config, which is never reset by macb_close() or gem_ptp_remove():

drivers/net/ethernet/cadence/macb.h:gem_ptp_do_rxstamp() {
	if (bp->tstamp_config.rx_filter == TSTAMP_DISABLED)
		return;

	gem_ptp_rxstamp(bp, skb, desc);
}

which reaches gem_hw_timestamp() -> gem_tsu_get_time():

	spin_lock_irqsave(&bp->tsu_clk_lock, flags);

So after userspace enables timestamping via gem_set_hwtst(), an ifdown/ifup
or a suspend/resume re-runs spin_lock_init() on a lock that a NAPI/softirq
context can be holding or contending. Does that break mutual exclusion over
the TN/TSL/TSH read sequence, and reset the lockdep class (or, on PREEMPT_RT,
the rt_mutex owner and waiter state) of a held lock?

Would initializing bp->tsu_clk_lock once at probe time, next to the other
spin_lock_init() calls in macb_init(), be a better fit than moving it a few
statements earlier inside gem_ptp_init()?

>  	/* nominal frequency and maximum adjustment in ppb */
>  	bp->tsu_rate = bp->ptp_info->get_tsu_rate(bp);
>  	bp->ptp_clock_info.max_adj = bp->ptp_info->get_ptp_max_adj();

[ ... ]

> @@ -345,8 +347,6 @@ void gem_ptp_init(struct net_device *netdev)
>  		return;
>  	}
>  
> -	spin_lock_init(&bp->tsu_clk_lock);
> -
>  	gem_ptp_init_tsu(bp);

[Severity: High]
The changelog says "ptp_clock_register() publishes the PTP character device,
so a userspace operation can enter a PTP callback as soon as registration
completes". If that is the premise, does moving only spin_lock_init() close
the window, given that gem_ptp_init_tsu() still programs the hardware after
ptp_clock_register() has returned?

In drivers/ptp/ptp_clock.c, ptp_clock_register() calls
posix_clock_register(&ptp->clock, &ptp->dev), which adds the cdev, so
/dev/ptpN exists before the driver has touched the TSU registers. In that
interval gettimex64/settime64/adjfine/adjtime are all callable.

Two effects seem possible in that window. First, the increment registers can
still be zero, because the close/suspend path halts the counter:

drivers/net/ethernet/cadence/macb_ptp.c:gem_ptp_clear_timer() {
	bp->tsu_incr.sub_ns = 0;
	bp->tsu_incr.ns = 0;

	gem_writel(bp, TISUBN, GEM_BF(SUBNSINCR, 0));
	gem_writel(bp, TI, GEM_BF(NSINCR, 0));
	gem_writel(bp, TA, 0);
}

so on a re-open, does gem_tsu_get_time() hand a frozen counter value back to
userspace as a valid timestamp?

Second, anything userspace sets in that window is overwritten a moment later:

drivers/net/ethernet/cadence/macb_ptp.c:gem_ptp_init_tsu() {
	ts = ns_to_timespec64(ktime_to_ns(ktime_get_real()));
	gem_tsu_set_time(&bp->ptp_clock_info, &ts);
	gem_tsu_incr_set(bp, &bp->tsu_incr);
	gem_writel(bp, TA, 0);
}

Does a settime64/adjfine/adjtime accepted between registration and this call
get silently discarded?

Also, the trailing gem_writel(bp, TA, 0) here is issued without
bp->tsu_clk_lock, while gem_tsu_set_time() notes its own sequence is not
atomic:

	/* TSH doesn't latch the time and no atomicity! */
	gem_writel(bp, TN, 0); /* clear to avoid overflow */
	gem_writel(bp, TSH, sech);

Can that unlocked TA write land inside a concurrent gem_tsu_set_time()
critical section?

Since gem_ptp_init_timer() and gem_ptp_init_tsu() do not need a live
bp->ptp_clock (gem_tsu_set_time() and gem_tsu_incr_set() only reach bp
through bp->ptp_clock_info via container_of), could ptp_clock_register() be
made the last statement of gem_ptp_init() instead? That would also make
hoisting spin_lock_init() unnecessary.

>  
>  	dev_info(&bp->pdev->dev, "%s ptp clock registered.\n",

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260830153520.2710520-1-runyu.xiao%40seu.edu.cn

[PATCH net v2] net: macb: initialize PTP state before registering clock

[Runyu Xiao]

gem_ptp_init() registers the PTP clock before initializing
bp->tsu_clk_lock and the TSU hardware. Since ptp_clock_register()
publishes the PTP character device, userspace may invoke PTP callbacks
before the lock and hardware are ready.

In addition, gem_ptp_init() is called from both the interface open and
resume paths. Reinitializing tsu_clk_lock there can reset the lock while
timestamp processing is using it.

Initialize tsu_clk_lock once during probe and initialize the TSU before
registering the PTP clock.

Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/all/20260830153520.2710520-1-runyu.xiao@seu.edu.cn/
Assisted-by: Codex:GPT-5
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
v2:
- Initialize tsu_clk_lock once during probe and initialize the TSU before
  registering the PTP clock.
---
 drivers/net/ethernet/cadence/macb_main.c | 1 +
 drivers/net/ethernet/cadence/macb_ptp.c  | 6 +-----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 76ee4f506..01b42b21a 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5876,6 +5876,7 @@ static int macb_probe(struct platform_device *pdev)
 	}
 	spin_lock_init(&bp->lock);
 	spin_lock_init(&bp->stats_lock);
+	spin_lock_init(&bp->tsu_clk_lock);
 
 	/* setup capabilities */
 	macb_configure_caps(bp, macb_config);
diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index e5195d7da..e9c9c2692 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -334,6 +334,7 @@ void gem_ptp_init(struct net_device *netdev)
 	bp->tsu_rate = bp->ptp_info->get_tsu_rate(bp);
 	bp->ptp_clock_info.max_adj = bp->ptp_info->get_ptp_max_adj();
 	gem_ptp_init_timer(bp);
+	gem_ptp_init_tsu(bp);
 	bp->ptp_clock = ptp_clock_register(&bp->ptp_clock_info, &netdev->dev);
 	if (IS_ERR(bp->ptp_clock)) {
 		pr_err("ptp clock register failed: %ld\n",
@@ -345,10 +346,6 @@ void gem_ptp_init(struct net_device *netdev)
 		return;
 	}
 
-	spin_lock_init(&bp->tsu_clk_lock);
-
-	gem_ptp_init_tsu(bp);
-
 	dev_info(&bp->pdev->dev, "%s ptp clock registered.\n",
 		 GEM_PTP_TIMER_NAME);
 }
@@ -467,4 +464,3 @@ int gem_set_hwtst(struct net_device *netdev,
 
 	return 0;
 }
-
-- 
2.34.1

Re: [PATCH net v2] net: macb: initialize PTP state before registering clock

[Théo Lebrun]

Hello Runyu,

Your To/Cc list is incorrect. Have you checked out a recent kernel and
used b4 or get_maintainer.pl?

That is documented in Documentation/process/submitting-patches.rst

And your clanker even has its own version nowadays that mentions it as
well: Documentation/process/coding-assistants.rst

On Fri Sep 4, 2026 at 5:04 AM CEST, Runyu Xiao wrote:
> gem_ptp_init() registers the PTP clock before initializing
> bp->tsu_clk_lock and the TSU hardware. Since ptp_clock_register()
> publishes the PTP character device, userspace may invoke PTP callbacks
> before the lock and hardware are ready.
>
> In addition, gem_ptp_init() is called from both the interface open and
> resume paths. Reinitializing tsu_clk_lock there can reset the lock while
> timestamp processing is using it.
>
> Initialize tsu_clk_lock once during probe and initialize the TSU before
> registering the PTP clock.
>
> Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/all/20260830153520.2710520-1-runyu.xiao@seu.edu.cn/
> Assisted-by: Codex:GPT-5
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>

This is technically correct, though I'm pretty sure the window is tiny.
I always prefer when such commits indicate if the issue was faced in
real life *or not*. So here the commit would be better off mentioning
that this is a theoretical bug never encountered in the wild.

Seeing your patches sent my guess is not.
https://lore.kernel.org/netdev/?q=f%3Arunyu.xiao%40seu.edu.cn

Also you forgot the Reviewed-by trailer sent to you on V1.

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

Thanks,

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