* Re: [PATCH net] net: stmmac: serialize PTP timestamping configuration with priv->lock
2026-10-02 10:37 [PATCH net] net: stmmac: serialize PTP timestamping configuration with priv->lock Lorenzo Bianconi
@ 2026-10-02 10:43 ` netdev-bot+sinfo
2026-10-02 11:01 ` Lorenzo Bianconi
2026-10-02 13:21 ` Lorenzo Bianconi
2026-10-06 11:11 ` netdev-bot+sashiko
2 siblings, 1 reply; 5+ messages in thread
From: netdev-bot+sinfo @ 2026-10-02 10:43 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Maxime Chevallier, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue,
netdev, linux-stm32, linux-arm-kernel
Hi!
This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:
- How the issue was discovered, e.g. hit in production, hit during
development, syzbot report, manual code inspection, LLM or static
analysis tool scan.
- Whether the issue was actually triggered, or is only theoretical
(e.g. found by code inspection). If it was triggered please include
the symptoms, like the stack trace or error messages.
- What hardware the change was tested on. For driver fixes please
mention the device (and if relevant firmware version) used for
testing, or say that the change was not tested on real hardware.
Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.
The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net: stmmac: serialize PTP timestamping configuration with priv->lock
2026-10-02 10:37 [PATCH net] net: stmmac: serialize PTP timestamping configuration with priv->lock Lorenzo Bianconi
2026-10-02 10:43 ` netdev-bot+sinfo
@ 2026-10-02 13:21 ` Lorenzo Bianconi
2026-10-06 11:11 ` netdev-bot+sashiko
2 siblings, 0 replies; 5+ messages in thread
From: Lorenzo Bianconi @ 2026-10-02 13:21 UTC (permalink / raw)
To: Maxime Chevallier, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue
Cc: netdev, linux-stm32, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 4402 bytes --]
> stmmac_dl_ts_coarse_set() runs under devl_lock only while
> stmmac_hwtstamp_set() runs under RTNL. They read/write
> systime_flags/tsfupdt_coarse and run stmmac_config_hw_tstamping().
>
> stmmac_dl_ts_coarse_set() snapshots priv->systime_flags, programs
> PTP_TCR, PTP_SSIR and PTP_TAR, and only then publishes tsfupdt_coarse
> and systime_flags. A concurrent stmmac_hwtstamp_set() can read the
> stale tsfupdt_coarse, build fine-mode flags, program PTP_TCR in fine
> mode and set hwts_rx_en.
>
> Take priv->lock in both stmmac_dl_ts_coarse_set() and
> stmmac_hwtstamp_set() so the timestamping configuration is updated
> under a single lock.
>
> Fixes: 4347287b8627 ("net: stmmac: propagate PTP addend and system time programming errors")
I will repost with proper Fixes tag in v2.
Regards,
Lorenzo
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 43 ++++++++++++++---------
> 1 file changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9741f97fa37a..a6793054a179 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -685,14 +685,7 @@ static int stmmac_hwtstamp_set(struct net_device *dev,
> u32 snap_type_sel = 0;
> u32 ts_master_en = 0;
> u32 ts_event_en = 0;
> -
> - if (!(priv->dma_cap.time_stamp || priv->adv_ts)) {
> - NL_SET_ERR_MSG_MOD(extack, "No support for HW time stamping");
> - priv->hwts_tx_en = 0;
> - priv->hwts_rx_en = 0;
> -
> - return -EOPNOTSUPP;
> - }
> + int ret = 0;
>
> if (!netif_running(dev)) {
> NL_SET_ERR_MSG_MOD(extack,
> @@ -700,13 +693,23 @@ static int stmmac_hwtstamp_set(struct net_device *dev,
> return -ENODEV;
> }
>
> - netdev_dbg(priv->dev, "%s config flags:0x%x, tx_type:0x%x, rx_filter:0x%x\n",
> - __func__, config->flags, config->tx_type, config->rx_filter);
> -
> if (config->tx_type != HWTSTAMP_TX_OFF &&
> config->tx_type != HWTSTAMP_TX_ON)
> return -ERANGE;
>
> + netdev_dbg(priv->dev, "%s config flags:0x%x, tx_type:0x%x, rx_filter:0x%x\n",
> + __func__, config->flags, config->tx_type, config->rx_filter);
> +
> + mutex_lock(&priv->lock);
> +
> + if (!(priv->dma_cap.time_stamp || priv->adv_ts)) {
> + NL_SET_ERR_MSG_MOD(extack, "No support for HW time stamping");
> + priv->hwts_tx_en = 0;
> + priv->hwts_rx_en = 0;
> + ret = -EOPNOTSUPP;
> + goto unlock;
> + }
> +
> if (priv->adv_ts) {
> switch (config->rx_filter) {
> case HWTSTAMP_FILTER_NONE:
> @@ -829,7 +832,8 @@ static int stmmac_hwtstamp_set(struct net_device *dev,
> break;
>
> default:
> - return -ERANGE;
> + ret = -ERANGE;
> + goto unlock;
> }
> } else {
> switch (config->rx_filter) {
> @@ -859,8 +863,10 @@ static int stmmac_hwtstamp_set(struct net_device *dev,
> stmmac_config_hw_tstamping(priv, priv->ptpaddr, priv->systime_flags);
>
> priv->tstamp_config = *config;
> +unlock:
> + mutex_unlock(&priv->lock);
>
> - return 0;
> + return ret;
> }
>
> /**
> @@ -7753,9 +7759,12 @@ static int stmmac_dl_ts_coarse_set(struct devlink *dl, u32 id,
> {
> struct stmmac_devlink_priv *dl_priv = devlink_priv(dl);
> struct stmmac_priv *priv = dl_priv->stmmac_priv;
> - u32 systime_flags = priv->systime_flags;
> + u32 systime_flags;
> int ret;
>
> + mutex_lock(&priv->lock);
> +
> + systime_flags = priv->systime_flags;
> if (ctx->val.vbool)
> systime_flags &= ~PTP_TCR_TSCFUPDT;
> else
> @@ -7768,13 +7777,15 @@ static int stmmac_dl_ts_coarse_set(struct devlink *dl, u32 id,
> if (ret) {
> NL_SET_ERR_MSG_MOD(extack,
> "failed to reconfigure PTP adjustment");
> - return ret;
> + goto unlock;
> }
>
> priv->tsfupdt_coarse = ctx->val.vbool;
> priv->systime_flags = systime_flags;
> +unlock:
> + mutex_unlock(&priv->lock);
>
> - return 0;
> + return ret;
> }
>
> static int stmmac_dl_ts_coarse_get(struct devlink *dl, u32 id,
>
> ---
> base-commit: 232d49dd4b40a666283de9e722899f088ed581b2
> change-id: 20261002-stmmac-hwtstamp-missing-lock-b6a7979c643c
>
> Best regards,
> --
> Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net: stmmac: serialize PTP timestamping configuration with priv->lock
2026-10-02 10:37 [PATCH net] net: stmmac: serialize PTP timestamping configuration with priv->lock Lorenzo Bianconi
2026-10-02 10:43 ` netdev-bot+sinfo
2026-10-02 13:21 ` Lorenzo Bianconi
@ 2026-10-06 11:11 ` netdev-bot+sashiko
2 siblings, 0 replies; 5+ messages in thread
From: netdev-bot+sashiko @ 2026-10-06 11:11 UTC (permalink / raw)
To: lorenzo.bianconi
Cc: maxime.chevallier, andrew+netdev, davem, edumazet, kuba, pabeni,
mcoquelin.stm32, alexandre.torgue, netdev, linux-stm32,
linux-arm-kernel
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 2 (1) · Low: 1 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] The fix is incomplete.
- [Medium] The tag `Fixes: 4347287b8627 ("net: stmmac: propagate PTP
addend and system time programming errors")` names a SHA that does not…
- [Medium] PTP_TCR and PTP_TAR are still programmed concurrently under
two different locks.
- [Low] In stmmac_hwtstamp_set()
(drivers/net/ethernet/stmicro/stmmac/stmmac_main.c), the patch moves…
Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
stmmac_dl_ts_coarse_set() reprograms PTP_TCR, PTP_SSIR and PTP_TAR and…
- [Low] This problem wasn't introduced by this patch, but in
stmmac_update_subsecond_increment() the rollback is incomplete when…
--
net: stmmac: serialize PTP timestamping configuration with priv->lock
This patch takes priv->lock in stmmac_hwtstamp_set() and in
stmmac_dl_ts_coarse_set(). The goal is to serialize updates to
systime_flags/tsfupdt_coarse and the PTP_TCR programming between the
RTNL-held hwtstamp path and the devl_lock-held devlink parameter path. The
hardware capability check in stmmac_hwtstamp_set() also moves under the
lock.
> Fixes: 4347287b8627 ("net: stmmac: propagate PTP addend and system time programming errors")
[Severity: Medium]
Is this the right Fixes target? 4347287b8627 does not seem to resolve in
the tree. The quoted subject matches 232d49dd4b40, which is the parent of
this patch.
The race also looks older than that commit. Before 232d49dd4b40,
stmmac_dl_ts_coarse_set() already did three things under devl_lock only:
- wrote priv->tsfupdt_coarse
- did a read-modify-write of priv->systime_flags
- called stmmac_update_subsecond_increment()
Meanwhile stmmac_hwtstamp_set() read tsfupdt_coarse and rewrote
systime_flags and PTP_TCR under RTNL.
Should this be the following instead?
Fixes: 6920fa0c764d ("net: stmmac: Add a devlink attribute to control timestamping mode")
With that tag, stable trees that have the devlink parameter but not
232d49dd4b40 would also pick up the fix.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9741f97fa37a2..a6793054a1796 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -685,14 +685,7 @@ static int stmmac_hwtstamp_set(struct net_device *dev,
> u32 snap_type_sel = 0;
> u32 ts_master_en = 0;
> u32 ts_event_en = 0;
> -
> - if (!(priv->dma_cap.time_stamp || priv->adv_ts)) {
> - NL_SET_ERR_MSG_MOD(extack, "No support for HW time stamping");
> - priv->hwts_tx_en = 0;
> - priv->hwts_rx_en = 0;
> -
> - return -EOPNOTSUPP;
> - }
> + int ret = 0;
>
> if (!netif_running(dev)) {
> NL_SET_ERR_MSG_MOD(extack,
> @@ -700,13 +693,23 @@ static int stmmac_hwtstamp_set(struct net_device *dev,
> return -ENODEV;
> }
>
> - netdev_dbg(priv->dev, "%s config flags:0x%x, tx_type:0x%x, rx_filter:0x%x\n",
> - __func__, config->flags, config->tx_type, config->rx_filter);
> -
> if (config->tx_type != HWTSTAMP_TX_OFF &&
> config->tx_type != HWTSTAMP_TX_ON)
> return -ERANGE;
>
> + netdev_dbg(priv->dev, "%s config flags:0x%x, tx_type:0x%x, rx_filter:0x%x\n",
> + __func__, config->flags, config->tx_type, config->rx_filter);
> +
> + mutex_lock(&priv->lock);
> +
> + if (!(priv->dma_cap.time_stamp || priv->adv_ts)) {
> + NL_SET_ERR_MSG_MOD(extack, "No support for HW time stamping");
> + priv->hwts_tx_en = 0;
> + priv->hwts_rx_en = 0;
> + ret = -EOPNOTSUPP;
> + goto unlock;
> + }
> +
[Severity: Low]
Was moving the capability check intended? It used to be the first check
in stmmac_hwtstamp_set(). Now it comes after the netif_running() and
tx_type checks.
The commit message does not mention the move. Userspace sees the
difference through SIOCSHWTSTAMP and ethtool tsconfig-set, via
dev_set_hwtstamp_phylib()->ndo_hwtstamp_set():
- On a MAC without timestamping support, a down interface now gets
-ENODEV with "Cannot change timestamping configuration while down".
It used to get -EOPNOTSUPP with "No support for HW time stamping".
- On such a MAC, an unsupported tx_type such as HWTSTAMP_TX_ONESTEP_SYNC
now gets -ERANGE instead of -EOPNOTSUPP.
- On those early returns, hwts_tx_en and hwts_rx_en are no longer
cleared.
The serialization does not seem to need the move:
- dma_cap.time_stamp is fixed at probe.
- adv_ts is written by stmmac_init_timestamping() on the open path
without priv->lock, so reading it under the lock does not protect it.
Could the check stay at the top of the function?
> if (priv->adv_ts) {
> switch (config->rx_filter) {
> case HWTSTAMP_FILTER_NONE:
[ ... ]
> @@ -859,8 +863,10 @@ static int stmmac_hwtstamp_set(struct net_device *dev,
> stmmac_config_hw_tstamping(priv, priv->ptpaddr, priv->systime_flags);
>
> priv->tstamp_config = *config;
> +unlock:
> + mutex_unlock(&priv->lock);
>
> - return 0;
> + return ret;
> }
[Severity: Medium]
Can the PTP_TCR read-modify-write here still interleave with the PHC
callbacks in stmmac_ptp.c? Those callbacks take priv->ptp_lock, not
priv->lock, so neither lock excludes the other.
For example, stmmac_adjust_freq() does:
stmmac_adjust_freq()
addend = adjust_by_scaled_ppm(priv->default_addend, scaled_ppm);
write_lock_irqsave(&priv->ptp_lock, flags);
stmmac_config_addend()
writel(addend, ioaddr + PTP_TAR);
value = readl(ioaddr + PTP_TCR);
value |= PTP_TCR_TSADDREG;
writel(value, ioaddr + PTP_TCR);
stmmac_adjust_time()->adjust_systime() does the same with PTP_TCR_TSUPDT.
stmmac_set_time()->init_systime() does the same with PTP_TCR_TSINIT.
Suppose one of these reads PTP_TCR before config_hw_tstamping() writes it
here, or in stmmac_dl_ts_coarse_set(), and writes it back afterwards. The
stale TSCFUPDT and filter bits would then be restored. PTP_TCR would no
longer match priv->systime_flags and priv->tsfupdt_coarse.
There are two related cases:
- stmmac_adjust_freq() reads priv->default_addend without any lock.
During a coarse to fine devlink switch, it could latch a TAR value
computed from the old addend.
- For PEROUT, stmmac_enable() passes priv->sub_second_inc and
priv->systime_flags to flex_pps_config under ptp_lock only. It could
see a mismatched pair.
These PHC paths behaved the same way before this patch. Still, the commit
message says the configuration is now "updated under a single lock".
Should the PTP_TCR/PTP_TAR writers in stmmac_ptp.c be covered by the same
lock?
> @@ -7753,9 +7759,12 @@ static int stmmac_dl_ts_coarse_set(struct devlink *dl, u32 id,
> {
> struct stmmac_devlink_priv *dl_priv = devlink_priv(dl);
> struct stmmac_priv *priv = dl_priv->stmmac_priv;
> - u32 systime_flags = priv->systime_flags;
> + u32 systime_flags;
> int ret;
>
> + mutex_lock(&priv->lock);
> +
> + systime_flags = priv->systime_flags;
[Severity: High]
Does this close the race the commit message describes? The open path
writes the same state while holding only RTNL:
__stmmac_open()
stmmac_setup_ptp()
stmmac_init_timestamping()
priv->systime_flags = STMMAC_HWTS_ACTIVE | PTP_TCR_TSCFUPDT;
priv->tsfupdt_coarse = false;
...
stmmac_init_tstamp_counter()
stmmac_update_subsecond_increment(priv, priv->systime_flags);
Only stmmac_resume() calls stmmac_init_timestamping() with priv->lock
held. The devlink param set path does not take RTNL. So devlink (devl_lock
plus priv->lock) and open (RTNL only) share no lock.
Consider this sequence:
CPU0 (devlink, vbool=true)
stmmac_dl_ts_coarse_set()
systime_flags = priv->systime_flags;
programs coarse PTP_TCR/PTP_SSIR/PTP_TAR
still inside config_addend()
CPU1 (ip link set up, or an MTU change via stmmac_change_mtu())
__stmmac_open()->stmmac_setup_ptp()->stmmac_init_timestamping()
priv->tsfupdt_coarse = false;
programs fine PTP_TCR/PTP_SSIR/PTP_TAR
CPU0
stores coarse sub_second_inc and default_addend
priv->tsfupdt_coarse = true;
priv->systime_flags = systime_flags; /* coarse */
The software state now says coarse mode while the hardware runs in fine
mode. The next stmmac_hwtstamp_set() would program PTP_TCR without
TSCFUPDT on top of the fine-mode SSIR. Would the PHC then advance at twice
the real rate?
This function also does not check netif_running(). It can program PTP
registers while stmmac_hw_setup() is resetting the DMA/MAC. It can also
run while __stmmac_release()->stmmac_release_ptp() is disabling
clk_ptp_ref.
stmmac_init_timestamping() still reads and writes systime_flags and
tsfupdt_coarse and calls stmmac_config_hw_tstamping() outside priv->lock.
Is the "single lock" statement in the commit message accurate in that
case?
> if (ctx->val.vbool)
> systime_flags &= ~PTP_TCR_TSCFUPDT;
> else
[Severity: Medium]
This problem was not introduced by this patch. After this point,
stmmac_dl_ts_coarse_set() calls stmmac_update_subsecond_increment(). That
reprograms PTP_TCR, PTP_SSIR and PTP_TAR and busy-polls TSADDREG. There is
no netif_running() check, no runtime PM reference and no check that
clk_ptp_ref is enabled.
The devlink parameter is registered in __stmmac_dvr_probe(), which then
does:
/* Let pm_runtime_put() disable the clocks.
* If CONFIG_PM is not enabled, the clocks will stay powered.
*/
pm_runtime_put(device);
stmmac_release() calls stmmac_release_ptp():
stmmac_ptp_unregister(priv);
clk_disable_unprepare(priv->plat->clk_ptp_ref);
and then pm_runtime_put(). Runtime suspend then gates stmmac_clk and pclk
through stmmac_pltfr_runtime_suspend()->stmmac_bus_clks_config(priv,
false).
Running "devlink dev param set ... phc_coarse_adj" on a down interface
would then do MMIO on a MAC whose bus clock may be gated. Other
register-access paths in the driver avoid this:
- MDIO, set_mac_address and VLAN add/kill call
pm_runtime_resume_and_get() first.
- stmmac_hwtstamp_set() returns -ENODEV when the interface is down.
On some SoCs, MMIO to a clock-gated peripheral can hang the interconnect
or raise an external abort. At minimum the write is lost, or TSADDREG
never clears, which costs a 100 ms atomic busy-poll and returns an error.
The setting would not last anyway. The next open resets it, since
stmmac_init_timestamping() sets tsfupdt_coarse = false.
> @@ -7768,13 +7777,15 @@ static int stmmac_dl_ts_coarse_set(struct devlink *dl, u32 id,
> if (ret) {
> NL_SET_ERR_MSG_MOD(extack,
> "failed to reconfigure PTP adjustment");
> - return ret;
> + goto unlock;
> }
[Severity: Low]
This is a pre-existing issue. Is the rollback in
stmmac_update_subsecond_increment() complete when stmmac_config_addend()
fails?
By the time the TSADDREG poll times out, config_addend() has already
written the new mode's addend to PTP_TAR and issued TSADDREG. The error
label only restores PTP_TCR and PTP_SSIR:
error:
/* Restore previous configuration */
stmmac_config_hw_tstamping(priv, priv->ptpaddr, priv->systime_flags);
stmmac_config_sub_second_increment(priv, priv->ptpaddr,
priv->plat->clk_ptp_rate, xmac,
NULL);
TSADDREG is not in STMMAC_HWTS_CFG_MASK, so the TCR read-modify-write
leaves it set. Suppose the pending latch completes later. The accumulator
would use the rejected mode's addend while TCR/SSIR are back in the old
mode. priv->default_addend would still hold the old value.
The sibling helper stmmac_restore_subsecond_increment() does reprogram the
addend on rollback.
Here, stmmac_dl_ts_coarse_set() reports the failure through extack and
returns an error, but the hardware is left partly changed. This error path
came from the parent commit 232d49dd4b40.
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-stmmac-hwtstamp-missing-lock-v1-1-189ce151a624%40oss.qualcomm.com
^ permalink raw reply [flat|nested] 5+ messages in thread