* Re: [PATCH net v2] net: stmmac: serialize PTP timestamping configuration with priv->lock
2026-10-05 8:11 [PATCH net v2] net: stmmac: serialize PTP timestamping configuration with priv->lock Lorenzo Bianconi
@ 2026-10-05 8:13 ` netdev-bot+sinfo
2026-10-05 8:21 ` Lorenzo Bianconi
2026-10-05 9:08 ` Maxime Chevallier
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: netdev-bot+sinfo @ 2026-10-05 8:13 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Maxime Chevallier, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue,
Russell King (Oracle), Kory Maincent, 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] 6+ messages in thread* Re: [PATCH net v2] net: stmmac: serialize PTP timestamping configuration with priv->lock
2026-10-05 8:13 ` netdev-bot+sinfo
@ 2026-10-05 8:21 ` Lorenzo Bianconi
0 siblings, 0 replies; 6+ messages in thread
From: Lorenzo Bianconi @ 2026-10-05 8:21 UTC (permalink / raw)
To: netdev-bot+sinfo
Cc: Maxime Chevallier, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue,
Russell King (Oracle), Kory Maincent, netdev, linux-stm32,
linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]
> 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.
The issue was spotted by sashiko.
>
> - 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.
This is a theoretical issue found during code inspection.
>
> - 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.
I tested this patch on a Qualcomm Rb3-gen2 board.
Regards,
Lorenzo
>
> 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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: stmmac: serialize PTP timestamping configuration with priv->lock
2026-10-05 8:11 [PATCH net v2] net: stmmac: serialize PTP timestamping configuration with priv->lock Lorenzo Bianconi
2026-10-05 8:13 ` netdev-bot+sinfo
@ 2026-10-05 9:08 ` Maxime Chevallier
2026-10-06 12:45 ` Lorenzo Bianconi
2026-10-07 20:13 ` netdev-bot+sashiko
3 siblings, 0 replies; 6+ messages in thread
From: Maxime Chevallier @ 2026-10-05 9:08 UTC (permalink / raw)
To: Lorenzo Bianconi, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue,
Russell King (Oracle), Kory Maincent
Cc: netdev, linux-stm32, linux-arm-kernel
Hi Lorenzo,
On 10/5/26 10:11, Lorenzo Bianconi wrote:
> 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: 6920fa0c764d ("net: stmmac: Add a devlink attribute to control timestamping mode")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
Thanks for fixing this,
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Maxime
> ---
> Changes in v2:
> - Use proper Fixes tag.
> - Link to v1: https://lore.kernel.org/r/20261002-stmmac-hwtstamp-missing-lock-v1-1-189ce151a624@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: aaaaf87ea99b8766c9a8aa0e71aa42e6bc8a5320
> change-id: 20261002-stmmac-hwtstamp-missing-lock-b6a7979c643c
>
> Best regards,
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net v2] net: stmmac: serialize PTP timestamping configuration with priv->lock
2026-10-05 8:11 [PATCH net v2] net: stmmac: serialize PTP timestamping configuration with priv->lock Lorenzo Bianconi
2026-10-05 8:13 ` netdev-bot+sinfo
2026-10-05 9:08 ` Maxime Chevallier
@ 2026-10-06 12:45 ` Lorenzo Bianconi
2026-10-07 20:13 ` netdev-bot+sashiko
3 siblings, 0 replies; 6+ messages in thread
From: Lorenzo Bianconi @ 2026-10-06 12:45 UTC (permalink / raw)
To: Maxime Chevallier, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue,
Russell King (Oracle), Kory Maincent
Cc: netdev, linux-stm32, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 4597 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: 6920fa0c764d ("net: stmmac: Add a devlink attribute to control timestamping mode")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
I need to rebase this patch in order to fix new conflicts with net tree.
Regards,
Lorenzo
> ---
> Changes in v2:
> - Use proper Fixes tag.
> - Link to v1: https://lore.kernel.org/r/20261002-stmmac-hwtstamp-missing-lock-v1-1-189ce151a624@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: aaaaf87ea99b8766c9a8aa0e71aa42e6bc8a5320
> 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] 6+ messages in thread* Re: [PATCH net v2] net: stmmac: serialize PTP timestamping configuration with priv->lock
2026-10-05 8:11 [PATCH net v2] net: stmmac: serialize PTP timestamping configuration with priv->lock Lorenzo Bianconi
` (2 preceding siblings ...)
2026-10-06 12:45 ` Lorenzo Bianconi
@ 2026-10-07 20:13 ` netdev-bot+sashiko
3 siblings, 0 replies; 6+ messages in thread
From: netdev-bot+sashiko @ 2026-10-07 20:13 UTC (permalink / raw)
To: lorenzo.bianconi
Cc: maxime.chevallier, andrew+netdev, davem, edumazet, kuba, pabeni,
mcoquelin.stm32, alexandre.torgue, rmk+kernel, kory.maincent,
netdev, linux-stm32, linux-arm-kernel
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 1 (1) · Medium: 0 (2) · Low: 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: the devlink path can still race with an
RTNL-only path on the timestamping state.
- [Low] In stmmac_hwtstamp_set(), the patch moves the
'!(priv->dma_cap.time_stamp || priv->adv_ts)' -EOPNOTSUPP check after…
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
stmmac_dl_ts_coarse_set() does PTP register MMIO with no…
- [Medium] This problem wasn't introduced by this patch, but the PHC
clock callbacks in stmmac_ptp.c still touch the same PTP registers and…
- [Medium] This problem wasn't introduced by this patch, but the kernel-
doc of stmmac_init_tstamp_counter() says it 'Will be rerun after…
--
net: stmmac: serialize PTP timestamping configuration with priv->lock
This patch takes priv->lock in both stmmac_dl_ts_coarse_set() and
stmmac_hwtstamp_set(). The goal is to stop the devlink phc_coarse_adj
handler and the SIOCSHWTSTAMP handler from racing on systime_flags,
tsfupdt_coarse and the PTP_TCR programming. It also moves the hardware
capability check in stmmac_hwtstamp_set() under the new lock.
> 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 the capability check meant to move after the netif_running() and
tx_type checks? The commit message does not mention this change.
Before this patch, hardware without timestamping support always got
-EOPNOTSUPP with "No support for HW time stamping".
Now, if the interface is down on such hardware, it gets -ENODEV with
"Cannot change timestamping configuration while down". A tx_type other
than OFF/ON gets -ERANGE. HWTSTAMP_TX_ONESTEP_SYNC is one example, and
net_hwtstamp_validate() accepts it.
dev_set_hwtstamp() only checks netif_device_present() before calling into
the driver, so userspace will see the different errno.
The new locking does not seem to need this reordering. Could the
capability check stay first?
[ ... ]
> @@ -859,8 +863,10 @@ static int stmmac_hwtstamp_set(struct net_device *dev,
> stmmac_config_hw_tstamping(priv, priv->ptpaddr, priv->systime_flags);
[Severity: Medium]
This is a pre-existing issue, but the PHC callbacks in stmmac_ptp.c touch
the same PTP_TCR register and fields under a different lock,
priv->ptp_lock. Taking priv->lock here does not serialize against them.
Three callbacks do a readl/modify/writel of PTP_TCR to set TSADDREG,
TSUPDT or TSINIT:
stmmac_adjust_freq()->config_addend()
stmmac_adjust_time()->adjust_systime()
stmmac_set_time()->init_systime()
For example, config_addend() does:
value = readl(ioaddr + PTP_TCR);
value |= PTP_TCR_TSADDREG;
writel(value, ioaddr + PTP_TCR);
config_hw_tstamping() also does a read-modify-write of PTP_TCR, under
priv->lock only. It is called here and from
stmmac_dl_ts_coarse_set()->stmmac_update_subsecond_increment():
u32 regval = readl(ioaddr + PTP_TCR);
regval &= ~STMMAC_HWTS_CFG_MASK;
regval |= data;
writel(regval, ioaddr + PTP_TCR);
Suppose a PHC op reads PTP_TCR before this write and writes it back after.
Can that bring back old mode bits such as TSCFUPDT?
If so, config_sub_second_increment() would pick SSIR from the reverted
TCR. The hardware would then no longer match priv->systime_flags and
tsfupdt_coarse.
There are two related unlocked reads:
- stmmac_adjust_freq() reads priv->default_addend with no lock.
- stmmac_enable(PTP_CLK_REQ_PEROUT) reads priv->sub_second_inc and
priv->systime_flags under ptp_lock, while the writers hold priv->lock.
Can stmmac_enable() see a sub_second_inc and systime_flags pair that do
not match?
>
> priv->tstamp_config = *config;
[Severity: Medium]
This is a pre-existing issue, but the kernel-doc of
stmmac_init_tstamp_counter() says:
* Will be rerun after resuming from suspend, case in which the timestamping
* flags updated by stmmac_hwtstamp_set() also need to be restored.
Is that still accurate? stmmac_resume() calls stmmac_init_timestamping().
That function resets the state before it calls
stmmac_init_tstamp_counter():
memset(&priv->tstamp_config, 0, sizeof(priv->tstamp_config));
priv->systime_flags = STMMAC_HWTS_ACTIVE | PTP_TCR_TSCFUPDT;
priv->tsfupdt_coarse = false;
hwts_tx_en and hwts_rx_en are cleared too. That means the configuration
stored here is lost across suspend/resume, and so is the devlink
phc_coarse_adj setting. Devlink get then reports false.
The reset was added on purpose by commit 232d49dd4b40 ("net: stmmac:
propagate PTP addend and system time programming errors"). Even before
that, systime_flags was overwritten on reinit.
Should the comment or the resume behavior be updated?
> +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;
[Severity: High]
Does this fully close the race described in the commit message?
The devlink set handler holds devl_lock and priv->lock, but not RTNL. The
open path updates the same state while holding only RTNL:
stmmac_open() / stmmac_change_mtu()
__stmmac_open()
stmmac_setup_ptp()
stmmac_init_timestamping()
stmmac_init_tstamp_counter()
stmmac_update_subsecond_increment()
stmmac_init_timestamping() does:
priv->systime_flags = STMMAC_HWTS_ACTIVE | PTP_TCR_TSCFUPDT;
priv->tsfupdt_coarse = false;
It also resets hwts_tx_en, hwts_rx_en and tstamp_config, then programs
PTP_TCR, PTP_SSIR and PTP_TAR. None of this takes priv->lock.
Could this interleaving happen?
CPU0 (devlink phc_coarse_adj=true)
stmmac_dl_ts_coarse_set()
systime_flags = priv->systime_flags;
stmmac_update_subsecond_increment() /* coarse TCR/SSIR/TAR */
CPU1 (ip link set up, or an MTU change)
stmmac_init_timestamping()
priv->systime_flags = STMMAC_HWTS_ACTIVE | PTP_TCR_TSCFUPDT;
priv->tsfupdt_coarse = false;
stmmac_init_tstamp_counter() /* fine mode */
CPU0
priv->tsfupdt_coarse = ctx->val.vbool;
priv->systime_flags = systime_flags;
The hardware is then in fine mode, while priv->systime_flags and devlink
get both say coarse. Other orderings can leave sub_second_inc and
default_addend out of step with PTP_TCR.
The commit message says the configuration "is updated under a single
lock", but the open and MTU change paths never take priv->lock. Should
the open path's timestamping reinit also take priv->lock? Another option
is to serialize the devlink setter with RTNL.
> 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;
> }
[Severity: High]
This is a pre-existing issue, but can stmmac_dl_ts_coarse_set() do PTP
register MMIO while the device clocks are gated?
The devlink param is registered at probe through stmmac_register_devlink(),
and probe ends with pm_runtime_put(). stmmac_release() also calls
pm_runtime_put(), after stmmac_release_ptp() has disabled clk_ptp_ref.
stmmac_runtime_suspend() then gates stmmac_clk and pclk through
stmmac_bus_clks_config(priv, false).
The devlink path has no netif_running() check and no
pm_runtime_resume_and_get(). So with the interface down, or never opened,
this path runs:
devlink dev param set ... name phc_coarse_adj value true cmode runtime
stmmac_dl_ts_coarse_set()
stmmac_update_subsecond_increment()
config_hw_tstamping()
readl(ioaddr + PTP_TCR)
On many ARM SoCs, could this trigger an external abort or a bus hang?
On platforms where the access does not fault, the TSADDREG handshake can
busy-wait in readl_poll_timeout_atomic() for up to 100 ms. With this patch
that wait happens while holding priv->lock.
stmmac_hwtstamp_set() refuses to run while the interface is down. The MDIO
accessors take a runtime PM reference before doing MMIO. Should the
devlink setter do one of these too?
This goes back to commit 6920fa0c764d, and the added priv->lock does not
change it.
>
> priv->tsfupdt_coarse = ctx->val.vbool;
> priv->systime_flags = systime_flags;
> +unlock:
> + mutex_unlock(&priv->lock);
>
> - return 0;
> + return ret;
> }
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005-stmmac-hwtstamp-missing-lock-v2-1-3b3bf4fe3f45%40oss.qualcomm.com
^ permalink raw reply [flat|nested] 6+ messages in thread