From mboxrd@z Thu Jan 1 00:00:00 1970 From: linux@arm.linux.org.uk (Russell King - ARM Linux) Date: Tue, 5 Aug 2014 10:21:07 +0100 Subject: Suspend/resume broken on mx5/mx6 running 4.16 In-Reply-To: <20140805061658.GG2167@dragon> References: <9930f46074e04c90a58fa6b01e055ff2@BLUPR03MB373.namprd03.prod.outlook.com> <20140805061658.GG2167@dragon> Message-ID: <20140805092107.GM30282@n2100.arm.linux.org.uk> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Aug 05, 2014 at 02:16:59PM +0800, Shawn Guo wrote: > On Tue, Aug 05, 2014 at 02:06:18PM +0800, Duan Fugang-B38611 wrote: > > I use linux net tree cannot reproduce the issue like your log, but show another issue as below log: > > (imx6dl sabresd board, Nfs mount rootfs) > > > > root at freescale ~$ uname -r > > 3.16.0-rc5-01146-gda388973d > > Fabio already bisect the issue down to commit a71e3c37960c (net: phy: > Set the driver when registering an MDIO bus device). This commit > landed on mainline after v3.16-rc7, and that may be the reason you > do not see it on net tree (3.16.0-rc5-01146-gda388973d). Oh my... that commit looks totally bogus. It has the effect (as can be seen from the oops) of attaching the MDIO bus device (itself is a bus-less device) to the platform driver, which means that if the platform driver supports power management, it will be called to power manage the MDIO bus device. Moreover, drivers do not expect to be called for power management operations for devices which they haven't probed, and certainly not for devices which aren't part of the same bus that the driver is registered against. The commit text says: net: phy: Set the driver when registering an MDIO bus device mdiobus_register() registers a device which is already bound to a driver. Hence, the driver pointer should be set properly in order to track down the driver associated to the MDIO bus. This will be used to allow ethernet driver to pin down a MDIO bus driver, preventing it from being unloaded while the PHY device is running. which misses the implications of adding an unknown parent driver to that class device - and the argument that it's just to track down the parent driver is totally bogus. That can already be done - it's the parent device's driver pointer. Let's take the example of FEC. /sys/devices/soc0/soc/2100000.aips-bus/2188000.ethernet/mdio_bus/2188000.ethernet/ lrwxrwxrwx 1 root root 0 Aug 5 10:07 device -> ../../../2188000.ethernet drwxr-xr-x 2 root root 0 Aug 5 10:07 power lrwxrwxrwx 1 root root 0 Aug 5 10:07 subsystem -> ../../../../../../../class/mdio_bus -rw-r--r-- 1 root root 4096 Aug 5 10:07 uevent (note that the "%s-%x" format for this device in fec_main.c has been truncated - that's another bug!) Remembering that this is a class device, these devices have a "device" symlink which point at the parent device. Normal devices which are bound to a driver have a "driver" symlink. So, the driver can be reached by following the "device" pointer, and then following the "driver" symlink: /sys/devices/soc0/soc/2100000.aips-bus/2188000.ethernet/mdio_bus/2188000.ethernet/device/ ... lrwxrwxrwx 1 root root 0 Aug 5 10:08 driver -> ../../../../../bus/platform/drivers/fec .. So, given that there are already perfectly good ways to discover the information stated in the commit message, and that this commit causes regression, I think this commit should be reverted. Greg, do you concur? -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net.