On 1/4/21 10:43 AM, 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); >> > > 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. > > This is exactly why I do not understand why some drivers insist on > calling of_phy_connect() and its variants on .ndo_open() and not while > probing the device - you can access the debug stats only if the > interface was started. Doing the connect in ndo_open() allows you to keep the PHY in whatever the state it was prior to the kernel managing it, which if everything is correctly designed means in a low power state. Your Ethernet driver's probe function may be called on boot and you may never use the network device at all, so it is a waste of energy to power on the PHY, have it potentially link with its link partner while you still have no chance of doing any configuration to it because you have not brought up the network interface. -- Florian
