stmmac_setup_ptp() returns void and swallows both PTP setup errors:
the PTP reference clock enable and stmmac_init_timestamping()
failures are logged but never propagated. When they fail, the MAC
system time counter is left in its post-reset, non-running state,
while the driver keeps operating as if timestamping were up.
This matters for TAPRIO/EST qdisc offloading, which derives the EST
base time from the hardware timestamp counter: arming the gate list
against a non-advancing time base would leave the schedule permanently
stuck. Make stmmac_setup_ptp() return an error code.
Fixes: 92ba6888510c ("stmmac: add the support for PTP hw clock driver")
Fixes: 0ad2be79f254 ("net: stmmac: Balance PTP reference clock enable/disable")
Signed-off-by: Lorenzo Bianconi <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 66 ++++++++++++++++-------
1 file changed, 48 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 24656b35350b..d33c82bd3ee4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -885,6 +885,11 @@ static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
return 0;
}
+static bool stmmac_check_timestamp_cap(struct stmmac_priv *priv)
+{
+ return priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp;
+}
+
/**
* stmmac_init_timestamping - initialise timestamping
* @priv: driver private structure@@ -900,11 +905,6 @@ static int stmmac_init_timestamping(struct stmmac_priv *priv)
if (priv->plat->ptp_clk_freq_config)
priv->plat->ptp_clk_freq_config(priv);
- if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp)) {
- netdev_info(priv->dev, "PTP not supported by HW\n");
- return -EOPNOTSUPP;
- }
-
ret = stmmac_init_tstamp_counter(priv, STMMAC_HWTS_ACTIVE |
PTP_TCR_TSCFUPDT);
if (ret) {@@ -937,22 +937,39 @@ static int stmmac_init_timestamping(struct stmmac_priv *priv)
return 0;
}
-static void stmmac_setup_ptp(struct stmmac_priv *priv)
+static int stmmac_setup_ptp(struct stmmac_priv *priv)
{
int ret;
+ if (!stmmac_check_timestamp_cap(priv)) {
+ netdev_info(priv->dev, "PTP not supported by HW\n");
+ return 0;
+ }
+
ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
- if (ret < 0)
+ if (ret < 0) {
netdev_warn(priv->dev,
"failed to enable PTP reference clock: %pe\n",
ERR_PTR(ret));
+ return ret;
+ }
+
+ ret = stmmac_init_timestamping(priv);
+ if (ret) {
+ clk_disable_unprepare(priv->plat->clk_ptp_ref);
+ return ret;
+ }
- if (stmmac_init_timestamping(priv) == 0)
- stmmac_ptp_register(priv);
+ stmmac_ptp_register(priv);
+
+ return 0;
}
static void stmmac_release_ptp(struct stmmac_priv *priv)
{
+ if (!stmmac_check_timestamp_cap(priv))
+ return;
+
stmmac_ptp_unregister(priv);
clk_disable_unprepare(priv->plat->clk_ptp_ref);
}@@ -4161,10 +4178,12 @@ static int __stmmac_open(struct net_device *dev,
ret = stmmac_hw_setup(dev);
if (ret < 0) {
netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
- goto init_error;
+ return ret;
}
- stmmac_setup_ptp(priv);
+ ret = stmmac_setup_ptp(priv);
+ if (ret)
+ goto ptp_error;
stmmac_init_coalesce(priv);
@@ -4187,9 +4206,10 @@ static int __stmmac_open(struct net_device *dev,
for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
-
stmmac_release_ptp(priv);
-init_error:
+ptp_error:
+ stmmac_stop_all_dma(priv);
+
return ret;
}
@@ -8326,13 +8346,14 @@ int stmmac_resume(struct device *dev)
ret = stmmac_hw_setup(ndev);
if (ret < 0) {
netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
- stmmac_legacy_serdes_power_down(priv);
- mutex_unlock(&priv->lock);
- rtnl_unlock();
- return ret;
+ goto error_unlock;
}
- stmmac_init_timestamping(priv);
+ if (stmmac_check_timestamp_cap(priv)) {
+ ret = stmmac_init_timestamping(priv);
+ if (ret)
+ goto error_stop_dma;
+ }
stmmac_init_coalesce(priv);
phylink_rx_clk_stop_block(priv->phylink);@@ -8356,6 +8377,15 @@ int stmmac_resume(struct device *dev)
netif_device_attach(ndev);
return 0;
+
+error_stop_dma:
+ stmmac_stop_all_dma(priv);
+error_unlock:
+ stmmac_legacy_serdes_power_down(priv);
+ mutex_unlock(&priv->lock);
+ rtnl_unlock();
+
+ return ret;
}
EXPORT_SYMBOL_GPL(stmmac_resume);
---
base-commit: 641d03105cc0d2437e32fdeec164f91a4ccef6c4
change-id: 20260904-stmmac-ptp-error-propagate-6c0147b4ce29
Best regards,
--
Lorenzo Bianconi [off-list ref]
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 1 (1) · Medium: 1 · Low: 2
- [High] Undisclosed functional regression: PTP setup failures are now
fatal to `ndo_open()` and to system resume.
- [Medium] Unbalanced clk_ptp_ref enable/disable against the platform
noirq PM callbacks.
- [Low] The kernel-doc block above `stmmac_init_timestamping()`
(stmmac_main.c:893-899) still states 'this is to verify if the HW…
- [Low] Both new unwind paths halt the DMA but never undo
`stmmac_hw_setup()`'s `stmmac_mac_set(priv, priv->ioaddr, true)`…
Pre-existing issues:
- [High] Pre-existing double free / use-after-free of the DMA descriptor
rings, whose trigger set this patch widens.
--
net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()
This converts stmmac_setup_ptp() from void to int so that the PTP
reference clock enable and stmmac_init_timestamping() failures are
propagated, and adds unwind paths in __stmmac_open() and stmmac_resume()
for those failures. The HW timestamp capability check moves out of
stmmac_init_timestamping() into the new stmmac_check_timestamp_cap()
helper.
A few questions below.
quoted hunk
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 24656b35350b1..d33c82bd3ee46 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -885,6 +885,11 @@ static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
return 0;
}
+static bool stmmac_check_timestamp_cap(struct stmmac_priv *priv)
+{
+ return priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp;
+}
+
/**
* stmmac_init_timestamping - initialise timestamping
* @priv: driver private structure@@ -900,11 +905,6 @@ static int stmmac_init_timestamping(struct stmmac_priv *priv)
if (priv->plat->ptp_clk_freq_config)
priv->plat->ptp_clk_freq_config(priv);
- if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp)) {
- netdev_info(priv->dev, "PTP not supported by HW\n");
- return -EOPNOTSUPP;
- }
-
[Severity: Low]
Should the kernel-doc block just above stmmac_init_timestamping() be updated
in the same patch? It still says:
* Description: this is to verify if the HW supports the PTPv1 or PTPv2.
* This is done by looking at the HW cap. register.
* This function also registers the ptp driver.
After this hunk the HW cap. register check lives in
stmmac_check_timestamp_cap()/stmmac_setup_ptp(), which is also why
stmmac_resume() now has to open-code "if (stmmac_check_timestamp_cap(priv))",
and stmmac_ptp_register() is only called from stmmac_setup_ptp(). The
-EOPNOTSUPP return the description implies is no longer produced by any
path here, and the function's new error returns are not described.
quoted hunk
ret = stmmac_init_tstamp_counter(priv, STMMAC_HWTS_ACTIVE |
PTP_TCR_TSCFUPDT);
if (ret) {@@ -937,22 +937,39 @@ static int stmmac_init_timestamping(struct stmmac_priv *priv)
return 0;
}
-static void stmmac_setup_ptp(struct stmmac_priv *priv)
+static int stmmac_setup_ptp(struct stmmac_priv *priv)
{
int ret;
+ if (!stmmac_check_timestamp_cap(priv)) {
+ netdev_info(priv->dev, "PTP not supported by HW\n");
+ return 0;
+ }
+
ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
[Severity: Medium]
Does this early return unbalance clk_ptp_ref against the platform noirq PM
callbacks?
Before this patch stmmac_setup_ptp() always did clk_prepare_enable() on open
and stmmac_release_ptp() always did clk_disable_unprepare() on close, so a
running interface always held one enable/prepare reference. With the new
capability gate in both functions, hardware whose dma_cap.time_stamp and
dma_cap.atime_stamp are clear never gets the clock enabled. Note
stmmac_hw_init() leaves priv->dma_cap all-zero when there is no HW feature
register ("No HW DMA feature register supported").
stmmac_pltfr_noirq_suspend() in stmmac_platform.c still does this
unconditionally for any running non-WoL interface:
if (!priv->wolopts) {
/* Disable clock in case of PWM is off */
clk_disable_unprepare(priv->plat->clk_ptp_ref);
With the enable/prepare counts at zero, does that hit the clk core
enable_count/prepare_count underflow warning? And after
stmmac_pltfr_noirq_resume() re-enables it unconditionally, the close path
stmmac_release_ptp() now returns early for the same hardware, so does that
reference stay held for good?
These four sites appear to be the only clk_ptp_ref enable/disable users, and
stmmac_pltfr_pm_ops installs those noirq callbacks for a large number of
platform glue drivers.
quoted hunk
- if (ret < 0)
+ if (ret < 0) {
netdev_warn(priv->dev,
"failed to enable PTP reference clock: %pe\n",
ERR_PTR(ret));
+ return ret;
+ }
+
+ ret = stmmac_init_timestamping(priv);
+ if (ret) {
+ clk_disable_unprepare(priv->plat->clk_ptp_ref);
+ return ret;
+ }
- if (stmmac_init_timestamping(priv) == 0)
- stmmac_ptp_register(priv);
+ stmmac_ptp_register(priv);
+
+ return 0;
}
static void stmmac_release_ptp(struct stmmac_priv *priv)
{
+ if (!stmmac_check_timestamp_cap(priv))
+ return;
+
stmmac_ptp_unregister(priv);
clk_disable_unprepare(priv->plat->clk_ptp_ref);
}@@ -4161,10 +4178,12 @@ static int __stmmac_open(struct net_device *dev,
ret = stmmac_hw_setup(dev);
if (ret < 0) {
netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
- goto init_error;
+ return ret;
}
- stmmac_setup_ptp(priv);
+ ret = stmmac_setup_ptp(priv);
+ if (ret)
+ goto ptp_error;
[Severity: High]
Can this make "ip link set dev ethX up" fail on configurations that work
today?
stmmac_setup_ptp() -> stmmac_init_timestamping() -> stmmac_init_tstamp_counter()
returns -EINVAL for a zero PTP rate:
if (!priv->plat->clk_ptp_rate) {
netdev_err(priv->dev, "Invalid PTP clock rate");
return -EINVAL;
}
Before this patch that only produced the "PTP init failed" warning and the
interface still came up without timestamping.
Is "timestamp capable but clk_ptp_rate == 0" a real configuration?
stmmac_register_devlink() already tolerates exactly that combination:
if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp) ||
!priv->plat->clk_ptp_rate)
return 0;
and stmmac_probe_config_dt() derives the rate from a clock that may be NULL:
if (IS_ERR(plat->clk_ptp_ref)) {
plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
plat->clk_ptp_ref = NULL;
with clk_get_rate(NULL) returning 0 (plat->stmmac_clk is left NULL on the
"Cannot get CSR clock" path, and is not fetched at all for
snps,dwc-qos-ethernet-4.10).
On such a board, would the interface become permanently un-openable for
users who never touch PTP or TAPRIO? The same question applies to
stmmac_resume() below, where the new goto skips netif_device_attach() and
leaves a previously working link down. Since there are two Fixes: tags this
will be picked up for stable, so it may be worth spelling the behaviour
change out in the changelog, or keeping the failure non-fatal for the
datapath and only refusing the PTP/EST-dependent features.
quoted hunk
stmmac_init_coalesce(priv);
@@ -4187,9 +4206,10 @@ static int __stmmac_open(struct net_device *dev,
for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
-
stmmac_release_ptp(priv);
-init_error:
+ptp_error:
+ stmmac_stop_all_dma(priv);
+
return ret;
}
[Severity: High]
This isn't a bug introduced by this patch, but the new ptp_error exit widens
the set of ways __stmmac_open() can fail after it has already done:
memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
so priv->dma_conf holds its own copy of every pointer in the caller's
dma_conf. On the MTU reopen path the caller reacts to failure like this:
stmmac_change_mtu()
ret = __stmmac_open(dev, dma_conf);
if (ret) {
free_dma_desc_resources(priv, dma_conf);
kfree(dma_conf);
The interface is left administratively up, and priv->dma_conf still points
at the freed dma_alloc_coherent() buffers and the freed tx_skbuff/rx_skbuff
arrays, since nulling pointers inside the caller's copy cannot clear the
memcpy'd copy. A later "ip link set dev ethX down" then runs:
__stmmac_release()
stmmac_stop_all_dma(priv);
/* Release and free the Rx/Tx resources */
free_dma_desc_resources(priv, &priv->dma_conf);
Does that free the same rings a second time? The pre-existing trigger was
an stmmac_request_irq() failure via irq_error; with this patch a PTP
reference clock enable failure reaches the same state.
[Severity: Low]
Should this unwind also clear the MAC Rx/Tx enable that stmmac_hw_setup()
set?
stmmac_hw_setup() does:
/* Enable the MAC Rx/Tx */
stmmac_mac_set(priv, priv->ioaddr, true);
stmmac_stop_all_dma() only clears the DMA channel start bits and deinits the
channels. The MAC enable is cleared in the mac_link_down callback, in the
reset path and in stmmac_suspend(), and on the ptp_error path
phylink_start() was never reached, so mac_link_down() cannot run.
stmmac_open() then frees the descriptor rings and drops the runtime-PM
reference, leaving the MAC clock-gated with its Rx/Tx still armed. All
stmmac_hw_setup() failure returns happen before the stmmac_mac_set() call,
so this residue is specific to the new post-setup failure exits.
quoted hunk
@@ -8326,13 +8346,14 @@ int stmmac_resume(struct device *dev)
ret = stmmac_hw_setup(ndev);
if (ret < 0) {
netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
- stmmac_legacy_serdes_power_down(priv);
- mutex_unlock(&priv->lock);
- rtnl_unlock();
- return ret;
+ goto error_unlock;
}
- stmmac_init_timestamping(priv);
+ if (stmmac_check_timestamp_cap(priv)) {
+ ret = stmmac_init_timestamping(priv);
+ if (ret)
+ goto error_stop_dma;
+ }
stmmac_init_coalesce(priv);
phylink_rx_clk_stop_block(priv->phylink);@@ -8356,6 +8377,15 @@ int stmmac_resume(struct device *dev)
netif_device_attach(ndev);
return 0;
+
+error_stop_dma:
+ stmmac_stop_all_dma(priv);
+error_unlock:
+ stmmac_legacy_serdes_power_down(priv);
+ mutex_unlock(&priv->lock);
+ rtnl_unlock();
+
+ return ret;
}
[Severity: Low]
Same question on the resume side: stmmac_suspend() explicitly did
stmmac_mac_set(priv, priv->ioaddr, false);
but error_stop_dma/error_unlock stops the DMA and calls
stmmac_legacy_serdes_power_down() while the MAC Rx/Tx enabled by
stmmac_hw_setup() is still set, so the SerDes is removed under an enabled
MAC. Would adding stmmac_mac_set(priv, priv->ioaddr, false) to this unwind
restore the state that stmmac_suspend() had established?
Also, on this path netif_device_attach() is never reached while
netif_running() is still true, so the netdev stays detached after a resume
failure. Is that the intended outcome for a PTP init error?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-stmmac-ptp-error-propagate-v1-1-80f01b03dafa%40oss.qualcomm.com
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 1 (1) · Medium: 1 · Low: 2
- [High] Undisclosed functional regression: PTP setup failures are now
fatal to `ndo_open()` and to system resume.
- [Medium] Unbalanced clk_ptp_ref enable/disable against the platform
noirq PM callbacks.
- [Low] The kernel-doc block above `stmmac_init_timestamping()`
(stmmac_main.c:893-899) still states 'this is to verify if the HW…
- [Low] Both new unwind paths halt the DMA but never undo
`stmmac_hw_setup()`'s `stmmac_mac_set(priv, priv->ioaddr, true)`…
Pre-existing issues:
- [High] Pre-existing double free / use-after-free of the DMA descriptor
rings, whose trigger set this patch widens.
--
net: stmmac: propagate PTP init failures in __stmmac_open() and stmmac_resume()
I think I have already posted a v2 for this series:
https://patchwork.kernel.org/project/netdevbpf/patch/20260907-stmmac-ptp-error-propagate-v2-1-4a2e8e41e860@oss.qualcomm.com/
This one is marked as Superseded in patchwork.
Regards,
Lorenzo
This converts stmmac_setup_ptp() from void to int so that the PTP
reference clock enable and stmmac_init_timestamping() failures are
propagated, and adds unwind paths in __stmmac_open() and stmmac_resume()
for those failures. The HW timestamp capability check moves out of
stmmac_init_timestamping() into the new stmmac_check_timestamp_cap()
helper.
A few questions below.
quoted
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 24656b35350b1..d33c82bd3ee46 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -885,6 +885,11 @@ static int stmmac_init_tstamp_counter(struct stmmac_priv *priv,
return 0;
}
+static bool stmmac_check_timestamp_cap(struct stmmac_priv *priv)
+{
+ return priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp;
+}
+
/**
* stmmac_init_timestamping - initialise timestamping
* @priv: driver private structure@@ -900,11 +905,6 @@ static int stmmac_init_timestamping(struct stmmac_priv *priv)
if (priv->plat->ptp_clk_freq_config)
priv->plat->ptp_clk_freq_config(priv);
- if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp)) {
- netdev_info(priv->dev, "PTP not supported by HW\n");
- return -EOPNOTSUPP;
- }
-
[Severity: Low]
Should the kernel-doc block just above stmmac_init_timestamping() be updated
in the same patch? It still says:
* Description: this is to verify if the HW supports the PTPv1 or PTPv2.
* This is done by looking at the HW cap. register.
* This function also registers the ptp driver.
After this hunk the HW cap. register check lives in
stmmac_check_timestamp_cap()/stmmac_setup_ptp(), which is also why
stmmac_resume() now has to open-code "if (stmmac_check_timestamp_cap(priv))",
and stmmac_ptp_register() is only called from stmmac_setup_ptp(). The
-EOPNOTSUPP return the description implies is no longer produced by any
path here, and the function's new error returns are not described.
quoted
ret = stmmac_init_tstamp_counter(priv, STMMAC_HWTS_ACTIVE |
PTP_TCR_TSCFUPDT);
if (ret) {@@ -937,22 +937,39 @@ static int stmmac_init_timestamping(struct stmmac_priv *priv)
return 0;
}
-static void stmmac_setup_ptp(struct stmmac_priv *priv)
+static int stmmac_setup_ptp(struct stmmac_priv *priv)
{
int ret;
+ if (!stmmac_check_timestamp_cap(priv)) {
+ netdev_info(priv->dev, "PTP not supported by HW\n");
+ return 0;
+ }
+
ret = clk_prepare_enable(priv->plat->clk_ptp_ref);
[Severity: Medium]
Does this early return unbalance clk_ptp_ref against the platform noirq PM
callbacks?
Before this patch stmmac_setup_ptp() always did clk_prepare_enable() on open
and stmmac_release_ptp() always did clk_disable_unprepare() on close, so a
running interface always held one enable/prepare reference. With the new
capability gate in both functions, hardware whose dma_cap.time_stamp and
dma_cap.atime_stamp are clear never gets the clock enabled. Note
stmmac_hw_init() leaves priv->dma_cap all-zero when there is no HW feature
register ("No HW DMA feature register supported").
stmmac_pltfr_noirq_suspend() in stmmac_platform.c still does this
unconditionally for any running non-WoL interface:
if (!priv->wolopts) {
/* Disable clock in case of PWM is off */
clk_disable_unprepare(priv->plat->clk_ptp_ref);
With the enable/prepare counts at zero, does that hit the clk core
enable_count/prepare_count underflow warning? And after
stmmac_pltfr_noirq_resume() re-enables it unconditionally, the close path
stmmac_release_ptp() now returns early for the same hardware, so does that
reference stay held for good?
These four sites appear to be the only clk_ptp_ref enable/disable users, and
stmmac_pltfr_pm_ops installs those noirq callbacks for a large number of
platform glue drivers.
quoted
- if (ret < 0)
+ if (ret < 0) {
netdev_warn(priv->dev,
"failed to enable PTP reference clock: %pe\n",
ERR_PTR(ret));
+ return ret;
+ }
+
+ ret = stmmac_init_timestamping(priv);
+ if (ret) {
+ clk_disable_unprepare(priv->plat->clk_ptp_ref);
+ return ret;
+ }
- if (stmmac_init_timestamping(priv) == 0)
- stmmac_ptp_register(priv);
+ stmmac_ptp_register(priv);
+
+ return 0;
}
static void stmmac_release_ptp(struct stmmac_priv *priv)
{
+ if (!stmmac_check_timestamp_cap(priv))
+ return;
+
stmmac_ptp_unregister(priv);
clk_disable_unprepare(priv->plat->clk_ptp_ref);
}@@ -4161,10 +4178,12 @@ static int __stmmac_open(struct net_device *dev,
ret = stmmac_hw_setup(dev);
if (ret < 0) {
netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
- goto init_error;
+ return ret;
}
- stmmac_setup_ptp(priv);
+ ret = stmmac_setup_ptp(priv);
+ if (ret)
+ goto ptp_error;
[Severity: High]
Can this make "ip link set dev ethX up" fail on configurations that work
today?
stmmac_setup_ptp() -> stmmac_init_timestamping() -> stmmac_init_tstamp_counter()
returns -EINVAL for a zero PTP rate:
if (!priv->plat->clk_ptp_rate) {
netdev_err(priv->dev, "Invalid PTP clock rate");
return -EINVAL;
}
Before this patch that only produced the "PTP init failed" warning and the
interface still came up without timestamping.
Is "timestamp capable but clk_ptp_rate == 0" a real configuration?
stmmac_register_devlink() already tolerates exactly that combination:
if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp) ||
!priv->plat->clk_ptp_rate)
return 0;
and stmmac_probe_config_dt() derives the rate from a clock that may be NULL:
if (IS_ERR(plat->clk_ptp_ref)) {
plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk);
plat->clk_ptp_ref = NULL;
with clk_get_rate(NULL) returning 0 (plat->stmmac_clk is left NULL on the
"Cannot get CSR clock" path, and is not fetched at all for
snps,dwc-qos-ethernet-4.10).
On such a board, would the interface become permanently un-openable for
users who never touch PTP or TAPRIO? The same question applies to
stmmac_resume() below, where the new goto skips netif_device_attach() and
leaves a previously working link down. Since there are two Fixes: tags this
will be picked up for stable, so it may be worth spelling the behaviour
change out in the changelog, or keeping the failure non-fatal for the
datapath and only refusing the PTP/EST-dependent features.
quoted
stmmac_init_coalesce(priv);
@@ -4187,9 +4206,10 @@ static int __stmmac_open(struct net_device *dev,
for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
-
stmmac_release_ptp(priv);
-init_error:
+ptp_error:
+ stmmac_stop_all_dma(priv);
+
return ret;
}
[Severity: High]
This isn't a bug introduced by this patch, but the new ptp_error exit widens
the set of ways __stmmac_open() can fail after it has already done:
memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
so priv->dma_conf holds its own copy of every pointer in the caller's
dma_conf. On the MTU reopen path the caller reacts to failure like this:
stmmac_change_mtu()
ret = __stmmac_open(dev, dma_conf);
if (ret) {
free_dma_desc_resources(priv, dma_conf);
kfree(dma_conf);
The interface is left administratively up, and priv->dma_conf still points
at the freed dma_alloc_coherent() buffers and the freed tx_skbuff/rx_skbuff
arrays, since nulling pointers inside the caller's copy cannot clear the
memcpy'd copy. A later "ip link set dev ethX down" then runs:
__stmmac_release()
stmmac_stop_all_dma(priv);
/* Release and free the Rx/Tx resources */
free_dma_desc_resources(priv, &priv->dma_conf);
Does that free the same rings a second time? The pre-existing trigger was
an stmmac_request_irq() failure via irq_error; with this patch a PTP
reference clock enable failure reaches the same state.
[Severity: Low]
Should this unwind also clear the MAC Rx/Tx enable that stmmac_hw_setup()
set?
stmmac_hw_setup() does:
/* Enable the MAC Rx/Tx */
stmmac_mac_set(priv, priv->ioaddr, true);
stmmac_stop_all_dma() only clears the DMA channel start bits and deinits the
channels. The MAC enable is cleared in the mac_link_down callback, in the
reset path and in stmmac_suspend(), and on the ptp_error path
phylink_start() was never reached, so mac_link_down() cannot run.
stmmac_open() then frees the descriptor rings and drops the runtime-PM
reference, leaving the MAC clock-gated with its Rx/Tx still armed. All
stmmac_hw_setup() failure returns happen before the stmmac_mac_set() call,
so this residue is specific to the new post-setup failure exits.
quoted
@@ -8326,13 +8346,14 @@ int stmmac_resume(struct device *dev)
ret = stmmac_hw_setup(ndev);
if (ret < 0) {
netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
- stmmac_legacy_serdes_power_down(priv);
- mutex_unlock(&priv->lock);
- rtnl_unlock();
- return ret;
+ goto error_unlock;
}
- stmmac_init_timestamping(priv);
+ if (stmmac_check_timestamp_cap(priv)) {
+ ret = stmmac_init_timestamping(priv);
+ if (ret)
+ goto error_stop_dma;
+ }
stmmac_init_coalesce(priv);
phylink_rx_clk_stop_block(priv->phylink);@@ -8356,6 +8377,15 @@ int stmmac_resume(struct device *dev)
netif_device_attach(ndev);
return 0;
+
+error_stop_dma:
+ stmmac_stop_all_dma(priv);
+error_unlock:
+ stmmac_legacy_serdes_power_down(priv);
+ mutex_unlock(&priv->lock);
+ rtnl_unlock();
+
+ return ret;
}
[Severity: Low]
Same question on the resume side: stmmac_suspend() explicitly did
stmmac_mac_set(priv, priv->ioaddr, false);
but error_stop_dma/error_unlock stops the DMA and calls
stmmac_legacy_serdes_power_down() while the MAC Rx/Tx enabled by
stmmac_hw_setup() is still set, so the SerDes is removed under an enabled
MAC. Would adding stmmac_mac_set(priv, priv->ioaddr, false) to this unwind
restore the state that stmmac_suspend() had established?
Also, on this path netif_device_attach() is never reached while
netif_running() is still true, so the netdev stays detached after a resume
failure. Is that the intended outcome for a PTP init error?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-stmmac-ptp-error-propagate-v1-1-80f01b03dafa%40oss.qualcomm.com