[PATCH] serial: core: Fix missing shutdown and startup for serial base port

[PATCH] serial: core: Fix missing shutdown and startup for serial base port

[Tony Lindgren]

We are seeing start_tx being called after port shutdown as noted by Jiri.
This happens because we are missing the startup and shutdown related
functions for the serial base port.

Let's fix the issue by adding startup and shutdown functions for the
serial base port to block tx flushing for the serial base port when the
port is not in use.

Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Reported-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/serial_base.h |  4 ++++
 drivers/tty/serial/serial_core.c | 20 ++++++++++++++++---
 drivers/tty/serial/serial_port.c | 34 ++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h
--- a/drivers/tty/serial/serial_base.h
+++ b/drivers/tty/serial/serial_base.h
@@ -22,6 +22,7 @@ struct serial_ctrl_device {
 struct serial_port_device {
 	struct device dev;
 	struct uart_port *port;
+	unsigned int tx_enabled:1;
 };
 
 int serial_base_ctrl_init(void);
@@ -30,6 +31,9 @@ void serial_base_ctrl_exit(void);
 int serial_base_port_init(void);
 void serial_base_port_exit(void);
 
+void serial_base_port_startup(struct uart_port *port);
+void serial_base_port_shutdown(struct uart_port *port);
+
 int serial_base_driver_register(struct device_driver *driver);
 void serial_base_driver_unregister(struct device_driver *driver);
 
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -323,16 +323,26 @@ static int uart_startup(struct tty_struct *tty, struct uart_state *state,
 			bool init_hw)
 {
 	struct tty_port *port = &state->port;
+	struct uart_port *uport;
 	int retval;
 
 	if (tty_port_initialized(port))
-		return 0;
+		goto out_base_port_startup;
 
 	retval = uart_port_startup(tty, state, init_hw);
-	if (retval)
+	if (retval) {
 		set_bit(TTY_IO_ERROR, &tty->flags);
+		return retval;
+	}
 
-	return retval;
+out_base_port_startup:
+	uport = uart_port_check(state);
+	if (!uport)
+		return -EIO;
+
+	serial_base_port_startup(uport);
+
+	return 0;
 }
 
 /*
@@ -355,6 +365,9 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
 	if (tty)
 		set_bit(TTY_IO_ERROR, &tty->flags);
 
+	if (uport)
+		serial_base_port_shutdown(uport);
+
 	if (tty_port_initialized(port)) {
 		tty_port_set_initialized(port, false);
 
@@ -1775,6 +1788,7 @@ static void uart_tty_port_shutdown(struct tty_port *port)
 	uport->ops->stop_rx(uport);
 	uart_port_unlock_irq(uport);
 
+	serial_base_port_shutdown(uport);
 	uart_port_shutdown(port);
 
 	/*
diff --git a/drivers/tty/serial/serial_port.c b/drivers/tty/serial/serial_port.c
--- a/drivers/tty/serial/serial_port.c
+++ b/drivers/tty/serial/serial_port.c
@@ -39,8 +39,12 @@ static int serial_port_runtime_resume(struct device *dev)
 
 	/* Flush any pending TX for the port */
 	uart_port_lock_irqsave(port, &flags);
+	if (!port_dev->tx_enabled)
+		goto unlock;
 	if (__serial_port_busy(port))
 		port->ops->start_tx(port);
+
+unlock:
 	uart_port_unlock_irqrestore(port, flags);
 
 out:
@@ -60,6 +64,11 @@ static int serial_port_runtime_suspend(struct device *dev)
 		return 0;
 
 	uart_port_lock_irqsave(port, &flags);
+	if (!port_dev->tx_enabled) {
+		uart_port_unlock_irqrestore(port, flags);
+		return 0;
+	}
+
 	busy = __serial_port_busy(port);
 	if (busy)
 		port->ops->start_tx(port);
@@ -71,6 +80,31 @@ static int serial_port_runtime_suspend(struct device *dev)
 	return busy ? -EBUSY : 0;
 }
 
+static void serial_base_port_set_tx(struct uart_port *port,
+				    struct serial_port_device *port_dev,
+				    bool enabled)
+{
+	unsigned long flags;
+
+	uart_port_lock_irqsave(port, &flags);
+	port_dev->tx_enabled = enabled;
+	uart_port_unlock_irqrestore(port, flags);
+}
+
+void serial_base_port_startup(struct uart_port *port)
+{
+	struct serial_port_device *port_dev = port->port_dev;
+
+	serial_base_port_set_tx(port, port_dev, true);
+}
+
+void serial_base_port_shutdown(struct uart_port *port)
+{
+	struct serial_port_device *port_dev = port->port_dev;
+
+	serial_base_port_set_tx(port, port_dev, false);
+}
+
 static DEFINE_RUNTIME_DEV_PM_OPS(serial_port_pm,
 				 serial_port_runtime_suspend,
 				 serial_port_runtime_resume, NULL);
-- 
2.44.0

Re: [PATCH] serial: core: Fix missing shutdown and startup for serial base port

[Andy Shevchenko]

On Thu, Apr 11, 2024 at 08:58:45AM +0300, Tony Lindgren wrote:
> We are seeing start_tx being called after port shutdown as noted by Jiri.
> This happens because we are missing the startup and shutdown related
> functions for the serial base port.
> 
> Let's fix the issue by adding startup and shutdown functions for the
> serial base port to block tx flushing for the serial base port when the
> port is not in use.

I'm going to test this later today, meanwhile some comments below.

...

> +out_base_port_startup:
> +	uport = uart_port_check(state);
> +	if (!uport)
> +		return -EIO;
> +
> +	serial_base_port_startup(uport);

So, we call this even on uninitialised TTY. Is it okay?

> +	return 0;
>  }


...

>  	if (tty)
>  		set_bit(TTY_IO_ERROR, &tty->flags);

> +	if (uport)
> +		serial_base_port_shutdown(uport);

Why not to call it after the below check to be reverse-symmetrical with startup?

>  	if (tty_port_initialized(port)) {
>  		tty_port_set_initialized(port, false);
>  

...

>  	/* Flush any pending TX for the port */
>  	uart_port_lock_irqsave(port, &flags);
> +	if (!port_dev->tx_enabled)
> +		goto unlock;

Can't this be integrated into...

>  	if (__serial_port_busy(port))

...this call?

>  		port->ops->start_tx(port);
> +
> +unlock:
>  	uart_port_unlock_irqrestore(port, flags);
>  

...

>  	uart_port_lock_irqsave(port, &flags);
> +	if (!port_dev->tx_enabled) {
> +		uart_port_unlock_irqrestore(port, flags);
> +		return 0;
> +	}
> +
>  	busy = __serial_port_busy(port);
>  	if (busy)
>  		port->ops->start_tx(port);

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: core: Fix missing shutdown and startup for serial base port

[Tony Lindgren]

Hi,

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [240411 13:06]:
> On Thu, Apr 11, 2024 at 08:58:45AM +0300, Tony Lindgren wrote:
> > +out_base_port_startup:
> > +	uport = uart_port_check(state);
> > +	if (!uport)
> > +		return -EIO;
> > +
> > +	serial_base_port_startup(uport);
> 
> So, we call this even on uninitialised TTY. Is it okay?

To me it seems we should do it unconditionally unless there are
reasons to tie to the TTY init logic. This should be checked
though, maybe it needs to be tied to the TTY logic.

> >  	/* Flush any pending TX for the port */
> >  	uart_port_lock_irqsave(port, &flags);
> > +	if (!port_dev->tx_enabled)
> > +		goto unlock;
> 
> Can't this be integrated into...
> 
> >  	if (__serial_port_busy(port))
> 
> ...this call?

Yes so it seems thanks, it can be done to simplify the code.

Regards,

Tony

Re: [PATCH] serial: core: Fix missing shutdown and startup for serial base port

[Andy Shevchenko]

On Thu, Apr 11, 2024 at 08:58:45AM +0300, Tony Lindgren wrote:
> We are seeing start_tx being called after port shutdown as noted by Jiri.
> This happens because we are missing the startup and shutdown related
> functions for the serial base port.
> 
> Let's fix the issue by adding startup and shutdown functions for the
> serial base port to block tx flushing for the serial base port when the
> port is not in use.

I tried to test this on the current max3100.c driver, but this doesn't change
anything to me. The scenario is that:

- load the driver with dyndbg on
- attach device (I have done it via SSDT overlay)
- call `stty -F /dev/ttyMAX0` to see it works
- call `stty -F /dev/ttyMAX0 115200` to setup speed
- test case:
  a) run `cat /proc/interrupts > /dev/ttyMAX0`
  b) press Ctrl + C
  c) (most cases) press Ctrl + C
- repeat the previous step several times

The outcome (with or without this change) is that
- it repeatedly calls start_tx()
- most of the times as you may notice it requires actually to press
  Ctrl + C _twice to stop the queueing

The testing environment is the tty-next + this patch.

I admit that max3100 may be buggy, but this change doesn't fix anything for it.

-- 
With Best Regards,
Andy Shevchenko