serial: 8250_omap: suspend issue with console_suspend disabled

Hi,

I already sent a mail related to this topic to the linux-serial mailing
list
(https://lore.kernel.org/linux-serial/8a856171-e743-737e-eb9d-42852e4e4f19@bootlin.com)
But as I also noticed a power management issue, i create a new thread
including more people and more details.

After switching to Linux 6.6-rc1, I met an issue during suspend to idle
with 8250_omap driver (no_console_suspend is set).
It is also valid for suspend to ram.
The driver fails to suspend the uart port used for the serial console so
the suspend sequence is stopped.

My target is the K3 J7200 SoC.

[  114.629197] port 2800000.serial:0.0: PM: calling
pm_runtime_force_suspend+0x0/0x134 @ 114, parent: 2800000.serial:0
[  114.639617] port 2800000.serial:0.0: PM:
pm_runtime_force_suspend+0x0/0x134 returned 0 after 2 usecs
[  114.648739] omap8250 2800000.serial: PM: calling
omap8250_suspend+0x0/0xf4 @ 114, parent: bus@100000
[  114.657861] omap8250 2800000.serial: PM: dpm_run_callback():
omap8250_suspend+0x0/0xf4 returns -16
[  114.666808] omap8250 2800000.serial: PM: omap8250_suspend+0x0/0xf4
returned -16 after 8951 usecs
[  114.675580] omap8250 2800000.serial: PM: failed to suspend: error -16
[  114.682011] PM: suspend of devices aborted after 675.644 msecs
[  114.687833] PM: start suspend of devices aborted after 681.777 msecs
[  114.694175] PM: Some devices failed to suspend, or early wake event
detected

The following sequence is used to suspend to idle:
$ echo 1 > /sys/power/pm_debug_messages
$ echo 1 > /sys/power/pm_print_times
$ echo 8 > /proc/sys/kernel/printk
$ echo 0 > /sys/module/printk/parameters/console_suspend
$ echo enabled >
/sys/devices/platform/bus@100000/2800000.serial/tty/ttyS2/power/wakeup
$ echo s2idle > /sys/power/mem_sleep
$ echo mem > /sys/power/state

The regression was introduced in commit 20a41a62618d "serial: 8250_omap:
Use force_suspend and resume for system suspend"

Before commit 20a41a62618d, omap8250_suspend returned 0.
Now pm_runtime_force_suspend is called and its return code is used by
omap8250_suspend.

static int omap8250_suspend(struct device *dev)
{
	struct omap8250_priv *priv = dev_get_drvdata(dev);
	struct uart_8250_port *up = serial8250_get_port(priv->line);
	int err;

	serial8250_suspend_port(priv->line);

	err = pm_runtime_resume_and_get(dev);
	if (err)
		return err;
	if (!device_may_wakeup(dev))
		priv->wer = 0;
	serial_out(up, UART_OMAP_WER, priv->wer);
	err = pm_runtime_force_suspend(dev);
	flush_work(&priv->qos_work);

	return err;
}

The pm_runtime_force_suspend function calls omap8250_runtime_suspend
which returns -EBUSY because console suspend was disabled (which is my
case), as explained in the code.

/*
 * When using 'no_console_suspend', the console UART must not be
 * suspended. Since driver suspend is managed by runtime suspend,
 * preventing runtime suspend (by returning error) will keep device
 * active during suspend.
 */
if (priv->is_suspending && !console_suspend_enabled) {
	if (up && uart_console(&up->port))
		return -EBUSY;
}

The port is used by the console, so we don't want to suspend it (console
suspend was disabled).
Of course, if console_suspend is enabled and messages are disabled there
is no issue.
For now my workaround is to always return 0 in omap8250_suspend.

                up = serial8250_get_port(priv->line);
@@ -1724,8 +1726,10 @@ static int omap8250_runtime_suspend(struct device
*dev)
         * active during suspend.
         */
        if (priv->is_suspending && !console_suspend_enabled) {
-               if (up && uart_console(&up->port))
+               if (up && uart_console(&up->port)) {
+                       pd->flags |= GENPD_FLAG_ALWAYS_ON;
                        return -EBUSY;
+               }
        }

        if (priv->habit & UART_ERRATA_CLOCK_DISABLE) {

For these two issues, I have workarounds but I don't know how to fix
them correctly.

Best Regards,

On 9/20/23 07:38, Tony Lindgren wrote:
> Hi,
> 
> * Thomas Richard <thomas.richard@bootlin.com> [230915 09:57]:
>> The regression was introduced in commit 20a41a62618d "serial: 8250_omap:
>> Use force_suspend and resume for system suspend"
> ...
> 
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> +++ b/drivers/tty/serial/8250/8250_omap.c
>> @@ -1630,7 +1630,7 @@ static int omap8250_suspend(struct device *dev)
>>         err = pm_runtime_force_suspend(dev);
>>         flush_work(&priv->qos_work);
>>
>> -       return err;
>> +       return 0;
>>  }
> 
> Maybe we can now just simplify things a bit here with the patch below.
> Care to give it a try, it's compile tested only so far.
> 

I tested it, it works for me.

>> Once the omap8250_suspend doesn't return an error, the suspend sequence
>> can continue, but I get an other issue.
>> This issue is not related to commit 20a41a62618d, it has already been
>> present.
>> The power domain of the console is powered-off, so no more messages are
>> printed, and the SoC is stucked.
>> As the uart port is used as console, we don't want to power-off it.
>> My workaround is to set the corresponding power domain to
>> GENPD_FLAG_ALWAYS_ON, so the uart port is not powered-off.
> 
> The runtime PM usage count should keep the related power domain on though,
> sounds like this issue somewhere else if the power domains get force
> suspended with runtime PM usage count?
> 
> Regards,
> 
> Tony
> 
> 8< ------------------------------
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1617,7 +1617,7 @@ static int omap8250_suspend(struct device *dev)
>  {
>  	struct omap8250_priv *priv = dev_get_drvdata(dev);
>  	struct uart_8250_port *up = serial8250_get_port(priv->line);
> -	int err;
> +	int err = 0;
>  
>  	serial8250_suspend_port(priv->line);
>  
> @@ -1627,7 +1627,8 @@ static int omap8250_suspend(struct device *dev)
>  	if (!device_may_wakeup(dev))
>  		priv->wer = 0;
>  	serial_out(up, UART_OMAP_WER, priv->wer);
> -	err = pm_runtime_force_suspend(dev);
> +	if (uart_console(&up->port) && console_suspend_enabled)
> +		err = pm_runtime_force_suspend(dev);
>  	flush_work(&priv->qos_work);
>  
>  	return err;
> @@ -1636,11 +1637,15 @@ static int omap8250_suspend(struct device *dev)
>  static int omap8250_resume(struct device *dev)
>  {
>  	struct omap8250_priv *priv = dev_get_drvdata(dev);
> +	struct uart_8250_port *up = serial8250_get_port(priv->line);
>  	int err;
>  
> -	err = pm_runtime_force_resume(dev);
> -	if (err)
> -		return err;
> +	if (uart_console(&up->port) && console_suspend_enabled) {
> +		err = pm_runtime_force_resume(dev);
> +		if (err)
> +			return err;
> +	}
> +
>  	serial8250_resume_port(priv->line);
>  	/* Paired with pm_runtime_resume_and_get() in omap8250_suspend() */
>  	pm_runtime_mark_last_busy(dev);
> @@ -1717,16 +1722,6 @@ static int omap8250_runtime_suspend(struct device *dev)
>  
>  	if (priv->line >= 0)
>  		up = serial8250_get_port(priv->line);
> -	/*
> -	 * When using 'no_console_suspend', the console UART must not be
> -	 * suspended. Since driver suspend is managed by runtime suspend,
> -	 * preventing runtime suspend (by returning error) will keep device
> -	 * active during suspend.
> -	 */
> -	if (priv->is_suspending && !console_suspend_enabled) {
> -		if (up && uart_console(&up->port))
> -			return -EBUSY;
> -	}
>  
>  	if (priv->habit & UART_ERRATA_CLOCK_DISABLE) {
>  		int ret;

On 9/21/23 09:58, Thomas Richard wrote:
> On 9/20/23 07:38, Tony Lindgren wrote:
>> Hi,
>>
>> * Thomas Richard <thomas.richard@bootlin.com> [230915 09:57]:
>>> The regression was introduced in commit 20a41a62618d "serial: 8250_omap:
>>> Use force_suspend and resume for system suspend"
>> ...
>>
>>> --- a/drivers/tty/serial/8250/8250_omap.c
>>> +++ b/drivers/tty/serial/8250/8250_omap.c
>>> @@ -1630,7 +1630,7 @@ static int omap8250_suspend(struct device *dev)
>>>         err = pm_runtime_force_suspend(dev);
>>>         flush_work(&priv->qos_work);
>>>
>>> -       return err;
>>> +       return 0;
>>>  }
>>
>> Maybe we can now just simplify things a bit here with the patch below.
>> Care to give it a try, it's compile tested only so far.
>>
> 
> I tested it, it works for me.

Tony,

As your proposal works well, do you plan to send a patch ?
Or would you prefer me to send it ?

Regards,

Thomas

> 
>>> Once the omap8250_suspend doesn't return an error, the suspend sequence
>>> can continue, but I get an other issue.
>>> This issue is not related to commit 20a41a62618d, it has already been
>>> present.
>>> The power domain of the console is powered-off, so no more messages are
>>> printed, and the SoC is stucked.
>>> As the uart port is used as console, we don't want to power-off it.
>>> My workaround is to set the corresponding power domain to
>>> GENPD_FLAG_ALWAYS_ON, so the uart port is not powered-off.
>>
>> The runtime PM usage count should keep the related power domain on though,
>> sounds like this issue somewhere else if the power domains get force
>> suspended with runtime PM usage count?
>>
>> Regards,
>>
>> Tony
>>
>> 8< ------------------------------
>> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> +++ b/drivers/tty/serial/8250/8250_omap.c
>> @@ -1617,7 +1617,7 @@ static int omap8250_suspend(struct device *dev)
>>  {
>>  	struct omap8250_priv *priv = dev_get_drvdata(dev);
>>  	struct uart_8250_port *up = serial8250_get_port(priv->line);
>> -	int err;
>> +	int err = 0;
>>  
>>  	serial8250_suspend_port(priv->line);
>>  
>> @@ -1627,7 +1627,8 @@ static int omap8250_suspend(struct device *dev)
>>  	if (!device_may_wakeup(dev))
>>  		priv->wer = 0;
>>  	serial_out(up, UART_OMAP_WER, priv->wer);
>> -	err = pm_runtime_force_suspend(dev);
>> +	if (uart_console(&up->port) && console_suspend_enabled)
>> +		err = pm_runtime_force_suspend(dev);
>>  	flush_work(&priv->qos_work);
>>  
>>  	return err;
>> @@ -1636,11 +1637,15 @@ static int omap8250_suspend(struct device *dev)
>>  static int omap8250_resume(struct device *dev)
>>  {
>>  	struct omap8250_priv *priv = dev_get_drvdata(dev);
>> +	struct uart_8250_port *up = serial8250_get_port(priv->line);
>>  	int err;
>>  
>> -	err = pm_runtime_force_resume(dev);
>> -	if (err)
>> -		return err;
>> +	if (uart_console(&up->port) && console_suspend_enabled) {
>> +		err = pm_runtime_force_resume(dev);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>>  	serial8250_resume_port(priv->line);
>>  	/* Paired with pm_runtime_resume_and_get() in omap8250_suspend() */
>>  	pm_runtime_mark_last_busy(dev);
>> @@ -1717,16 +1722,6 @@ static int omap8250_runtime_suspend(struct device *dev)
>>  
>>  	if (priv->line >= 0)
>>  		up = serial8250_get_port(priv->line);
>> -	/*
>> -	 * When using 'no_console_suspend', the console UART must not be
>> -	 * suspended. Since driver suspend is managed by runtime suspend,
>> -	 * preventing runtime suspend (by returning error) will keep device
>> -	 * active during suspend.
>> -	 */
>> -	if (priv->is_suspending && !console_suspend_enabled) {
>> -		if (up && uart_console(&up->port))
>> -			return -EBUSY;
>> -	}
>>  
>>  	if (priv->habit & UART_ERRATA_CLOCK_DISABLE) {
>>  		int ret;
> 
>

* Thomas Richard <thomas.richard@bootlin.com> [230925 13:03]:
> On 9/21/23 09:58, Thomas Richard wrote:
> > I tested it, it works for me.
> 
> Tony,
> 
> As your proposal works well, do you plan to send a patch ?
> Or would you prefer me to send it ?

Thanks I'll type up the description and send it.

Regards,

Tony

* Tony Lindgren <tony@atomide.com> [700101 02:00]:
> * Thomas Richard <thomas.richard@bootlin.com> [230925 13:03]:
> > On 9/21/23 09:58, Thomas Richard wrote:
> > > I tested it, it works for me.
> > 
> > Tony,
> > 
> > As your proposal works well, do you plan to send a patch ?
> > Or would you prefer me to send it ?
> 
> Thanks I'll type up the description and send it.

Sent now as:

[PATCH] serial: 8250_omap: Fix errors with no_console_suspend

Regards,

Tony

On 9/20/23 07:38, Tony Lindgren wrote:
> Hi,
> 
> * Thomas Richard <thomas.richard@bootlin.com> [230915 09:57]:
>> The regression was introduced in commit 20a41a62618d "serial: 8250_omap:
>> Use force_suspend and resume for system suspend"
> ...
> 
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> +++ b/drivers/tty/serial/8250/8250_omap.c
>> @@ -1630,7 +1630,7 @@ static int omap8250_suspend(struct device *dev)
>>         err = pm_runtime_force_suspend(dev);
>>         flush_work(&priv->qos_work);
>>
>> -       return err;
>> +       return 0;
>>  }
> 
> Maybe we can now just simplify things a bit here with the patch below.
> Care to give it a try, it's compile tested only so far.
> 
>> Once the omap8250_suspend doesn't return an error, the suspend sequence
>> can continue, but I get an other issue.
>> This issue is not related to commit 20a41a62618d, it has already been
>> present.
>> The power domain of the console is powered-off, so no more messages are
>> printed, and the SoC is stucked.
>> As the uart port is used as console, we don't want to power-off it.
>> My workaround is to set the corresponding power domain to
>> GENPD_FLAG_ALWAYS_ON, so the uart port is not powered-off.
> 
> The runtime PM usage count should keep the related power domain on though,
> sounds like this issue somewhere else if the power domains get force
> suspended with runtime PM usage count?

If I understand correctly, there are 2 solutions to keep the power
domain on through.
The first one is to set the flag GENPD_FLAG_ALWAYS_ON for the power domain.
The second one is to set the wakup_path of the device (using
device_set_wakeup_path) and set the flag GENPD_FLAG_ACTIVE_WAKEUP to the
power domain.

I didn't found any flag or option to say that the device is not
suspended, but it is not an error, so we don't want to poweroff the
power domain.

Regards,

Thomas

> 
> Regards,
> 
> Tony
> 
> 8< ------------------------------
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1617,7 +1617,7 @@ static int omap8250_suspend(struct device *dev)
>  {
>  	struct omap8250_priv *priv = dev_get_drvdata(dev);
>  	struct uart_8250_port *up = serial8250_get_port(priv->line);
> -	int err;
> +	int err = 0;
>  
>  	serial8250_suspend_port(priv->line);
>  
> @@ -1627,7 +1627,8 @@ static int omap8250_suspend(struct device *dev)
>  	if (!device_may_wakeup(dev))
>  		priv->wer = 0;
>  	serial_out(up, UART_OMAP_WER, priv->wer);
> -	err = pm_runtime_force_suspend(dev);
> +	if (uart_console(&up->port) && console_suspend_enabled)
> +		err = pm_runtime_force_suspend(dev);
>  	flush_work(&priv->qos_work);
>  
>  	return err;
> @@ -1636,11 +1637,15 @@ static int omap8250_suspend(struct device *dev)
>  static int omap8250_resume(struct device *dev)
>  {
>  	struct omap8250_priv *priv = dev_get_drvdata(dev);
> +	struct uart_8250_port *up = serial8250_get_port(priv->line);
>  	int err;
>  
> -	err = pm_runtime_force_resume(dev);
> -	if (err)
> -		return err;
> +	if (uart_console(&up->port) && console_suspend_enabled) {
> +		err = pm_runtime_force_resume(dev);
> +		if (err)
> +			return err;
> +	}
> +
>  	serial8250_resume_port(priv->line);
>  	/* Paired with pm_runtime_resume_and_get() in omap8250_suspend() */
>  	pm_runtime_mark_last_busy(dev);
> @@ -1717,16 +1722,6 @@ static int omap8250_runtime_suspend(struct device *dev)
>  
>  	if (priv->line >= 0)
>  		up = serial8250_get_port(priv->line);
> -	/*
> -	 * When using 'no_console_suspend', the console UART must not be
> -	 * suspended. Since driver suspend is managed by runtime suspend,
> -	 * preventing runtime suspend (by returning error) will keep device
> -	 * active during suspend.
> -	 */
> -	if (priv->is_suspending && !console_suspend_enabled) {
> -		if (up && uart_console(&up->port))
> -			return -EBUSY;
> -	}
>  
>  	if (priv->habit & UART_ERRATA_CLOCK_DISABLE) {
>  		int ret;

On 10/10/23 08:51, Tony Lindgren wrote:
> Hi,
> 
> * Thomas Richard <thomas.richard@bootlin.com> [231009 15:13]:
>>> The runtime PM usage count should keep the related power domain on though,
>>> sounds like this issue somewhere else if the power domains get force
>>> suspended with runtime PM usage count?
>>
>> If I understand correctly, there are 2 solutions to keep the power
>> domain on through.
>> The first one is to set the flag GENPD_FLAG_ALWAYS_ON for the power domain.
>> The second one is to set the wakup_path of the device (using
>> device_set_wakeup_path) and set the flag GENPD_FLAG_ACTIVE_WAKEUP to the
>> power domain.
>>
>> I didn't found any flag or option to say that the device is not
>> suspended, but it is not an error, so we don't want to poweroff the
>> power domain.
> 
> If no_console_suspend is set then GENPD_FLAG_ALWAYS_ON makes sense to
> me as we want to see the debug messages. This will also alter the SoCs
> suspend state though, so no_console_suspend is of limited use. Can you
> please send an updated patch against tty-next branch for this?

Please find below a proposal based on tty-next branch.
I had to add your patch 'serial: 8250_omap: Fix errors with
no_console_suspend' as it is not present in this branch.
And I need it to not have an error in omap8250_suspend.

> 
> It would be good to understand why the related power domain gets suspended
> with active runtime PM usage count though. To me it seems this might be
> an issue somewhere in the SoC related power domain code that just tries
> to force suspend everything.

I found nothing to say, there is a device in use (and not suspended) in
this power domain, so do not poweroff it.
Maybe I missed something.

Regards,

Thomas

> 
> Regards,
> 
> Tony

diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 4b33f4529aed..2d42f485c987 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -28,6 +28,7 @@
 #include <linux/pm_wakeirq.h>
 #include <linux/dma-mapping.h>
 #include <linux/sys_soc.h>
+#include <linux/pm_domain.h>

 #include "8250.h"

@@ -115,6 +116,12 @@
 /* RX FIFO occupancy indicator */
 #define UART_OMAP_RX_LVL		0x19

+/*
+ * Copy of the genpd flags for the console.
+ * Only used if console suspend is disabled
+ */
+static unsigned int genpd_flags_console;
+
 struct omap8250_priv {
 	void __iomem *membase;
 	int line;
@@ -1623,6 +1630,7 @@ static int omap8250_suspend(struct device *dev)
 {
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
 	struct uart_8250_port *up = serial8250_get_port(priv->line);
+	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
 	int err = 0;

 	serial8250_suspend_port(priv->line);
@@ -1633,8 +1641,19 @@ static int omap8250_suspend(struct device *dev)
 	if (!device_may_wakeup(dev))
 		priv->wer = 0;
 	serial_out(up, UART_OMAP_WER, priv->wer);
-	if (uart_console(&up->port) && console_suspend_enabled)
-		err = pm_runtime_force_suspend(dev);
+	if (uart_console(&up->port)) {
+		if (console_suspend_enabled)
+			err = pm_runtime_force_suspend(dev);
+		else {
+			/*
+			 * The pd shall not be powered-off (no console suspend).
+			 * Make copy of genpd flags before to set it always on.
+			 * The original value is restored during the resume.
+			 */
+			genpd_flags_console = genpd->flags;
+			genpd->flags |= GENPD_FLAG_ALWAYS_ON;
+		}
+	}
 	flush_work(&priv->qos_work);

 	return err;
@@ -1644,12 +1663,16 @@ static int omap8250_resume(struct device *dev)
 {
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
 	struct uart_8250_port *up = serial8250_get_port(priv->line);
+	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
 	int err;

 	if (uart_console(&up->port) && console_suspend_enabled) {
-		err = pm_runtime_force_resume(dev);
-		if (err)
-			return err;
+		if (console_suspend_enabled) {
+			err = pm_runtime_force_resume(dev);
+			if (err)
+				return err;
+		} else
+			genpd->flags = genpd_flags_console;
 	}

 	serial8250_resume_port(priv->line);