Re: [PATCH net] net: stmmac: initialize ptp_lock at probe time

On Sep 08, netdev-bot+sashiko@kernel.org wrote:

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

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

- [High] Incomplete fix: the TAPRIO offload path that this commit
  legitimizes for a never-opened interface still performs MMIO on a…

Pre-existing issues:
- [Medium] EST state (priv->est->enable, priv->est->gcl[]) is accessed
  with inconsistent locking between the taprio offload path and the PTP…
- [Medium] An EST/taprio schedule programmed while the interface is down
  is silently discarded by the MAC/DMA software reset performed at open,…

--

net: stmmac: initialize ptp_lock at probe time

This moves rwlock_init(&priv->ptp_lock) from stmmac_ptp_register() to
__stmmac_dvr_probe() so that the lock is initialized even when the
interface has never been opened, which is the case when tc_taprio_configure()
calls the PTP gettime64() callback to compute the EST base time.

No Link: tags present in the commit header.

A few questions about the paths this touches.

quoted

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 24656b35350b1..5fe7e95fdd345 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

@@ -8025,6 +8025,7 @@ static int __stmmac_dvr_probe(struct device *device,
 	stmmac_napi_add(ndev);
 
 	mutex_init(&priv->lock);
+	rwlock_init(&priv->ptp_lock);

[Severity: High]
The commit message describes the down-interface gettime64() call as a
legitimate use ("the lock is also used while the interface is down and has
never been opened").  Is that call safe once the lock is initialized?

__stmmac_dvr_probe() ends with pm_runtime_put(), and
stmmac_runtime_suspend() in stmmac_platform.c gates the CSR clocks:

	stmmac_bus_clks_config(priv, false);
	  clk_disable_unprepare(priv->plat->stmmac_clk);
	  clk_disable_unprepare(priv->plat->pclk);

So a probed-but-never-opened device is runtime suspended.  The taprio
offload path does not take a runtime-PM reference and does not check
netif_running():

  stmmac_setup_tc()  case TC_SETUP_QDISC_TAPRIO
    tc_setup_taprio()
      tc_taprio_configure()
        priv->ptp_clock_ops.gettime64()  ->  stmmac_get_time()
                                               stmmac_get_systime(priv->ptpaddr)
        stmmac_est_configure()           ->  writes/polls priv->estaddr

Every other register-touching ndo in this driver wraps the access in
pm_runtime_resume_and_get(), for example stmmac_set_mac_address(),
stmmac_vlan_rx_add_vid(), stmmac_vlan_rx_kill_vid() and __stmmac_open().
Can the MMIO in stmmac_get_systime() and est_configure() read/write a
clock-gated MAC/PTP block here, and on SoCs that actually gate pclk, can
that abort or stall?

The goal of this patch is just fixing the undefined behaviour due to the
uninitialized rwlock. Moreover, the reported issue is fixed in the following
patch (not merged yet):
https://lore.kernel.org/netdev/20260827-stmmac-setup-tc-enable-pm-v2-1-a9b8a5948f41@oss.qualcomm.com/ (local)

There is a second aspect to the same call.  The PTP counter is only
programmed from the open path:

  __stmmac_open()
    stmmac_setup_ptp()
      stmmac_init_timestamping()
        stmmac_init_tstamp_counter()   /* TCR enable, SSINC, init systime */

If the interface was never opened, does gettime64() return a usable time
base, and does stmmac_calc_tas_basetime() then program priv->est->btr[]
from a meaningless value?

Would it make sense to gate TC_SETUP_QDISC_TAPRIO on netif_running() (or
on the PTP counter being started), and/or take a runtime-PM reference in
stmmac_setup_tc(), in addition to moving the rwlock_init()?  Reproducer
is "tc qdisc replace dev ethX root taprio ... flags 0x2" on an interface
that has never been brought up; taprio_enable_offload() in
net/sched/sch_taprio.c has no netif_running() gate.

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but since
the change makes the down-interface configuration sequence easier to
reach it seems worth mentioning.  Is an EST schedule programmed while the
interface is down preserved across open?

This issue is not introduced by this patch and it is already fixed in the
following series (not merged yet):
https://lore.kernel.org/netdev/20260902-stmmac-est-reapply-after-open-v3-0-e72a6df5a7ef@oss.qualcomm.com/ (local)

tc_taprio_configure() commits priv->est->enable, btr[]/ctr[] and
max_sdu[] and pushes them to hardware via stmmac_est_configure().  Then
open does:

  __stmmac_open()
    stmmac_hw_setup()
      /* DMA initialization and SW reset */
      stmmac_init_dma_engine(priv);

which asserts DMA SWR and resets the MAC/MTL registers, including
MTL_EST_CONTROL, the GCL RAM and BTR/CTR.  stmmac_est_configure() has no
call site in stmmac_main.c (only tc_taprio_configure() and the disable
label in stmmac_tc.c, plus stmmac_adjust_time() in stmmac_ptp.c), and
taprio_dev_notifier() in net/sched/sch_taprio.c only recomputes
picos_per_byte/max_sdu on NETDEV_UP rather than re-issuing the offload.

Does that leave priv->est->enable true with EST disabled in hardware,
while stmmac_xmit(), stmmac_xdp_xmit_back() and the XSK path keep
enforcing priv->est->max_sdu[] and the taprio dump reports an active
offloaded schedule?

quoted

 
 	stmmac_fpe_init(priv);
 

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
index 960249960004c..3bfcc9760dce7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c

@@ -365,7 +365,6 @@ void stmmac_ptp_register(struct stmmac_priv *priv)
 	if (priv->plat->crosststamp)
 		priv->ptp_clock_ops.getcrosststamp = stmmac_getcrosststamp;
 
-	rwlock_init(&priv->ptp_lock);

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but while
looking at the locking in this file: is the est_lock coverage of
priv->est consistent between the taprio path and the adjtime path?

This issue is not introduced by this patch and it is already fixed in the
following series (not merged yet):
https://lore.kernel.org/netdev/20260902-stmmac-est-reapply-after-open-v3-0-e72a6df5a7ef@oss.qualcomm.com/ (local)

Regards,
Lorenzo

In stmmac_adjust_time() the decision is taken with no lock held:

	/* If EST is enabled, disabled it before adjust ptp time. */
	if (priv->est && priv->est->enable) {

and it is acted upon later, after est_lock has been taken again, where
priv->est->enable is set back to true and stmmac_est_configure() is
called.

Meanwhile tc_taprio_configure() publishes the new size and enable flag
under the lock:

	mutex_lock(&priv->est_lock);
	priv->est->gcl_size = size;
	priv->est->enable = qopt->cmd == TAPRIO_CMD_REPLACE;
	mutex_unlock(&priv->est_lock);

but fills the entries with the lock dropped:

		priv->est->gcl[i] = delta_ns | (gates << wid);

tc_taprio_configure() runs under rtnl from ndo_setup_tc while
stmmac_adjust_time() runs from PTP_CLOCK_ADJTIME on /dev/ptpX without
rtnl, so the two can run concurrently.

Can an adjtime landing in that window make stmmac_est_configure()
program the hardware gate list from a partially written gcl[] with the
new gcl_size already visible?

And can a "tc qdisc del" that clears enable and takes the disable: path
be undone by a concurrent adjtime that re-enables EST from its stale
unlocked observation?

quoted

 	mutex_init(&priv->aux_ts_lock);
 
 	priv->ptp_clock = ptp_clock_register(&priv->ptp_clock_ops,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-stmmac-fix-ptp-clock-init-v1-1-df70eb1eb04d%40oss.qualcomm.com