[PATCH v3 0/2] serial: core: Don't clobber the baud rate on hangup via B0

[PATCH v3 0/2] serial: core: Don't clobber the baud rate on hangup via B0

[Maciej W. Rozycki]

[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]

Greg,

 This is v3 of the change for device baud rate preservation where the B0 
rate has been requested for hangup that we discussed at LPC.

 Following the observations regarding v2 I have now excluded the 8250 
console restoration path from rewriting port->hup_speed; this path only 
takes the most recent baud rate previously set and reinstates it with 
hardware without changing the setting chosen.  See the individual patches 
for further details of the changes made.

 I have verified it with the system and driver originally affected, that 
is a Broadcom BCM91250A "SWARM" system and sb1250-duart driver, but now 
also an MTI Malta system and the 8250 driver, with both showing the same 
symptoms on real hardware (worth mentioning since Malta is commonly used 
emulated in QEMU); cf. SIBYTE_SWARM and MIPS_MALTA in arch/mips/Kconfig.  
I have retained the console output logs previously attached for the bad vs 
fixed case; output is virtually the same across both systems.

 All that needed to be done to reproduce it was to keep killing getty(8)s 
repeatedly until init(8) decided something was wrong and chose to put 
their inittab(5) entries into a snooze, leaving the line(s) at the wrong 
speed setting.  I have done further experimentation with actual kernel 
console output by issuing:

# echo h >/proc/sysrq-trigger

once the console port has been hung up and that obviously also produced 
corrupt output without the fix and correct output with the fix applied.

 I have no means to verify the serial8250_console_restore() path; the fix 
is expected to make no functional change there.

 Since the return path from uart_get_baud_rate() has become more complex 
now than just supplying the baud rate chosen to the caller I went back to 
the original idea of having this as a small patch series, where 1/2 merely 
merges the two successful return paths into one.

 NB this fixes the missing %B0 literal markup as a part of the doc update.  
I've concluded it's not worth splitting off into a separate patch.  Let me 
know if you think otherwise.

 Previous iterations:

- v2 at: <https://lore.kernel.org/r/alpine.DEB.2.21.2610120150230.55767@angie.orcam.me.uk/>.

- v1 at: <https://lore.kernel.org/r/alpine.DEB.2.21.2605242242330.1450@angie.orcam.me.uk/>.

  Maciej

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/plain; CHARSET=US-ASCII; NAME=swarm-bad.log, Size: 1526 bytes --]

^[[H^[[J\rswarm.orcam.me.uk login: \0þ\0þ\0\0\0\0\0\0þ\0\0ð\0\0ð\0þ\0\0ð\0\0ð\0þ\0þ\0\0ð\0\0ð\0þ\0\0\0À\0þ\0þ\0þ\0\0ð\0þ\0\0ð\0\0\0\0\0\0\0ø\0\0ð\0þ\0\0ð\0\0ð\0þ\0\0\0À\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0ð\0\0\0À\0\0ð\0þ\0\0ð\0þ\0þ\0\0\0\0\0\0þ\0\0ð\0\0ð\0\0\0À\0\0ð\0\0\0\0\0\0\0ø\0\0ð\0\0ð\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0þ\0þ\0þ\0\0\0\0\0\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0\0ð\0\0ð\0\0\0À\0þ\0þ\0þ\0þ\0þ\0þ\0þ\0þ\0\0\0\0\0\0\0ð\0\0ð\0\0ð\0\0ð\0þ\0þ\0\0\0\0\0\0þ\0þ\0\0ð\0þ\0\0\0À\0þ\0þ\0\0ð\0þ\0\0ð\0\0\0\0\0\0\0ð\0\0ð\0\0\0À\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0\0\0\0\0þ\0\0ð\0\0\0À\0þ\0\0\0À\0þ\0þ\0þ\0þ\0\0ð\0þ\0\0\0À\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0ð\0\0ð\0þ\0þ\0þ\0þ\0\0ð\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0þ\0þ\0þ\0\0ð\0\0\0\0\0\0\0ø\0\0ð\0þ\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0þ\0þ\0þ\0\0\0À\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0\0\0\0\0\0ð\0þ\0\0\0\0\0\0þ\0þ\0\0\0\0\0\0þ\0\0ð\0\0ð\0þ\0\0ð\0\0ð\0þ\0þ\0\0ð\0\0ð\0þ\0\0\0À\0þ\0þ\0þ\0\0ð\0þ\0\0ð\0\0\0\0\0\0\0ø\0\0ð\0þ\0\0ð\0\0ð\0þ\0\0\0À\0\0ð\0þ\0\0\0\0\0\0\0ð\0\0ð\0\0ð\0\0\0À\0\0ð\0þ\0\0ð\0þ\0þ\0þ\0\0\0À\0\0ð\0\0ð\0\0\0À\0\0ð\0\0\0\0\0\0\0ð\0\0ð\0\0ð\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0þ\0þ\0þ\0\0\0\0\0\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0\0À\0þ\0þ\0þ\0þ\0þ\0þ\0þ\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0ð\0\0ð\0þ\0þ\0\0\0\0\0\0þ\0þ\0\0ð\0þ\0\0\0À\0þ\0þ\0\0ð\0þ\0\0ð\0\0\0\0\0\0\0ø\0\0ð\0\0\0À\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0\0\0\0\0þ\0\0ð\0\0\0À\0þ\0\0\0À\0þ\0þ\0þ\0þ\0\0ð\0þ\0\0\0À\0\0ð\0þ\0\0\0\0\0\0\0ð\0\0ð\0\0ð\0\0ð\0þ\0þ\0þ\0þ\0\0ð\0\0ð\0þ\0\0\0\0\0\0\0ð\0\0ð\0þ\0þ\0þ\0\0ð\0\0\0\0\0\0\0ø\0\0ð\0þ\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0þ\0þ\0þ\0\0\0À\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0\0\0\0\0\0ð\0þ\0\0\0\0\0\0þ\0þ\0\0\0\0\0\0þ\0\0ð\0\0ð\0þ\0\0ð\0\0ð\0þ\0þ\0\0ð\0\0ð\0þ\0\0\0À\0þ\0þ\0þ\0\0ð\0þ\0\0ð\0\0\0\0\0\0\0ð\0\0ð\0\0ð\0þ\0þ\0þ\0þ\0þ\0\0\0\0\0\0\0ø\0\0ð\0þ\0þ\0þ\0þ\0þ\0þ\0þ\0\0ð\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0\0\0\0\0þ\0þ\0\0ð\0\0ð\0þ\0þ\0þ\0þ\0þ\0\0\0À\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0\0À\0þ\0þ\0þ\0þ\0\0ð\0þ\0\0ð\0\0ð\0þ\0\0\0À\0þ\0þ\0\0\0\0\0\0\0ð\0\0ð\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0\0\0\0\0\0\0ð\0\0ð\0\0\0À\0þ\0þ\0\0\0\0\0\0þ\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0ð\0\0ð\0þ\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0\0À\0þ\0þ\0þ\0þ\0\0ð\0þ\0\0ð\0þ\0þ\0þ\0þ\0\0ð\0þ\0\0\0À\0þ\0þ\0þ\0þ\0\0\0\0\0\0\0ð\0þ\0\0\0\0\0^[[H^[[J\rswarm.orcam.me.uk login: 

[-- Attachment #3: Type: text/plain, Size: 229 bytes --]

^[[H^[[Jswarm.orcam.me.uk login: INIT: Id "S0" respawning too fast: disabled for 5 minutes
INIT: Id "S1" respawning too fast: disabled for 5 minutes
INIT: no more processes left in this runlevel
^[[H^[[Jswarm.orcam.me.uk login: 

[PATCH v3 1/2] serial: core: Unify uart_get_baud_rate() success return paths

[Maciej W. Rozycki]

In preparation to adding extra return actions merge the two success 
return paths in uart_get_baud_rate() into one.

Since as from commit 091ea8e5d34e ("serial: core: prevent division by 
zero by always returning non-zero baud rate") it is now guaranteed that 
the B0 baud rate is within [min; max], deferring to the regular return 
path in the B0 case causes no functional change.

That is as long as max >= max(min, 9600), which I believe is always the 
case and none of UART hardware our drivers provide support for has its 
highest baud rate below 9600.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
New change in v3.
---
 drivers/tty/serial/serial_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

linux-serial-uart-get-baud-rate-return.diff
Index: linux-macro/drivers/tty/serial/serial_core.c
===================================================================
--- linux-macro.orig/drivers/tty/serial/serial_core.c
+++ linux-macro/drivers/tty/serial/serial_core.c
@@ -515,7 +515,7 @@ uart_get_baud_rate(struct uart_port *por
 		 * Special case: B0 rate.
 		 */
 		if (baud == 0)
-			return max(min, 9600);
+			baud = max(min, 9600);
 
 		if (baud >= min && baud <= max)
 			return baud;

[PATCH v3 2/2] serial: core: Don't clobber the baud rate on hangup via B0

[Maciej W. Rozycki]

Requesting hangup via the B0 baud rate has this nasty side effect of 
clobbering the previously set rate for the port unless it happens to be 
9600bps or the port minimum rate if higher.  Consequently if there's a 
consumer still active at the other end, then it receives corrupt data 
from any other producers outputting messages to the port, such as 
init(8) or the kernel console.

There doesn't appear to be any particular reason for this clobbering 
other than that we have been doing it since 2.1.131, so take a saner 
approach and keep track of the most recently set baud rate and then use 
it whenever hangup has been requested.

There is one issue however with serial8250_console_restore(), possibly 
called from the NMI context on some platforms, in which case there is a 
concurrency issue owing to `port->mutex' not being held by the function.  
Consequently `port->hup_speed' could be written midway through an update 
being made by a termios call, causing the value saved to clobber the 
baud rate programmed into hardware with a later B0 rate selection.

Handle it by renaming uart_get_baud_rate() to uart_get_baud_rate_save() 
and giving the function an extra `save' parameter to control whether or 
not to have the baud rate saved in `port->hup_speed' that is returned by 
the function.  Then use that parameter to suppress saving when called by 
serial8250_console_restore(), observing that the call is only ever made 
to restore the baud rate most recently set.  Any concurrent interrupted 
termios call, once it has been passed control again, will then program 
hardware consistently with the value it may have written already or will 
otherwise write to `port->hup_speed'.

In the unlikely case `port->hup_speed' is zero where the B0 rate has 
been requested as the first one since the system boot for a non-console 
device, resort to the current default of max(min, 9600).

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
Changes from v2, 
<https://lore.kernel.org/r/alpine.DEB.2.21.2610120231550.55767@angie.orcam.me.uk/>:

- Add a `save' parameter to uart_get_baud_rate() and rename the function 
  to uart_get_baud_rate_save().

- Add a uart_get_baud_rate() static inline wrapper for regular cases in 
  drivers such as handling termios calls.

- Use the `save' parameter appropriately in serial8250_get_baud_rate() to 
  handle serial8250_console_restore() calls.

Changes from v1, 
<https://lore.kernel.org/r/alpine.DEB.2.21.2605242242330.1450@angie.orcam.me.uk/>:

- Cache the most recently set actual baud rate in port->hup_speed and use 
  it whenever the B0 rate has been requested since the old termios may be 
  NULL or have the B0 rate recorded.
---
 drivers/tty/serial/8250/8250_port.c |    9 +++++----
 drivers/tty/serial/serial_core.c    |   25 ++++++++++++++++---------
 include/linux/serial_core.h         |   28 +++++++++++++++++++++++++---
 3 files changed, 46 insertions(+), 16 deletions(-)

linux-serial-uart-get-baud-rate-hup.diff
Index: linux-macro/drivers/tty/serial/8250/8250_port.c
===================================================================
--- linux-macro.orig/drivers/tty/serial/8250/8250_port.c
+++ linux-macro/drivers/tty/serial/8250/8250_port.c
@@ -2626,7 +2626,8 @@ static void serial8250_set_divisor(struc
 
 static unsigned int serial8250_get_baud_rate(struct uart_port *port,
 					     struct ktermios *termios,
-					     const struct ktermios *old)
+					     const struct ktermios *old,
+					     bool save)
 {
 	unsigned int tolerance = port->uartclk / 100;
 	unsigned int min;
@@ -2651,7 +2652,7 @@ static unsigned int serial8250_get_baud_
 	 * slower than nominal still match standard baud rates without
 	 * causing transmission errors.
 	 */
-	return uart_get_baud_rate(port, termios, old, min, max);
+	return uart_get_baud_rate_save(port, termios, old, min, max, save);
 }
 
 /*
@@ -2842,7 +2843,7 @@ serial8250_do_set_termios(struct uart_po
 
 	serial8250_set_mini(port, termios);
 	lcr = serial8250_compute_lcr(up, termios->c_cflag);
-	baud = serial8250_get_baud_rate(port, termios, old);
+	baud = serial8250_get_baud_rate(port, termios, old, true);
 	quot = serial8250_get_divisor(port, baud, &frac);
 
 	/*
@@ -3339,7 +3340,7 @@ static void serial8250_console_restore(s
 		termios.c_ospeed = port->state->port.tty->termios.c_ospeed;
 	}
 
-	baud = serial8250_get_baud_rate(port, &termios, NULL);
+	baud = serial8250_get_baud_rate(port, &termios, NULL, false);
 	quot = serial8250_get_divisor(port, baud, &frac);
 
 	serial8250_set_divisor(port, baud, quot, frac);
Index: linux-macro/drivers/tty/serial/serial_core.c
===================================================================
--- linux-macro.orig/drivers/tty/serial/serial_core.c
+++ linux-macro/drivers/tty/serial/serial_core.c
@@ -454,29 +454,32 @@ uart_update_timeout(struct uart_port *po
 EXPORT_SYMBOL(uart_update_timeout);
 
 /**
- * uart_get_baud_rate - return baud rate for a particular port
+ * uart_get_baud_rate_save - return baud rate for a particular port
  * @port: uart_port structure describing the port in question.
  * @termios: desired termios settings
  * @old: old termios (or %NULL)
  * @min: minimum acceptable baud rate
  * @max: maximum acceptable baud rate
+ * @save: whether to save the baud rate chosen
  *
  * Decode the termios structure into a numeric baud rate, taking account of the
- * magic 38400 baud rate (with spd_* flags), and mapping the %B0 rate to 9600
+ * magic 38400 baud rate (with spd_* flags), and mapping the %B0 rate to the
+ * previously saved rate provided by @port, or if this is unset, then 9600
  * baud or min argument, whichever is greater.
  *
  * If the new baud rate is invalid, try the @old termios setting. If it's still
  * invalid, clip to the nearest chip supported rate.
  *
  * The @termios structure is updated to reflect the baud rate we're actually
- * going to be using. Don't do this for the case where B0 is requested ("hang
- * up").
+ * going to be using, except where %B0 is requested ("hang up"). Likewise the
+ * @port structure, except where @save is %false.
  *
  * Locking: caller dependent
  */
 unsigned int
-uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
-		   const struct ktermios *old, unsigned int min, unsigned int max)
+uart_get_baud_rate_save(struct uart_port *port,
+			struct ktermios *termios, const struct ktermios *old,
+			unsigned int min, unsigned int max, bool save)
 {
 	unsigned int try;
 	unsigned int baud;
@@ -515,10 +518,14 @@ uart_get_baud_rate(struct uart_port *por
 		 * Special case: B0 rate.
 		 */
 		if (baud == 0)
-			baud = max(min, 9600);
+			baud = (port->hup_speed ?
+				port->hup_speed : max(min, 9600));
 
-		if (baud >= min && baud <= max)
+		if (baud >= min && baud <= max) {
+			if (save)
+				port->hup_speed = baud;
 			return baud;
+		}
 
 		/*
 		 * If the range cannot be met then try again with
@@ -548,7 +555,7 @@ uart_get_baud_rate(struct uart_port *por
 	WARN_ON(1);
 	return 0;
 }
-EXPORT_SYMBOL(uart_get_baud_rate);
+EXPORT_SYMBOL(uart_get_baud_rate_save);
 
 /**
  * uart_get_divisor - return uart clock divisor
Index: linux-macro/include/linux/serial_core.h
===================================================================
--- linux-macro.orig/include/linux/serial_core.h
+++ linux-macro/include/linux/serial_core.h
@@ -573,6 +573,7 @@ struct uart_port {
 
 	bool			hw_stopped;		/* sw-assisted CTS flow state */
 	bool			cons_flow;		/* user specified console flow control */
+	speed_t			hup_speed;		/* speed to use for B0 rate (hang-up) */
 	unsigned int		mctrl;			/* current modem ctrl settings */
 	unsigned int		frame_time;		/* frame timing in ns */
 	unsigned int		type;			/* port type */
@@ -1036,11 +1037,32 @@ enum UART_TX_FLAGS {
  */
 void uart_update_timeout(struct uart_port *port, unsigned int cflag,
 			 unsigned int baud);
-unsigned int uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
-				const struct ktermios *old, unsigned int min,
-				unsigned int max);
+unsigned int uart_get_baud_rate_save(struct uart_port *port,
+				     struct ktermios *termios,
+				     const struct ktermios *old,
+				     unsigned int min, unsigned int max,
+				     bool save);
 unsigned int uart_get_divisor(struct uart_port *port, unsigned int baud);
 
+/**
+ * uart_get_baud_rate - return baud rate for a particular port
+ * @port: uart_port structure describing the port in question.
+ * @termios: desired termios settings
+ * @old: old termios (or %NULL)
+ * @min: minimum acceptable baud rate
+ * @max: maximum acceptable baud rate
+ *
+ * See uart_get_baud_rate_save() for more details.
+ */
+static inline unsigned int uart_get_baud_rate(struct uart_port *port,
+					      struct ktermios *termios,
+					      const struct ktermios *old,
+					      unsigned int min,
+					      unsigned int max)
+{
+	return uart_get_baud_rate_save(port, termios, old, min, max, true);
+}
+
 /*
  * Calculates FIFO drain time.
  */

Re: [PATCH v3 2/2] serial: core: Don't clobber the baud rate on hangup via B0

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] serial: core: out-of-range hup_speed on B0 in uart_get_baud_rate_save() returns 0
- [Medium] serial: dz: dz_set_termios() saves unvalidated baud in hup_speed breaking B0
- [Low] serial: docs: stale uart_get_baud_rate kernel-doc identifier in driver.rst
--

commit 73f85543b84c52391e1ff13d5519c702bc72e894
Author: Maciej W. Rozycki <macro@orcam.me.uk>

serial: core: Don't clobber the baud rate on hangup via B0

This commit saves the most recently selected baud rate in port->hup_speed
and uses it instead of max(min, 9600) when hangup is requested via B0. It
also renames uart_get_baud_rate() to uart_get_baud_rate_save() with a save
parameter so serial8250_console_restore() can avoid modifying
port->hup_speed without holding port->mutex.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 012f22cf3a60e..18d42491e86d2 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -458,29 +458,32 @@ uart_update_timeout(struct uart_port *port, unsigned int cflag,
>  EXPORT_SYMBOL(uart_update_timeout);
>  
>  /**
> - * uart_get_baud_rate - return baud rate for a particular port
> + * uart_get_baud_rate_save - return baud rate for a particular port

[Severity: Low]
Does Documentation/driver-api/serial/driver.rst also need to be updated for
this rename? It still lists uart_get_baud_rate under the kernel-doc
identifiers for drivers/tty/serial/serial_core.c:

Documentation/driver-api/serial/driver.rst:
.. kernel-doc:: drivers/tty/serial/serial_core.c
   :identifiers: uart_update_timeout uart_get_baud_rate uart_get_divisor

which causes a kernel-doc warning when building the documentation and omits
both uart_get_baud_rate_save() and the new static inline
uart_get_baud_rate() in include/linux/serial_core.h.

[ ... ]

> @@ -519,10 +522,14 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
>  		 * Special case: B0 rate.
>  		 */
>  		if (baud == 0)
> -			baud = max(min, 9600);
> +			baud = (port->hup_speed ?
> +				port->hup_speed : max(min, 9600));
>  
> -		if (baud >= min && baud <= max)
> +		if (baud >= min && baud <= max) {
> +			if (save)
> +				port->hup_speed = baud;
>  			return baud;
> +		}

[Severity: High]
What happens in uart_get_baud_rate_save() when B0 is requested and
port->hup_speed is outside the [min, max] range passed by the caller?

Several 8250 drivers dynamically adjust port->uartclk from
tty_termios_baud_rate(termios) before calling serial8250_do_set_termios() or
dw8250_do_set_termios(). For example, in drivers/tty/serial/8250/8250_mid.c:

mid8250_set_termios()
    unsigned int baud = tty_termios_baud_rate(termios);
    ...
    fuart = baud * 16;
    ...
    /* Gracefully handle the B0 case: fall back to B9600 */
    fuart = fuart ? fuart : 9600 * 16;
    ...
    p->uartclk = fuart;
    serial8250_do_set_termios(p, termios, old);

Similar logic exists in byt_set_termios() in
drivers/tty/serial/8250/8250_lpss.c and dw8250_set_termios() in
drivers/tty/serial/8250/8250_dw.c. In addition, port->uartclk or
UPF_MAGIC_MULTIPLIER can be changed via uart_set_info() or
serial8250_update_uartclk().

If a high baud rate (such as 3000000 on Tangier/Penwell in
mid8250_set_termios() or 5000000 on BayTrail in byt_set_termios()) is set first,
that speed is saved in port->hup_speed. When B0 is subsequently requested via
uart_set_termios(), port->uartclk is lowered for the B0 call, reducing max in
serial8250_get_baud_rate() below both port->hup_speed and the speed recorded
in old:

1. On try == 0, baud == 0 is replaced with port->hup_speed, which exceeds max.
   Execution falls through, clears CBAUD in termios, encodes old's high baud
   rate into termios (overwriting B0), sets old = NULL, and continues.
2. On try == 1, baud from old still exceeds max. Because old is now NULL, it
   clips baud to max - 1, encodes that into termios, exits the loop, triggers
   WARN_ON(1), and returns 0.

Returning 0 from uart_get_baud_rate_save() leads to a divide-by-zero in
callers such as serial8250_get_divisor(), dw8250_get_divisor(), and
uart_update_timeout(). Furthermore, because B0 in termios->c_cflag was
overwritten with a non-zero speed, uart_set_termios() in
drivers/tty/serial/serial_core.c no longer sees (cflag & CBAUD) == B0:

uart_set_termios()
    /* Handle transition to B0 status */
    if (((old_termios->c_cflag & CBAUD) != B0) && ((cflag & CBAUD) == B0))
        uart_clear_mctrl(uport, TIOCM_RTS | TIOCM_DTR);

so RTS and DTR are not dropped on hangup.

A related issue occurs in mtk8250_set_termios() in
drivers/tty/serial/8250/8250_mtk.c, which calls serial8250_do_set_termios()
with old == NULL (where max is (port->uartclk + tolerance) / 16) before
calling uart_get_baud_rate() with max = port->uartclk:

mtk8250_set_termios()
    baud = tty_termios_baud_rate(termios);
    serial8250_do_set_termios(port, termios, NULL);
    tty_termios_encode_baud_rate(termios, baud, baud);
    baud = uart_get_baud_rate(port, termios, old,
                              port->uartclk / 16 / UART_DIV_MAX,
                              port->uartclk);

When a high-speed rate above (port->uartclk + tolerance) / 16 is saved in
port->hup_speed and B0 is later requested, the first call inside
serial8250_do_set_termios() sees port->hup_speed > max with old == NULL and
overwrites port->hup_speed with the clipped rate (port->uartclk + tolerance) /
16 - 1 before mtk8250_set_termios() reaches its second uart_get_baud_rate()
call.

Should the B0 fallback clamp port->hup_speed to [min, max] (and avoid saving
back a clamped rate over port->hup_speed when baud == 0)?

[Severity: Medium]
Can saving port->hup_speed inside uart_get_baud_rate_save() also break B0
hangup in drivers/tty/serial/dz.c when an unsupported baud rate within
[50, 9600] is requested?

In dz_set_termios():

drivers/tty/serial/dz.c:dz_set_termios() {
    ...
    baud = uart_get_baud_rate(uport, termios, old_termios, 50, 9600);
    bflag = dz_encode_baud_rate(baud);
    if (bflag < 0) {
        if (old_termios) {
            /* Keep unchanged. */
            baud = tty_termios_baud_rate(old_termios);
            bflag = dz_encode_baud_rate(baud);
        }
        if (bflag < 0) {        /* Resort to 9600.  */
            baud = 9600;
            bflag = DZ_B9600;
        }
        tty_termios_encode_baud_rate(termios, baud, baud);
    }
    ...
}

If userspace requests a baud rate in [50, 9600] that dz_encode_baud_rate()
rejects (such as B200), uart_get_baud_rate() saves 200 into uport->hup_speed
before dz_encode_baud_rate() returns -1 and falls back to old_termios or 9600.

When B0 is later requested to hang up, uart_get_baud_rate() returns
uport->hup_speed (200), dz_encode_baud_rate(200) fails again, and the fallback
block calls tty_termios_encode_baud_rate(termios, baud, baud) with the
non-zero fallback rate. That overwrites B0 in termios->c_cflag, preventing
uart_set_termios() from clearing TIOCM_RTS | TIOCM_DTR.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/alpine.DEB.2.21.2611030057510.15685@angie.orcam.me.uk?part=2

Re: [PATCH v3 1/2] serial: core: Unify uart_get_baud_rate() success return paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: core: B0 fallback in uart_get_baud_rate() clobbers termios
--

commit b1a1a662c408c018aa01ee6285ba7172be487d43
Author: Maciej W. Rozycki <macro@orcam.me.uk>

serial: core: Unify uart_get_baud_rate() success return paths

This commit merges the B0 return path in uart_get_baud_rate() with the
regular range-checked return path in preparation for adding extra return
actions.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index b06d2649be343..012f22cf3a60e 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -519,7 +519,7 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
>  		 * Special case: B0 rate.
>  		 */
>  		if (baud == 0)
> -			return max(min, 9600);
> +			baud = max(min, 9600);
>  
>  		if (baud >= min && baud <= max)
>  			return baud;

[Severity: High]
Can this break B0 hangup and trigger a division by zero when max is less
than max(min, 9600)?

Commit 091ea8e5d34e ("serial: core: prevent division by zero by always
returning non-zero baud rate") removed the hung_up guard from the
out-of-range fallback path in uart_get_baud_rate() because baud == 0
returned max(min, 9600) immediately before reaching the range check. With
this change, B0 is subject to the baud <= max check, and if that fails it
falls through into the termios-mutating fallback path:

drivers/tty/serial/serial_core.c:uart_get_baud_rate() {
	...
		if (baud == 0)
			baud = max(min, 9600);

		if (baud >= min && baud <= max)
			return baud;

		termios->c_cflag &= ~CBAUD;
		if (old) {
			baud = tty_termios_baud_rate(old);
			tty_termios_encode_baud_rate(termios, baud, baud);
			old = NULL;
			continue;
		}
	...
}

Several in-tree callers can pass max < max(min, 9600) to
uart_get_baud_rate():

1. In drivers/tty/serial/mvebu-uart.c, mvebu_uart_probe() allows
   port->uartclk to remain 0 on non-extended UART0 ports when no DT clock
   is present, and mvebu_uart_set_termios() then passes min_baud = 0 and
   max_baud = 0:

   drivers/tty/serial/mvebu-uart.c:mvebu_uart_set_termios() {
	...
	min_baud = DIV_ROUND_UP(port->uartclk, BRDV_BAUD_MAX *
				OSAMP_MAX_DIVISOR);
	max_baud = port->uartclk / 80;

	baud = uart_get_baud_rate(port, termios, old, min_baud, max_baud);
	...
   }

2. When the UART clock is parented to a 32.768 kHz LSE oscillator,
   stm32_usart_set_termios() in drivers/tty/serial/stm32-usart.c passes
   max = uart_clk / 8 (4096), and lpuart_set_termios() /
   lpuart32_set_termios() in drivers/tty/serial/fsl_lpuart.c pass
   max = port->uartclk / 16 (2048) or port->uartclk / 4 (8192):

   drivers/tty/serial/stm32-usart.c:stm32_usart_set_termios() {
	...
	uart_clk = clk_get_rate(stm32_port->clk);

	baud = uart_get_baud_rate(port, termios, old, 0, uart_clk / 8);
	...
   }

3. In drivers/tty/serial/serial_core.c, uart_set_info() checks
   new_info->baud_base < 9600 as a signed int before computing
   uport->uartclk = new_info->baud_base * 16 as a 32-bit unsigned int,
   which can wrap to a small non-zero value (for example, 16 when
   baud_base is 0x10000001) and yield max = 1 in
   serial8250_get_baud_rate().

When max < max(min, 9600) and userspace requests B0:

- If old has a non-zero baud rate within [min, max], the first iteration
  clears CBAUD and encodes old's speed into termios. Because B0 is
  overwritten in termios->c_cflag, uart_set_termios() no longer sees a
  transition to B0 and fails to drop TIOCM_RTS and TIOCM_DTR:

  drivers/tty/serial/serial_core.c:uart_set_termios() {
	...
	/* Handle transition to B0 status */
	if (((old_termios->c_cflag & CBAUD) != B0) && ((cflag & CBAUD) == B0))
		uart_clear_mctrl(uport, TIOCM_RTS | TIOCM_DTR);
	...
  }

- If old is also B0 (or when max == 0 in mvebu_uart_set_termios()), both
  loop iterations fail the baud <= max check, clobber termios with
  max - 1, trigger WARN_ON(1), and return 0. Callers such as
  stm32_usart_set_termios(), lpuart_set_termios(), and
  serial8250_do_set_termios() then pass baud = 0 to divisor calculations
  and uart_update_timeout(), resulting in a division by zero.

Note that the follow-up commit "serial: core: Don't clobber the baud rate
on hangup via B0" changes the B0 assignment to:

	baud = port->hup_speed ? port->hup_speed : max(min, 9600);

which still fails the baud <= max check whenever port->hup_speed is 0
(such as before any valid speed has been saved, or when max == 0) or when
port->hup_speed exceeds a reduced max.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/alpine.DEB.2.21.2611030057510.15685@angie.orcam.me.uk?part=1