The aux_ts_lock mutex is only required while the PTP clock has been successfully registered. stmmac_ptp_register() does not return any errors (as we don't wish to prevent the netdev being opened if PTP fails), stmmac_ptp_unregister() was coded to allow it to be called irrespective of whether PTP was successfully registered or not. Arrange for the aux_ts_lock mutex to be destroyed if the PTP clock is not functional during stmmac_ptp_register(), and only destroy it in stmmac_ptp_unregister() if we had a PTP clock registered. Signed-off-by: Russell King (Oracle) --- drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c index 69bd8ace139c..993ff4e87e55 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c @@ -370,8 +370,12 @@ void stmmac_ptp_register(struct stmmac_priv *priv) if (IS_ERR(priv->ptp_clock)) { netdev_err(priv->dev, "ptp_clock_register failed\n"); priv->ptp_clock = NULL; - } else if (priv->ptp_clock) + } + + if (priv->ptp_clock) netdev_info(priv->dev, "registered PTP clock\n"); + else + mutex_destroy(&priv->aux_ts_lock); } /** @@ -387,7 +391,7 @@ void stmmac_ptp_unregister(struct stmmac_priv *priv) priv->ptp_clock = NULL; pr_debug("Removed PTP HW clock successfully on %s\n", priv->dev->name); - } - mutex_destroy(&priv->aux_ts_lock); + mutex_destroy(&priv->aux_ts_lock); + } } -- 2.47.3 Follow the principle of unpublish from userspace and then teardown resources. Disable the PTP clock only after unregistering with the PTP subsystem, which ensures that we only stop the clock that ticks the timesource after we have removed the PTP device. Signed-off-by: Russell King (Oracle) --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- 1 file 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 419cb49ee5a2..5d76cf7957ab 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -817,8 +817,8 @@ static int stmmac_init_ptp(struct stmmac_priv *priv) static void stmmac_release_ptp(struct stmmac_priv *priv) { - clk_disable_unprepare(priv->plat->clk_ptp_ref); stmmac_ptp_unregister(priv); + clk_disable_unprepare(priv->plat->clk_ptp_ref); } /** -- 2.47.3 The cleanup function for stmmac_setup_ptp() is stmmac_release_ptp() which entirely undoes the effects of stmmac_setup_ptp() by unregistering the PTP device and then stopping the PTP clock, whereas stmmac_hw_teardown() will only stop the PTP clock while leaving the PTP device registered. This can lead to a kernel oops - if __stmmac_open() fails after registering the PTP clock, the PTP device will remain registered, and if the module is removed, subsequent PTP device accesses will lead to a kernel oops. Signed-off-by: Russell King (Oracle) --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- 1 file 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 5d76cf7957ab..167405aac5b8 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -4032,7 +4032,7 @@ 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_hw_teardown(dev); + stmmac_release_ptp(priv); init_error: phylink_disconnect_phy(priv->phylink); init_phy_error: -- 2.47.3 Neither stmmac_xdp_release() nor the normal paths of stmmac_xdp_open() touch clk_ptp_ref, so stmmac_xdp_open() should not be doing anything with this clock. However, in its error path, it calls stmmac_hw_teardown() which disables and unprepares this clock, which can lead to the clock state becoming unbalanced when the netdev is taken administratively down. Remove this call to stmmac_hw_teardown(), and as this is the last user of this function, remove the function as well. Signed-off-by: Russell King (Oracle) --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 167405aac5b8..8cb1a97e18af 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3568,13 +3568,6 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register) return 0; } -static void stmmac_hw_teardown(struct net_device *dev) -{ - struct stmmac_priv *priv = netdev_priv(dev); - - clk_disable_unprepare(priv->plat->clk_ptp_ref); -} - static void stmmac_free_irq(struct net_device *dev, enum request_irq_err irq_err, int irq_idx) { @@ -6992,7 +6985,6 @@ int stmmac_xdp_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_hw_teardown(dev); init_error: free_dma_desc_resources(priv, &priv->dma_conf); dma_desc_error: -- 2.47.3 Nothing outside of stmmac_main.c makes use of stmmac_init_tstamp_counter(), so there's no point exporting it for modules, or even having it non-static. Remove the export and make it static. Signed-off-by: Russell King (Oracle) --- drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 - drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index ec6bccb13710..151f08e5e85d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -392,7 +392,6 @@ int stmmac_pcs_setup(struct net_device *ndev); void stmmac_pcs_clean(struct net_device *ndev); void stmmac_set_ethtool_ops(struct net_device *netdev); -int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags); void stmmac_ptp_register(struct stmmac_priv *priv); void stmmac_ptp_unregister(struct stmmac_priv *priv); int stmmac_xdp_open(struct net_device *dev); diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 8cb1a97e18af..efce7b37f704 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -726,7 +726,8 @@ static int stmmac_hwtstamp_get(struct net_device *dev, * Will be rerun after resuming from suspend, case in which the timestamping * flags updated by stmmac_hwtstamp_set() also need to be restored. */ -int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags) +static int stmmac_init_tstamp_counter(struct stmmac_priv *priv, + u32 systime_flags) { bool xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac; struct timespec64 now; @@ -770,7 +771,6 @@ int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 systime_flags) return 0; } -EXPORT_SYMBOL_GPL(stmmac_init_tstamp_counter); /** * stmmac_init_ptp - init PTP -- 2.47.3 Signed-off-by: Russell King (Oracle) --- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 41 +++++++++++-------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index efce7b37f704..cb058e4c6ea9 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3965,10 +3965,6 @@ static int __stmmac_open(struct net_device *dev, if (!priv->tx_lpi_timer) priv->tx_lpi_timer = eee_timer * 1000; - ret = pm_runtime_resume_and_get(priv->device); - if (ret < 0) - return ret; - if ((!priv->hw->xpcs || xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73)) { ret = stmmac_init_phy(dev); @@ -3976,7 +3972,7 @@ static int __stmmac_open(struct net_device *dev, netdev_err(priv->dev, "%s: Cannot attach to PHY (error: %d)\n", __func__, ret); - goto init_phy_error; + return ret; } } @@ -4028,8 +4024,6 @@ static int __stmmac_open(struct net_device *dev, stmmac_release_ptp(priv); init_error: phylink_disconnect_phy(priv->phylink); -init_phy_error: - pm_runtime_put(priv->device); return ret; } @@ -4043,21 +4037,23 @@ static int stmmac_open(struct net_device *dev) if (IS_ERR(dma_conf)) return PTR_ERR(dma_conf); + ret = pm_runtime_resume_and_get(priv->device); + if (ret < 0) + goto err; + ret = __stmmac_open(dev, dma_conf); - if (ret) + if (ret) { + pm_runtime_put(priv->device); +err: free_dma_desc_resources(priv, dma_conf); + } kfree(dma_conf); + return ret; } -/** - * stmmac_release - close entry point of the driver - * @dev : device pointer. - * Description: - * This is the stop entry point of the driver. - */ -static int stmmac_release(struct net_device *dev) +static void __stmmac_release(struct net_device *dev) { struct stmmac_priv *priv = netdev_priv(dev); u32 chan; @@ -4097,6 +4093,19 @@ static int stmmac_release(struct net_device *dev) if (stmmac_fpe_supported(priv)) ethtool_mmsv_stop(&priv->fpe_cfg.mmsv); +} + +/** + * stmmac_release - close entry point of the driver + * @dev : device pointer. + * Description: + * This is the stop entry point of the driver. + */ +static int stmmac_release(struct net_device *dev) +{ + struct stmmac_priv *priv = netdev_priv(dev); + + __stmmac_release(dev); pm_runtime_put(priv->device); @@ -5895,7 +5904,7 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu) return PTR_ERR(dma_conf); } - stmmac_release(dev); + __stmmac_release(dev); ret = __stmmac_open(dev, dma_conf); if (ret) { -- 2.47.3 Move the stmmac_init_ptp() messages from stmmac_hw_setup() to stmmac_init_ptp(), which will allow further cleanups. Signed-off-by: Russell King (Oracle) --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index cb058e4c6ea9..716c7e21baf1 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -788,8 +788,13 @@ static int stmmac_init_ptp(struct stmmac_priv *priv) priv->plat->ptp_clk_freq_config(priv); ret = stmmac_init_tstamp_counter(priv, STMMAC_HWTS_ACTIVE); - if (ret) + if (ret) { + if (ret == -EOPNOTSUPP) + netdev_info(priv->dev, "PTP not supported by HW\n"); + else + netdev_warn(priv->dev, "PTP init failed\n"); return ret; + } priv->adv_ts = 0; /* Check if adv_ts can be enabled for dwmac 4.x / xgmac core */ @@ -3497,12 +3502,7 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register) ERR_PTR(ret)); } - ret = stmmac_init_ptp(priv); - if (ret == -EOPNOTSUPP) - netdev_info(priv->dev, "PTP not supported by HW\n"); - else if (ret) - netdev_warn(priv->dev, "PTP init failed\n"); - else if (ptp_register) + if (stmmac_init_ptp(priv) == 0 && ptp_register) stmmac_ptp_register(priv); if (priv->use_riwt) { -- 2.47.3 In preparation to cleaning up the (re-)initialisation of timestamping, rename the existing stmmac_init_ptp() to stmmac_init_timestamping() which better reflects its functionality. Signed-off-by: Russell King (Oracle) --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 716c7e21baf1..7cbac3ac2a9d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -773,13 +773,13 @@ static int stmmac_init_tstamp_counter(struct stmmac_priv *priv, } /** - * stmmac_init_ptp - init PTP + * stmmac_init_timestamping - initialise timestamping * @priv: driver private structure * 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. */ -static int stmmac_init_ptp(struct stmmac_priv *priv) +static int stmmac_init_timestamping(struct stmmac_priv *priv) { bool xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac; int ret; @@ -3502,7 +3502,7 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register) ERR_PTR(ret)); } - if (stmmac_init_ptp(priv) == 0 && ptp_register) + if (stmmac_init_timestamping(priv) == 0 && ptp_register) stmmac_ptp_register(priv); if (priv->use_riwt) { -- 2.47.3 Add a function to setup PTP, which will enable the clock, initialise the timestamping, and register with the PTP clock subsystem. Call this when we want to register the PTP clock in stmmac_hw_setup(), otherwise just call the Signed-off-by: Russell King (Oracle) --- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 7cbac3ac2a9d..ea2d3e555fe8 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -820,6 +820,20 @@ static int stmmac_init_timestamping(struct stmmac_priv *priv) return 0; } +static void stmmac_setup_ptp(struct stmmac_priv *priv) +{ + int ret; + + ret = clk_prepare_enable(priv->plat->clk_ptp_ref); + if (ret < 0) + netdev_warn(priv->dev, + "failed to enable PTP reference clock: %pe\n", + ERR_PTR(ret)); + + if (stmmac_init_timestamping(priv) == 0) + stmmac_ptp_register(priv); +} + static void stmmac_release_ptp(struct stmmac_priv *priv) { stmmac_ptp_unregister(priv); @@ -3494,16 +3508,10 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register) stmmac_mmc_setup(priv); - if (ptp_register) { - ret = clk_prepare_enable(priv->plat->clk_ptp_ref); - if (ret < 0) - netdev_warn(priv->dev, - "failed to enable PTP reference clock: %pe\n", - ERR_PTR(ret)); - } - - if (stmmac_init_timestamping(priv) == 0 && ptp_register) - stmmac_ptp_register(priv); + if (ptp_register) + stmmac_setup_ptp(priv); + else + stmmac_init_timestamping(priv); if (priv->use_riwt) { u32 queue; -- 2.47.3 Move the PTP support check from stmmac_init_tstamp_counter() into stmmac_init_timestamping() as it makes more sense to be there. Signed-off-by: Russell King (Oracle) --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index ea2d3e555fe8..ff12c4b34eb6 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -734,9 +734,6 @@ static int stmmac_init_tstamp_counter(struct stmmac_priv *priv, u32 sec_inc = 0; u64 temp = 0; - if (!(priv->dma_cap.time_stamp || priv->dma_cap.atime_stamp)) - return -EOPNOTSUPP; - if (!priv->plat->clk_ptp_rate) { netdev_err(priv->dev, "Invalid PTP clock rate"); return -EINVAL; @@ -787,12 +784,14 @@ 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); if (ret) { - if (ret == -EOPNOTSUPP) - netdev_info(priv->dev, "PTP not supported by HW\n"); - else - netdev_warn(priv->dev, "PTP init failed\n"); + netdev_warn(priv->dev, "PTP init failed\n"); return ret; } -- 2.47.3 Move the call to stmmac_init_timestamping() or stmmac_setup_ptp() out of stmmac_hw_setup() to its caller after stmmac_hw_setup() has successfully completed. This slightly changes the ordering during setup, but should be safe to do. Signed-off-by: Russell King (Oracle) --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index ff12c4b34eb6..8c8ca5999bd8 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3436,7 +3436,7 @@ static void stmmac_safety_feat_configuration(struct stmmac_priv *priv) * 0 on success and an appropriate (-)ve integer as defined in errno.h * file on failure. */ -static int stmmac_hw_setup(struct net_device *dev, bool ptp_register) +static int stmmac_hw_setup(struct net_device *dev) { struct stmmac_priv *priv = netdev_priv(dev); u32 rx_cnt = priv->plat->rx_queues_to_use; @@ -3507,11 +3507,6 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register) stmmac_mmc_setup(priv); - if (ptp_register) - stmmac_setup_ptp(priv); - else - stmmac_init_timestamping(priv); - if (priv->use_riwt) { u32 queue; @@ -4000,12 +3995,14 @@ static int __stmmac_open(struct net_device *dev, } } - ret = stmmac_hw_setup(dev, true); + ret = stmmac_hw_setup(dev); if (ret < 0) { netdev_err(priv->dev, "%s: Hw setup failed\n", __func__); goto init_error; } + stmmac_setup_ptp(priv); + stmmac_init_coalesce(priv); phylink_start(priv->phylink); @@ -7917,7 +7914,7 @@ int stmmac_resume(struct device *dev) stmmac_free_tx_skbufs(priv); stmmac_clear_descriptors(priv, &priv->dma_conf); - ret = stmmac_hw_setup(ndev, false); + ret = stmmac_hw_setup(ndev); if (ret < 0) { netdev_err(priv->dev, "%s: Hw setup failed\n", __func__); mutex_unlock(&priv->lock); @@ -7925,6 +7922,8 @@ int stmmac_resume(struct device *dev) return ret; } + stmmac_init_timestamping(priv); + stmmac_init_coalesce(priv); phylink_rx_clk_stop_block(priv->phylink); stmmac_set_rx_mode(ndev); -- 2.47.3