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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Nov 03, 2026 at 10:07:58PM +0000, Maciej W. Rozycki wrote:
> 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).

All lines above (except the last ones in the paragraphs) have a single trailing
white space. Usually checkpatch warns at this.

>  /**
> - * uart_get_baud_rate - return baud rate for a particular port
> + * uart_get_baud_rate_save - return baud rate for a particular port

Now it also saves it, so

 * uart_get_baud_rate_save - return and save 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
>   */

...

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

Outer parentheses are redundant. Also use Elvis.

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

-- 
With Best Regards,
Andy Shevchenko