Re: [PATCH RESEND] USB: serial: mos7720: fix actual baud rate not reported back to TTY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] usb: serial: mos7720: wrong base clock in
  calc_baud_rate_divisor()
- [Medium] usb: serial: mos7720: rejected baud rate left in termios on
  error
--

commit 31af65a86747f94a34c72846cf8b44bc48c4bc0f
Author: Agalakov Daniil <empt1nesss@proton.me>

USB: serial: mos7720: fix actual baud rate not reported back to TTY

This commit updates calc_baud_rate_divisor() and send_cmd_write_baud_rate()
to write back the computed baud rate via a pointer argument, and checks the
return status in change_port_settings() before calling tty_encode_baud_rate().

> diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c
> index 104f43a8f4d81..231d47b329942 100644
> --- a/drivers/usb/serial/mos7720.c
> +++ b/drivers/usb/serial/mos7720.c
> @@ -1265,9 +1265,9 @@ static const struct divisor_table_entry divisor_table[] = {
>  /*****************************************************************************
>   * calc_baud_rate_divisor
>   *	this function calculates the proper baud rate divisor for the specified
> - *	baud rate.
> + *	baud rate and updates the baudrate parameter with the actual value.
>   *****************************************************************************/
> -static int calc_baud_rate_divisor(struct usb_serial_port *port, int baudrate, int *divisor)
> +static int calc_baud_rate_divisor(struct usb_serial_port *port, int *baudrate, int *divisor)
>  {
>  	int i;
>  	__u16 custom;
> @@ -1275,10 +1275,10 @@ static int calc_baud_rate_divisor(struct usb_serial_port *port, int baudrate, in
>  	__u16 round;
>  
>  
> -	dev_dbg(&port->dev, "%s - %d\n", __func__, baudrate);
> +	dev_dbg(&port->dev, "%s - %d\n", __func__, *baudrate);
>  
>  	for (i = 0; i < ARRAY_SIZE(divisor_table); i++) {
> -		if (divisor_table[i].baudrate == baudrate) {
> +		if (divisor_table[i].baudrate == *baudrate) {
>  			*divisor = divisor_table[i].divisor;
>  			return 0;
>  		}
> @@ -1286,18 +1286,19 @@ static int calc_baud_rate_divisor(struct usb_serial_port *port, int baudrate, in
>  
>  	/* After trying for all the standard baud rates    *
>  	 * Try calculating the divisor for this baud rate  */
> -	if (baudrate > 75 &&  baudrate < 230400) {
> +	if (*baudrate > 75 && *baudrate < 230400) {
>  		/* get the divisor */
> -		custom = (__u16)(230400L  / baudrate);
> +		custom = (__u16)(230400L  / *baudrate);
>  
>  		/* Check for round off */
> -		round1 = (__u16)(2304000L / baudrate);
> +		round1 = (__u16)(2304000L / *baudrate);
>  		round = (__u16)(round1 - (custom * 10));
>  		if (round > 4)
>  			custom++;
>  		*divisor = custom;
> +		*baudrate = 230400L / custom;

[Severity: Medium]
Does this calculate *baudrate against the wrong base clock?

In divisor_table[] right above, the standard-mode divisors use a 115200 Hz
base clock:

drivers/usb/serial/mos7720.c:divisor_table[] {
    ...
	{   38400,	3},
	{   57600,	2},
	{   115200,	1},
};

For any rate not in divisor_table[] (such as B200 or custom BOTHER speeds),
using 230400L / custom reports twice the hardware baud rate produced by
that divisor.  When custom is 1 (requested rate in 153601..230399),
*baudrate becomes 230400 and is encoded into tty->termios, causing
subsequent change_port_settings() calls to enter the baud >= 230400 branch
and call the non-functional set_higher_rates().

[ ... ]

> @@ -1475,10 +1476,8 @@ static void change_port_settings(struct tty_struct *tty,
>  	}
>  
>  	dev_dbg(&port->dev, "%s - baud rate = %d\n", __func__, baud);
> -	status = send_cmd_write_baud_rate(mos7720_port, baud);
> -	/* FIXME: needs to write actual resulting baud back not just
> -	   blindly do so */
> -	if (cflag & CBAUD)
> +	status = send_cmd_write_baud_rate(mos7720_port, &baud);
> +	if (!status && (cflag & CBAUD))
>  		tty_encode_baud_rate(tty, baud, baud);

[Severity: Medium]
When send_cmd_write_baud_rate() fails, does skipping tty_encode_baud_rate()
leave the rejected baud rate in tty->termios?

tty_set_termios() copies *new_termios into tty->termios before invoking the
driver's set_termios() callback.  If a caller requests an unsupported rate
such as B75 (which is absent from divisor_table[] and excluded by
*baudrate > 75), calc_baud_rate_divisor() returns -EINVAL without updating
the UART divisor registers, but skipping tty_encode_baud_rate() leaves B75
in tty->termios instead of restoring the previous rate from old_termios.

In addition, send_cmd_write_baud_rate() ignores the return values of its
write_mos_reg() calls, and the baud >= 230400 branch right above still
returns early without calling tty_encode_baud_rate() despite removing the
FIXME comment.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110081426.26265-1-shkiper325@gmail.com?part=1