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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- 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 --]