[PATCH 0/5] tty: fixes on top of summer cleanup

[PATCH 0/5] tty: fixes on top of summer cleanup

[Jiri Slaby (SUSE)]

These are fixes which were suggested or popped out of the summer cleanup
series.

Jiri Slaby (SUSE) (5):
  serial: 8250: extract serial8250_init_mctrl()
  serial: 8250: extract serial8250_iir_txen_test()
  serial: 8250: rename lsr_TEMT, iir_NOINT to lowercase
  serial: 8250: document doubled "type == PORT_8250_CIR" check
  tty: fix tty_port_tty_*hangup() kernel-doc

 Documentation/driver-api/tty/tty_port.rst |  5 +-
 drivers/tty/serial/8250/8250_core.c       |  2 +
 drivers/tty/serial/8250/8250_port.c       | 73 ++++++++++++++---------
 drivers/tty/tty_port.c                    |  5 --
 include/linux/tty_port.h                  |  9 +++
 5 files changed, 58 insertions(+), 36 deletions(-)

-- 
2.49.0

[PATCH 1/5] serial: 8250: extract serial8250_init_mctrl()

[Jiri Slaby (SUSE)]

After commit 795158691cc0 ("serial: 8250: extract
serial8250_initialize()"), split serial8250_initialize() even more --
the mctrl part of this code can be separated into
serial8250_init_mctrl() -- done now.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 48c30e158cb8..ca82ce26715a 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2216,6 +2216,23 @@ static void serial8250_THRE_test(struct uart_port *port)
 		up->bugs |= UART_BUG_THRE;
 }
 
+static void serial8250_init_mctrl(struct uart_port *port)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+
+	if (up->port.flags & UPF_FOURPORT) {
+		if (!up->port.irq)
+			up->port.mctrl |= TIOCM_OUT1;
+	} else
+		/*
+		 * Most PC uarts need OUT2 raised to enable interrupts.
+		 */
+		if (port->irq)
+			up->port.mctrl |= TIOCM_OUT2;
+
+	serial8250_set_mctrl(port, port->mctrl);
+}
+
 static void serial8250_initialize(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
@@ -2225,16 +2242,7 @@ static void serial8250_initialize(struct uart_port *port)
 	serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
 
 	uart_port_lock_irqsave(port, &flags);
-	if (port->flags & UPF_FOURPORT) {
-		if (!port->irq)
-			port->mctrl |= TIOCM_OUT1;
-	} else {
-		/* Most PC uarts need OUT2 raised to enable interrupts. */
-		if (port->irq)
-			port->mctrl |= TIOCM_OUT2;
-	}
-
-	serial8250_set_mctrl(port, port->mctrl);
+	serial8250_init_mctrl(port);
 
 	/*
 	 * Serial over Lan (SoL) hack:
-- 
2.49.0

[PATCH 2/5] serial: 8250: extract serial8250_iir_txen_test()

[Jiri Slaby (SUSE)]

After commit 795158691cc0 ("serial: 8250: extract
serial8250_initialize()"), split serial8250_initialize() even more --
the TX enable test part of this code can be separated into
serial8250_iir_txen_test().

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 49 ++++++++++++++++-------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index ca82ce26715a..584563c45424 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2233,16 +2233,19 @@ static void serial8250_init_mctrl(struct uart_port *port)
 	serial8250_set_mctrl(port, port->mctrl);
 }
 
-static void serial8250_initialize(struct uart_port *port)
+static void serial8250_iir_txen_test(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
-	unsigned long flags;
 	bool lsr_TEMT, iir_NOINT;
 
-	serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
+	if (port->quirks & UPQ_NO_TXEN_TEST)
+		return;
 
-	uart_port_lock_irqsave(port, &flags);
-	serial8250_init_mctrl(port);
+	/* Do a quick test to see if we receive an interrupt when we enable the TX irq. */
+	serial_port_out(port, UART_IER, UART_IER_THRI);
+	lsr_TEMT = serial_port_in(port, UART_LSR) & UART_LSR_TEMT;
+	iir_NOINT = serial_port_in(port, UART_IIR) & UART_IIR_NO_INT;
+	serial_port_out(port, UART_IER, 0);
 
 	/*
 	 * Serial over Lan (SoL) hack:
@@ -2250,26 +2253,30 @@ static void serial8250_initialize(struct uart_port *port)
 	 * Lan.  Those chips take a longer time than a normal serial device to signalize that a
 	 * transmission data was queued. Due to that, the above test generally fails. One solution
 	 * would be to delay the reading of iir. However, this is not reliable, since the timeout is
-	 * variable. So, let's just don't test if we receive TX irq.  This way, we'll never enable
-	 * UART_BUG_TXEN.
+	 * variable. So, in case of UPQ_NO_TXEN_TEST, let's just don't test if we receive TX irq.
+	 * This way, we'll never enable UART_BUG_TXEN.
 	 */
-	if (!(port->quirks & UPQ_NO_TXEN_TEST)) {
-		/* Do a quick test to see if we receive an interrupt when we enable the TX irq. */
-		serial_port_out(port, UART_IER, UART_IER_THRI);
-		lsr_TEMT = serial_port_in(port, UART_LSR) & UART_LSR_TEMT;
-		iir_NOINT = serial_port_in(port, UART_IIR) & UART_IIR_NO_INT;
-		serial_port_out(port, UART_IER, 0);
-
-		if (lsr_TEMT && iir_NOINT) {
-			if (!(up->bugs & UART_BUG_TXEN)) {
-				up->bugs |= UART_BUG_TXEN;
-				dev_dbg(port->dev, "enabling bad tx status workarounds\n");
-			}
-		} else {
-			up->bugs &= ~UART_BUG_TXEN;
+	if (lsr_TEMT && iir_NOINT) {
+		if (!(up->bugs & UART_BUG_TXEN)) {
+			up->bugs |= UART_BUG_TXEN;
+			dev_dbg(port->dev, "enabling bad tx status workarounds\n");
 		}
+		return;
 	}
 
+	/* FIXME: why is this needed? */
+	up->bugs &= ~UART_BUG_TXEN;
+}
+
+static void serial8250_initialize(struct uart_port *port)
+{
+	unsigned long flags;
+
+	serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
+
+	uart_port_lock_irqsave(port, &flags);
+	serial8250_init_mctrl(port);
+	serial8250_iir_txen_test(port);
 	uart_port_unlock_irqrestore(port, flags);
 }
 
-- 
2.49.0

[PATCH 3/5] serial: 8250: rename lsr_TEMT, iir_NOINT to lowercase

[Jiri Slaby (SUSE)]

There are already variables like 'iir_noint1' and 'iir_noint2'. Follow
the preexisting lowercase naming of variables. So s/lsr_TEMT/lsr_temt/
and 'iir_NOINT' likewise.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 584563c45424..f67b206d1676 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2236,15 +2236,15 @@ static void serial8250_init_mctrl(struct uart_port *port)
 static void serial8250_iir_txen_test(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
-	bool lsr_TEMT, iir_NOINT;
+	bool lsr_temt, iir_noint;
 
 	if (port->quirks & UPQ_NO_TXEN_TEST)
 		return;
 
 	/* Do a quick test to see if we receive an interrupt when we enable the TX irq. */
 	serial_port_out(port, UART_IER, UART_IER_THRI);
-	lsr_TEMT = serial_port_in(port, UART_LSR) & UART_LSR_TEMT;
-	iir_NOINT = serial_port_in(port, UART_IIR) & UART_IIR_NO_INT;
+	lsr_temt = serial_port_in(port, UART_LSR) & UART_LSR_TEMT;
+	iir_noint = serial_port_in(port, UART_IIR) & UART_IIR_NO_INT;
 	serial_port_out(port, UART_IER, 0);
 
 	/*
@@ -2256,7 +2256,7 @@ static void serial8250_iir_txen_test(struct uart_port *port)
 	 * variable. So, in case of UPQ_NO_TXEN_TEST, let's just don't test if we receive TX irq.
 	 * This way, we'll never enable UART_BUG_TXEN.
 	 */
-	if (lsr_TEMT && iir_NOINT) {
+	if (lsr_temt && iir_noint) {
 		if (!(up->bugs & UART_BUG_TXEN)) {
 			up->bugs |= UART_BUG_TXEN;
 			dev_dbg(port->dev, "enabling bad tx status workarounds\n");
-- 
2.49.0

[PATCH 4/5] serial: 8250: document doubled "type == PORT_8250_CIR" check

[Jiri Slaby (SUSE)]

The check for "port.type == PORT_8250_CIR" is present twice in
serial8250_register_8250_port(). The latter was already tried to be
dropped by 1104321a7b3b ("serial: Delete dead code for CIR serial
ports") and then reverted by 9527b82ae3af ("Revert "serial: Delete dead
code for CIR serial ports"").

Document this weirdness with a reason.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Link: https://lore.kernel.org/all/aFcDOx1bdB34I5hS@surfacebook.localdomain/
---
 drivers/tty/serial/8250/8250_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index a6ecb8575da4..feb920c5b2e8 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -717,6 +717,7 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
 		nr_uarts++;
 	}
 
+	/* Check if it is CIR already. We check this below again, see there why. */
 	if (uart->port.type == PORT_8250_CIR) {
 		ret = -ENODEV;
 		goto unlock;
@@ -815,6 +816,7 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
 	if (up->dl_write)
 		uart->dl_write = up->dl_write;
 
+	/* Check the type (again)! It might have changed by the port.type assignment above. */
 	if (uart->port.type != PORT_8250_CIR) {
 		if (uart_console_registered(&uart->port))
 			pm_runtime_get_sync(uart->port.dev);
-- 
2.49.0

[PATCH 5/5] tty: fix tty_port_tty_*hangup() kernel-doc

[Jiri Slaby (SUSE)]

The commit below added a new helper, but omitted to move (and add) the
corressponding kernel-doc. Do it now.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Fixes: 2b5eac0f8c6e ("tty: introduce and use tty_port_tty_vhangup() helper")
Link: https://lore.kernel.org/all/b23d566c-09dc-7374-cc87-0ad4660e8b2e@linux.intel.com/
Reported-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-doc@vger.kernel.org
---
 Documentation/driver-api/tty/tty_port.rst | 5 +++--
 drivers/tty/tty_port.c                    | 5 -----
 include/linux/tty_port.h                  | 9 +++++++++
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/Documentation/driver-api/tty/tty_port.rst b/Documentation/driver-api/tty/tty_port.rst
index 5cb90e954fcf..504a353f2682 100644
--- a/Documentation/driver-api/tty/tty_port.rst
+++ b/Documentation/driver-api/tty/tty_port.rst
@@ -42,9 +42,10 @@ TTY Refcounting
 TTY Helpers
 -----------
 
+.. kernel-doc::  include/linux/tty_port.h
+   :identifiers: tty_port_tty_hangup tty_port_tty_vhangup
 .. kernel-doc::  drivers/tty/tty_port.c
-   :identifiers: tty_port_tty_hangup tty_port_tty_wakeup
-
+   :identifiers: tty_port_tty_wakeup
 
 Modem Signals
 -------------
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 903eebdbe12d..5b4d5fb99a59 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -391,11 +391,6 @@ void tty_port_hangup(struct tty_port *port)
 }
 EXPORT_SYMBOL(tty_port_hangup);
 
-/**
- * tty_port_tty_hangup - helper to hang up a tty
- * @port: tty port
- * @check_clocal: hang only ttys with %CLOCAL unset?
- */
 void __tty_port_tty_hangup(struct tty_port *port, bool check_clocal, bool async)
 {
 	struct tty_struct *tty = tty_port_tty_get(port);
diff --git a/include/linux/tty_port.h b/include/linux/tty_port.h
index 021f9a8415c0..332ddb93603e 100644
--- a/include/linux/tty_port.h
+++ b/include/linux/tty_port.h
@@ -251,11 +251,20 @@ static inline int tty_port_users(struct tty_port *port)
 	return port->count + port->blocked_open;
 }
 
+/**
+ * tty_port_tty_hangup - helper to hang up a tty asynchronously
+ * @port: tty port
+ * @check_clocal: hang only ttys with %CLOCAL unset?
+ */
 static inline void tty_port_tty_hangup(struct tty_port *port, bool check_clocal)
 {
 	__tty_port_tty_hangup(port, check_clocal, true);
 }
 
+/**
+ * tty_port_tty_vhangup - helper to hang up a tty synchronously
+ * @port: tty port
+ */
 static inline void tty_port_tty_vhangup(struct tty_port *port)
 {
 	__tty_port_tty_hangup(port, false, false);
-- 
2.49.0

Re: [PATCH 4/5] serial: 8250: document doubled "type == PORT_8250_CIR" check

[Andy Shevchenko]

On Mon, Jun 23, 2025 at 10:46 AM Jiri Slaby (SUSE) <jirislaby@kernel.org> wrote:
>
> The check for "port.type == PORT_8250_CIR" is present twice in
> serial8250_register_8250_port(). The latter was already tried to be
> dropped by 1104321a7b3b ("serial: Delete dead code for CIR serial
> ports") and then reverted by 9527b82ae3af ("Revert "serial: Delete dead
> code for CIR serial ports"").
>
> Document this weirdness with a reason.

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] serial: 8250: extract serial8250_init_mctrl()

[Andy Shevchenko]

Mon, Jun 23, 2025 at 09:46:02AM +0200, Jiri Slaby (SUSE) kirjoitti:
> After commit 795158691cc0 ("serial: 8250: extract
> serial8250_initialize()"), split serial8250_initialize() even more --
> the mctrl part of this code can be separated into
> serial8250_init_mctrl() -- done now.

...

> +static void serial8250_init_mctrl(struct uart_port *port)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +
> +	if (up->port.flags & UPF_FOURPORT) {
> +		if (!up->port.irq)
> +			up->port.mctrl |= TIOCM_OUT1;

I am not sure I understand why it was changed from using port directly to
up->port.

> +	} else
> +		/*
> +		 * Most PC uarts need OUT2 raised to enable interrupts.
> +		 */
> +		if (port->irq)
> +			up->port.mctrl |= TIOCM_OUT2;

Having {} in this branch is also better.

> +	serial8250_set_mctrl(port, port->mctrl);
> +}

...

I specifically left below to point out the original code.

> -	if (port->flags & UPF_FOURPORT) {
> -		if (!port->irq)
> -			port->mctrl |= TIOCM_OUT1;
> -	} else {
> -		/* Most PC uarts need OUT2 raised to enable interrupts. */
> -		if (port->irq)
> -			port->mctrl |= TIOCM_OUT2;
> -	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] serial: 8250: extract serial8250_init_mctrl()

[Jiri Slaby]

On 23. 06. 25, 20:11, Andy Shevchenko wrote:
> Mon, Jun 23, 2025 at 09:46:02AM +0200, Jiri Slaby (SUSE) kirjoitti:
>> After commit 795158691cc0 ("serial: 8250: extract
>> serial8250_initialize()"), split serial8250_initialize() even more --
>> the mctrl part of this code can be separated into
>> serial8250_init_mctrl() -- done now.
> 
> ...
> 
>> +static void serial8250_init_mctrl(struct uart_port *port)
>> +{
>> +	struct uart_8250_port *up = up_to_u8250p(port);
>> +
>> +	if (up->port.flags & UPF_FOURPORT) {
>> +		if (!up->port.irq)
>> +			up->port.mctrl |= TIOCM_OUT1;
> 
> I am not sure I understand why it was changed from using port directly to
> up->port.

Ugh, that's a mistake. During rebase likely. Thanks for catching.


-- 
js
suse labs