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

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

[Sudeep Holla]

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(-)

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

Re: [PATCH] spi: qup: skip clk_disable_unprepare if the device is already runtime suspended

[Andy Gross]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: qup: skip clk_disable_unprepare if the device is already runtime suspended

[Andy Gross]

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>

Re: [PATCH] spi: qup: skip clk_disable_unprepare if the device is already runtime suspended

[Sudeep Holla]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>

Thanks.

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: qup: skip clk_disable_unprepare if the device is already runtime suspended

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

Re: [PATCH] spi: qup: skip clk_disable_unprepare if the device is already runtime suspended

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 960 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Applied "spi: qup: skip clk_disable_unprepare if the device is already runtime suspended" to the spi tree

[Mark Brown]

The patch

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

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 9d04d8bc4c18765f6a1f7b632fffe47b4578fb26 Mon Sep 17 00:00:00 2001
From: Sudeep Holla <sudeep.holla@arm.com>
Date: Thu, 25 Aug 2016 13:33:28 +0100
Subject: [PATCH] spi: qup: skip clk_disable_unprepare if the device is already
 runtime suspended

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
a splat from unpreparing a non-prepared clock.

This patch fixes the issue by executing clk_disable_unprepare conditionally
in spi_qup_suspend.

[Reworded commit message to remove irrelevant backtrace -- broonie]
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Tested-by: Andy Gross <andy.gross@linaro.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-qup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

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

Applied "spi: qup: skip clk_disable_unprepare if the device is already runtime suspended" to the spi tree

[Mark Brown]

The patch

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

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 9d04d8bc4c18765f6a1f7b632fffe47b4578fb26 Mon Sep 17 00:00:00 2001
From: Sudeep Holla <sudeep.holla@arm.com>
Date: Thu, 25 Aug 2016 13:33:28 +0100
Subject: [PATCH] spi: qup: skip clk_disable_unprepare if the device is already
 runtime suspended

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
a splat from unpreparing a non-prepared clock.

This patch fixes the issue by executing clk_disable_unprepare conditionally
in spi_qup_suspend.

[Reworded commit message to remove irrelevant backtrace -- broonie]
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Tested-by: Andy Gross <andy.gross@linaro.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-qup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

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

Re: [PATCH] spi: qup: skip clk_disable_unprepare if the device is already runtime suspended

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

Re: [PATCH] spi: qup: skip clk_disable_unprepare if the device is already runtime suspended

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1954 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH] spi: qup: skip clk_disable_unprepare if the device is already runtime suspended

[Sudeep Holla]

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.

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: qup: skip clk_disable_unprepare if the device is already runtime suspended

[Sudeep Holla]

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.

-- 
Regards,
Sudeep