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 <[email protected]>
Reported-by: Thomas Richard <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
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
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 <[email protected]>
> Reported-by: Thomas Richard <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
Tested-by: Thomas Richard <[email protected]>
> ---
> 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 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 <[email protected]> [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
* Dhruva Gole <[email protected]> [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
* Tony Lindgren <[email protected]> [230927 07:29]:
> * Andy Shevchenko <[email protected]> [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
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 <[email protected]>
> > Reported-by: Thomas Richard <[email protected]>
Don't we want a closes: tag?
> > Signed-off-by: Tony Lindgren <[email protected]>
>
> Tested-by: Thomas Richard <[email protected]>
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 <[email protected]>
> > +
> > 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 <[email protected]>