Re: [PATCH net 3/3] net: stmmac: close the interface after failed hardware resume

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi,

On 9/21/26 18:54, James Hilliard wrote:
> System suspend disables NAPI and suspends phylink but retains the IRQs
> and DMA resources. If hardware setup fails during resume, the interface
> remains administratively up with NAPI still disabled. Closing it later
> calls napi_disable() a second time and can hang indefinitely.
> 
> On hardware-setup failure, stop DMA and disable the MAC, then release the
> suspended data path without repeating NAPI disable. Stop phylink directly
> from its suspended state rather than restarting the link on hardware
> which failed to resume. Drop the driver mutex before teardown, retaining
> RTNL across cleanup and network-core close.
> 
> Close the netdev to detach the PHY, release its runtime-PM reference and
> clear its administrative state. Reattach the now-down netdev so a later
> open can allocate new resources and retry. Preserve the original resume
> error and leave successful resume unchanged.

So this changes the admin state of the interface upon resume failure ?

using netif_close() in drivers is not common at all, only 3 drivers do that,
and among that, fbnic does that for the selftests...

Let's instead focus on making the state coherent.

Before entering suspend, interface is up and running. suspend happens, then
resume, and something goes wrong. As you mention, the next logical step as
the interface is now dead is to bring it down, and freshly set it back up.

I'd say, either the __stmmac_open / __stmmac_release are not at the right 
granularity, or there's an ordering issue, but we should focus on making
sure down'ing the interface after such a failure puts it back in a clean
state. I'm not really convinced with the boolean to the release indicating
if something's already cleaned or not.

Maxime> 
> Fixes: 6896c2449a18 ("net: stmmac: Check stmmac_hw_setup() in stmmac_resume()")
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 29 ++++++++++++++++-------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 44b59d0f56b5..87cb0978ed6a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4245,7 +4245,7 @@ static int stmmac_open(struct net_device *dev)
>  	return ret;
>  }
>  
> -static void __stmmac_release(struct net_device *dev)
> +static void __stmmac_release(struct net_device *dev, bool napi_disabled)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	u8 chan;
> @@ -4258,7 +4258,9 @@ static void __stmmac_release(struct net_device *dev)
>  	/* Stop and disconnect the PHY */
>  	phylink_stop(priv->phylink);
>  
> -	stmmac_disable_all_queues(priv);
> +	/* Suspend has already disabled NAPI when hardware resume fails. */
> +	if (!napi_disabled)
> +		stmmac_disable_all_queues(priv);
>  
>  	for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++)
>  		hrtimer_cancel(&priv->dma_conf.tx_queue[chan].txtimer);
> @@ -4297,7 +4299,7 @@ static int stmmac_release(struct net_device *dev)
>  	if (device_may_wakeup(priv->device))
>  		phylink_speed_down(priv->phylink, false);
>  
> -	__stmmac_release(dev);
> +	__stmmac_release(dev, false);
>  
>  	stmmac_legacy_serdes_power_down(priv);
>  	phylink_disconnect_phy(priv->phylink);
> @@ -6174,7 +6176,7 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
>  			return PTR_ERR(dma_conf);
>  		}
>  
> -		__stmmac_release(dev);
> +		__stmmac_release(dev, false);
>  
>  		ret = __stmmac_open(dev, dma_conf);
>  		if (ret) {
> @@ -8356,10 +8358,7 @@ 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_stop_dma;
>  	}
>  
>  	stmmac_init_timestamping(priv);
> @@ -8386,6 +8385,20 @@ int stmmac_resume(struct device *dev)
>  	netif_device_attach(ndev);
>  
>  	return 0;
> +
> +error_stop_dma:
> +	stmmac_stop_all_dma(priv);
> +	stmmac_mac_set(priv, priv->ioaddr, false);
> +	mutex_unlock(&priv->lock);
> +	/* Release the suspended data path before ndo_stop(), which must not
> +	 * disable NAPI or free these resources a second time.
> +	 */
> +	__stmmac_release(ndev, true);
> +	netif_close(ndev);
> +	netif_device_attach(ndev);
> +	rtnl_unlock();
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(stmmac_resume);
>  
>