We now get errors on system suspend if no_console_suspend is set as
reported by Thomas. The errors started with commit 20a41a62618d ("serial:
8250_omap: Use force_suspend and resume for system suspend").

Let's fix the issue by checking for console_suspend_enabled in the system
suspend and resume path.

Note that with this fix the checks for console_suspend_enabled in
omap8250_runtime_suspend() become useless. We now keep runtime PM usage
count for an attached kernel console starting with commit bedb404e91bb
("serial: 8250_port: Don't use power management for kernel console").

Fixes: 20a41a62618d ("serial: 8250_omap: Use force_suspend and resume for system suspend")
Cc: Udit Kumar <u-kumar1@ti.com>
Reported-by: Thomas Richard <thomas.richard@bootlin.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

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

On Tue, Sep 26, 2023 at 09:13:17AM +0300, Tony Lindgren wrote:
> We now get errors on system suspend if no_console_suspend is set as
> reported by Thomas. The errors started with commit 20a41a62618d ("serial:
> 8250_omap: Use force_suspend and resume for system suspend").
> 
> Let's fix the issue by checking for console_suspend_enabled in the system
> suspend and resume path.
> 
> Note that with this fix the checks for console_suspend_enabled in
> omap8250_runtime_suspend() become useless. We now keep runtime PM usage
> count for an attached kernel console starting with commit bedb404e91bb
> ("serial: 8250_port: Don't use power management for kernel console").

...

Btw, how close are we to getting rid the pm_runtime_irq_safe() call?

-- 
With Best Regards,
Andy Shevchenko

* Andy Shevchenko <andriy.shevchenko@intel.com> [230926 11:59]:
> Btw, how close are we to getting rid the pm_runtime_irq_safe() call?

Very close, I think still doable for v6.7 merge window.. Below is what I'm
testing with, there's one error that I've seen that may or may not be
related.

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
@@ -8,6 +8,7 @@
  *
  */
 
+#include <linux/atomic.h>
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/io.h>
@@ -130,6 +131,7 @@ struct omap8250_priv {
 
 	u8 tx_trigger;
 	u8 rx_trigger;
+	atomic_t active;
 	bool is_suspending;
 	int wakeirq;
 	int wakeups_enabled;
@@ -632,14 +634,21 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 	unsigned int iir, lsr;
 	int ret;
 
+	pm_runtime_get_noresume(port->dev);
+
+	/* Shallow idle state wake-up to an IO interrupt? */
+	if (atomic_add_unless(&priv->active, 1, 1)) {
+		priv->latency = priv->calc_latency;
+		schedule_work(&priv->qos_work);
+	}
+
 #ifdef CONFIG_SERIAL_8250_DMA
 	if (up->dma) {
 		ret = omap_8250_dma_handle_irq(port);
-		return IRQ_RETVAL(ret);
+		goto out_runtime_put;
 	}
 #endif
 
-	serial8250_rpm_get(up);
 	lsr = serial_port_in(port, UART_LSR);
 	iir = serial_port_in(port, UART_IIR);
 	ret = serial8250_handle_irq(port, iir);
@@ -676,7 +685,9 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 		schedule_delayed_work(&up->overrun_backoff, delay);
 	}
 
-	serial8250_rpm_put(up);
+out_runtime_put:
+	pm_runtime_mark_last_busy(port->dev);
+	pm_runtime_put(port->dev);
 
 	return IRQ_RETVAL(ret);
 }
@@ -1270,11 +1281,8 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 	u16 status;
 	u8 iir;
 
-	serial8250_rpm_get(up);
-
 	iir = serial_port_in(port, UART_IIR);
 	if (iir & UART_IIR_NO_INT) {
-		serial8250_rpm_put(up);
 		return IRQ_HANDLED;
 	}
 
@@ -1305,7 +1313,6 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 
 	uart_unlock_and_check_sysrq(port);
 
-	serial8250_rpm_put(up);
 	return 1;
 }
 
@@ -1500,8 +1507,6 @@ static int omap8250_probe(struct platform_device *pdev)
 	if (!of_get_available_child_count(pdev->dev.of_node))
 		pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
 
-	pm_runtime_irq_safe(&pdev->dev);
-
 	pm_runtime_get_sync(&pdev->dev);
 
 	omap_serial_fill_features_erratas(&up, priv);
@@ -1740,6 +1745,7 @@ static int omap8250_runtime_suspend(struct device *dev)
 
 	priv->latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
 	schedule_work(&priv->qos_work);
+	atomic_set(&priv->active, 0);
 
 	return 0;
 }
@@ -1749,6 +1755,10 @@ static int omap8250_runtime_resume(struct device *dev)
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
 	struct uart_8250_port *up = NULL;
 
+	/* Did the hardware wake to a device IO interrupt before a wakeirq? */
+	if (atomic_read(&priv->active))
+		return 0;
+
 	if (priv->line >= 0)
 		up = serial8250_get_port(priv->line);
 
@@ -1764,8 +1774,10 @@ static int omap8250_runtime_resume(struct device *dev)
 		spin_unlock_irq(&up->port.lock);
 	}
 
+	atomic_set(&priv->active, 1);
 	priv->latency = priv->calc_latency;
 	schedule_work(&priv->qos_work);
+
 	return 0;
 }
 
-- 
2.42.0

* Tony Lindgren <tony@atomide.com> [230927 07:29]:
> * Andy Shevchenko <andriy.shevchenko@intel.com> [230926 11:59]:
> > Btw, how close are we to getting rid the pm_runtime_irq_safe() call?
> 
> Very close, I think still doable for v6.7 merge window.. Below is what I'm
> testing with, there's one error that I've seen that may or may not be
> related.

I'm unable to reproduce the issue I was seeing with v6.6-rc3 with and without
the pm_runtime_irq_safe() dropping patch. So AFAIK no issues dropping
pm_runtime_irq_safe().

I was seeing some warning earlier after detaching kernel console and doing
any sysrq trigger on the serial port, seems like it was unrelated.

Regards,

Tony

Hi Tony,

Thanks for the fix.

On 9/26/23 08:13, Tony Lindgren wrote:
> We now get errors on system suspend if no_console_suspend is set as
> reported by Thomas. The errors started with commit 20a41a62618d ("serial:
> 8250_omap: Use force_suspend and resume for system suspend").
> 
> Let's fix the issue by checking for console_suspend_enabled in the system
> suspend and resume path.
> 
> Note that with this fix the checks for console_suspend_enabled in
> omap8250_runtime_suspend() become useless. We now keep runtime PM usage
> count for an attached kernel console starting with commit bedb404e91bb
> ("serial: 8250_port: Don't use power management for kernel console").
> 
> Fixes: 20a41a62618d ("serial: 8250_omap: Use force_suspend and resume for system suspend")
> Cc: Udit Kumar <u-kumar1@ti.com>
> Reported-by: Thomas Richard <thomas.richard@bootlin.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Tested-by: Thomas Richard <thomas.richard@bootlin.com>

> ---
>  drivers/tty/serial/8250/8250_omap.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> 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

On Sep 26, 2023 at 09:51:30 +0200, Thomas Richard wrote:
> Hi Tony,
> 
> Thanks for the fix.
> 
> On 9/26/23 08:13, Tony Lindgren wrote:
> > We now get errors on system suspend if no_console_suspend is set as
> > reported by Thomas. The errors started with commit 20a41a62618d ("serial:
> > 8250_omap: Use force_suspend and resume for system suspend").
> > 
> > Let's fix the issue by checking for console_suspend_enabled in the system
> > suspend and resume path.
> > 
> > Note that with this fix the checks for console_suspend_enabled in
> > omap8250_runtime_suspend() become useless. We now keep runtime PM usage
> > count for an attached kernel console starting with commit bedb404e91bb
> > ("serial: 8250_port: Don't use power management for kernel console").
> > 
> > Fixes: 20a41a62618d ("serial: 8250_omap: Use force_suspend and resume for system suspend")
> > Cc: Udit Kumar <u-kumar1@ti.com>
> > Reported-by: Thomas Richard <thomas.richard@bootlin.com>

Don't we want a closes: tag?

> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> Tested-by: Thomas Richard <thomas.richard@bootlin.com>

Thanks for testing Thomas

> 
> > ---
> >  drivers/tty/serial/8250/8250_omap.c | 25 ++++++++++---------------
> >  1 file changed, 10 insertions(+), 15 deletions(-)
> > 
> > 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;
> > +	}

LGTM, thanks for the fix Tony.

Reviewed-by: Dhruva Gole <d-gole@ti.com>

> > +
> >  	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
> 

-- 
Best regards,
Dhruva Gole <d-gole@ti.com>

* Dhruva Gole <d-gole@ti.com> [230926 11:32]:
> Don't we want a closes: tag?

Seems it should only be used if there's some bugzilla type report and
not for email threads.

Regards,

Tony