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

Hello Andrew,

On Mon, Mar 13, 2023 at 04:07:12PM +0100, Andrew Lunn wrote:
> 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.

While creating the patch I checked, and just from looking at the code
I'd say it doesn't access 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.

fec_enet_mii_remove only calls mdiobus_unregister + mdiobus_free which
should be software only?!

> regulator_disable() probably does actually work because that is a
> different hardware block unaffected by the suspend.

fec_suspend() calls

        if (fep->reg_phy && !(fep->wol_flag & FEC_WOL_FLAG_ENABLE))
                regulator_disable(fep->reg_phy);

and if resume failed I have to assume that fec_resume() didn't come
around reenabling it. So not disabling the regulator in .remove() is
correct.

> So i think you need to decide:
> 
> exit immediately if resume fails, leaving dangling PHYs, netdev,
> regulator etc

I think keeping netdev is very prone to surprises. You'd still have eth0
(or however your device is called), it might even work somewhat, or it
might oops because devm_platform_ioremap_resource is undone.
 
> 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.

(Described above why I didn't.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)