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

[netdev-bot+sashiko@kernel.org]

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