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

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

[Lorenzo Bianconi]

priv->ptp_lock is only initialized in stmmac_ptp_register(), which runs
during __stmmac_open(). However, the lock is also used while the
interface is down and has never been opened: tc_taprio_configure()
invokes the PTP gettime64() callback to compute the EST base time when
offloading a TAPRIO schedule, and stmmac_get_time() takes
priv->ptp_lock. Using an uninitialized rwlock is undefined behaviour.
Move the rwlock_init() to __stmmac_dvr_probe(), together with the other
private locks, so that ptp_lock is always valid regardless of the
interface state.

Fixes: b60189e0392f ("net: stmmac: Integrate EST with TAPRIO scheduler API")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 24656b35350b..5fe7e95fdd34 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);
 
 	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 960249960004..3bfcc9760dce 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);
 	mutex_init(&priv->aux_ts_lock);
 
 	priv->ptp_clock = ptp_clock_register(&priv->ptp_clock_ops,

---
base-commit: 641d03105cc0d2437e32fdeec164f91a4ccef6c4
change-id: 20260904-stmmac-fix-ptp-clock-init-b8eba4328243

Best regards,
-- 
Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

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

[Maxime Chevallier]

Hi,

On 9/4/26 12:32, Lorenzo Bianconi wrote:
> priv->ptp_lock is only initialized in stmmac_ptp_register(), which runs
> during __stmmac_open(). However, the lock is also used while the
> interface is down and has never been opened: tc_taprio_configure()
> invokes the PTP gettime64() callback to compute the EST base time when
> offloading a TAPRIO schedule, and stmmac_get_time() takes
> priv->ptp_lock. Using an uninitialized rwlock is undefined behaviour.
> Move the rwlock_init() to __stmmac_dvr_probe(), together with the other
> private locks, so that ptp_lock is always valid regardless of the
> interface state.
> 
> Fixes: b60189e0392f ("net: stmmac: Integrate EST with TAPRIO scheduler API")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
>  drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c  | 1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 24656b35350b..5fe7e95fdd34 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);
>  
>  	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 960249960004..3bfcc9760dce 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);
>  	mutex_init(&priv->aux_ts_lock);
>  
>  	priv->ptp_clock = ptp_clock_register(&priv->ptp_clock_ops,
> 
> ---
> base-commit: 641d03105cc0d2437e32fdeec164f91a4ccef6c4
> change-id: 20260904-stmmac-fix-ptp-clock-init-b8eba4328243
> 
> Best regards,

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

[netdev-bot+sashiko]

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.

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

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?

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?

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

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?

>  	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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 8277 bytes --]

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.
> 
> > 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/

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

> 
> 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?
> 
> >  
> >  	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/

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 04 Sep 2026 12:32:55 +0200 you wrote:
> priv->ptp_lock is only initialized in stmmac_ptp_register(), which runs
> during __stmmac_open(). However, the lock is also used while the
> interface is down and has never been opened: tc_taprio_configure()
> invokes the PTP gettime64() callback to compute the EST base time when
> offloading a TAPRIO schedule, and stmmac_get_time() takes
> priv->ptp_lock. Using an uninitialized rwlock is undefined behaviour.
> Move the rwlock_init() to __stmmac_dvr_probe(), together with the other
> private locks, so that ptp_lock is always valid regardless of the
> interface state.
> 
> [...]

Here is the summary with links:
  - [net] net: stmmac: initialize ptp_lock at probe time
    https://git.kernel.org/netdev/net/c/0338c68e22ab

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html