From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Dooks Date: Mon, 17 Mar 2014 14:01:02 +0000 Subject: Re: [PATCH] sh_eth: ensure pm_runtime cannot suspend the device during init Message-Id: <5327001E.7000803@codethink.co.uk> List-Id: References: <1394823605-31883-1-git-send-email-ben.dooks@codethink.co.uk> In-Reply-To: <1394823605-31883-1-git-send-email-ben.dooks@codethink.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-sh@vger.kernel.org On 17/03/14 13:54, Laurent Pinchart wrote: > Hi Ben, > > On Monday 17 March 2014 13:44:02 Ben Dooks wrote: >> On 17/03/14 13:38, Geert Uytterhoeven wrote: >>> On Mon, Mar 17, 2014 at 2:36 PM, Ben Dooks wrote: >>>> I have yet to ascertain how this ends up happening with device probe, >>>> it seems to be very dependant on the code. >>> >>> Adding a WARN() in cpg_mstp_clock_endisable(): >>> >>> [] (cpg_mstp_clock_endisable) from [] >>> (cpg_mstp_clock_disable+0x14/0x18) >>> r10:eec7a000 r9:00000003 r8:eed41508 r7:00000001 r6:a0000113 r5:eec23a00 >>> r4:eec23a00 >>> [] (cpg_mstp_clock_disable) from [] >>> (__clk_disable+0x68/0x78) >>> [] (__clk_disable) from [] (clk_disable+0x20/0x2c) >>> r4:60000193 r3:00000002 >>> [] (clk_disable) from [] (pm_clk_suspend+0x54/0x78) >>> r5:eed41500 r4:eed414c0 >>> [] (pm_clk_suspend) from [] (__rpm_callback+0x4c/0x7c) >>> r8:0000000c r7:00000004 r6:00000000 r5:eed42810 r4:eed42810 r3:c01fcfac >>> [] (__rpm_callback) from [] (rpm_callback+0x50/0x84) >>> r5:eed42810 r4:eec7a000 >>> [] (rpm_callback) from [] (rpm_suspend+0x214/0x3e4) >>> r6:00000000 r5:00000000 r4:eed42810 r3:eed42810 >>> [] (rpm_suspend) from [] (rpm_idle+0x184/0x19c) >>> r10:00000010 r9:00000000 r8:ee4710bc r7:00000004 r6:00000004 r5:00000000 >>> r4:eed42810 >>> [] (rpm_idle) from [] (__pm_runtime_idle+0x64/0x7c) >>> r6:60000113 r5:eed42870 r4:eed42810 r3:eed42870 >>> [] (__pm_runtime_idle) from [] >>> (sh_eth_get_stats+0x228/0x23c) >>> r7:ee471000 r6:c0356bd4 r5:ee428000 r4:ee428000 >>> [] (sh_eth_get_stats) from [] >>> (dev_get_stats+0x5c/0x84) >>> r4:eec7bc30 r3:c023ec68 >>> [] (dev_get_stats) from [] >>> (rtnl_fill_ifinfo+0x410/0x8dc) >>> r6:00000000 r5:ee428000 r4:eed76ec0 r3:00000684 >>> [] (rtnl_fill_ifinfo) from [] (rtmsg_ifinfo+0x6c/0xd4) >>> r10:00000010 r9:ee428510 r8:ffffffff r7:eed76ec0 r6:ee428000 r5:000000d0 >>> r4:00000000 >>> [] (rtmsg_ifinfo) from [] >>> (register_netdevice+0x3dc/0x428)> >>> r10:00000000 r8:eed42810 r7:ee428010 r6:ee428000 r5:c047f600 r4:00000000 >>> [] (register_netdevice) from [] >>> (register_netdev+0x1c/0x2c)> >>> r7:eed42800 r6:ef203978 r5:ee46e790 r4:ee428000 >>> [] (register_netdev) from [] >>> (sh_eth_drv_probe+0x994/0xb7c) >>> r4:ee428000 r3:00000040 >>> [] (sh_eth_drv_probe) from [] >>> (platform_drv_probe+0x20/0x50) >>> r10:c042b548 r9:c045c544 r8:00000065 r7:c047d830 r6:00000000 r5:c047d830 >>> r4:eed42810 > > [snip] > >> this explains it, the call to stats causes a get_sync/put_sync which >> puts the device into a state where it /could/ be suspended and thus >> does get suspended in this case from the pm code. >> >> I'm not /sure/ why the pm_runtime code is not protecting against >> running this when a device probe is in progress, but it seems the >> best thing is to ensure that we always do a get/put sync in the >> driver to ensure we have a reference during probe. > > Wouldn't it be better to register the MDIO bus before registering the network > device ? It looks like the current order is prone to race conditions. Not sure, requires input? > Another potential issue, does the network layer guarantee that the device will > always be opened by an .ndo_open() call before performing any MDIO operation ? > sh_mmd_ctrl, sh_set_mdio and sh_get_mdio access sh-eth registers, could we be > missing runtime PM calls in those call paths ? I'm not sure if there is any guarantee, I don't think the PHY driver does any sort of open on probe. I do have a patch to ensure that the MDIO operations are wrapped with pm_runtime_{get,put}_sync() calls as the probe of the PHY often fails without it. -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius