diff mbox series

[v3,5/6] spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops

Message ID 20220216014028.8123-6-jon.lin@rock-chips.com (mailing list archive)
State Accepted
Commit e882575efc771f130a24322377dc1033551da11d
Headers show
Series New support and problem adjustment of SPI rockchip | expand

Commit Message

Jon Lin Feb. 16, 2022, 1:40 a.m. UTC 
From: shengfei Xu <xsf@rock-chips.com>

the wakeup interrupt handler which is guaranteed not to run while
@resume noirq() is being executed. the patch can help to avoid the
wakeup source try to access spi when the spi is in suspend mode.

Signed-off-by: shengfei Xu <xsf@rock-chips.com>
Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

Changes in v3: None
Changes in v2: None
Changes in v1: None

 drivers/spi/spi-rockchip.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Ondřej Jirman June 21, 2022, 3:42 p.m. UTC | #1 
Hello Jon,

On Wed, Feb 16, 2022 at 09:40:27AM +0800, Jon Lin wrote:
> From: shengfei Xu <xsf@rock-chips.com>
> 
> the wakeup interrupt handler which is guaranteed not to run while
> @resume noirq() is being executed. the patch can help to avoid the
> wakeup source try to access spi when the spi is in suspend mode.

This patch causes oops on suspend every single time, because it tries to disable
already disabled clocks (one disable in runtime PM suspend, and other one in
system suspend). It also fails to properly handle errors from clk_prepare_enable
in rockchip_spi_resume, potentially causing even more clock enable/disable
imballance issues.

Please send a revert and figure out a better fix for the original issue.

kind regards,
	Ondrej

------------[ cut here ]------------
clk_spi1 already unprepared
WARNING: CPU: 5 PID: 423 at drivers/clk/clk.c:829 clk_core_unprepare+0x1a0/0x1c0
Modules linked in:
CPU: 5 PID: 423 Comm: waketest-onesho Tainted: G        W         5.18.5-00013-g12be33cc5bf1-dirty #142
Hardware name: Pine64 PinePhonePro (DT)
pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : clk_core_unprepare+0x1a0/0x1c0
lr : clk_core_unprepare+0x1a0/0x1c0
sp : ffff80000acf3a30
x29: ffff80000acf3a30 x28: 0000000000000000 x27: 0000000bbb2b9e2b
x26: 0000000000000002 x25: ffff8000088310b0 x24: 0000000000000000
x23: ffff800009fe2000 x22: 0000000000000002 x21: ffff0000044fe980
x20: ffff0000042d2600 x19: ffff00000269da00 x18: ffffffffffffffff
x17: 000000040044ffff x16: 00000034b5503510 x15: 0000000000000000
x14: ffff0000027b0d80 x13: 0a64657261706572 x12: 706e752079646165
x11: 0000000000000001 x10: fffffffffffe0000 x9 : ffff800008176438
x8 : ffff80000acf3a30 x7 : 61706572706e7520 x6 : 0000000000000001
x5 : ffff0000f77a4790 x4 : 0000000000000000 x3 : 0000000000000027
x2 : 0000000000000000 x1 : 0000000000000027 x0 : 000000000000001b
Call trace:
 clk_core_unprepare+0x1a0/0x1c0
 clk_unprepare+0x30/0x44
 rockchip_spi_suspend+0x40/0x70
 dpm_run_callback+0x5c/0x170
 __device_suspend_noirq+0xc8/0x2a0
 dpm_noirq_suspend_devices+0x118/0x2a0
 dpm_suspend_noirq+0x28/0x94
 suspend_devices_and_enter+0x184/0x840
 pm_suspend+0x374/0x470
 state_store+0x84/0x100
 kobj_attr_store+0x14/0x2c
 sysfs_kf_write+0x48/0x54
 kernfs_fop_write_iter+0x120/0x1e4
 new_sync_write+0xc0/0x12c
 vfs_write+0x210/0x2d0
 ksys_write+0x60/0xe0
 __arm64_sys_write+0x20/0x30
 invoke_syscall+0x4c/0x110
 el0_svc_common.constprop.0+0x48/0xf0
 do_el0_svc+0x28/0x80
 el0_svc+0x14/0x50
 el0t_64_sync_handler+0xe0/0x110
 el0t_64_sync+0x148/0x14c
---[ end trace 0000000000000000 ]---
------------[ cut here ]------------
pclk_spi1 already disabled
WARNING: CPU: 5 PID: 423 at drivers/clk/clk.c:971 clk_core_disable+0x1d8/0x1f0
Modules linked in:
CPU: 5 PID: 423 Comm: waketest-onesho Tainted: G        W         5.18.5-00013-g12be33cc5bf1-dirty #142
Hardware name: Pine64 PinePhonePro (DT)
pstate: 800000c5 (Nzcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : clk_core_disable+0x1d8/0x1f0
lr : clk_core_disable+0x1d8/0x1f0
sp : ffff80000acf3a20
x29: ffff80000acf3a20 x28: 0000000000000000 x27: 0000000bbb2b9e2b
x26: 0000000000000002 x25: ffff8000088310b0 x24: 0000000000000000
x23: ffff800009fe2000 x22: 0000000000000002 x21: ffff0000044fe980
x20: ffff000002687a00 x19: ffff000002687a00 x18: ffffffffffffffff
x17: 000000040044ffff x16: 00000034b5503510 x15: 0000000000000000
x14: ffff0000027b0d80 x13: 0a64656c62617369 x12: 642079646165726c
x11: 0000000000000001 x10: fffffffffffe0000 x9 : ffff800008176438
x8 : ffff80000acf3a20 x7 : 656c626173696420 x6 : 0000000000000001
x5 : ffff0000f77a4790 x4 : 0000000000000000 x3 : 0000000000000027
x2 : 0000000000000000 x1 : 0000000000000027 x0 : 000000000000001a
Call trace:
 clk_core_disable+0x1d8/0x1f0
 clk_disable+0x34/0x4c
 rockchip_spi_suspend+0x4c/0x70
 dpm_run_callback+0x5c/0x170
 __device_suspend_noirq+0xc8/0x2a0
 dpm_noirq_suspend_devices+0x118/0x2a0
 dpm_suspend_noirq+0x28/0x94
 suspend_devices_and_enter+0x184/0x840
 pm_suspend+0x374/0x470
 state_store+0x84/0x100
 kobj_attr_store+0x14/0x2c
 sysfs_kf_write+0x48/0x54
 kernfs_fop_write_iter+0x120/0x1e4
 new_sync_write+0xc0/0x12c
 vfs_write+0x210/0x2d0
 ksys_write+0x60/0xe0
 __arm64_sys_write+0x20/0x30
 invoke_syscall+0x4c/0x110
 el0_svc_common.constprop.0+0x48/0xf0
 do_el0_svc+0x28/0x80
 el0_svc+0x14/0x50
 el0t_64_sync_handler+0xe0/0x110
 el0t_64_sync+0x148/0x14c
---[ end trace 0000000000000000 ]---
------------[ cut here ]------------
pclk_spi1 already unprepared
WARNING: CPU: 5 PID: 423 at drivers/clk/clk.c:829 clk_core_unprepare+0x1a0/0x1c0
Modules linked in:
CPU: 5 PID: 423 Comm: waketest-onesho Tainted: G        W         5.18.5-00013-g12be33cc5bf1-dirty #142
Hardware name: Pine64 PinePhonePro (DT)
pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : clk_core_unprepare+0x1a0/0x1c0
lr : clk_core_unprepare+0x1a0/0x1c0
sp : ffff80000acf3a30
x29: ffff80000acf3a30 x28: 0000000000000000 x27: 0000000bbb2b9e2b
x26: 0000000000000002 x25: ffff8000088310b0 x24: 0000000000000000
x23: ffff800009fe2000 x22: 0000000000000002 x21: ffff0000044fe980
x20: ffff0000044fe900 x19: ffff000002687a00 x18: ffffffffffffffff
x17: 000000040044ffff x16: 00000034b5503510 x15: 0000000000000000
x14: ffff0000027b0d80 x13: 0a64657261706572 x12: 706e752079646165
x11: 0000000000000001 x10: fffffffffffe0000 x9 : ffff800008176438
x8 : ffff80000acf3a30 x7 : 61706572706e7520 x6 : 0000000000000001
x5 : ffff0000f77a4790 x4 : 0000000000000000 x3 : 0000000000000027
x2 : 0000000000000000 x1 : 0000000000000027 x0 : 000000000000001c
Call trace:
 clk_core_unprepare+0x1a0/0x1c0
 clk_unprepare+0x30/0x44
 rockchip_spi_suspend+0x54/0x70
 dpm_run_callback+0x5c/0x170
 __device_suspend_noirq+0xc8/0x2a0
 dpm_noirq_suspend_devices+0x118/0x2a0
 dpm_suspend_noirq+0x28/0x94
 suspend_devices_and_enter+0x184/0x840
 pm_suspend+0x374/0x470
 state_store+0x84/0x100
 kobj_attr_store+0x14/0x2c
 sysfs_kf_write+0x48/0x54
 kernfs_fop_write_iter+0x120/0x1e4
 new_sync_write+0xc0/0x12c
 vfs_write+0x210/0x2d0
 ksys_write+0x60/0xe0
 __arm64_sys_write+0x20/0x30
 invoke_syscall+0x4c/0x110
 el0_svc_common.constprop.0+0x48/0xf0
 do_el0_svc+0x28/0x80
 el0_svc+0x14/0x50
 el0t_64_sync_handler+0xe0/0x110
 el0t_64_sync+0x148/0x14c
---[ end trace 0000000000000000 ]---

> Signed-off-by: shengfei Xu <xsf@rock-chips.com>
> Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> Changes in v1: None
> 
>  drivers/spi/spi-rockchip.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index 83da8fdb3c02..8b4d56ee2193 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -964,14 +964,14 @@ static int rockchip_spi_suspend(struct device *dev)
>  {
>  	int ret;
>  	struct spi_controller *ctlr = dev_get_drvdata(dev);
> +	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
>  
>  	ret = spi_controller_suspend(ctlr);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = pm_runtime_force_suspend(dev);
> -	if (ret < 0)
> -		return ret;
> +	clk_disable_unprepare(rs->spiclk);
> +	clk_disable_unprepare(rs->apb_pclk);
>  
>  	pinctrl_pm_select_sleep_state(dev);
>  
> @@ -986,10 +986,14 @@ static int rockchip_spi_resume(struct device *dev)
>  
>  	pinctrl_pm_select_default_state(dev);
>  
> -	ret = pm_runtime_force_resume(dev);
> +	ret = clk_prepare_enable(rs->apb_pclk);
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = clk_prepare_enable(rs->spiclk);
> +	if (ret < 0)
> +		clk_disable_unprepare(rs->apb_pclk);
> +
>  	ret = spi_controller_resume(ctlr);
>  	if (ret < 0) {
>  		clk_disable_unprepare(rs->spiclk);
> @@ -1031,7 +1035,7 @@ static int rockchip_spi_runtime_resume(struct device *dev)
>  #endif /* CONFIG_PM */
>  
>  static const struct dev_pm_ops rockchip_spi_pm = {
> -	SET_SYSTEM_SLEEP_PM_OPS(rockchip_spi_suspend, rockchip_spi_resume)
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rockchip_spi_suspend, rockchip_spi_resume)
>  	SET_RUNTIME_PM_OPS(rockchip_spi_runtime_suspend,
>  			   rockchip_spi_runtime_resume, NULL)
>  };
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Brian Norris Aug. 23, 2024, 9:28 p.m. UTC | #2 
On Tue, Jun 21, 2022 at 05:42:18PM +0200, Ondřej Jirman wrote:
> On Wed, Feb 16, 2022 at 09:40:27AM +0800, Jon Lin wrote:
> > From: shengfei Xu <xsf@rock-chips.com>
> > 
> > the wakeup interrupt handler which is guaranteed not to run while
> > @resume noirq() is being executed. the patch can help to avoid the
> > wakeup source try to access spi when the spi is in suspend mode.
> 
> This patch causes oops on suspend every single time, because it tries to disable
> already disabled clocks (one disable in runtime PM suspend, and other one in
> system suspend). It also fails to properly handle errors from clk_prepare_enable
> in rockchip_spi_resume, potentially causing even more clock enable/disable
> imballance issues.
> 
> Please send a revert and figure out a better fix for the original issue.

Did anyone ever resolve this? I still see this issue on 6.10.6.

IIUC, we can actually do a partial revert -- we *should* be using
pm_runtime_force_{suspend,resume}() (so that we coordinate clk-disable,
etc. with the runtime PM state), but we also want to run in the noirq
phase.

If I don't see anyone else's solution or input, I'll plan on submitting
a partial revert, which seems to test out OK for me.

Brian

P.S. One can work around this problem by disabling runtime PM for the
controllers:

for i in /sys/bus/platform/drivers/rockchip-spi/*/power/control; do
  echo on >$i
done

But obviously that's not ideal.
Brian Norris Aug. 26, 2024, 6:30 p.m. UTC | #3 
Hi Jon,

You sent HTML email, so it likely didn't reach the mailing list. There
are some tips here that might help:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
https://www.infradead.org/~dwmw2/email.html

Anyway, I'll quote what I can in response below:

On Sat, Aug 24, 2024 at 01:42:14PM +0800, Jon Lin wrote:
>    I made a submission in the community, and RK's SDK processing plan is
>    to
>    make compatibility processing for PM runtime and NOIRQ.

I'm not 100% sure what that means, but I see you submitted this:

https://lore.kernel.org/all/20240825035422.900370-1-jon.lin@rock-chips.com/
[PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

I may give that a proper review later, but I might request you take a
look at my patch in addition or instead, which I believe is simpler and
clearer, with a better description of what it's solving and why:

https://lore.kernel.org/all/20240823214235.1718769-1-briannorris@chromium.org/
[PATCH] spi: rockchip: Resolve unbalanced runtime PM / system PM handling

Please let me know if that patch works for you.

Brian
diff mbox series

Patch

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 83da8fdb3c02..8b4d56ee2193 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -964,14 +964,14 @@  static int rockchip_spi_suspend(struct device *dev)
 {
 	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
 	ret = spi_controller_suspend(ctlr);
 	if (ret < 0)
 		return ret;
 
-	ret = pm_runtime_force_suspend(dev);
-	if (ret < 0)
-		return ret;
+	clk_disable_unprepare(rs->spiclk);
+	clk_disable_unprepare(rs->apb_pclk);
 
 	pinctrl_pm_select_sleep_state(dev);
 
@@ -986,10 +986,14 @@  static int rockchip_spi_resume(struct device *dev)
 
 	pinctrl_pm_select_default_state(dev);
 
-	ret = pm_runtime_force_resume(dev);
+	ret = clk_prepare_enable(rs->apb_pclk);
 	if (ret < 0)
 		return ret;
 
+	ret = clk_prepare_enable(rs->spiclk);
+	if (ret < 0)
+		clk_disable_unprepare(rs->apb_pclk);
+
 	ret = spi_controller_resume(ctlr);
 	if (ret < 0) {
 		clk_disable_unprepare(rs->spiclk);
@@ -1031,7 +1035,7 @@  static int rockchip_spi_runtime_resume(struct device *dev)
 #endif /* CONFIG_PM */
 
 static const struct dev_pm_ops rockchip_spi_pm = {
-	SET_SYSTEM_SLEEP_PM_OPS(rockchip_spi_suspend, rockchip_spi_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rockchip_spi_suspend, rockchip_spi_resume)
 	SET_RUNTIME_PM_OPS(rockchip_spi_runtime_suspend,
 			   rockchip_spi_runtime_resume, NULL)
 };