[PATCH 0/7] ftdi_sio driver improvements

[PATCH 0/7] ftdi_sio driver improvements

[Marek Behún]

Hello Greg,

Pali has worked out some improvements for the ftdi_sio USB serial
driver and I have tested them.

Marek

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

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

-- 
2.35.1

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

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Divisor in the register is a 17-bit wide number.
Therefore we need to clamp it on overflow.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-by: Marek Behún <kabel@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 b440d338a895..ea40f367e70c 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1157,6 +1157,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)
@@ -1181,6 +1183,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. */
@@ -1204,6 +1208,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.35.1

[PATCH 2/7] USB: serial: ftdi_sio: Add missing baudrate validation

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

More FTDI variants limit the minimal baudrate value. Add lower bound
checks.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-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 ea40f367e70c..717b97f4e094 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1330,7 +1330,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__);
@@ -1343,7 +1343,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)		||
@@ -1367,7 +1367,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.35.1

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

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

As a code cleanup for future changes, extract divisor code for SIO chip
into new function ftdi_sio_baud_to_divisor().

No functional change.

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

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 717b97f4e094..45a4eeb1fc70 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1150,6 +1150,23 @@ 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;
@@ -1309,23 +1326,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.35.1

[PATCH 4/7] USB: serial: ftdi_sio: Do not reset baudrate to 9600 on error

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

On failure to set new baudrate, reset baudrate to the previous value
(as is done by other serial drivers) instead of resetting to 9600.

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

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 45a4eeb1fc70..30744d5779e2 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1299,7 +1299,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;
@@ -1322,6 +1322,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) {
@@ -1330,8 +1332,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 == (u32)-1) {
+				baud = 9600;
+				div_value = ftdi_sio_baud_to_divisor(baud);
+			}
 			div_okay = 0;
 		}
 		break;
@@ -1340,8 +1346,8 @@ 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);
+			baud = (old_baud >= 183 && old_baud <= 3000000) ? old_baud : 9600;
+			div_value = ftdi_232am_baud_to_divisor(baud);
 			div_okay = 0;
 		}
 		break;
@@ -1363,9 +1369,9 @@ 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);
+			baud = (old_baud >= 183 && old_baud <= 3000000) ? old_baud : 9600;
+			div_value = ftdi_232bm_baud_to_divisor(baud);
 			div_okay = 0;
-			baud = 9600;
 		}
 		break;
 	case FT2232H: /* FT2232H chip */
@@ -1377,9 +1383,14 @@ 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 {
+				baud = (old_baud >= 1200 && old_baud <= 12000000) ? old_baud : 9600;
+				div_value = ftdi_2232h_baud_to_divisor(baud);
+			}
 			div_okay = 0;
-			baud = 9600;
 		}
 		break;
 	} /* priv->chip_type */
@@ -1394,7 +1405,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;
@@ -1402,7 +1413,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 ||
@@ -1525,7 +1536,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, 0);
 	}
 	mutex_unlock(&priv->cfg_lock);
 	return 0;
@@ -2774,9 +2785,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) : 0;
+
 		/* 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.35.1

[PATCH 5/7] USB: serial: ftdi_sio: Fix baudrate rounding for ASYNC_SPD_CUST

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Use DIV_ROUND_CLOSEST() for more accurate baudrate calculation for
ASYNC_SPD_CUST instead of rounding it just down.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-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 30744d5779e2..ac84d5779966 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1317,7 +1317,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.35.1

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

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

When ASYNC_SPD_CUST is used, update and recalculate custom_divisor and
c_*speed fields from newly set baudrate value, so that userspace GET
functions can see current configuration.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-by: Marek Behún <kabel@kernel.org>
---
 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 ac84d5779966..3bf5750e76de 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1303,6 +1303,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;
@@ -1317,6 +1318,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);
@@ -1401,7 +1403,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 baudrate */
+	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 baudrate */
+	if (fix_custom_divisor)
+		tty->termios.c_ispeed = tty->termios.c_ospeed = old_baud;
+
 	return div_value;
 }
 
@@ -2674,6 +2688,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.35.1

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

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Calculate baudrate value in c_*speed fields to the real value which was set
to hardware. For this operation add a new set of functions divisor_to_baud
for each chip and use it for calculating the real baudrate value.

Each divisor_to_baud function is constructed as an inverse function of
corresponding baud_to_divisor function.

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

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 3bf5750e76de..1f78ae695a1b 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1167,6 +1167,23 @@ 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;
@@ -1189,11 +1206,27 @@ 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 };
@@ -1212,11 +1245,30 @@ 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 };
@@ -1244,11 +1296,31 @@ 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))
 
@@ -1342,6 +1414,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) {
@@ -1352,6 +1425,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 */
@@ -1375,22 +1449,27 @@ 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 {
 				baud = (old_baud >= 1200 && old_baud <= 12000000) ? old_baud : 9600;
 				div_value = ftdi_2232h_baud_to_divisor(baud);
+				baud = ftdi_2232h_divisor_to_baud(div_value);
 			}
 			div_okay = 0;
 		}
-- 
2.35.1

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

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 04:53:50PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> As a code cleanup for future changes, extract divisor code for SIO chip
> into new function ftdi_sio_baud_to_divisor().
> 
> No functional change.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Tested-by: Marek Behún <kabel@kernel.org>

If you forward on patches from someone else, you too need to sign off on
them.  Please read the documentation for the details as to what that all
means.

We can't take these as-is without that, for obvious reasons.

thanks,

greg k-h

Re: [PATCH 0/7] ftdi_sio driver improvements

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 04:53:47PM +0200, Marek Behún wrote:
> Hello Greg,
> 
> Pali has worked out some improvements for the ftdi_sio USB serial
> driver and I have tested them.

Why me?  Please always use scripts/get_maintainer.pl

Use that for when you fix these up and resend, see my other response as
to why we can't take these as-is.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 04:53:48PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Divisor in the register is a 17-bit wide number.
> Therefore we need to clamp it on overflow.

Why, what is wrong with it overflowing, what will happen if it does?


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

What commit does this fix?  Is it a bugfix?  Can this ever happen in a
device?  Should it be backported?.

thanks,

greg k-h

Re: [PATCH 2/7] USB: serial: ftdi_sio: Add missing baudrate validation

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 04:53:49PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> More FTDI variants limit the minimal baudrate value. Add lower bound
> checks.

Which variants limit it?  Did you just break existing devices and keep
them from running at really low baud rates?

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 04:53:50PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> As a code cleanup for future changes, extract divisor code for SIO chip
> into new function ftdi_sio_baud_to_divisor().
> 
> No functional change.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Tested-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/usb/serial/ftdi_sio.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 717b97f4e094..45a4eeb1fc70 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1150,6 +1150,23 @@ 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;

Why not just return 9600 as a default here like the original code did?

And returning a negative number for a u32 is not a good idea for the
obvious reasons you found when you tried to test for it...

thanks,

greg k-h

Re: [PATCH 4/7] USB: serial: ftdi_sio: Do not reset baudrate to 9600 on error

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 04:53:51PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> On failure to set new baudrate, reset baudrate to the previous value
> (as is done by other serial drivers) instead of resetting to 9600.

Where is it mandated that this is correct?  Why not keep the existing
functionality?  Did you just break systems that tried to set invalid
values and ended up with 9600 as a default?

What changed to make this a new requirement?

thanks,

greg k-h

Re: [PATCH 5/7] USB: serial: ftdi_sio: Fix baudrate rounding for ASYNC_SPD_CUST

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 04:53:52PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Use DIV_ROUND_CLOSEST() for more accurate baudrate calculation for
> ASYNC_SPD_CUST instead of rounding it just down.

Why?  What does this change or fix?

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 04:53:54PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Calculate baudrate value in c_*speed fields to the real value which was set
> to hardware. For this operation add a new set of functions divisor_to_baud
> for each chip and use it for calculating the real baudrate value.
> 
> Each divisor_to_baud function is constructed as an inverse function of
> corresponding baud_to_divisor function.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Tested-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/usb/serial/ftdi_sio.c | 79 +++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 3bf5750e76de..1f78ae695a1b 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1167,6 +1167,23 @@ 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;
> @@ -1189,11 +1206,27 @@ 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 };
> @@ -1212,11 +1245,30 @@ 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);
> +}

Always run your patches through checkpatch so you do not get grumpy
maintainers telling you to run your patches through checkpatch...

thanks,

greg k-h

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

[Marek Behún]

On Thu, 7 Jul 2022 17:07:53 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Jul 07, 2022 at 04:53:48PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > Divisor in the register is a 17-bit wide number.
> > Therefore we need to clamp it on overflow.  
> 
> Why, what is wrong with it overflowing, what will happen if it does?

The divisor register is 17-bits wide (14 bits integer part, 3 bits
fractional). So suppose that we compute divisor 0x20001. Writing to
the register puts 0x00001 there, cause the 17th bit gets discarded
(since the register is 17-bits wide). Which will result in dividing by
1.

The best thing we can do if we overflow is put the maximum value to the
divisor, to get the lowest baudrate possible.

> 
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Tested-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/usb/serial/ftdi_sio.c | 6 ++++++
> >  1 file changed, 6 insertions(+)  
> 
> What commit does this fix?  Is it a bugfix?  Can this ever happen in a
> device?  Should it be backported?.

It is a bugfix; it can happen (happened to Pali when he was trying some
low baudrates); it should be backported.

But it was first introduced in the commit
  1da177e4c3f4 ("Linux-2.6.12-rc2")
which is the very first commit in git :)

Marek

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

[Marek Behún]

On Thu, 7 Jul 2022 17:06:15 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Jul 07, 2022 at 04:53:50PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > As a code cleanup for future changes, extract divisor code for SIO chip
> > into new function ftdi_sio_baud_to_divisor().
> > 
> > No functional change.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Tested-by: Marek Behún <kabel@kernel.org>  
> 
> If you forward on patches from someone else, you too need to sign off on
> them.  Please read the documentation for the details as to what that all
> means.
> 
> We can't take these as-is without that, for obvious reasons.

Sorry, forgot about that. Will do.

Marek

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

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 05:37:53PM +0200, Marek Behún wrote:
> On Thu, 7 Jul 2022 17:07:53 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Thu, Jul 07, 2022 at 04:53:48PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > > 
> > > Divisor in the register is a 17-bit wide number.
> > > Therefore we need to clamp it on overflow.  
> > 
> > Why, what is wrong with it overflowing, what will happen if it does?
> 
> The divisor register is 17-bits wide (14 bits integer part, 3 bits
> fractional). So suppose that we compute divisor 0x20001. Writing to
> the register puts 0x00001 there, cause the 17th bit gets discarded
> (since the register is 17-bits wide). Which will result in dividing by
> 1.

So worst case, if you have a broken device, is you will get the wrong
baudrate?  This isn't really a "bug" then, but just making things a bit
better if you have a crazy device?

> The best thing we can do if we overflow is put the maximum value to the
> divisor, to get the lowest baudrate possible.

Ok, so not a big deal.

> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Tested-by: Marek Behún <kabel@kernel.org>
> > > ---
> > >  drivers/usb/serial/ftdi_sio.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)  
> > 
> > What commit does this fix?  Is it a bugfix?  Can this ever happen in a
> > device?  Should it be backported?.
> 
> It is a bugfix; it can happen (happened to Pali when he was trying some
> low baudrates); it should be backported.

Ok, then properly tag cc: stable please.

thanks,

greg k-h

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

[Marek Behún]

On Thu, 7 Jul 2022 17:09:45 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Jul 07, 2022 at 04:53:50PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > As a code cleanup for future changes, extract divisor code for SIO chip
> > into new function ftdi_sio_baud_to_divisor().
> > 
> > No functional change.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Tested-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/usb/serial/ftdi_sio.c | 34 ++++++++++++++++++++--------------
> >  1 file changed, 20 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> > index 717b97f4e094..45a4eeb1fc70 100644
> > --- a/drivers/usb/serial/ftdi_sio.c
> > +++ b/drivers/usb/serial/ftdi_sio.c
> > @@ -1150,6 +1150,23 @@ 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;  
> 
> Why not just return 9600 as a default here like the original code did?

It keeps at 9600 as original code.
Before:
  div_value = ftdi_sio_b9600;
  baud = 9600;

After:
  baud = 9600;
  div_value = ftdi_sio_baud_to_divisor(baud);

The new function that converts between the enum values and real values
now indicates when conversion is not possible.

> And returning a negative number for a u32 is not a good idea for the
> obvious reasons you found when you tried to test for it...

Would s32 be ok?

Re: [PATCH 4/7] USB: serial: ftdi_sio: Do not reset baudrate to 9600 on error

[Marek Behún]

On Thu, 7 Jul 2022 17:10:42 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Jul 07, 2022 at 04:53:51PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > On failure to set new baudrate, reset baudrate to the previous value
> > (as is done by other serial drivers) instead of resetting to 9600.  
> 
> Where is it mandated that this is correct?  Why not keep the existing
> functionality?  Did you just break systems that tried to set invalid
> values and ended up with 9600 as a default?

Pali says all other drivers keep previous value on failure. He got
frustrated when working with FTDI devices because they behaved
differently.

> What changed to make this a new requirement?
> 
> thanks,
> 
> greg k-h

Re: [PATCH 4/7] USB: serial: ftdi_sio: Do not reset baudrate to 9600 on error

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 05:52:35PM +0200, Marek Behún wrote:
> On Thu, 7 Jul 2022 17:10:42 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Thu, Jul 07, 2022 at 04:53:51PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > > 
> > > On failure to set new baudrate, reset baudrate to the previous value
> > > (as is done by other serial drivers) instead of resetting to 9600.  
> > 
> > Where is it mandated that this is correct?  Why not keep the existing
> > functionality?  Did you just break systems that tried to set invalid
> > values and ended up with 9600 as a default?
> 
> Pali says all other drivers keep previous value on failure. He got
> frustrated when working with FTDI devices because they behaved
> differently.

That's fine, but again, you are changing user-visible behavior here, so
you need a real justification for it and be aware that it might break
people's workflow.

thanks,

greg k-h

Re: [PATCH 5/7] USB: serial: ftdi_sio: Fix baudrate rounding for ASYNC_SPD_CUST

[Marek Behún]

On Thu, 7 Jul 2022 17:11:00 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Jul 07, 2022 at 04:53:52PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > Use DIV_ROUND_CLOSEST() for more accurate baudrate calculation for
> > ASYNC_SPD_CUST instead of rounding it just down.  
> 
> Why?  What does this change or fix?

To compute more accurate baudrate when given custom divisor.

User requests a baudrate B.
Application uses old API, so it computes divisor D for baudrate B.
The driver then tries to compute back the requested baudrate B. To
compute it back more accurately, rounding to closes value should be
used.

Marek

Re: [PATCH 5/7] USB: serial: ftdi_sio: Fix baudrate rounding for ASYNC_SPD_CUST

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 06:08:18PM +0200, Marek Behún wrote:
> On Thu, 7 Jul 2022 17:11:00 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Thu, Jul 07, 2022 at 04:53:52PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > > 
> > > Use DIV_ROUND_CLOSEST() for more accurate baudrate calculation for
> > > ASYNC_SPD_CUST instead of rounding it just down.  
> > 
> > Why?  What does this change or fix?
> 
> To compute more accurate baudrate when given custom divisor.
> 
> User requests a baudrate B.
> Application uses old API, so it computes divisor D for baudrate B.
> The driver then tries to compute back the requested baudrate B. To
> compute it back more accurately, rounding to closes value should be
> used.

Then please describe this in the changelog text, don't make us guess...

Re: [PATCH 2/7] USB: serial: ftdi_sio: Add missing baudrate validation

[Marek Behún]

On Thu, 7 Jul 2022 17:08:35 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Thu, Jul 07, 2022 at 04:53:49PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > More FTDI variants limit the minimal baudrate value. Add lower bound
> > checks.  
> 
> Which variants limit it?  Did you just break existing devices and keep
> them from running at really low baud rates?

The variants for which the patch adds the checks. Does this need
to be listed it in commit message?

Marek

Re: [PATCH 2/7] USB: serial: ftdi_sio: Add missing baudrate validation

[Greg Kroah-Hartman]

On Thu, Jul 07, 2022 at 06:22:01PM +0200, Marek Behún wrote:
> On Thu, 7 Jul 2022 17:08:35 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Thu, Jul 07, 2022 at 04:53:49PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > > 
> > > More FTDI variants limit the minimal baudrate value. Add lower bound
> > > checks.  
> > 
> > Which variants limit it?  Did you just break existing devices and keep
> > them from running at really low baud rates?
> 
> The variants for which the patch adds the checks. Does this need
> to be listed it in commit message?

Why wouldn't you want to see that in a changelog text?

Re: [PATCH 4/7] USB: serial: ftdi_sio: Do not reset baudrate to 9600 on error

[Andy Shevchenko]

On Thu, Jul 7, 2022 at 4:57 PM Marek Behún <kabel@kernel.org> wrote:
>
> From: Pali Rohár <pali@kernel.org>
>
> On failure to set new baudrate, reset baudrate to the previous value

a new
baud rate

> (as is done by other serial drivers) instead of resetting to 9600.

...

> +                       baud = old_baud ? old_baud : 9600;

Can be written as

  baud = old_baud ?: 9600;

...

> +                       if (div_value == (u32)-1) {

Oh, unsigned -1? Can you define it with a meaningful name and depends
on the semantics use U32_MAX or GENMASK()?

> +                               baud = 9600;
> +                               div_value = ftdi_sio_baud_to_divisor(baud);
> +                       }

...

> +                       baud = (old_baud >= 183 && old_baud <= 3000000) ? old_baud : 9600;

These repetitive magics are error prone (easy to make a mistake or
off-by-one error). Can you create a simple helper for this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/7] USB: serial: ftdi_sio: Do not reset baudrate to 9600 on error

[m.brock]

Andy Shevchenko schreef op 2022-07-08 17:51:
>> +                       baud = old_baud ? old_baud : 9600;
> 
> Can be written as
> 
>   baud = old_baud ?: 9600;

That is invalid C !
This is a GCC extension.

Maarten

Re: [PATCH 4/7] USB: serial: ftdi_sio: Do not reset baudrate to 9600 on error

[Marek Behún]

On Tue, 12 Jul 2022 13:28:17 +0200
m.brock@vanmierlo.com wrote:

> Andy Shevchenko schreef op 2022-07-08 17:51:
> >> +                       baud = old_baud ? old_baud : 9600;  
> > 
> > Can be written as
> > 
> >   baud = old_baud ?: 9600;  
> 
> That is invalid C !
> This is a GCC extension.

Kernel is saturated with usage of GCC extensions.

Re: [PATCH 4/7] USB: serial: ftdi_sio: Do not reset baudrate to 9600 on error

[Marek Behún]

I forgot to look at your comments before sending v2, ouch :(

On Fri, 8 Jul 2022 17:51:00 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, Jul 7, 2022 at 4:57 PM Marek Behún <kabel@kernel.org> wrote:
> >
> > From: Pali Rohár <pali@kernel.org>
> >
> > On failure to set new baudrate, reset baudrate to the previous value  
> 
> a new
> baud rate
> 
> > (as is done by other serial drivers) instead of resetting to 9600.  
> 
> ...
> 
> > +                       baud = old_baud ? old_baud : 9600;  
> 
> Can be written as
> 
>   baud = old_baud ?: 9600;
> 
> ...
> 
> > +                       if (div_value == (u32)-1) {  
> 
> Oh, unsigned -1? Can you define it with a meaningful name and depends
> on the semantics use U32_MAX or GENMASK()?
> 
> > +                               baud = 9600;
> > +                               div_value = ftdi_sio_baud_to_divisor(baud);
> > +                       }  
> 
> ...
> 
> > +                       baud = (old_baud >= 183 && old_baud <= 3000000) ? old_baud : 9600;  
> 
> These repetitive magics are error prone (easy to make a mistake or
> off-by-one error). Can you create a simple helper for this?
>