[PATCH v3 0/7] ftdi_sio driver improvements

[PATCH v3 0/7] ftdi_sio driver improvements

[Pali Rohár]

Changes since v2:
- Use u32 for div_value
- Add line break after the colons in switch
- Avoid ternary operators where possilble
- Use baud rate 9600 instead of 0 when unspecified
- Fix commit messages

Pali Rohár (7):
  USB: serial: ftdi_sio: Fix divisor overflow
  USB: serial: ftdi_sio: Add missing baud rate validation
  USB: serial: ftdi_sio: Extract SIO divisor code to function
  USB: serial: ftdi_sio: Do not reset baud rate to 9600 Baud on error
  USB: serial: ftdi_sio: Fix baud rate rounding for ASYNC_SPD_CUST
  USB: serial: ftdi_sio: Fix custom_divisor for TIOCGSERIAL and c_*speed
    for TCGETS2
  USB: serial: ftdi_sio: Fill c_*speed fields with real baud rate

 drivers/usb/serial/ftdi_sio.c | 215 +++++++++++++++++++++++++++++-----
 1 file changed, 185 insertions(+), 30 deletions(-)

-- 
2.20.1

[PATCH v3 1/7] USB: serial: ftdi_sio: Fix divisor overflow

[Pali Rohár]

The baud rate generating divisor is a 17-bit wide (14 bits integer part
and 3 bits fractional part).

Example:
  base clock = 48 MHz
  requested baud rate = 180 Baud
  divisor = 48,000,000 / (16 * 180) = 0b100000100011010.101

  In this case the MSB gets discarded because of the overflow, and the
  actually used divisor will be 0b100011010.101 = 282.625, resulting
  in baud rate 10615 Baud, instead of the requested 180 Baud.

The best possible thing to do in this case is to generate lowest possible
baud rate (in the example it would be 183 Baud), by using maximum possible
divisor.

In case of divisor overflow, use maximum possible divisor.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/usb/serial/ftdi_sio.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 52d59be92034..b2febe8d573a 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1162,6 +1162,8 @@ static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
 	int divisor3 = DIV_ROUND_CLOSEST(base, 2 * baud);
 	if ((divisor3 & 0x7) == 7)
 		divisor3++; /* round x.7/8 up to x+1 */
+	if (divisor3 > GENMASK(16, 0))
+		divisor3 = GENMASK(16, 0);
 	divisor = divisor3 >> 3;
 	divisor3 &= 0x7;
 	if (divisor3 == 1)
@@ -1186,6 +1188,8 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base)
 	u32 divisor;
 	/* divisor shifted 3 bits to the left */
 	int divisor3 = DIV_ROUND_CLOSEST(base, 2 * baud);
+	if (divisor3 > GENMASK(16, 0))
+		divisor3 = GENMASK(16, 0);
 	divisor = divisor3 >> 3;
 	divisor |= (u32)divfrac[divisor3 & 0x7] << 14;
 	/* Deal with special cases for highest baud rates. */
@@ -1209,6 +1213,8 @@ static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base)
 
 	/* hi-speed baud rate is 10-bit sampling instead of 16-bit */
 	divisor3 = DIV_ROUND_CLOSEST(8 * base, 10 * baud);
+	if (divisor3 > GENMASK(16, 0))
+		divisor3 = GENMASK(16, 0);
 
 	divisor = divisor3 >> 3;
 	divisor |= (u32)divfrac[divisor3 & 0x7] << 14;
-- 
2.20.1

[PATCH v3 2/7] USB: serial: ftdi_sio: Add missing baud rate validation

[Pali Rohár]

Add lower checks for requested baud rate for FT8U232AM, FT232BM, FT2232C,
FT232RL, FTX, FT2232H, FT4232H and FT232H. For all of these the minimum
baud rate they can generate is 183 Baud.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index b2febe8d573a..bfa601fc14fe 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1335,7 +1335,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 		}
 		break;
 	case FT8U232AM: /* 8U232AM chip */
-		if (baud <= 3000000) {
+		if (baud >= 183 && baud <= 3000000) {
 			div_value = ftdi_232am_baud_to_divisor(baud);
 		} else {
 			dev_dbg(dev, "%s - Baud rate too high!\n", __func__);
@@ -1348,7 +1348,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 	case FT2232C: /* FT2232C chip */
 	case FT232RL: /* FT232RL chip */
 	case FTX:     /* FT-X series */
-		if (baud <= 3000000) {
+		if (baud >= 183 && baud <= 3000000) {
 			u16 product_id = le16_to_cpu(
 				port->serial->dev->descriptor.idProduct);
 			if (((product_id == FTDI_NDI_HUC_PID)		||
@@ -1372,7 +1372,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 	case FT232H:  /* FT232H chip */
 		if ((baud <= 12000000) && (baud >= 1200)) {
 			div_value = ftdi_2232h_baud_to_divisor(baud);
-		} else if (baud < 1200) {
+		} else if (baud >= 183 && baud < 1200) {
 			div_value = ftdi_232bm_baud_to_divisor(baud);
 		} else {
 			dev_dbg(dev, "%s - Baud rate too high!\n", __func__);
-- 
2.20.1

[PATCH v3 3/7] USB: serial: ftdi_sio: Extract SIO divisor code to function

[Pali Rohár]

In preparation for following changes, extract divisor code for SIO chip
into new function ftdi_sio_baud_to_divisor(), as is done for other
chips.

No functional change.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c | 45 ++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index bfa601fc14fe..fe8a7c5fa0e9 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1155,6 +1155,34 @@ static struct usb_serial_driver * const serial_drivers[] = {
  * ***************************************************************************
  */
 
+static u32 ftdi_sio_baud_to_divisor(int baud)
+{
+	switch (baud) {
+	case 300:
+		return ftdi_sio_b300;
+	case 600:
+		return ftdi_sio_b600;
+	case 1200:
+		return ftdi_sio_b1200;
+	case 2400:
+		return ftdi_sio_b2400;
+	case 4800:
+		return ftdi_sio_b4800;
+	case 9600:
+		return ftdi_sio_b9600;
+	case 19200:
+		return ftdi_sio_b19200;
+	case 38400:
+		return ftdi_sio_b38400;
+	case 57600:
+		return ftdi_sio_b57600;
+	case 115200:
+		return ftdi_sio_b115200;
+	default:
+		return -1;
+	}
+}
+
 static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
 {
 	unsigned short int divisor;
@@ -1314,23 +1342,12 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 		baud = 9600;
 	switch (priv->chip_type) {
 	case SIO: /* SIO chip */
-		switch (baud) {
-		case 300: div_value = ftdi_sio_b300; break;
-		case 600: div_value = ftdi_sio_b600; break;
-		case 1200: div_value = ftdi_sio_b1200; break;
-		case 2400: div_value = ftdi_sio_b2400; break;
-		case 4800: div_value = ftdi_sio_b4800; break;
-		case 9600: div_value = ftdi_sio_b9600; break;
-		case 19200: div_value = ftdi_sio_b19200; break;
-		case 38400: div_value = ftdi_sio_b38400; break;
-		case 57600: div_value = ftdi_sio_b57600;  break;
-		case 115200: div_value = ftdi_sio_b115200; break;
-		} /* baud */
-		if (div_value == 0) {
+		div_value = ftdi_sio_baud_to_divisor(baud);
+		if (div_value == (u32)-1) {
 			dev_dbg(dev, "%s - Baudrate (%d) requested is not supported\n",
 				__func__,  baud);
-			div_value = ftdi_sio_b9600;
 			baud = 9600;
+			div_value = ftdi_sio_baud_to_divisor(baud);
 			div_okay = 0;
 		}
 		break;
-- 
2.20.1

[PATCH v3 4/7] USB: serial: ftdi_sio: Do not reset baud rate to 9600 Baud on error

[Pali Rohár]

If setting new baud rate fails, some other drivers leave the device at
previous baud rate, and ftdi_sio resets to 9600 Baud.

Change this behavior to not reset baud rate to 9600.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c | 47 ++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index fe8a7c5fa0e9..1ab6bf48516f 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1315,7 +1315,7 @@ static int update_mctrl(struct usb_serial_port *port, unsigned int set,
 
 
 static u32 get_ftdi_divisor(struct tty_struct *tty,
-						struct usb_serial_port *port)
+			    struct usb_serial_port *port, speed_t old_baud)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 	struct device *dev = &port->dev;
@@ -1338,6 +1338,8 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 			__func__, priv->custom_divisor, baud);
 	}
 
+	if (!baud)
+		baud = old_baud;
 	if (!baud)
 		baud = 9600;
 	switch (priv->chip_type) {
@@ -1346,8 +1348,12 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 		if (div_value == (u32)-1) {
 			dev_dbg(dev, "%s - Baudrate (%d) requested is not supported\n",
 				__func__,  baud);
-			baud = 9600;
+			baud = old_baud ? old_baud : 9600;
 			div_value = ftdi_sio_baud_to_divisor(baud);
+			if (div_value == -1) {
+				baud = 9600;
+				div_value = ftdi_sio_baud_to_divisor(baud);
+			}
 			div_okay = 0;
 		}
 		break;
@@ -1356,8 +1362,11 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 			div_value = ftdi_232am_baud_to_divisor(baud);
 		} else {
 			dev_dbg(dev, "%s - Baud rate too high!\n", __func__);
-			baud = 9600;
-			div_value = ftdi_232am_baud_to_divisor(9600);
+			if (old_baud >= 183 && old_baud <= 3000000)
+				baud = old_baud;
+			else
+				baud = 9600;
+			div_value = ftdi_232am_baud_to_divisor(baud);
 			div_okay = 0;
 		}
 		break;
@@ -1379,9 +1388,12 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 			div_value = ftdi_232bm_baud_to_divisor(baud);
 		} else {
 			dev_dbg(dev, "%s - Baud rate too high!\n", __func__);
-			div_value = ftdi_232bm_baud_to_divisor(9600);
+			if (old_baud >= 183 && old_baud <= 3000000)
+				baud = old_baud;
+			else
+				baud = 9600;
+			div_value = ftdi_232bm_baud_to_divisor(baud);
 			div_okay = 0;
-			baud = 9600;
 		}
 		break;
 	case FT2232H: /* FT2232H chip */
@@ -1393,9 +1405,17 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 			div_value = ftdi_232bm_baud_to_divisor(baud);
 		} else {
 			dev_dbg(dev, "%s - Baud rate too high!\n", __func__);
-			div_value = ftdi_232bm_baud_to_divisor(9600);
+			if (old_baud >= 183 && old_baud < 1200) {
+				baud = old_baud;
+				div_value = ftdi_232bm_baud_to_divisor(baud);
+			} else {
+				if (old_baud >= 1200 && old_baud <= 12000000)
+					baud = old_baud;
+				else
+					baud = 9600;
+				div_value = ftdi_2232h_baud_to_divisor(baud);
+			}
 			div_okay = 0;
-			baud = 9600;
 		}
 		break;
 	} /* priv->chip_type */
@@ -1410,7 +1430,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 	return div_value;
 }
 
-static int change_speed(struct tty_struct *tty, struct usb_serial_port *port)
+static int change_speed(struct tty_struct *tty, struct usb_serial_port *port, speed_t old_baud)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 	u16 value;
@@ -1418,7 +1438,7 @@ static int change_speed(struct tty_struct *tty, struct usb_serial_port *port)
 	u32 index_value;
 	int rv;
 
-	index_value = get_ftdi_divisor(tty, port);
+	index_value = get_ftdi_divisor(tty, port, old_baud);
 	value = (u16)index_value;
 	index = (u16)(index_value >> 16);
 	if (priv->chip_type == FT2232C || priv->chip_type == FT2232H ||
@@ -1541,7 +1561,7 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 		if (priv->flags & ASYNC_SPD_MASK)
 			dev_warn_ratelimited(&port->dev, "use of SPD flags is deprecated\n");
 
-		change_speed(tty, port);
+		change_speed(tty, port, 9600);
 	}
 	mutex_unlock(&priv->cfg_lock);
 	return 0;
@@ -2790,9 +2810,12 @@ static void ftdi_set_termios(struct tty_struct *tty,
 		/* Drop RTS and DTR */
 		clear_mctrl(port, TIOCM_DTR | TIOCM_RTS);
 	} else {
+		speed_t old_baud =
+			old_termios ? tty_termios_baud_rate(old_termios) : 9600;
+
 		/* set the baudrate determined before */
 		mutex_lock(&priv->cfg_lock);
-		if (change_speed(tty, port))
+		if (change_speed(tty, port, old_baud))
 			dev_err(ddev, "%s urb failed to set baudrate\n", __func__);
 		mutex_unlock(&priv->cfg_lock);
 		/* Ensure RTS and DTR are raised when baudrate changed from 0 */
-- 
2.20.1

[PATCH v3 5/7] USB: serial: ftdi_sio: Fix baud rate rounding for ASYNC_SPD_CUST

[Pali Rohár]

To compute more accurate baud rate when user uses ASYNC_SPD_CUST API,
use DIV_ROUND_CLOSEST() instead of just rounding down.

Rationale:
  Application uses old API, so it computes divisor D for baud rate B.

  The driver then tries to compute back the requested baud rate B, but
  rounds down in the division.

  Using rounding to closest value instead should increate accuracy here.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 1ab6bf48516f..718c86db2900 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1333,7 +1333,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 	if (baud == 38400 &&
 	    ((priv->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST) &&
 	     (priv->custom_divisor)) {
-		baud = priv->baud_base / priv->custom_divisor;
+		baud = DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor);
 		dev_dbg(dev, "%s - custom divisor %d sets baud rate to %d\n",
 			__func__, priv->custom_divisor, baud);
 	}
-- 
2.20.1

[PATCH v3 6/7] USB: serial: ftdi_sio: Fix custom_divisor for TIOCGSERIAL and c_*speed for TCGETS2

[Pali Rohár]

When ASYNC_SPD_CUST is used, update custom_divisor field for TIOCGSERIAL
and c_*speed fields for TCGETS2 so that they correspond to the newly set
baud rate value.

So userspace TCGETS2 ioctls in new c_*speed fields will see the true baud
rate that is being used.

This is needed for switching userspace applications to use TCGETS2 API as
currently new c_*speed fields does not report correct values. Without this
change userspace applications still have to use old deprecated TIOCGSERIAL
to retrieve current baud rate.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
Please note that if c_*speed fields of TCGETS2 are not going to be fixed
then userspace application cannot be switched to use this new TCGETS2 API
due to this issue.
---
 drivers/usb/serial/ftdi_sio.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 718c86db2900..79b00912c81c 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1319,6 +1319,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 	struct device *dev = &port->dev;
+	int fix_custom_divisor = 0;
 	u32 div_value = 0;
 	int div_okay = 1;
 	int baud;
@@ -1333,6 +1334,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 	if (baud == 38400 &&
 	    ((priv->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST) &&
 	     (priv->custom_divisor)) {
+		fix_custom_divisor = 1;
 		baud = DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor);
 		dev_dbg(dev, "%s - custom divisor %d sets baud rate to %d\n",
 			__func__, priv->custom_divisor, baud);
@@ -1426,7 +1428,19 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 			ftdi_chip_name[priv->chip_type]);
 	}
 
+	/* Fix deprecated async-compatible custom_divisor hack and update tty baud rate */
+	if (fix_custom_divisor) {
+		priv->custom_divisor = DIV_ROUND_CLOSEST(priv->baud_base, baud);
+		old_baud = baud;
+		baud = 38400;
+	}
+
 	tty_encode_baud_rate(tty, baud, baud);
+
+	/* For async-compatible custom_divisor store into TCGETS2 c_*speed fields real baud rate */
+	if (fix_custom_divisor)
+		tty->termios.c_ispeed = tty->termios.c_ospeed = old_baud;
+
 	return div_value;
 }
 
@@ -2699,6 +2713,8 @@ static void ftdi_set_termios(struct tty_struct *tty,
 		dev_dbg(ddev, "%s: forcing baud rate for this device\n", __func__);
 		tty_encode_baud_rate(tty, priv->force_baud,
 					priv->force_baud);
+		termios->c_ispeed = termios->c_ospeed =
+			DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor);
 	}
 
 	/* Force RTS-CTS if this device requires it. */
-- 
2.20.1

[PATCH v3 7/7] USB: serial: ftdi_sio: Fill c_*speed fields with real baud rate

[Pali Rohár]

Calculate baud rate value in c_*speed fields to the real values which were
set on hardware. For this operation, add a new set of methods
*_divisor_to_baud() for each chip and use them for calculating the real
baud rate values.

Each *_divisor_to_baud() method is constructed as an inverse function of
its corresponding *_baud_to_divisor() method.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c | 93 +++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 79b00912c81c..350ed14b014c 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1183,6 +1183,34 @@ static u32 ftdi_sio_baud_to_divisor(int baud)
 	}
 }
 
+static int ftdi_sdio_divisor_to_baud(u32 divisor)
+{
+	switch (divisor) {
+	case ftdi_sio_b300:
+		return 300;
+	case ftdi_sio_b600:
+		return 600;
+	case ftdi_sio_b1200:
+		return 1200;
+	case ftdi_sio_b2400:
+		return 2400;
+	case ftdi_sio_b4800:
+		return 4800;
+	case ftdi_sio_b9600:
+		return 9600;
+	case ftdi_sio_b19200:
+		return 19200;
+	case ftdi_sio_b38400:
+		return 38400;
+	case ftdi_sio_b57600:
+		return 57600;
+	case ftdi_sio_b115200:
+		return 115200;
+	default:
+		return 9600;
+	}
+}
+
 static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
 {
 	unsigned short int divisor;
@@ -1205,11 +1233,28 @@ static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
 	return divisor;
 }
 
+static int ftdi_232am_divisor_base_to_baud(unsigned short int divisor, int base)
+{
+	static const unsigned char divfrac_inv[4] = { 0, 4, 2, 1 };
+	unsigned int divisor3;
+
+	if (divisor == 0)
+		divisor = 1;
+	divisor3 = (GENMASK(13, 0) & divisor) << 3;
+	divisor3 |= divfrac_inv[(divisor >> 14) & 0x3];
+	return DIV_ROUND_CLOSEST(base, 2 * divisor3);
+}
+
 static unsigned short int ftdi_232am_baud_to_divisor(int baud)
 {
 	 return ftdi_232am_baud_base_to_divisor(baud, 48000000);
 }
 
+static int ftdi_232am_divisor_to_baud(unsigned short int divisor)
+{
+	return ftdi_232am_divisor_base_to_baud(divisor, 48000000);
+}
+
 static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base)
 {
 	static const unsigned char divfrac[8] = { 0, 3, 2, 4, 1, 5, 6, 7 };
@@ -1228,11 +1273,31 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base)
 	return divisor;
 }
 
+static int ftdi_232bm_divisor_base_to_baud(u32 divisor, int base)
+{
+	static const unsigned char divfrac_inv[8] = { 0, 4, 2, 1, 3, 5, 6, 7 };
+	u32 divisor3;
+
+	/* Deal with special cases for highest baud rates. */
+	if (divisor == 0)
+		divisor = 1;		/* 1.0 */
+	else if (divisor == 1)
+		divisor = 0x4001;	/* 1.5 */
+	divisor3 = (GENMASK(13, 0) & divisor) << 3;
+	divisor3 |= divfrac_inv[(divisor >> 14) & 0x7];
+	return DIV_ROUND_CLOSEST(base, 2 * divisor3);
+}
+
 static u32 ftdi_232bm_baud_to_divisor(int baud)
 {
 	 return ftdi_232bm_baud_base_to_divisor(baud, 48000000);
 }
 
+static int ftdi_232bm_divisor_to_baud(u32 divisor)
+{
+	return ftdi_232bm_divisor_base_to_baud(divisor, 48000000);
+}
+
 static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base)
 {
 	static const unsigned char divfrac[8] = { 0, 3, 2, 4, 1, 5, 6, 7 };
@@ -1260,11 +1325,32 @@ static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base)
 	return divisor;
 }
 
+static int ftdi_2232h_divisor_base_to_baud(u32 divisor, int base)
+{
+	static const unsigned char divfrac_inv[8] = { 0, 4, 2, 1, 3, 5, 6, 7 };
+	u32 divisor3;
+
+	divisor &= GENMASK(16, 0);
+	/* Deal with special cases for highest baud rates. */
+	if (divisor == 0)
+		divisor = 1;		/* 1.0 */
+	else if (divisor == 1)
+		divisor = 0x4001;	/* 1.5 */
+	divisor3 = (GENMASK(13, 0) & divisor) << 3;
+	divisor3 |= divfrac_inv[(divisor >> 14) & 0x7];
+	return DIV_ROUND_CLOSEST(8 * base, 10 * divisor3);
+}
+
 static u32 ftdi_2232h_baud_to_divisor(int baud)
 {
 	 return ftdi_2232h_baud_base_to_divisor(baud, 120000000);
 }
 
+static int ftdi_2232h_divisor_to_baud(u32 divisor)
+{
+	return ftdi_2232h_divisor_base_to_baud(divisor, 120000000);
+}
+
 #define set_mctrl(port, set)		update_mctrl((port), (set), 0)
 #define clear_mctrl(port, clear)	update_mctrl((port), 0, (clear))
 
@@ -1358,6 +1444,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 			}
 			div_okay = 0;
 		}
+		baud = ftdi_sdio_divisor_to_baud(div_value);
 		break;
 	case FT8U232AM: /* 8U232AM chip */
 		if (baud >= 183 && baud <= 3000000) {
@@ -1371,6 +1458,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 			div_value = ftdi_232am_baud_to_divisor(baud);
 			div_okay = 0;
 		}
+		baud = ftdi_232am_divisor_to_baud(div_value);
 		break;
 	case FT232BM: /* FT232BM chip */
 	case FT2232C: /* FT2232C chip */
@@ -1397,25 +1485,30 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
 			div_value = ftdi_232bm_baud_to_divisor(baud);
 			div_okay = 0;
 		}
+		baud = ftdi_232bm_divisor_to_baud(div_value);
 		break;
 	case FT2232H: /* FT2232H chip */
 	case FT4232H: /* FT4232H chip */
 	case FT232H:  /* FT232H chip */
 		if ((baud <= 12000000) && (baud >= 1200)) {
 			div_value = ftdi_2232h_baud_to_divisor(baud);
+			baud = ftdi_2232h_divisor_to_baud(div_value);
 		} else if (baud >= 183 && baud < 1200) {
 			div_value = ftdi_232bm_baud_to_divisor(baud);
+			baud = ftdi_232bm_divisor_to_baud(div_value);
 		} else {
 			dev_dbg(dev, "%s - Baud rate too high!\n", __func__);
 			if (old_baud >= 183 && old_baud < 1200) {
 				baud = old_baud;
 				div_value = ftdi_232bm_baud_to_divisor(baud);
+				baud = ftdi_232bm_divisor_to_baud(div_value);
 			} else {
 				if (old_baud >= 1200 && old_baud <= 12000000)
 					baud = old_baud;
 				else
 					baud = 9600;
 				div_value = ftdi_2232h_baud_to_divisor(baud);
+				baud = ftdi_2232h_divisor_to_baud(div_value);
 			}
 			div_okay = 0;
 		}
-- 
2.20.1

Re: [PATCH v3 3/7] USB: serial: ftdi_sio: Extract SIO divisor code to function

[Greg Kroah-Hartman]

On Sat, Sep 24, 2022 at 12:27:14PM +0200, Pali Rohár wrote:
> In preparation for following changes,

What do you mean by "following changes"?  That doesn't work well when
looking in a changelog series.  Spell out what you are going to do in a
future change, as to why this is necessary.

thanks,

greg k-h

Re: [PATCH v3 3/7] USB: serial: ftdi_sio: Extract SIO divisor code to function

[Pali Rohár]

On Saturday 24 September 2022 12:47:06 Greg Kroah-Hartman wrote:
> On Sat, Sep 24, 2022 at 12:27:14PM +0200, Pali Rohár wrote:
> > In preparation for following changes,
> 
> What do you mean by "following changes"?  That doesn't work well when
> looking in a changelog series.  Spell out what you are going to do in a
> future change, as to why this is necessary.
> 
> thanks,
> 
> greg k-h

Ok. Anything else is needed to address?

Re: [PATCH v3 3/7] USB: serial: ftdi_sio: Extract SIO divisor code to function

[Pali Rohár]

On Sunday 09 October 2022 14:17:07 Pali Rohár wrote:
> On Saturday 24 September 2022 12:47:06 Greg Kroah-Hartman wrote:
> > On Sat, Sep 24, 2022 at 12:27:14PM +0200, Pali Rohár wrote:
> > > In preparation for following changes,
> > 
> > What do you mean by "following changes"?  That doesn't work well when
> > looking in a changelog series.  Spell out what you are going to do in a
> > future change, as to why this is necessary.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Ok. Anything else is needed to address?

Ping?

Re: [PATCH v3 3/7] USB: serial: ftdi_sio: Extract SIO divisor code to function

[Greg Kroah-Hartman]

On Tue, Nov 01, 2022 at 11:50:57PM +0100, Pali Rohár wrote:
> On Sunday 09 October 2022 14:17:07 Pali Rohár wrote:
> > On Saturday 24 September 2022 12:47:06 Greg Kroah-Hartman wrote:
> > > On Sat, Sep 24, 2022 at 12:27:14PM +0200, Pali Rohár wrote:
> > > > In preparation for following changes,
> > > 
> > > What do you mean by "following changes"?  That doesn't work well when
> > > looking in a changelog series.  Spell out what you are going to do in a
> > > future change, as to why this is necessary.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Ok. Anything else is needed to address?
> 
> Ping?

We have no idea, sorry, as we have no context here at all and it was
thousands of patches ago in our reviews.  Please just fix things up
based on this review and resubmit, you never need to ask.

greg k-h

Re: [PATCH v3 3/7] USB: serial: ftdi_sio: Extract SIO divisor code to function

[Johan Hovold]

On Tue, Nov 01, 2022 at 11:50:57PM +0100, Pali Rohár wrote:
> On Sunday 09 October 2022 14:17:07 Pali Rohár wrote:
> > On Saturday 24 September 2022 12:47:06 Greg Kroah-Hartman wrote:
> > > On Sat, Sep 24, 2022 at 12:27:14PM +0200, Pali Rohár wrote:
> > > > In preparation for following changes,
> > > 
> > > What do you mean by "following changes"?  That doesn't work well when
> > > looking in a changelog series.  Spell out what you are going to do in a
> > > future change, as to why this is necessary.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Ok. Anything else is needed to address?
> 
> Ping?

I'll try to look at this again next week. I did notice that your SoB
chains are wrong as Marek did not submit these changes and it's not
clear whether he's an author at all.

Johan

Re: [PATCH v3 3/7] USB: serial: ftdi_sio: Extract SIO divisor code to function

[Pali Rohár]

On Wednesday 02 November 2022 02:47:50 Greg Kroah-Hartman wrote:
> On Tue, Nov 01, 2022 at 11:50:57PM +0100, Pali Rohár wrote:
> > On Sunday 09 October 2022 14:17:07 Pali Rohár wrote:
> > > On Saturday 24 September 2022 12:47:06 Greg Kroah-Hartman wrote:
> > > > On Sat, Sep 24, 2022 at 12:27:14PM +0200, Pali Rohár wrote:
> > > > > In preparation for following changes,
> > > > 
> > > > What do you mean by "following changes"?  That doesn't work well when
> > > > looking in a changelog series.  Spell out what you are going to do in a
> > > > future change, as to why this is necessary.
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > Ok. Anything else is needed to address?
> > 
> > Ping?
> 
> We have no idea, sorry, as we have no context here at all and it was
> thousands of patches ago in our reviews.  Please just fix things up
> based on this review and resubmit, you never need to ask.
> 
> greg k-h

It was already done in v3 and it contains only 7 patches, not thousands.

Re: [PATCH v3 1/7] USB: serial: ftdi_sio: Fix divisor overflow

[Johan Hovold]

On Sat, Sep 24, 2022 at 12:27:12PM +0200, Pali Rohár wrote:
> The baud rate generating divisor is a 17-bit wide (14 bits integer part
> and 3 bits fractional part).
> 
> Example:
>   base clock = 48 MHz
>   requested baud rate = 180 Baud
>   divisor = 48,000,000 / (16 * 180) = 0b100000100011010.101
> 
>   In this case the MSB gets discarded because of the overflow, and the
>   actually used divisor will be 0b100011010.101 = 282.625, resulting
>   in baud rate 10615 Baud, instead of the requested 180 Baud.
> 
> The best possible thing to do in this case is to generate lowest possible
> baud rate (in the example it would be 183 Baud), by using maximum possible
> divisor.

As I already commented on v2:

	Actually, the best way to handle this is to add a sanity check
	for the lowest supported check as you do in the next patch. That
	one makes this change superfluous.

> In case of divisor overflow, use maximum possible divisor.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Tested-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>

And as I mentioned the other week, this SoB change is not correct as it
is you who are submitting these patches now and it's not clear whether
you intended that Marek should be attributed as co-author or not.

Johan

Re: [PATCH v3 2/7] USB: serial: ftdi_sio: Add missing baud rate validation

[Johan Hovold]

On Sat, Sep 24, 2022 at 12:27:13PM +0200, Pali Rohár wrote:
> Add lower checks for requested baud rate for FT8U232AM, FT232BM, FT2232C,
> FT232RL, FTX, FT2232H, FT4232H and FT232H. For all of these the minimum
> baud rate they can generate is 183 Baud.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Tested-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/usb/serial/ftdi_sio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index b2febe8d573a..bfa601fc14fe 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1335,7 +1335,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  		}
>  		break;
>  	case FT8U232AM: /* 8U232AM chip */
> -		if (baud <= 3000000) {
> +		if (baud >= 183 && baud <= 3000000) {
>  			div_value = ftdi_232am_baud_to_divisor(baud);
>  		} else {
>  			dev_dbg(dev, "%s - Baud rate too high!\n", __func__);

As someone else already pointed out, you should update these too. Please
drop the exclamation mark while at it:

	dev_dbg(dev, "invalid line speed\n")

should do.

Note that this series needs to be rebased on 6.1-rc too as the context
has changed.

> @@ -1348,7 +1348,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  	case FT2232C: /* FT2232C chip */
>  	case FT232RL: /* FT232RL chip */
>  	case FTX:     /* FT-X series */
> -		if (baud <= 3000000) {
> +		if (baud >= 183 && baud <= 3000000) {
>  			u16 product_id = le16_to_cpu(
>  				port->serial->dev->descriptor.idProduct);
>  			if (((product_id == FTDI_NDI_HUC_PID)		||

I think there's another dev_dbg() here below.

> @@ -1372,7 +1372,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  	case FT232H:  /* FT232H chip */
>  		if ((baud <= 12000000) && (baud >= 1200)) {
>  			div_value = ftdi_2232h_baud_to_divisor(baud);
> -		} else if (baud < 1200) {
> +		} else if (baud >= 183 && baud < 1200) {
>  			div_value = ftdi_232bm_baud_to_divisor(baud);
>  		} else {
>  			dev_dbg(dev, "%s - Baud rate too high!\n", __func__);

Same here.

Johan

Re: [PATCH v3 3/7] USB: serial: ftdi_sio: Extract SIO divisor code to function

[Johan Hovold]

On Sat, Sep 24, 2022 at 12:27:14PM +0200, Pali Rohár wrote:
> In preparation for following changes, extract divisor code for SIO chip
> into new function ftdi_sio_baud_to_divisor(), as is done for other
> chips.

Please spell out what you mean by "following changes".

You're also now using the new helper to set the fallback rate, which
should at least be mentioned as it looks like a separate and unnecessary
change currently.

> No functional change.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Tested-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/usb/serial/ftdi_sio.c | 45 ++++++++++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index bfa601fc14fe..fe8a7c5fa0e9 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c

> @@ -1314,23 +1342,12 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  		baud = 9600;
>  	switch (priv->chip_type) {
>  	case SIO: /* SIO chip */
> -		switch (baud) {
> -		case 300: div_value = ftdi_sio_b300; break;
> -		case 600: div_value = ftdi_sio_b600; break;
> -		case 1200: div_value = ftdi_sio_b1200; break;
> -		case 2400: div_value = ftdi_sio_b2400; break;
> -		case 4800: div_value = ftdi_sio_b4800; break;
> -		case 9600: div_value = ftdi_sio_b9600; break;
> -		case 19200: div_value = ftdi_sio_b19200; break;
> -		case 38400: div_value = ftdi_sio_b38400; break;
> -		case 57600: div_value = ftdi_sio_b57600;  break;
> -		case 115200: div_value = ftdi_sio_b115200; break;
> -		} /* baud */
> -		if (div_value == 0) {
> +		div_value = ftdi_sio_baud_to_divisor(baud);
> +		if (div_value == (u32)-1) {
>  			dev_dbg(dev, "%s - Baudrate (%d) requested is not supported\n",
>  				__func__,  baud);
> -			div_value = ftdi_sio_b9600;
>  			baud = 9600;
> +			div_value = ftdi_sio_baud_to_divisor(baud);
>  			div_okay = 0;
>  		}
>  		break;

This one too needs to be rebased, but you'll notice that.

Johan

Re: [PATCH v3 4/7] USB: serial: ftdi_sio: Do not reset baud rate to 9600 Baud on error

[Johan Hovold]

On Sat, Sep 24, 2022 at 12:27:15PM +0200, Pali Rohár wrote:
> If setting new baud rate fails, some other drivers leave the device at
> previous baud rate, and ftdi_sio resets to 9600 Baud.
> 
> Change this behavior to not reset baud rate to 9600.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Tested-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/usb/serial/ftdi_sio.c | 47 ++++++++++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index fe8a7c5fa0e9..1ab6bf48516f 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1315,7 +1315,7 @@ static int update_mctrl(struct usb_serial_port *port, unsigned int set,
>  
>  
>  static u32 get_ftdi_divisor(struct tty_struct *tty,
> -						struct usb_serial_port *port)
> +			    struct usb_serial_port *port, speed_t old_baud)
>  {
>  	struct ftdi_private *priv = usb_get_serial_port_data(port);
>  	struct device *dev = &port->dev;
> @@ -1338,6 +1338,8 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  			__func__, priv->custom_divisor, baud);
>  	}
>  
> +	if (!baud)
> +		baud = old_baud;
>  	if (!baud)
>  		baud = 9600;

While not apparent from just looking at the diff, this is only used for
SPD_CUST for and for which you now always pass in 9600 as old_baud.

So just drop this.

>  	switch (priv->chip_type) {
> @@ -1346,8 +1348,12 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  		if (div_value == (u32)-1) {
>  			dev_dbg(dev, "%s - Baudrate (%d) requested is not supported\n",
>  				__func__,  baud);
> -			baud = 9600;
> +			baud = old_baud ? old_baud : 9600;

Please avoid the ternary operator.

>  			div_value = ftdi_sio_baud_to_divisor(baud);

How can setting the old speed fail here? That should not be possible
after you checked for B0, right?

> +			if (div_value == -1) {

Cast for consistency (unless you remove this).

> +				baud = 9600;
> +				div_value = ftdi_sio_baud_to_divisor(baud);
> +			}
>  			div_okay = 0;
>  		}
>  		break;
> @@ -1356,8 +1362,11 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  			div_value = ftdi_232am_baud_to_divisor(baud);
>  		} else {
>  			dev_dbg(dev, "%s - Baud rate too high!\n", __func__);
> -			baud = 9600;
> -			div_value = ftdi_232am_baud_to_divisor(9600);
> +			if (old_baud >= 183 && old_baud <= 3000000)
> +				baud = old_baud;
> +			else
> +				baud = 9600;
> +			div_value = ftdi_232am_baud_to_divisor(baud);
>  			div_okay = 0;
>  		}
>  		break;
> @@ -1379,9 +1388,12 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  			div_value = ftdi_232bm_baud_to_divisor(baud);
>  		} else {
>  			dev_dbg(dev, "%s - Baud rate too high!\n", __func__);
> -			div_value = ftdi_232bm_baud_to_divisor(9600);
> +			if (old_baud >= 183 && old_baud <= 3000000)
> +				baud = old_baud;
> +			else
> +				baud = 9600;
> +			div_value = ftdi_232bm_baud_to_divisor(baud);
>  			div_okay = 0;
> -			baud = 9600;
>  		}
>  		break;
>  	case FT2232H: /* FT2232H chip */
> @@ -1393,9 +1405,17 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  			div_value = ftdi_232bm_baud_to_divisor(baud);
>  		} else {
>  			dev_dbg(dev, "%s - Baud rate too high!\n", __func__);
> -			div_value = ftdi_232bm_baud_to_divisor(9600);
> +			if (old_baud >= 183 && old_baud < 1200) {
> +				baud = old_baud;
> +				div_value = ftdi_232bm_baud_to_divisor(baud);
> +			} else {
> +				if (old_baud >= 1200 && old_baud <= 12000000)
> +					baud = old_baud;
> +				else
> +					baud = 9600;
> +				div_value = ftdi_2232h_baud_to_divisor(baud);
> +			}

Please try to refactor this so that you don't have to open-code the same
test for baud as well as old_baud.

This function was hard to read already before this series and we should
not make it worse.

>  			div_okay = 0;
> -			baud = 9600;
>  		}
>  		break;
>  	} /* priv->chip_type */
> @@ -1410,7 +1430,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  	return div_value;
>  }
>  
> -static int change_speed(struct tty_struct *tty, struct usb_serial_port *port)
> +static int change_speed(struct tty_struct *tty, struct usb_serial_port *port, speed_t old_baud)
>  {
>  	struct ftdi_private *priv = usb_get_serial_port_data(port);
>  	u16 value;
> @@ -1418,7 +1438,7 @@ static int change_speed(struct tty_struct *tty, struct usb_serial_port *port)
>  	u32 index_value;
>  	int rv;
>  
> -	index_value = get_ftdi_divisor(tty, port);
> +	index_value = get_ftdi_divisor(tty, port, old_baud);
>  	value = (u16)index_value;
>  	index = (u16)(index_value >> 16);
>  	if (priv->chip_type == FT2232C || priv->chip_type == FT2232H ||
> @@ -1541,7 +1561,7 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
>  		if (priv->flags & ASYNC_SPD_MASK)
>  			dev_warn_ratelimited(&port->dev, "use of SPD flags is deprecated\n");
>  
> -		change_speed(tty, port);
> +		change_speed(tty, port, 9600);
>  	}
>  	mutex_unlock(&priv->cfg_lock);
>  	return 0;
> @@ -2790,9 +2810,12 @@ static void ftdi_set_termios(struct tty_struct *tty,
>  		/* Drop RTS and DTR */
>  		clear_mctrl(port, TIOCM_DTR | TIOCM_RTS);
>  	} else {
> +		speed_t old_baud =
> +			old_termios ? tty_termios_baud_rate(old_termios) : 9600;

Use c_ospeed directly, and avoid the ternary operator.

> +
>  		/* set the baudrate determined before */
>  		mutex_lock(&priv->cfg_lock);
> -		if (change_speed(tty, port))
> +		if (change_speed(tty, port, old_baud))

But perhaps you should just pass in old_termios (or NULL in
set_serial_info()) so that you handle this in one place.

>  			dev_err(ddev, "%s urb failed to set baudrate\n", __func__);
>  		mutex_unlock(&priv->cfg_lock);
>  		/* Ensure RTS and DTR are raised when baudrate changed from 0 */

Johan

Re: [PATCH v3 5/7] USB: serial: ftdi_sio: Fix baud rate rounding for ASYNC_SPD_CUST

[Johan Hovold]

On Sat, Sep 24, 2022 at 12:27:16PM +0200, Pali Rohár wrote:
> To compute more accurate baud rate when user uses ASYNC_SPD_CUST API,
> use DIV_ROUND_CLOSEST() instead of just rounding down.
> 
> Rationale:
>   Application uses old API, so it computes divisor D for baud rate B.

s/old/deprecated/

>   The driver then tries to compute back the requested baud rate B, but
>   rounds down in the division.
> 
>   Using rounding to closest value instead should increate accuracy here.

typo: increase

> Signed-off-by: Pali Rohár <pali@kernel.org>
> Tested-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/usb/serial/ftdi_sio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 1ab6bf48516f..718c86db2900 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1333,7 +1333,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  	if (baud == 38400 &&
>  	    ((priv->flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST) &&
>  	     (priv->custom_divisor)) {
> -		baud = priv->baud_base / priv->custom_divisor;
> +		baud = DIV_ROUND_CLOSEST(priv->baud_base, priv->custom_divisor);
>  		dev_dbg(dev, "%s - custom divisor %d sets baud rate to %d\n",
>  			__func__, priv->custom_divisor, baud);
>  	}

I'm having second thoughts about this one. The SPD_CUST hack should not
be used anymore, but it was supposed to be used to set the hardware
divisor directly. Someone was creative and reinterpreted this for for
the FTDI driver to mean software divisor instead. So instead of
understanding how the hardware determines rates from divisors, you know
needed knowledge of how the FTDI driver happens to work, including that
it rounds down. And now you're changing that again.

Perhaps we should just leave this as is. This interface has been
deprecated for decades and comes with a deprecated warning since several
years. Would be nice to drop it completely instead.

Johan

Re: [PATCH v3 6/7] USB: serial: ftdi_sio: Fix custom_divisor for TIOCGSERIAL and c_*speed for TCGETS2

[Johan Hovold]

On Sat, Sep 24, 2022 at 12:27:17PM +0200, Pali Rohár wrote:
> When ASYNC_SPD_CUST is used, update custom_divisor field for TIOCGSERIAL
> and c_*speed fields for TCGETS2 so that they correspond to the newly set
> baud rate value.
> 
> So userspace TCGETS2 ioctls in new c_*speed fields will see the true baud
> rate that is being used.
> 
> This is needed for switching userspace applications to use TCGETS2 API as
> currently new c_*speed fields does not report correct values. Without this
> change userspace applications still have to use old deprecated TIOCGSERIAL
> to retrieve current baud rate.

Still no. Not happening.

We should not try to determine the rates used when setting (hardware)
divisors directly through the deprecated SPD_CUST hack. Serial core
does not do so for a reason, including that you can set 38400 once and
then fiddle around with setserial all you want without the magic value
changing.

USB serial regressed at one point by starting to report back the rate it
would try to set. I left it in place because it took a fair while before
anyone noticed and no one should be using this interface anyway.

But if you try to generalise this, I'd rather fix that bug or just rip
this out completely.

Johan

Re: [PATCH v3 7/7] USB: serial: ftdi_sio: Fill c_*speed fields with real baud rate

[Johan Hovold]

On Sat, Sep 24, 2022 at 12:27:18PM +0200, Pali Rohár wrote:
> Calculate baud rate value in c_*speed fields to the real values which were
> set on hardware. For this operation, add a new set of methods
> *_divisor_to_baud() for each chip and use them for calculating the real
> baud rate values.
> 
> Each *_divisor_to_baud() method is constructed as an inverse function of
> its corresponding *_baud_to_divisor() method.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Tested-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/usb/serial/ftdi_sio.c | 93 +++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 79b00912c81c..350ed14b014c 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1183,6 +1183,34 @@ static u32 ftdi_sio_baud_to_divisor(int baud)
>  	}
>  }
>  
> +static int ftdi_sdio_divisor_to_baud(u32 divisor)
> +{
> +	switch (divisor) {
> +	case ftdi_sio_b300:
> +		return 300;
> +	case ftdi_sio_b600:
> +		return 600;
> +	case ftdi_sio_b1200:
> +		return 1200;
> +	case ftdi_sio_b2400:
> +		return 2400;
> +	case ftdi_sio_b4800:
> +		return 4800;
> +	case ftdi_sio_b9600:
> +		return 9600;
> +	case ftdi_sio_b19200:
> +		return 19200;
> +	case ftdi_sio_b38400:
> +		return 38400;
> +	case ftdi_sio_b57600:
> +		return 57600;
> +	case ftdi_sio_b115200:
> +		return 115200;
> +	default:
> +		return 9600;
> +	}
> +}

As I mentioned before, this one should no be needed as the SIO only
supports this discrete set of values (or errors out).

> +
>  static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
>  {
>  	unsigned short int divisor;
> @@ -1205,11 +1233,28 @@ static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
>  	return divisor;
>  }
>  
> +static int ftdi_232am_divisor_base_to_baud(unsigned short int divisor, int base)
> +{
> +	static const unsigned char divfrac_inv[4] = { 0, 4, 2, 1 };
> +	unsigned int divisor3;
> +
> +	if (divisor == 0)
> +		divisor = 1;
> +	divisor3 = (GENMASK(13, 0) & divisor) << 3;
> +	divisor3 |= divfrac_inv[(divisor >> 14) & 0x3];
> +	return DIV_ROUND_CLOSEST(base, 2 * divisor3);
> +}

Still no motivation to review these.

[...]
  
> @@ -1358,6 +1444,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  			}
>  			div_okay = 0;
>  		}
> +		baud = ftdi_sdio_divisor_to_baud(div_value);
>  		break;
>  	case FT8U232AM: /* 8U232AM chip */
>  		if (baud >= 183 && baud <= 3000000) {
> @@ -1371,6 +1458,7 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  			div_value = ftdi_232am_baud_to_divisor(baud);
>  			div_okay = 0;
>  		}
> +		baud = ftdi_232am_divisor_to_baud(div_value);
>  		break;
>  	case FT232BM: /* FT232BM chip */
>  	case FT2232C: /* FT2232C chip */
> @@ -1397,25 +1485,30 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  			div_value = ftdi_232bm_baud_to_divisor(baud);
>  			div_okay = 0;
>  		}
> +		baud = ftdi_232bm_divisor_to_baud(div_value);
>  		break;
>  	case FT2232H: /* FT2232H chip */
>  	case FT4232H: /* FT4232H chip */
>  	case FT232H:  /* FT232H chip */
>  		if ((baud <= 12000000) && (baud >= 1200)) {
>  			div_value = ftdi_2232h_baud_to_divisor(baud);
> +			baud = ftdi_2232h_divisor_to_baud(div_value);
>  		} else if (baud >= 183 && baud < 1200) {
>  			div_value = ftdi_232bm_baud_to_divisor(baud);
> +			baud = ftdi_232bm_divisor_to_baud(div_value);
>  		} else {
>  			dev_dbg(dev, "%s - Baud rate too high!\n", __func__);
>  			if (old_baud >= 183 && old_baud < 1200) {
>  				baud = old_baud;
>  				div_value = ftdi_232bm_baud_to_divisor(baud);
> +				baud = ftdi_232bm_divisor_to_baud(div_value);
>  			} else {
>  				if (old_baud >= 1200 && old_baud <= 12000000)
>  					baud = old_baud;
>  				else
>  					baud = 9600;
>  				div_value = ftdi_2232h_baud_to_divisor(baud);
> +				baud = ftdi_2232h_divisor_to_baud(div_value);

But this must be cleaned up as mentioned earlier in this thread.

>  			}
>  			div_okay = 0;
>  		}

Johan