net: stmmac: timestamping/ptp cleanups
Subsystems (5)
cover net: stmmac: timestamping/ptp cleanups
Hi,
This series cleans up the hardware timestamping / PTP initialisation and cleanup code in the stmmac driver. Several key points in no particular order:
1. Golden rule: unregister first, then release resources. stmmac_release_ptp didn't do this.
2. Avoid leaking resources - __stmmac_open() failure leaves the timestamping support initialised, but stops its clock. Also violates (1).
3. Avoid double-release of resources - stmmac_open() followed by stmmac_xdp_open() failing results in the PTP clock prepare and enable counts being released, and if the interface is then brought down, they are incorrectly released again. As XDP doesn't gain any additional prepare/enables on the PTP clock, remove this incorrect cleanup.
4. Changing the MTU of the interface is disruptive to PTP, and remains so as long as. This is not fixed by this series (too invasive at the moment.)
5. Avoid exporting functions that aren't used...
6. Avoid unnecessary runtime PM state manipulations (no point manipulating this when MTU changes).
7. Make the PTP/timestamping initialisation more readable - no point calling functions in the same file from one callsite that return error codes from one location in the called function, to only have the sole callee print messages depending on that return code. Also simplifying the mess in stmmac_hw_setup(). Also placing support checks in a better location. Also getting rid of the "ptp_register" boolean through this restructuring.
Not tested beyond compile testing. (I don't have my Jetson Xavier NX platform.) So anyone testing this and providing feedback would be most welcome.
On that point... I hardly (never?) seem to get testing feedback from anyone when touching stmmac. I suspect that's because of the structure of the driver, where MAINTAINERS only lists people for their appropriate dwmac-* files. Thus they don't get Cc'd for core stmmac changes. Not sure what the solution is, but manually picking out all the entries in MAINTAINERS every time doesn't scale.
Therefore, I suggest merging this into net-next so people get to test it by way of it being in a tree they might be using.
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 - drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 113 ++++++++++++---------- drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 10 +- 3 files changed, 67 insertions(+), 57 deletions(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
PATCH 1/11 net: stmmac: ptp: improve handling of aux_ts_lock lifetime
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.
--- drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c +7 -3
@@ -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); + } }
Signature · 1 line
2.47.3
PATCH 2/11 net: stmmac: disable PTP clock after unregistering PTP
In reply to Russell King (Oracle)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.
--- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +1 -1
@@ -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); }/**
Signature · 1 line
2.47.3
PATCH 3/11 net: stmmac: fix PTP error cleanup in __stmmac_open()
In reply to Russell King (Oracle)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.
--- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +1 -1
@@ -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:
Signature · 1 line
2.47.3
PATCH 4/11 net: stmmac: fix stmmac_xdp_open() clk_ptp_ref error cleanup
In reply to Russell King (Oracle)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.
--- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 -------- 1 file changed, 8 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +0 -8
@@ -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:
Signature · 1 line
2.47.3
PATCH 5/11 net: stmmac: unexport stmmac_init_tstamp_counter()
In reply to Russell King (Oracle)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.
--- drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 - drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac.h +0 -1
@@ -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);
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +2 -2
@@ -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
Signature · 1 line
2.47.3
PATCH 6/11 net: stmmac: add __stmmac_release() to complement __stmmac_open()
In reply to Russell King (Oracle)--- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 41 +++++++++++-------- 1 file changed, 25 insertions(+), 16 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +25 -16
@@ -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) {
Signature · 1 line
2.47.3
Re: [Linux-stm32] [PATCH net-next 06/11] net: stmmac: add __stmmac_release() to complement __stmmac_open()
Quoted 1 line · Russell King (Oracle)
Signed-off-by Russell King (Oracle) <[email protected]>
Hi Russell,
This is missing a commit message.
Quoted 98 lines
--- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 41 +++++++++++-------- 1 file changed, 25 insertions(+), 16 deletions(-)drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +25
-16@@ -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) {
PATCH 7/11 net: stmmac: move stmmac_init_ptp() messages into function
In reply to Russell King (Oracle)Move the stmmac_init_ptp() messages from stmmac_hw_setup() to stmmac_init_ptp(), which will allow further cleanups.
--- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +7 -7
@@ -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) {
Signature · 1 line
2.47.3
PATCH 8/11 net: stmmac: rename stmmac_init_ptp()
In reply to Russell King (Oracle)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.
--- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +3 -3
@@ -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) {
Signature · 1 line
2.47.3
Re: [Linux-stm32] [PATCH net-next 08/11] net: stmmac: rename stmmac_init_ptp()
Quoted 4 lines · Russell King (Oracle)
In preparation to cleaning up the (re-)initialisation of timestamping, rename the existing
stmmac_init_ptp()tostmmac_init_timestamping()which better reflects its functionality.
I agree it's mostly about time stamping but if the ptp_clk_freq_config() ops is implemented, then it's not only about timestamping. Wasn't it fine as is?
Quoted 34 lines
Signed-off-by Russell King (Oracle) <[email protected]>--- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +3
-3@@ -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) {
Re: [Linux-stm32] [PATCH net-next 08/11] net: stmmac: rename stmmac_init_ptp()
Quoted 11 lines · Gatien CHEVALLIER
Quoted 4 lines · Russell King (Oracle)
In preparation to cleaning up the (re-)initialisation of timestamping, rename the existing
stmmac_init_ptp()tostmmac_init_timestamping()which better reflects its functionality.I agree it's mostly about time stamping but if the
ptp_clk_freq_config()ops is implemented, then it's not only about timestamping. Wasn't it fine as is?
No, if you look at the history, various bits of PTP initialisation have had to be moved out of stmmac_init_ptp() due to various problems, and this includes initialisation of the TAI timekeeping block block (or what we call ptp_clock in the kernel.) It's become less about initialising the entire PTP subsystem, more about just the time- stamping part.
So, the rename is justified, even though there's still bits in there that need to be re-architected.
However, continuing to call it "init_ptp" when it doesn't initialise all of PTP, especially as the patches after this adds another function that _does_ to the full initialisation just doesn't make sense - in fact, it becomes down-right confusing.
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
PATCH 9/11 net: stmmac: add stmmac_setup_ptp()
In reply to Russell King (Oracle)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
--- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +18 -10
@@ -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;
Signature · 1 line
2.47.3
PATCH 10/11 net: stmmac: move PTP support check into stmmac_init_timestamping()
In reply to Russell King (Oracle)Move the PTP support check from stmmac_init_tstamp_counter() into stmmac_init_timestamping() as it makes more sense to be there.
--- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +6 -7
@@ -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; }
Signature · 1 line
2.47.3
PATCH 11/11 net: stmmac: move timestamping/ptp init to stmmac_hw_setup() caller
In reply to Russell King (Oracle)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.
--- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +7 -8
@@ -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);
Signature · 1 line
2.47.3
Quoted 6 lines
On that point... I hardly (never?) seem to get testing feedback from anyone when touching stmmac. I suspect that's because of the structure of the driver, where
MAINTAINERSonly lists people for their appropriate dwmac-* files. Thus they don't get Cc'd for core stmmac changes. Not sure what the solution is, but manually picking out all the entries inMAINTAINERSevery time doesn't scale.
One option might be to add an R: entry to the STMMAC ETHERNET DRIVER for everybody who Maintains a glue driver?
Andrew
Quoted 48 lines · Russell King (Oracle)
Hi,
This series cleans up the hardware timestamping / PTP initialisation and cleanup code in the stmmac driver. Several key points in no particular order:
1. Golden rule: unregister first, then release resources.
stmmac_release_ptpdidn't do this.2. Avoid leaking resources -
__stmmac_open()failure leaves the timestamping support initialised, but stops its clock. Also violates (1).3. Avoid double-release of resources -
stmmac_open()followed bystmmac_xdp_open()failing results in the PTP clock prepare and enable counts being released, and if the interface is then brought down, they are incorrectly released again. As XDP doesn't gain any additional prepare/enables on the PTP clock, remove this incorrect cleanup.4. Changing the MTU of the interface is disruptive to PTP, and remains so as long as. This is not fixed by this series (too invasive at the moment.)
5. Avoid exporting functions that aren't used...
6. Avoid unnecessary runtime PM state manipulations (no point manipulating this when MTU changes).
7. Make the PTP/timestamping initialisation more readable - no point calling functions in the same file from one callsite that return error codes from one location in the called function, to only have the sole callee print messages depending on that return code. Also simplifying the mess in
stmmac_hw_setup(). Also placing support checks in a better location. Also getting rid of the "ptp_register" boolean through this restructuring.Not tested beyond compile testing. (I don't have my Jetson Xavier NX platform.) So anyone testing this and providing feedback would be most welcome.
On that point... I hardly (never?) seem to get testing feedback from anyone when touching stmmac. I suspect that's because of the structure of the driver, where
MAINTAINERSonly lists people for their appropriate dwmac-* files. Thus they don't get Cc'd for core stmmac changes. Not sure what the solution is, but manually picking out all the entries inMAINTAINERSevery time doesn't scale.
Quoted 8 lines
Therefore, I suggest merging this into net-next so people get to test it by way of it being in a tree they might be using.
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 - drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 113 ++++++++++++---------- drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 10 +- 3 files changed, 67 insertions(+), 57 deletions(-)
Tried on the stm32mp135f-dk board and was able to run ptp4l with coherent timestamps, so: