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

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

[Tony Lindgren]

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

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

[Thomas Richard]

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

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

[Dhruva Gole]

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>

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

[Tony Lindgren]

* 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

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

[Andy Shevchenko]

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

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

[Tony Lindgren]

* 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

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

[Tony Lindgren]

* 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