diff mbox

spi: qup: skip clk_disable_unprepare if the device is already runtime suspended

Message ID 1472128408-7231-1-git-send-email-sudeep.holla@arm.com (mailing list archive)
State Accepted
Commit 9d04d8bc4c18765f6a1f7b632fffe47b4578fb26
Headers show

Commit Message

Sudeep Holla Aug. 25, 2016, 12:33 p.m. UTC
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(-)

Comments

Andy Gross Aug. 25, 2016, 1:26 p.m. UTC | #1
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-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudeep Holla Aug. 25, 2016, 1:27 p.m. UTC | #2
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.
Mark Brown Sept. 1, 2016, 8:29 p.m. UTC | #3
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.
Sudeep Holla Sept. 2, 2016, 8:42 a.m. UTC | #4
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.
Mark Brown Sept. 2, 2016, 9:38 a.m. UTC | #5
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.
Sudeep Holla Sept. 2, 2016, 10:45 a.m. UTC | #6
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 mbox

Patch

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;
 }