Re: [PATCH net-next 2/9] net: fec: Don't return early on error in .remove()

[Andrew Lunn <andrew@lunn.ch>]

On Mon, Mar 13, 2023 at 11:36:46AM +0100, Uwe Kleine-König wrote:
> If waking up the device in .remove() fails, exiting early results in
> strange state: The platform device will be unbound but not all resources
> are freed. E.g. the network device continues to exist without an parent.
> 
> Instead of an early error return, only skip the cleanup that was already
> done by suspend and release the remaining resources.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index c73e25f8995e..31d1dc5e9196 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -4465,15 +4465,13 @@ fec_drv_remove(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	int ret;
>  
> -	ret = pm_runtime_resume_and_get(&pdev->dev);
> -	if (ret < 0)
> -		return ret;
> +	ret = pm_runtime_get_sync(&pdev->dev);
>  
>  	cancel_work_sync(&fep->tx_timeout_work);
>  	fec_ptp_stop(pdev);
>  	unregister_netdev(ndev);
>  	fec_enet_mii_remove(fep);
> -	if (fep->reg_phy)
> +	if (ret >= 0 && fep->reg_phy)
>  		regulator_disable(fep->reg_phy);
>  
>  	if (of_phy_is_fixed_link(np))

I'm not sure this is correct. My experience with the FEC is that if
the device is run time suspended, access to the hardware does not
work. In the case i was debugging, MDIO bus reads/writes time out. I
think IO reads and writes turn into NOPs, but i don't actually know.

So if pm_runtime_resume_and_get() fails, fec_ptp_stop() probably does
not work if it touches the hardware. I guess fec_enet_mii_remove()
unregisters any PHYs, which could cause MDIO bus access to shut down
the PHYs, so i expect that also does not work. regulator_disable()
probably does actually work because that is a different hardware block
unaffected by the suspend.

So i think you need to decide:

exit immediately if resume fails, leaving dangling PHYs, netdev,
regulator etc

Keep going, but maybe everything is going to grind to a halt soon
afterwards when accessing the hardware.

You seem to prefer keep going, so i would also suggest you disable the
regulator.

	   Andrew