From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suman Anna Subject: Re: [PATCH] ARM: OMAP2+: hwmod: fix _idle() hwmod state sanity check sequence Date: Sun, 10 Apr 2016 16:13:14 -0500 Message-ID: <570AC1EA.3030501@ti.com> References: <1459812638-38789-1-git-send-email-s-anna@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Paul Walmsley Cc: Tony Lindgren , Tero Kristo , Lokesh Vutla , linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-Id: linux-omap@vger.kernel.org Hi Paul, On 04/10/2016 01:42 PM, Paul Walmsley wrote: > Hi Suman > > On Mon, 4 Apr 2016, Suman Anna wrote: > >> The omap_hwmod _enable() function can return success without setting >> the hwmod state to _HWMOD_STATE_ENABLED for IPs with reset lines when >> all of the reset lines are asserted. The omap_hwmod _idle() function >> also performs a similar check, but after checking for the hwmod state >> first. This triggers the WARN when pm_runtime_get and pm_runtime_put >> are invoked on IPs with all reset lines asserted. Reverse the checks >> for hwmod state and reset lines status to fix this. >> >> Issue found during a unbind operation on a device with reset lines >> still asserted, example backtrace below >> >> ------------[ cut here ]------------ >> WARNING: CPU: 1 PID: 879 at arch/arm/mach-omap2/omap_hwmod.c:2207 _idle+0x1e4/0x240() >> omap_hwmod: mmu_dsp: idle state can only be entered from enabled state >> Modules linked in: >> CPU: 1 PID: 879 Comm: sh Not tainted 4.4.0-00008-ga989d951331a #3 >> Hardware name: Generic OMAP5 (Flattened Device Tree) >> [] (unwind_backtrace) from [] (show_stack+0x10/0x14) >> [] (show_stack) from [] (dump_stack+0x90/0xc0) >> [] (dump_stack) from [] (warn_slowpath_common+0x78/0xb4) >> [] (warn_slowpath_common) from [] (warn_slowpath_fmt+0x30/0x40) >> [] (warn_slowpath_fmt) from [] (_idle+0x1e4/0x240) >> [] (_idle) from [] (omap_hwmod_idle+0x28/0x48) >> [] (omap_hwmod_idle) from [] (omap_device_idle+0x3c/0x90) >> [] (omap_device_idle) from [] (__rpm_callback+0x2c/0x60) >> [] (__rpm_callback) from [] (rpm_callback+0x20/0x80) >> [] (rpm_callback) from [] (rpm_suspend+0x138/0x74c) >> [] (rpm_suspend) from [] (__pm_runtime_idle+0x78/0xa8) >> [] (__pm_runtime_idle) from [] (__device_release_driver+0x64/0x100) >> [] (__device_release_driver) from [] (device_release_driver+0x20/0x2c) >> [] (device_release_driver) from [] (unbind_store+0x78/0xf8) >> [] (unbind_store) from [] (kernfs_fop_write+0xc0/0x1c4) >> [] (kernfs_fop_write) from [] (__vfs_write+0x20/0xdc) >> [] (__vfs_write) from [] (vfs_write+0x90/0x164) >> [] (vfs_write) from [] (SyS_write+0x44/0x9c) >> [] (SyS_write) from [] (ret_fast_syscall+0x0/0x1c) >> ---[ end trace a4182013c75a9f50 ]--- >> >> While at this, fix the sequence in _shutdown() as well, though there >> is no easy reproducible scenario. >> >> Fixes: 747834ab8347 ("ARM: OMAP2+: hwmod: revise hardreset behavior") >> Signed-off-by: Suman Anna > > Thanks, I'm going to queue this for v4.7, mostly to get in more testing > and integration time. Let me know if you think it's important enough to > go into v4.6-rc. Nope, not essential for the -rc cycle. I am fine with it getting queued for v4.7. > > >> There are couple of pr_debug statements that only print once the code >> flow passes through certain conditions. Not sure if you wanted them >> to be printed first thing when entering the functions or only if the >> function is really doing something. But it is still not consistent >> between _enable(), _idle() and _shutdown(). > > Yeah those need to be cleaned up. Seems like the best thing to do > is to move them all to the beginning of their respective functions. You want that fixed in this patch or a separate patch? regards Suman From mboxrd@z Thu Jan 1 00:00:00 1970 From: s-anna@ti.com (Suman Anna) Date: Sun, 10 Apr 2016 16:13:14 -0500 Subject: [PATCH] ARM: OMAP2+: hwmod: fix _idle() hwmod state sanity check sequence In-Reply-To: References: <1459812638-38789-1-git-send-email-s-anna@ti.com> Message-ID: <570AC1EA.3030501@ti.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Paul, On 04/10/2016 01:42 PM, Paul Walmsley wrote: > Hi Suman > > On Mon, 4 Apr 2016, Suman Anna wrote: > >> The omap_hwmod _enable() function can return success without setting >> the hwmod state to _HWMOD_STATE_ENABLED for IPs with reset lines when >> all of the reset lines are asserted. The omap_hwmod _idle() function >> also performs a similar check, but after checking for the hwmod state >> first. This triggers the WARN when pm_runtime_get and pm_runtime_put >> are invoked on IPs with all reset lines asserted. Reverse the checks >> for hwmod state and reset lines status to fix this. >> >> Issue found during a unbind operation on a device with reset lines >> still asserted, example backtrace below >> >> ------------[ cut here ]------------ >> WARNING: CPU: 1 PID: 879 at arch/arm/mach-omap2/omap_hwmod.c:2207 _idle+0x1e4/0x240() >> omap_hwmod: mmu_dsp: idle state can only be entered from enabled state >> Modules linked in: >> CPU: 1 PID: 879 Comm: sh Not tainted 4.4.0-00008-ga989d951331a #3 >> Hardware name: Generic OMAP5 (Flattened Device Tree) >> [] (unwind_backtrace) from [] (show_stack+0x10/0x14) >> [] (show_stack) from [] (dump_stack+0x90/0xc0) >> [] (dump_stack) from [] (warn_slowpath_common+0x78/0xb4) >> [] (warn_slowpath_common) from [] (warn_slowpath_fmt+0x30/0x40) >> [] (warn_slowpath_fmt) from [] (_idle+0x1e4/0x240) >> [] (_idle) from [] (omap_hwmod_idle+0x28/0x48) >> [] (omap_hwmod_idle) from [] (omap_device_idle+0x3c/0x90) >> [] (omap_device_idle) from [] (__rpm_callback+0x2c/0x60) >> [] (__rpm_callback) from [] (rpm_callback+0x20/0x80) >> [] (rpm_callback) from [] (rpm_suspend+0x138/0x74c) >> [] (rpm_suspend) from [] (__pm_runtime_idle+0x78/0xa8) >> [] (__pm_runtime_idle) from [] (__device_release_driver+0x64/0x100) >> [] (__device_release_driver) from [] (device_release_driver+0x20/0x2c) >> [] (device_release_driver) from [] (unbind_store+0x78/0xf8) >> [] (unbind_store) from [] (kernfs_fop_write+0xc0/0x1c4) >> [] (kernfs_fop_write) from [] (__vfs_write+0x20/0xdc) >> [] (__vfs_write) from [] (vfs_write+0x90/0x164) >> [] (vfs_write) from [] (SyS_write+0x44/0x9c) >> [] (SyS_write) from [] (ret_fast_syscall+0x0/0x1c) >> ---[ end trace a4182013c75a9f50 ]--- >> >> While at this, fix the sequence in _shutdown() as well, though there >> is no easy reproducible scenario. >> >> Fixes: 747834ab8347 ("ARM: OMAP2+: hwmod: revise hardreset behavior") >> Signed-off-by: Suman Anna > > Thanks, I'm going to queue this for v4.7, mostly to get in more testing > and integration time. Let me know if you think it's important enough to > go into v4.6-rc. Nope, not essential for the -rc cycle. I am fine with it getting queued for v4.7. > > >> There are couple of pr_debug statements that only print once the code >> flow passes through certain conditions. Not sure if you wanted them >> to be printed first thing when entering the functions or only if the >> function is really doing something. But it is still not consistent >> between _enable(), _idle() and _shutdown(). > > Yeah those need to be cleaned up. Seems like the best thing to do > is to move them all to the beginning of their respective functions. You want that fixed in this patch or a separate patch? regards Suman