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

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

[Daniil Iskhakov]

From: Agalakov Daniil <ade@amicon.ru>

The calc_baud_rate_divisor() function computes a divisor from the
requested baud rate using integer arithmetic, which means the actual
hardware baud rate may differ from the requested one. Previously this
rounded value was never propagated back to the TTY layer, so
tty_encode_baud_rate() was called with the requested rate rather than
the actual one.

Change baudrate parameters in calc_baud_rate_divisor() and
send_cmd_write_baud_rate() to pointers so the actual resulting baud
rate can be written back to the caller. Update *baudrate with the
computed value (230400 / custom) after divisor calculation.

In change_port_settings(), pass &baud to send_cmd_write_baud_rate()
and guard tty_encode_baud_rate() with a status check, so the TTY is
only updated on success and receives the true hardware baud rate.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 65d063ab21fe ("USB: mos7720: clean up termios")
Signed-off-by: Agalakov Daniil <ade@amicon.ru>
Co-developed-by: Daniil Iskhakov <dish@amicon.ru>
Signed-off-by: Daniil Iskhakov <dish@amicon.ru>
---
 drivers/usb/serial/mos7720.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c
index 94459408e7fb..1052217edf26 100644
--- a/drivers/usb/serial/mos7720.c
+++ b/drivers/usb/serial/mos7720.c
@@ -1264,9 +1264,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;
@@ -1274,10 +1274,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;
 		}
@@ -1285,18 +1285,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;
 
-		dev_dbg(&port->dev, "Baud %d = %d\n", baudrate, custom);
+		dev_dbg(&port->dev, "Baud %d = %d\n", *baudrate, custom);
 		return 0;
 	}
 
@@ -1307,10 +1308,10 @@ static int calc_baud_rate_divisor(struct usb_serial_port *port, int baudrate, in
 /*
  * send_cmd_write_baud_rate
  *	this function sends the proper command to change the baud rate of the
- *	specified port.
+ *	specified port and updates the baudrate parameter with the actual value.
  */
 static int send_cmd_write_baud_rate(struct moschip_port *mos7720_port,
-				    int baudrate)
+				    int *baudrate)
 {
 	struct usb_serial_port *port;
 	struct usb_serial *serial;
@@ -1325,7 +1326,7 @@ static int send_cmd_write_baud_rate(struct moschip_port *mos7720_port,
 	serial = port->serial;
 
 	number = port->port_number;
-	dev_dbg(&port->dev, "%s - baud = %d\n", __func__, baudrate);
+	dev_dbg(&port->dev, "%s - baud = %d\n", __func__, *baudrate);
 
 	/* Calculate the Divisor */
 	status = calc_baud_rate_divisor(port, baudrate, &divisor);
@@ -1474,10 +1475,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);
-- 
2.51.0

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

[Daniil Iskhakov]

Hi Johan, Greg,

Gentle ping on this patch. It's been over a month since the resend.

Could you please take a look when you have a chance?

Thanks,
Daniil

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

[Daniil Iskhakov]

Hi Johan, Greg,

Just following up on this patch once more.

Please let me know if there are any changes needed from my side, or if
there's anything I can do to help move this forward.

Thanks,
Daniil

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

[Daniil Iskhakov]

From: Agalakov Daniil <empt1nesss@proton.me>

The calc_baud_rate_divisor() function computes a divisor from the
requested baud rate using integer arithmetic, which means the actual
hardware baud rate may differ from the requested one. Previously this
rounded value was never propagated back to the TTY layer, so
tty_encode_baud_rate() was called with the requested rate rather than
the actual one.

Change baudrate parameters in calc_baud_rate_divisor() and
send_cmd_write_baud_rate() to pointers so the actual resulting baud
rate can be written back to the caller. Update *baudrate with the
computed value (230400 / custom) after divisor calculation.

In change_port_settings(), pass &baud to send_cmd_write_baud_rate()
and guard tty_encode_baud_rate() with a status check, so the TTY is
only updated on success and receives the true hardware baud rate.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 65d063ab21fe ("USB: mos7720: clean up termios")
Signed-off-by: Agalakov Daniil <empt1nesss@proton.me>
Co-developed-by: Daniil Iskhakov <shkiper325@gmail.com>
Signed-off-by: Daniil Iskhakov <shkiper325@gmail.com>
---
 drivers/usb/serial/mos7720.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c
index 94459408e7fb..1052217edf26 100644
--- a/drivers/usb/serial/mos7720.c
+++ b/drivers/usb/serial/mos7720.c
@@ -1264,9 +1264,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;
@@ -1274,10 +1274,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;
 		}
@@ -1285,18 +1285,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;
 
-		dev_dbg(&port->dev, "Baud %d = %d\n", baudrate, custom);
+		dev_dbg(&port->dev, "Baud %d = %d\n", *baudrate, custom);
 		return 0;
 	}
 
@@ -1307,10 +1308,10 @@ static int calc_baud_rate_divisor(struct usb_serial_port *port, int baudrate, in
 /*
  * send_cmd_write_baud_rate
  *	this function sends the proper command to change the baud rate of the
- *	specified port.
+ *	specified port and updates the baudrate parameter with the actual value.
  */
 static int send_cmd_write_baud_rate(struct moschip_port *mos7720_port,
-				    int baudrate)
+				    int *baudrate)
 {
 	struct usb_serial_port *port;
 	struct usb_serial *serial;
@@ -1325,7 +1326,7 @@ static int send_cmd_write_baud_rate(struct moschip_port *mos7720_port,
 	serial = port->serial;
 
 	number = port->port_number;
-	dev_dbg(&port->dev, "%s - baud = %d\n", __func__, baudrate);
+	dev_dbg(&port->dev, "%s - baud = %d\n", __func__, *baudrate);
 
 	/* Calculate the Divisor */
 	status = calc_baud_rate_divisor(port, baudrate, &divisor);
@@ -1474,10 +1475,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);
-- 
2.51.0

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

[sashiko-bot]

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