From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Date: Mon, 17 Mar 2014 13:54:54 +0000 Subject: Re: [PATCH] sh_eth: ensure pm_runtime cannot suspend the device during init Message-Id: <1639014.E1tSH8nlne@avalon> 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 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. 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 ? -- Regards, Laurent Pinchart