[PATCH] serial: 8250_port: Fix imprecise external abort for mctrl if inactive

[PATCH] serial: 8250_port: Fix imprecise external abort for mctrl if inactive

[Tony Lindgren]

We can get an imprecise external abort on uart_shutdown() at
serial8250_do_set_mctrl() if the UART is autoidled.

We don't want to add PM runtime calls to serial8250_do_set_mctrl()
beyond checking the usage count as it gets called from interrupts
disabled and spinlock held from uart_update_mctrl().

We can just bail out early from serial8250_do_set_mctrl() if the UART
is inactive. We have uart_shutdown() call uart_port_dtr_rts() with
value of 0 just to disable DTR and RTS.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Sebastian Reichel <sre@kernel.org>
Cc: Vignesh Raghavendra <vigneshr@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_port.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2001,11 +2001,20 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
 	return serial8250_do_get_mctrl(port);
 }
 
+/*
+ * Called from uart_update_mctrl() with spinlock held, so we don't want
+ * add PM runtime calls here beyond checking the usage count. If the
+ * UART is not active, we can just bail out early.
+ */
 void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned char mcr;
 
+	if (up->capabilities & UART_CAP_RPM &&
+	    !pm_runtime_get_if_in_use(up->port.dev))
+		return;
+
 	if (port->rs485.flags & SER_RS485_ENABLED) {
 		if (serial8250_in_MCR(up) & UART_MCR_RTS)
 			mctrl |= TIOCM_RTS;
@@ -2018,6 +2027,9 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
 
 	serial8250_out_MCR(up, mcr);
+
+	if (up->capabilities & UART_CAP_RPM)
+		pm_runtime_put(up->port.dev);
 }
 EXPORT_SYMBOL_GPL(serial8250_do_set_mctrl);
 
-- 
2.26.2

Re: [PATCH] serial: 8250_port: Fix imprecise external abort for mctrl if inactive

[Johan Hovold]

On Mon, Jun 01, 2020 at 05:18:13PM -0700, Tony Lindgren wrote:
> We can get an imprecise external abort on uart_shutdown() at
> serial8250_do_set_mctrl() if the UART is autoidled.
> 
> We don't want to add PM runtime calls to serial8250_do_set_mctrl()
> beyond checking the usage count as it gets called from interrupts
> disabled and spinlock held from uart_update_mctrl().
> 
> We can just bail out early from serial8250_do_set_mctrl() if the UART
> is inactive. We have uart_shutdown() call uart_port_dtr_rts() with
> value of 0 just to disable DTR and RTS.

No, sorry. This is just putting another band-aid on this broken mess (I
never realised it was this bad).

As others have apparently already pointed out in the past, there are
paths that will end up calling sleeping pm_runtime_get_sync() in atomic
context (e.g serial8250_stop_tx()).

In other places this all seems to work mostly by chance by relying on
autosuspend keeping the clocks enabled long enough to not hit broken
paths (e.g. serial8250_do_set_mctrl()) which fail to enable clocks.

Note that uart_port_dtr_rts() is called from other paths, for example on
close and hangup, which would now fail to lower DTR/RTS as expected (it
still appears to work mostly by chance since there's later call in
serial8250_do_shutdown() which updates MCR to clear TIOCM_OUT2).

There's shouldn't be anything fundamental preventing you from adding the
missing resume calls to the mctrl paths even if it may require reworking
(and fixing) the whole RPM implementation (which would be a good thing
of course).

> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Sebastian Reichel <sre@kernel.org>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2001,11 +2001,20 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
>  	return serial8250_do_get_mctrl(port);
>  }
>  
> +/*
> + * Called from uart_update_mctrl() with spinlock held, so we don't want
> + * add PM runtime calls here beyond checking the usage count. If the
> + * UART is not active, we can just bail out early.
> + */
>  void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(port);
>  	unsigned char mcr;
>  
> +	if (up->capabilities & UART_CAP_RPM &&
> +	    !pm_runtime_get_if_in_use(up->port.dev))
> +		return;
> +
>  	if (port->rs485.flags & SER_RS485_ENABLED) {
>  		if (serial8250_in_MCR(up) & UART_MCR_RTS)
>  			mctrl |= TIOCM_RTS;
> @@ -2018,6 +2027,9 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  	mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
>  
>  	serial8250_out_MCR(up, mcr);
> +
> +	if (up->capabilities & UART_CAP_RPM)
> +		pm_runtime_put(up->port.dev);
>  }
>  EXPORT_SYMBOL_GPL(serial8250_do_set_mctrl);

Johan

Re: [PATCH] serial: 8250_port: Fix imprecise external abort for mctrl if inactive

[Andy Shevchenko]

On Tue, Jun 2, 2020 at 11:09 AM Johan Hovold <johan@kernel.org> wrote:
> On Mon, Jun 01, 2020 at 05:18:13PM -0700, Tony Lindgren wrote:

...

> There's shouldn't be anything fundamental preventing you from adding the
> missing resume calls to the mctrl paths even if it may require reworking
> (and fixing) the whole RPM implementation (which would be a good thing
> of course).

Yes, for serial core I have long standing patch series to implement
RPM (more or less?) properly.

However, OMAP is a beast which prevents us to go due to a big hack
called pm_runtime_irq_safe().
Tony is aware of this and I think the above is somehow related to removal of it.

But I completely agree that the goal is to get better runtime PM
implementation over all.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_port: Fix imprecise external abort for mctrl if inactive

[Tony Lindgren]

* Andy Shevchenko <andy.shevchenko@gmail.com> [200602 08:33]:
> On Tue, Jun 2, 2020 at 11:09 AM Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Jun 01, 2020 at 05:18:13PM -0700, Tony Lindgren wrote:
> 
> ...
> 
> > There's shouldn't be anything fundamental preventing you from adding the
> > missing resume calls to the mctrl paths even if it may require reworking
> > (and fixing) the whole RPM implementation (which would be a good thing
> > of course).
> 
> Yes, for serial core I have long standing patch series to implement
> RPM (more or less?) properly.

Yeah let's try after the merge window.

Not sure what else to do with the fix though. We currently have
8250_port.c not really aware of the hardare state for PM runtime at
least for the hang-up path.

> However, OMAP is a beast which prevents us to go due to a big hack
> called pm_runtime_irq_safe().
> Tony is aware of this and I think the above is somehow related to removal of it.

Now that we can detach and reattach the kernel serial console,
there should not be any need for pm_runtime_irq_safe() anymore :)

And the UART wake-up from deeper idle states can only happen with
help of external hardware like GPIO controller or pinctrl controller.

And for the always-on wake-up interrupt controllers we have the
Linux generic wakeirqs to wake-up serial device on events.

So I think the way to procedd with pm_runtime_irq_safe() removal
for serial drivers is to block serial PM runtime unless we have a
wakeirq configured for omaps in devicetree. In the worst case the
regression is that PM runtime for serial won't work unless properly
configured.

And the UART wakeup latency will be a bit longer compared to
pm_runtime_irq_safe() naturally.

> But I completely agree that the goal is to get better runtime PM
> implementation over all.

Yes agreed.

Regards,

Tony

Re: [PATCH] serial: 8250_port: Fix imprecise external abort for mctrl if inactive

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [200602 13:38]:
> * Andy Shevchenko <andy.shevchenko@gmail.com> [200602 08:33]:
> Now that we can detach and reattach the kernel serial console,
> there should not be any need for pm_runtime_irq_safe() anymore :)

Below is a hastily tested RFC patch to remove pm_runtime_irq_safe()
for 8250_omap.c that seems to work for idle use case :)

> And the UART wake-up from deeper idle states can only happen with
> help of external hardware like GPIO controller or pinctrl controller.

It does not yet include the check for configured wakeirq though.
And omap-serial.c needs a similar patch or maybe we can attempt
to just drop it this time as 8250_omap.c should be used nowadays.
Or just drop PM from omap-serial.c if it can't be dropped.

Andy, is the change below the only remaining blocker now for
your serial PM runtime changes?

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
@@ -123,6 +123,7 @@ struct omap8250_priv {
 	spinlock_t rx_dma_lock;
 	bool rx_dma_broken;
 	bool throttled;
+	unsigned int active:1;
 };
 
 struct omap8250_dma_params {
@@ -598,9 +599,13 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 {
 	struct uart_port *port = dev_id;
 	struct uart_8250_port *up = up_to_u8250p(port);
+	struct omap8250_priv *priv = up->port.private_data;
 	unsigned int iir;
 	int ret;
 
+	if (!priv->active)
+		return IRQ_NONE;
+
 #ifdef CONFIG_SERIAL_8250_DMA
 	if (up->dma) {
 		ret = omap_8250_dma_handle_irq(port);
@@ -608,10 +613,10 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 	}
 #endif
 
-	serial8250_rpm_get(up);
 	iir = serial_port_in(port, UART_IIR);
 	ret = serial8250_handle_irq(port, iir);
-	serial8250_rpm_put(up);
+
+	pm_runtime_mark_last_busy(port->dev);
 
 	return IRQ_RETVAL(ret);
 }
@@ -1110,13 +1115,9 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 	unsigned long flags;
 	u8 iir;
 
-	serial8250_rpm_get(up);
-
 	iir = serial_port_in(port, UART_IIR);
-	if (iir & UART_IIR_NO_INT) {
-		serial8250_rpm_put(up);
+	if (iir & UART_IIR_NO_INT)
 		return IRQ_HANDLED;
-	}
 
 	spin_lock_irqsave(&port->lock, flags);
 
@@ -1144,7 +1145,7 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 	}
 
 	uart_unlock_and_check_sysrq(port, flags);
-	serial8250_rpm_put(up);
+
 	return 1;
 }
 
@@ -1329,11 +1330,10 @@ 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);
+	priv->active = pm_runtime_enabled(&pdev->dev);
 	up.port.handle_irq = omap8250_no_handle_irq;
 	priv->rx_trigger = RX_TRIGGER;
 	priv->tx_trigger = TX_TRIGGER;
@@ -1517,6 +1517,8 @@ static int omap8250_runtime_suspend(struct device *dev)
 	if (!priv)
 		return 0;
 
+	priv->active = 0;
+
 	up = serial8250_get_port(priv->line);
 	/*
 	 * When using 'no_console_suspend', the console UART must not be
@@ -1575,6 +1577,8 @@ static int omap8250_runtime_resume(struct device *dev)
 
 	pinctrl_pm_select_default_state(dev);
 
+	priv->active = 1;
+
 	return 0;
 }
 #endif
-- 
2.26.2

Re: [PATCH] serial: 8250_port: Fix imprecise external abort for mctrl if inactive

[Andy Shevchenko]

On Tue, Jun 02, 2020 at 11:55:15AM -0700, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [200602 13:38]:
> > * Andy Shevchenko <andy.shevchenko@gmail.com> [200602 08:33]:
> > Now that we can detach and reattach the kernel serial console,
> > there should not be any need for pm_runtime_irq_safe() anymore :)
> 
> Below is a hastily tested RFC patch to remove pm_runtime_irq_safe()
> for 8250_omap.c that seems to work for idle use case :)
> 
> > And the UART wake-up from deeper idle states can only happen with
> > help of external hardware like GPIO controller or pinctrl controller.
> 
> It does not yet include the check for configured wakeirq though.
> And omap-serial.c needs a similar patch or maybe we can attempt
> to just drop it this time as 8250_omap.c should be used nowadays.
> Or just drop PM from omap-serial.c if it can't be dropped.
> 
> Andy, is the change below the only remaining blocker now for
> your serial PM runtime changes?

In private chat we have got more or less working solution. We both will going
to give more tests and then I will share (at least as a branch on some public
Git service) the set.

-- 
With Best Regards,
Andy Shevchenko