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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e84585f2-e3d9-4a87-bfd4-a9ba458553b9@lunn.ch>
Date:   Mon, 13 Mar 2023 16:07:12 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Cc:     Wei Fang <wei.fang@....com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Shenwei Wang <shenwei.wang@....com>,
        Clark Wang <xiaoning.wang@....com>,
        NXP Linux Team <linux-imx@....com>, netdev@...r.kernel.org,
        kernel@...gutronix.de
Subject: Re: [PATCH net-next 2/9] net: fec: Don't return early on error in
 .remove()

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@...gutronix.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