[PATCH 2/2] serial: imx: fix endless loop during suspend

[PATCH 2/2] serial: imx: fix endless loop during suspend

[Martin Kaiser]

Before we go into suspend mode, we enable the imx uart's interrupt for
the awake bit in the UART Status Register 1. If, for some reason, the
awake bit is already set before we enter suspend mode, we get an
interrupt immediately when we enable interrupts for awake. The uart's
clk_ipg is already disabled at this point. We end up in the interrupt
handler, which usually tries to clear the awake bit. This doesn't work
with the clock disabled. Therefore, we keep getting interrupts forever,
resulting in an endless loop.

Move the calls to serial_imx_enable_wakeup() into the _noirq functions,
where interrupts are disabled and clk_ipg is active. This way, we can
safely clear the awake bit and enable the imx interrupt for awake.

Now that we do the wakeup configuration in .suspend_noirq, we need
separate functions for .suspend_noirq and .freeze_noirq. However,
.resume_noirq and .restore_noirq can still be shared. We just disable
the wakeup source there, this does not conflict with hibernation.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
Cc: stable@vger.kernel.org
---
 drivers/tty/serial/imx.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 250aa26..5df9172 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2221,8 +2221,10 @@ static void serial_imx_enable_wakeup(struct imx_port *sport, bool on)
 	unsigned int val;
 
 	val = readl(sport->port.membase + UCR3);
-	if (on)
+	if (on) {
+		writel(USR1_AWAKE, sport->port.membase + USR1);
 		val |= UCR3_AWAKEN;
+	}
 	else
 		val &= ~UCR3_AWAKEN;
 	writel(val, sport->port.membase + UCR3);
@@ -2245,6 +2247,9 @@ static int imx_serial_port_suspend_noirq(struct device *dev)
 	if (ret)
 		return ret;
 
+	/* enable wakeup from i.MX UART */
+	serial_imx_enable_wakeup(sport, true);
+
 	serial_imx_save_context(sport);
 
 	clk_disable(sport->clk_ipg);
@@ -2264,6 +2269,9 @@ static int imx_serial_port_resume_noirq(struct device *dev)
 
 	serial_imx_restore_context(sport);
 
+	/* disable wakeup from i.MX UART */
+	serial_imx_enable_wakeup(sport, false);
+
 	clk_disable(sport->clk_ipg);
 
 	return 0;
@@ -2291,9 +2299,6 @@ static int imx_serial_port_suspend(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct imx_port *sport = platform_get_drvdata(pdev);
 
-	/* enable wakeup from i.MX UART */
-	serial_imx_enable_wakeup(sport, true);
-
 	uart_suspend_port(&imx_reg, &sport->port);
 	disable_irq(sport->port.irq);
 
@@ -2306,9 +2311,6 @@ static int imx_serial_port_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct imx_port *sport = platform_get_drvdata(pdev);
 
-	/* disable wakeup from i.MX UART */
-	serial_imx_enable_wakeup(sport, false);
-
 	uart_resume_port(&imx_reg, &sport->port);
 	enable_irq(sport->port.irq);
 
-- 
2.1.4

Re: [PATCH 2/2] serial: imx: fix endless loop during suspend

[Fabio Estevam]

Hi Martin,

On Wed, Dec 27, 2017 at 3:27 PM, Martin Kaiser <martin@kaiser.cx> wrote:
> Before we go into suspend mode, we enable the imx uart's interrupt for
> the awake bit in the UART Status Register 1. If, for some reason, the
> awake bit is already set before we enter suspend mode, we get an
> interrupt immediately when we enable interrupts for awake. The uart's
> clk_ipg is already disabled at this point. We end up in the interrupt
> handler, which usually tries to clear the awake bit. This doesn't work
> with the clock disabled. Therefore, we keep getting interrupts forever,
> resulting in an endless loop.
>
> Move the calls to serial_imx_enable_wakeup() into the _noirq functions,
> where interrupts are disabled and clk_ipg is active. This way, we can
> safely clear the awake bit and enable the imx interrupt for awake.
>
> Now that we do the wakeup configuration in .suspend_noirq, we need
> separate functions for .suspend_noirq and .freeze_noirq. However,
> .resume_noirq and .restore_noirq can still be shared. We just disable
> the wakeup source there, this does not conflict with hibernation.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> Cc: stable@vger.kernel.org

Which i.MX SoC did you use to test this patch?

On a imx6q-cuboxi I am no longer able to enter in suspend with this
path applied:

# echo enabled > /sys/class/tty/ttymxc0/power/wakeup
# echo mem > /sys/power/state
[    9.766903] PM: suspend entry (deep)
[    9.770658] PM: Syncing filesystems ... done.
[    9.815312] Freezing user space processes ... (elapsed 0.001 seconds) done.
[    9.824744] OOM killer disabled.
[    9.827998] Freezing remaining freezable tasks ... (elapsed 0.001
seconds) done.
[    9.837351] Suspending console(s) (use no_console_suspend to debug)
[    9.915495] PM: suspend devices took 0.080 seconds
[    9.928746] PM: noirq suspend of devices failed
[   10.196232] mmc0: queuing unknown CIS tuple 0x80 (2 bytes)
[   10.198148] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
[   10.200042] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
[   10.203420] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
[   10.206812] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
[   10.263097] PM: resume devices took 0.330 seconds
[   10.266639] ata1: SATA link down (SStatus 0 SControl 300)
[   10.310305] OOM killer enabled.
[   10.313458] Restarting tasks ... done.
[   10.319568] PM: suspend exit
sh: write error: Device or resource busy

Even if I do not press anything in the console the system gets out of
suspend automatically.

Thanks

Re: [PATCH 2/2] serial: imx: fix endless loop during suspend

[Martin Kaiser]

Hi Fabio,

thanks for testing my patch. Sorry for breaking suspend on your board.

Thus wrote Fabio Estevam (festevam@gmail.com):

> Which i.MX SoC did you use to test this patch?

I tested on an imx258.

> On a imx6q-cuboxi I am no longer able to enter in suspend with this
> path applied:

> # echo enabled > /sys/class/tty/ttymxc0/power/wakeup
> # echo mem > /sys/power/state
> [    9.766903] PM: suspend entry (deep)
> [    9.770658] PM: Syncing filesystems ... done.
> [    9.815312] Freezing user space processes ... (elapsed 0.001 seconds) done.
> [    9.824744] OOM killer disabled.
> [    9.827998] Freezing remaining freezable tasks ... (elapsed 0.001
> seconds) done.
> [    9.837351] Suspending console(s) (use no_console_suspend to debug)
> [    9.915495] PM: suspend devices took 0.080 seconds
> [    9.928746] PM: noirq suspend of devices failed
> [   10.196232] mmc0: queuing unknown CIS tuple 0x80 (2 bytes)
> [   10.198148] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
> [   10.200042] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
> [   10.203420] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
> [   10.206812] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
> [   10.263097] PM: resume devices took 0.330 seconds
> [   10.266639] ata1: SATA link down (SStatus 0 SControl 300)
> [   10.310305] OOM killer enabled.
> [   10.313458] Restarting tasks ... done.
> [   10.319568] PM: suspend exit
> sh: write error: Device or resource busy

> Even if I do not press anything in the console the system gets out of
> suspend automatically.

So the suspend_noirq step failed with -EBUSY.

My guess is that the following code path is taken

suspend_devices_and_enter()
   suspend_enter()
      dpm_suspend_noirq()
         dpm_noirq_suspend_devices()
            device_suspend_noirq()
               __device_suspend_noirq()
                  pm_wakeup_pending()

and pm_wakeup_pending() returns true. We'd then have an async_error
-EBUSY.

If that's the case, I don't understand why it happens for imx6q.
We should only have a pending wakeup event if wakeup_source_activate()
or ..._deactivate() has been called.

Seeing this kind of problem, I wonder if it's ok to move setting AWAKEN
from the suspend to the suspend_noirq step. The imx uart's interrupt is
now re-enabled and IRQD_WAKEUP_ARMED is set before we configure the uart
to generate such interrupts (by setting AWAKEN), whereas before, it was
the other way around. I'd be grateful if anyone could shed some light
on this. (Or more generally: When must the hardware be configured to
generate wakeup interrupts? Is it ok to do this in supend_noirq or must
it be done before?)

Fabio, could you post the output of

cat /sys/kernel/debug/suspend_stats

after supend failed, to confirm that we're failing below
device_suspend_noirq()?

Thanks,

   Martin

Re: [PATCH 2/2] serial: imx: fix endless loop during suspend

[Fabio Estevam]

Hi Martin,

On Tue, Jan 2, 2018 at 2:15 PM, Martin Kaiser <martin@kaiser.cx> wrote:

> Fabio, could you post the output of
>
> cat /sys/kernel/debug/suspend_stats
>
> after supend failed, to confirm that we're failing below
> device_suspend_noirq()?

Here it goes:

# cat /sys/kernel/debug/suspend_stats
success: 0
fail: 1
failed_freeze: 0
failed_prepare: 0
failed_suspend: 0
failed_suspend_late: 0
failed_suspend_noirq: 1
failed_resume: 0
failed_resume_early: 0
failed_resume_noirq: 0
failures:
  last_failed_dev:

  last_failed_errno:    -16
                        0
  last_failed_step:     suspend_noirq

Re: [PATCH 2/2] serial: imx: fix endless loop during suspend

[Fabio Estevam]

Hi Martin,

On Tue, Jan 2, 2018 at 2:15 PM, Martin Kaiser <martin@kaiser.cx> wrote:

> I tested on an imx258.

I am not able to reproduce this problem on a imx25 pdk running 4.14.11
or linux-next.

Is it 100% reproducible on your board?

Care to share its dts? Do you use multiple UART ports? Do any of them
use RTS/CTS?

Do you have any logs to share?

Thanks

Re: [PATCH 2/2] serial: imx: fix endless loop during suspend

[Martin Kaiser]

Hi Fabio,

Thus wrote Fabio Estevam (festevam@gmail.com):

> I am not able to reproduce this problem on a imx25 pdk running 4.14.11
> or linux-next.

this is no surprise. The problem shows up only if the AWAKE bit in UART
Status Register 1 is set before we go into suspend. My understanding is
that this bit will be set when the signal on the rx pin goes from high
to low. 

> Is it 100% reproducible on your board?

On my board, I have one uart port that's connected to an external chip.
A power cycle of this external chip creates the falling edge on rx and
makes the imx uart set the AWAKE bit. The problem can then be reproduced
100%.

It can be argued that this is an obscure scenario, but nevertheless, it
should not put the kernel into an endless loop.

This is my understanding of what happens:
- AWAKE is set in the USR1 register
- there's no uart transfer running, the clocks are disabled
   (As I write this I'm not sure why this is the case, clk_ipg should
    still be enabled but it seems that it's not. If I enable it
    manually, the behaviour is different.)
- we enter suspend
- AWAKEN (UART control register 3) is set so that AWAKE creates an interrupt
- we get an interrupt immediately
   (the imx interrupt is not yet disabled at this point. it'll be
    disabled later and then reenabled if the uart port acts as a wakeup
    source)
   -> we get into the interrupt handler with the clocks disabled
- the interrupt handler tries to clear the AWAKE bit, this does not work
  since the clocks are off, we exit and AWAKE is still set
- we get another interrupt immediately
   -> endless loop

What I'm trying to do with my patch is to clear the AWAKE bit before we
set AWAKEEN. We don't want to be woken up by events that happened before
we started going into suspend.

To do this, I have to find a suitable place where the clocks are
enabled. That's why I tried to move clearing AWAKE and setting AWAKEEN
into suspend_noirq, where the clocks are already enabled to save all
registers before we finally suspend. While this works ok on my board, it
cuases problems on imx6q.

I'm not sure what makes my patch fail for you. I could imagine it is
because now, the imx interrupt is enabled (as a wakeup source) before
AWAKEN is set. The current code sets AWAKEN first and then enables the
interrupt for the wakeup source.

I'll try a different approach that keeps this order.

> Care to share its dts? Do you use multiple UART ports? Do any of them
> use RTS/CTS?

There's nothing special regarding the uarts, There's a couple of them,
none of which are using rts/cts.

It all depends on the awake bit.

Best regards,

   Martin