| Message ID | 1472128408-7231-1-git-send-email-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Thu, Aug 25, 2016 at 01:33:28PM +0100, Sudeep Holla wrote: > If the spi device is already runtime suspended, if spi_qup_suspend is > executed during suspend-to-idle or suspend-to-ram it will result in the > following splat: > > WARNING: CPU: 3 PID: 1593 at drivers/clk/clk.c:476 clk_core_unprepare+0x80/0x90 > Modules linked in: > <snip> Thanks for fixing this. I had noticed this yesterday when I was testing your freeze patch but hadn't had time to dig in. Tested-by: Andy Gross <andy.gross@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andy, On 25/08/16 14:26, Andy Gross wrote: > On Thu, Aug 25, 2016 at 01:33:28PM +0100, Sudeep Holla wrote: >> If the spi device is already runtime suspended, if spi_qup_suspend is >> executed during suspend-to-idle or suspend-to-ram it will result in the >> following splat: >> >> WARNING: CPU: 3 PID: 1593 at drivers/clk/clk.c:476 clk_core_unprepare+0x80/0x90 >> Modules linked in: >> > > <snip> > > Thanks for fixing this. I had noticed this yesterday when I was testing your > freeze patch but hadn't had time to dig in. > Yes, even I tested the same once I got PSCI firmware :) > > Tested-by: Andy Gross <andy.gross@linaro.org> > Thanks.
On Thu, Aug 25, 2016 at 01:33:28PM +0100, Sudeep Holla wrote: > If the spi device is already runtime suspended, if spi_qup_suspend is > executed during suspend-to-idle or suspend-to-ram it will result in the > following splat: > > WARNING: CPU: 3 PID: 1593 at drivers/clk/clk.c:476 clk_core_unprepare+0x80/0x90 > Modules linked in: > > CPU: 3 PID: 1593 Comm: bash Tainted: G W 4.8.0-rc3 #14 > Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) > PC is at clk_core_unprepare+0x80/0x90 > LR is at clk_unprepare+0x28/0x40 > pc : [<ffff0000086eecf0>] lr : [<ffff0000086f0c58>] pstate: 60000145 Please think hard before including complete backtraces in upstream reports, they are very large and contain almost no useful information relative to their size so often obscure the relevant content in your message. If part of the backtrace is usefully illustrative then it's usually better to pull out the relevant sections.
Hi, On 01/09/16 21:29, Mark Brown wrote: > On Thu, Aug 25, 2016 at 01:33:28PM +0100, Sudeep Holla wrote: >> If the spi device is already runtime suspended, if spi_qup_suspend is >> executed during suspend-to-idle or suspend-to-ram it will result in the >> following splat: >> >> WARNING: CPU: 3 PID: 1593 at drivers/clk/clk.c:476 clk_core_unprepare+0x80/0x90 >> Modules linked in: >> >> CPU: 3 PID: 1593 Comm: bash Tainted: G W 4.8.0-rc3 #14 >> Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) >> PC is at clk_core_unprepare+0x80/0x90 >> LR is at clk_unprepare+0x28/0x40 >> pc : [<ffff0000086eecf0>] lr : [<ffff0000086f0c58>] pstate: 60000145 > > Please think hard before including complete backtraces in upstream > reports, they are very large and contain almost no useful information > relative to their size so often obscure the relevant content in your > message. If part of the backtrace is usefully illustrative then it's > usually better to pull out the relevant sections. > I removed most of the addresses and just retained the symbols(somehow the last line with pc and lr was left unintentionally). While you may have the above opinion, other maintainers may differ. In future, I will try to add it as a note just to describe the issue.
On Fri, Sep 02, 2016 at 09:42:04AM +0100, Sudeep Holla wrote: > On 01/09/16 21:29, Mark Brown wrote: > > On Thu, Aug 25, 2016 at 01:33:28PM +0100, Sudeep Holla wrote: > > > CPU: 3 PID: 1593 Comm: bash Tainted: G W 4.8.0-rc3 #14 > > > Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) > > > PC is at clk_core_unprepare+0x80/0x90 > > > LR is at clk_unprepare+0x28/0x40 > > > pc : [<ffff0000086eecf0>] lr : [<ffff0000086f0c58>] pstate: 60000145 > > Please think hard before including complete backtraces in upstream > > reports, they are very large and contain almost no useful information > > relative to their size so often obscure the relevant content in your > > message. If part of the backtrace is usefully illustrative then it's > > usually better to pull out the relevant sections. > I removed most of the addresses and just retained the symbols(somehow > the last line with pc and lr was left unintentionally). While you may > have the above opinion, other maintainers may differ. In future, I will > try to add it as a note just to describe the issue. Oh, *that's* why it looked so weird. Removing the addresses doesn't help here, the issue isn't that the addresses are confusing it's that you had a tiny commit message dwarfed by the backtrace preamble then a screenful of call stack which conveyed no meaningful information, including not just the entire callback path for a suspend (which doesn't tell us anything really, especially beyond the first frame) and going on to show the entire call stack from the sysfs write you used to trigger suspend which is even less relevant. This gives us 30 lines or so of splat (more than a screenful) for five lines of actual content with the important bit which describes what the change is supposed to be doing buried at the bottom. That's a really bad signal to noise ratio. What would've been better would be explaining why the change you are making fixes the problem.
On 02/09/16 10:38, Mark Brown wrote: > On Fri, Sep 02, 2016 at 09:42:04AM +0100, Sudeep Holla wrote: >> On 01/09/16 21:29, Mark Brown wrote: >>> On Thu, Aug 25, 2016 at 01:33:28PM +0100, Sudeep Holla wrote: > >>>> CPU: 3 PID: 1593 Comm: bash Tainted: G W 4.8.0-rc3 #14 >>>> Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) >>>> PC is at clk_core_unprepare+0x80/0x90 >>>> LR is at clk_unprepare+0x28/0x40 >>>> pc : [<ffff0000086eecf0>] lr : [<ffff0000086f0c58>] pstate: 60000145 > >>> Please think hard before including complete backtraces in upstream >>> reports, they are very large and contain almost no useful information >>> relative to their size so often obscure the relevant content in your >>> message. If part of the backtrace is usefully illustrative then it's >>> usually better to pull out the relevant sections. > >> I removed most of the addresses and just retained the symbols(somehow >> the last line with pc and lr was left unintentionally). While you may >> have the above opinion, other maintainers may differ. In future, I will >> try to add it as a note just to describe the issue. > > Oh, *that's* why it looked so weird. Removing the addresses doesn't > help here, the issue isn't that the addresses are confusing it's that > you had a tiny commit message dwarfed by the backtrace preamble then a > screenful of call stack which conveyed no meaningful information, > including not just the entire callback path for a suspend (which doesn't > tell us anything really, especially beyond the first frame) and going on > to show the entire call stack from the sysfs write you used to trigger > suspend which is even less relevant. > > This gives us 30 lines or so of splat (more than a screenful) for five > lines of actual content with the important bit which describes what the > change is supposed to be doing buried at the bottom. That's a really > bad signal to noise ratio. What would've been better would be > explaining why the change you are making fixes the problem. > Agreed.
diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c index c338ef1136f6..a047e9882da8 100644 --- a/drivers/spi/spi-qup.c +++ b/drivers/spi/spi-qup.c @@ -982,8 +982,10 @@ static int spi_qup_suspend(struct device *device) if (ret) return ret; - clk_disable_unprepare(controller->cclk); - clk_disable_unprepare(controller->iclk); + if (!pm_runtime_suspended(device)) { + clk_disable_unprepare(controller->cclk); + clk_disable_unprepare(controller->iclk); + } return 0; }
If the spi device is already runtime suspended, if spi_qup_suspend is executed during suspend-to-idle or suspend-to-ram it will result in the following splat: WARNING: CPU: 3 PID: 1593 at drivers/clk/clk.c:476 clk_core_unprepare+0x80/0x90 Modules linked in: CPU: 3 PID: 1593 Comm: bash Tainted: G W 4.8.0-rc3 #14 Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) PC is at clk_core_unprepare+0x80/0x90 LR is at clk_unprepare+0x28/0x40 pc : [<ffff0000086eecf0>] lr : [<ffff0000086f0c58>] pstate: 60000145 Call trace: clk_core_unprepare+0x80/0x90 spi_qup_suspend+0x68/0x90 platform_pm_suspend+0x24/0x68 dpm_run_callback.isra.7+0x1c/0x70 __device_suspend+0xf4/0x298 dpm_suspend+0x10c/0x228 dpm_suspend_start+0x68/0x78 suspend_devices_and_enter+0xb8/0x460 pm_suspend+0x1ec/0x240 state_store+0x84/0xf8 kobj_attr_store+0x14/0x28 sysfs_kf_write+0x48/0x58 kernfs_fop_write+0x15c/0x1f8 __vfs_write+0x1c/0x100 vfs_write+0x9c/0x1b8 SyS_write+0x44/0xa0 el0_svc_naked+0x24/0x28 This patch fixes the issue by executing clk_disable_unprepare conditionally in spi_qup_suspend. Cc: Andy Gross <andy.gross@linaro.org> Cc: David Brown <david.brown@linaro.org> Cc: Mark Brown <broonie@kernel.org> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/spi/spi-qup.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)