Re: [PATCH] [RFC] net: phy: Fix reboot crash if CONFIG_IP_PNP is not set

[Geert Uytterhoeven]

	Hi Ioana, Andrew,

On Mon, 4 Jan 2021, Ioana Ciornei wrote:
> On Mon, Jan 04, 2021 at 06:31:05PM +0100, Andrew Lunn wrote:
> > > The basic rules here should be, if the MDIO bus is registered, it is
> > > usable. There are things like PHY statistics, HWMON temperature
> > > sensors, etc, DSA switches, all which have a life cycle separate to
> > > the interface being up.
> >
> > [Goes and looks at the code]
> >
> > Yes, this is runtime PM which is broken.
> >
> > sh_mdio_init() needs to wrap the mdp->mii_bus->read and
> > mdp->mii_bus->write calls with calls to
> >
> > pm_runtime_get_sync(&mdp->pdev->dev);
> >
> > and
> >
> > pm_runtime_put_sync(&mdp->pdev->dev);

pm_runtime_put().

Thanks, that works (see patch below), but I'm still wondering if that is
the right fix...

> Agree. Thanks for actually looking into it.. I'm not really well versed
> in runtime PM.
>
> > The KSZ8041RNLI supports statistics, which ethtool --phy-stats can
> > read, and these will also going to cause problems.
>
> Not really, this driver connects to the PHY on .ndo_open(), thus any
> try to actually dump the PHY statistics before an ifconfig up would get
> an -EOPNOTSUPP since the dev->phydev is not yet populated.

I added a statically-linked ethtool binary to my initramfs, and can
confirm that retrieving the PHY statistics does not access the PHY
registers when the device is suspended:

    # ethtool --phy-statistics eth0
    no stats available
    # ifconfig eth0 up
    # ethtool --phy-statistics eth0
    PHY statistics:
	 phy_receive_errors: 0
	 phy_idle_errors: 0
    #

In the past, we've gone to great lengths to avoid accessing the PHY
registers when the device is suspended, usually in the statistics
handling (see e.g. [1][2]).  Hence I'm wondering if we should do the
same here, and handle this at a higher layer than the individual network
device driver (other drivers than sh_eth may be affected, too)?

Thanks!

[1] 124eee3f6955f7aa ("net: linkwatch: add check for netdevice being present to linkwatch_do_dev")
[2] https://lore.kernel.org/netdev/11beeaa9-57d5-e641-9486-f2ba202d0998@xxxxxxxxx/