Re: [PATCH] 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 issue(s) to consider:
- [Medium] usb: serial: mos7720: failed baud rate left in termios in change_port_settings()
- [Medium] usb: serial: mos7720: wrong base clock in calc_baud_rate_divisor()
--

commit 810101c2bd92bb52e1ced8f6f992bab94cb3a6d6
Author: Agalakov Daniil <empt1nesss@proton.me>

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

This commit changes calc_baud_rate_divisor() and send_cmd_write_baud_rate()
to take a pointer to the baud rate so that the calculated hardware baud
rate can be passed back to change_port_settings() and encoded into the TTY
termios structure when send_cmd_write_baud_rate() succeeds.

> 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]
Is 230400L the right base clock here in calc_baud_rate_divisor() in
drivers/usb/serial/mos7720.c?

Looking at divisor_table[] right above this function, standard mode
divisors on MOS7720 are based on 115200 / divisor:

static const struct divisor_table_entry divisor_table[] = {
	{   50,		2304},
	...
	{   9600,	12},
	{   19200,	6},
	{   38400,	3},
	{   57600,	2},
	{   115200,	1},
};

When a user requests a baud rate in (75, 230400) that is not in
divisor_table[] (such as B200 or a custom BOTHER speed), computing
*baudrate = 230400L / custom reports twice the hardware baud rate back to
tty->termios for that divisor.

Furthermore, if a baud rate in [153601, 230399] is requested, custom
evaluates to 1 (which is 115200 baud in divisor_table[]), and *baudrate is
set to 230400 and encoded into tty->termios. On the next
mos7720_set_termios() -> change_port_settings() call (for example when
updating parity or flow control), tty_get_baud_rate(tty) will return 230400
and enter the baud >= 230400 branch calling set_higher_rates(), which has a
comment noting that it does not work.

>  
> -		dev_dbg(&port->dev, "Baud %d = %d\n", baudrate, custom);
> +		dev_dbg(&port->dev, "Baud %d = %d\n", *baudrate, custom);
>  		return 0;
>  	}

[ ... ]

> @@ -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);
>  	/* Enable Interrupts */
>  	write_mos_reg(serial, port_number, MOS7720_IER, 0x0c);

[Severity: Medium]
When send_cmd_write_baud_rate() returns an error in change_port_settings()
(for example, -EINVAL from calc_baud_rate_divisor() when B75 or a custom
baud rate <= 75 other than 50 is requested), does skipping
tty_encode_baud_rate() leave the failed baud rate in tty->termios?

In drivers/tty/tty_ioctl.c, tty_set_termios() copies *new_termios into
tty->termios before invoking the driver callback:

	old_termios = tty->termios;
	tty->termios = *new_termios;
	...
	if (tty->ops->set_termios)
		tty->ops->set_termios(tty, &old_termios);

Because tty->termios has already been overwritten with the newly requested
termios prior to mos7720_set_termios() -> change_port_settings(), skipping
tty_encode_baud_rate() on error leaves tty->termios reporting the unapplied
baud rate while the hardware remains at the previous speed. Should the old
baud rate from old_termios be restored instead when
send_cmd_write_baud_rate() fails?

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