[PATCH v2 0/7] ftdi_sio driver improvements

[PATCH v2 0/7] ftdi_sio driver improvements

[Marek Behún]

Hi guys,

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

Changes since v1:
- addressed issues pointed out by Greg: better commit messages,
  fixed checkpatch warning, dropped usage of (u32)-1

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 and c_*speed for
    ASYNC_SPD_CUST
  USB: serial: ftdi_sio: Fill c_*speed fields with real baud rate

 drivers/usb/serial/ftdi_sio.c | 187 ++++++++++++++++++++++++++++------
 1 file changed, 156 insertions(+), 31 deletions(-)

-- 
2.35.1

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

[Marek Behún]

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

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 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 v2 2/7] USB: serial: ftdi_sio: Add missing baud rate validation

[Marek Behún]

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

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 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 v2 3/7] USB: serial: ftdi_sio: Extract SIO divisor code to function

[Marek Behún]

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

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 | 36 ++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 717b97f4e094..5b109714612f 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 int 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;
@@ -1286,7 +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;
-	u32 div_value = 0;
+	int div_value = 0;
 	int div_okay = 1;
 	int baud;
 
@@ -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 == -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 v2 4/7] USB: serial: ftdi_sio: Do not reset baud rate to 9600 Baud on error

[Marek Behún]

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

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

Change this behavior to that of other drivers so that /dev/ttyUSB*
devices behave in the same way.

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>
---
Greg pointed out that this make break some people's workflow, that they
may depend on this behavior.

This is of course possible, although IMO improbable: it would be weird
to depend on the fall back to 9600 Baud on error, instead of expecting
that the baud rate didn't change at all (like in other /dev/ttyUSB*
drivers).

Nonetheless if someone complains that they workflow got broken, we will
need to revert this.
---
 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 5b109714612f..cdbba1a9edd9 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 == -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;
@@ -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 v2 5/7] USB: serial: ftdi_sio: Fix baud rate rounding for ASYNC_SPD_CUST

[Marek Behún]

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

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 cdbba1a9edd9..5db1293bb7a2 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 v2 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 custom_divisor and c_*speed fields
so that they correspond to the newly set baud rate value, so that
userspace GET ioctls will see the true baud rate that is being used.

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 | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 5db1293bb7a2..39e8c5d06157 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;
 	int 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 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;
 }
 
@@ -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 v2 7/7] USB: serial: ftdi_sio: Fill c_*speed fields with real baud rate

[Marek Behún]

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

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 | 83 +++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 39e8c5d06157..838acce53e69 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1167,6 +1167,23 @@ static int 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,15 +1206,33 @@ 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 };
 	u32 divisor;
+
 	/* divisor shifted 3 bits to the left */
 	int divisor3 = DIV_ROUND_CLOSEST(base, 2 * baud);
 	if (divisor3 > GENMASK(16, 0))
@@ -1212,11 +1247,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 };
@@ -1244,11 +1299,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))
 
@@ -1342,6 +1418,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 +1429,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 +1453,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 v2 2/7] USB: serial: ftdi_sio: Add missing baud rate validation

[Rob Pearce]

On 12/07/2022 12:53, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
>
> 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 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__);
Due to the above change, this debug message is now incorrect.
> @@ -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__);
As above

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

[Marek Behún]

On Tue, 12 Jul 2022 18:27:28 +0100
Rob Pearce <rob@flitspace.org.uk> wrote:

> On 12/07/2022 12:53, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> >
> > 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 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__);  
> Due to the above change, this debug message is now incorrect.

Thx for noticing that. I will update. Also I will probably do some more
refactors, I am not yet sure.

Marek

> > @@ -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__);  
> As above
> 
> 

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

[Johan Hovold]

On Tue, Jul 12, 2022 at 01:53:00PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> 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.

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.

Johan

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

[Johan Hovold]

On Tue, Jul 12, 2022 at 01:53:02PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> 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 | 36 ++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 717b97f4e094..5b109714612f 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 int 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;

Please add a line break after the colons.

Adding another enum for the invalid value might be preferable.

> +	}
> +}
> +
>  static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
>  {
>  	unsigned short int divisor;
> @@ -1286,7 +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;
> -	u32 div_value = 0;
> +	int div_value = 0;

And don't change the type here.

>  	int div_okay = 1;
>  	int baud;
>  
> @@ -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 == -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;

Johan

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

[Johan Hovold]

On Tue, Jul 12, 2022 at 01:53:03PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> If setting new baud rate fails, all other drivers leave the device at
> previous baud rate, only ftdi_sio resets to 9600 Baud.

> Change this behavior to that of other drivers so that /dev/ttyUSB*
> devices behave in the same way.

These statements are not true. Several USB serial driver set 9600 baud
on errors for historical reasons. Yet others clamp. It's inconsistent at
best.

> 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>
> ---
> Greg pointed out that this make break some people's workflow, that they
> may depend on this behavior.
> 
> This is of course possible, although IMO improbable: it would be weird
> to depend on the fall back to 9600 Baud on error, instead of expecting
> that the baud rate didn't change at all (like in other /dev/ttyUSB*
> drivers).
> 
> Nonetheless if someone complains that they workflow got broken, we will
> need to revert this.

>  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;

This looks like it breaks B0 handling. Either way, an unrelated change.

>  	switch (priv->chip_type) {
> @@ -1330,8 +1332,12 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
>  		if (div_value == -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;
> @@ -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;

And please avoid using the ternary operator which tend to just hurt
readability.

Looks like this needs to be refactored in some way.

> @@ -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);

Zero has a special meaning (B0).

>  	}
>  	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;

Just use 9600 if you don't have an old termios. The termios rate should
be valid in that case.

Johan

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

[Johan Hovold]

On Tue, Jul 12, 2022 at 01:53:04PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> 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 cdbba1a9edd9..5db1293bb7a2 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);

Sure, why not, but this should only be used for the following debug
statement. (See next patch).

>  		dev_dbg(dev, "%s - custom divisor %d sets baud rate to %d\n",
>  			__func__, priv->custom_divisor, baud);
>  	}

Johan

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

[Johan Hovold]

On Tue, Jul 12, 2022 at 01:53:05PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> When ASYNC_SPD_CUST is used, update custom_divisor and c_*speed fields
> so that they correspond to the newly set baud rate value, so that
> userspace GET ioctls will see the true baud rate that is being used.

No, this is wrong.

In fact, there's a long-standing bug in this driver which started
reporting back the actual baud rate when using SPD_CUST. The rate should
be left unchanged at 38400 in that case.

> 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 | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 5db1293bb7a2..39e8c5d06157 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;
>  	int 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 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;
>  }
>  
> @@ -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. */

Johan

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

[Pali Rohár]

On Sunday 24 July 2022 14:28:43 Johan Hovold wrote:
> On Tue, Jul 12, 2022 at 01:53:05PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > When ASYNC_SPD_CUST is used, update custom_divisor and c_*speed fields
> > so that they correspond to the newly set baud rate value, so that
> > userspace GET ioctls will see the true baud rate that is being used.
> 
> No, this is wrong.
> 
> In fact, there's a long-standing bug in this driver which started
> reporting back the actual baud rate when using SPD_CUST.

Hello! And this commit is fixing also this bug as a side change.

> The rate should be left unchanged at 38400 in that case.

With this change, rate in c_cflag is unchanged and stays at B38400.

What is updated is the real baudrate in c_ispeed and c_ospeed
extensions.

It is really wrong? I thought that c_cflag should stay unchanged at
B38400 when ASYNC_SPD_CUST is used.

> > 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 | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> > index 5db1293bb7a2..39e8c5d06157 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;
> >  	int 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 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;
> >  }
> >  
> > @@ -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. */
> 
> Johan

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

[Johan Hovold]

On Tue, Jul 12, 2022 at 01:53:06PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> 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 | 83 +++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 39e8c5d06157..838acce53e69 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1167,6 +1167,23 @@ static int 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;
> +	}
> +}

This one should not be needed as sio only supports this discrete set of
values in the first place.

>  static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
>  {
>  	unsigned short int divisor;
> @@ -1189,15 +1206,33 @@ 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)

I believe "base" was used as a function name suffix in the inverse
function (due to the additional base argument).

> +{
> +	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);
> +}

I don't have the motivation to try to review these inverses right now.

Let's get the rest of the series in order first.

Johan

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

[Johan Hovold]

On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote:
> On Sunday 24 July 2022 14:28:43 Johan Hovold wrote:
> > On Tue, Jul 12, 2022 at 01:53:05PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > > 
> > > When ASYNC_SPD_CUST is used, update custom_divisor and c_*speed fields
> > > so that they correspond to the newly set baud rate value, so that
> > > userspace GET ioctls will see the true baud rate that is being used.
> > 
> > No, this is wrong.
> > 
> > In fact, there's a long-standing bug in this driver which started
> > reporting back the actual baud rate when using SPD_CUST.
> 
> Hello! And this commit is fixing also this bug as a side change.

Ah, indeed it is, or at least to some extent.

> > The rate should be left unchanged at 38400 in that case.
> 
> With this change, rate in c_cflag is unchanged and stays at B38400.

Right.

> What is updated is the real baudrate in c_ispeed and c_ospeed
> extensions.
> 
> It is really wrong? I thought that c_cflag should stay unchanged at
> B38400 when ASYNC_SPD_CUST is used.

Yeah, cflags stay unchanged, but you shouldn't touch those fields when
using the deprecated ASYNC_SPD_CUST hack.

Note that this currently only works because the ftdi driver uses
tty_get_baud_rate() instead of c_ospeed directly which is the
recommended (new) way.
 
> > > +	/* 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;
> > >  }
> > >  
> > > @@ -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. */

Johan

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

[Pali Rohár]

On Sunday 24 July 2022 14:54:58 Johan Hovold wrote:
> On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote:
> > On Sunday 24 July 2022 14:28:43 Johan Hovold wrote:
> > > On Tue, Jul 12, 2022 at 01:53:05PM +0200, Marek Behún wrote:
> > > > From: Pali Rohár <pali@kernel.org>
> > > > 
> > > > When ASYNC_SPD_CUST is used, update custom_divisor and c_*speed fields
> > > > so that they correspond to the newly set baud rate value, so that
> > > > userspace GET ioctls will see the true baud rate that is being used.
> > > 
> > > No, this is wrong.
> > > 
> > > In fact, there's a long-standing bug in this driver which started
> > > reporting back the actual baud rate when using SPD_CUST.
> > 
> > Hello! And this commit is fixing also this bug as a side change.
> 
> Ah, indeed it is, or at least to some extent.
> 
> > > The rate should be left unchanged at 38400 in that case.
> > 
> > With this change, rate in c_cflag is unchanged and stays at B38400.
> 
> Right.
> 
> > What is updated is the real baudrate in c_ispeed and c_ospeed
> > extensions.
> > 
> > It is really wrong? I thought that c_cflag should stay unchanged at
> > B38400 when ASYNC_SPD_CUST is used.
> 
> Yeah, cflags stay unchanged, but you shouldn't touch those fields when
> using the deprecated ASYNC_SPD_CUST hack.

Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed)
should contain current real speed. What is the reason that c_*speed
fields should have 38400 when ASYNC_SPD_CUST hack is set?

> Note that this currently only works because the ftdi driver uses
> tty_get_baud_rate() instead of c_ospeed directly which is the
> recommended (new) way.

Yes, tty_get_baud_rate() helper function is there for this purpose,
right?

> > > > +	/* 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;
> > > >  }
> > > >  
> > > > @@ -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. */
> 
> Johan

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

[Johan Hovold]

On Sun, Jul 24, 2022 at 02:59:08PM +0200, Pali Rohár wrote:
> On Sunday 24 July 2022 14:54:58 Johan Hovold wrote:
> > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote:

> > > What is updated is the real baudrate in c_ispeed and c_ospeed
> > > extensions.
> > > 
> > > It is really wrong? I thought that c_cflag should stay unchanged at
> > > B38400 when ASYNC_SPD_CUST is used.
> > 
> > Yeah, cflags stay unchanged, but you shouldn't touch those fields when
> > using the deprecated ASYNC_SPD_CUST hack.
> 
> Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed)
> should contain current real speed. What is the reason that c_*speed
> fields should have 38400 when ASYNC_SPD_CUST hack is set?

Because we shouldn't go adding new features built around the deprecated
ASYNC_SPD_CUST hack.

User picks 38400, sets that flag and magic happens with some drivers for
a while still while we look the other way.

This is not something that we should need to care about when using the
new interfaces.

> > Note that this currently only works because the ftdi driver uses
> > tty_get_baud_rate() instead of c_ospeed directly which is the
> > recommended (new) way.
> 
> Yes, tty_get_baud_rate() helper function is there for this purpose,
> right?

No.

Johan

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

[Pali Rohár]

On Sunday 24 July 2022 15:08:09 Johan Hovold wrote:
> On Sun, Jul 24, 2022 at 02:59:08PM +0200, Pali Rohár wrote:
> > On Sunday 24 July 2022 14:54:58 Johan Hovold wrote:
> > > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote:
> 
> > > > What is updated is the real baudrate in c_ispeed and c_ospeed
> > > > extensions.
> > > > 
> > > > It is really wrong? I thought that c_cflag should stay unchanged at
> > > > B38400 when ASYNC_SPD_CUST is used.
> > > 
> > > Yeah, cflags stay unchanged, but you shouldn't touch those fields when
> > > using the deprecated ASYNC_SPD_CUST hack.
> > 
> > Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed)
> > should contain current real speed. What is the reason that c_*speed
> > fields should have 38400 when ASYNC_SPD_CUST hack is set?
> 
> Because we shouldn't go adding new features built around the deprecated
> ASYNC_SPD_CUST hack.

But this is not a new feature in the old deprecated hack. It for the
new interface.

> User picks 38400, sets that flag and magic happens with some drivers for
> a while still while we look the other way.
> 
> This is not something that we should need to care about when using the
> new interfaces.

Exactly and with this patch it work like to described. User of new
interface does not have to care about old deprecated stuff and new
interface would always reports correct value.

> > > Note that this currently only works because the ftdi driver uses
> > > tty_get_baud_rate() instead of c_ospeed directly which is the
> > > recommended (new) way.
> > 
> > Yes, tty_get_baud_rate() helper function is there for this purpose,
> > right?
> 
> No.
> 
> Johan

So for what otherwise?

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

[Pali Rohár]

On Sunday 24 July 2022 14:06:38 Johan Hovold wrote:
> On Tue, Jul 12, 2022 at 01:53:02PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > 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 | 36 ++++++++++++++++++++---------------
> >  1 file changed, 21 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> > index 717b97f4e094..5b109714612f 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 int 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;
> 
> Please add a line break after the colons.
> 
> Adding another enum for the invalid value might be preferable.

Enum describe HW values, so it cannot be added to hw list.

> > +	}
> > +}
> > +
> >  static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
> >  {
> >  	unsigned short int divisor;
> > @@ -1286,7 +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;
> > -	u32 div_value = 0;
> > +	int div_value = 0;
> 
> And don't change the type here.

This type change was explicitly asked during v1 review. v1 had u32.

> >  	int div_okay = 1;
> >  	int baud;
> >  
> > @@ -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 == -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;
> 
> Johan

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

[Pali Rohár]

On Sunday 24 July 2022 14:20:58 Johan Hovold wrote:
> On Tue, Jul 12, 2022 at 01:53:03PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > If setting new baud rate fails, all other drivers leave the device at
> > previous baud rate, only ftdi_sio resets to 9600 Baud.
> 
> > Change this behavior to that of other drivers so that /dev/ttyUSB*
> > devices behave in the same way.
> 
> These statements are not true. Several USB serial driver set 9600 baud
> on errors for historical reasons. Yet others clamp. It's inconsistent at
> best.
> 
> > 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>
> > ---
> > Greg pointed out that this make break some people's workflow, that they
> > may depend on this behavior.
> > 
> > This is of course possible, although IMO improbable: it would be weird
> > to depend on the fall back to 9600 Baud on error, instead of expecting
> > that the baud rate didn't change at all (like in other /dev/ttyUSB*
> > drivers).
> > 
> > Nonetheless if someone complains that they workflow got broken, we will
> > need to revert this.
> 
> >  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;
> 
> This looks like it breaks B0 handling. Either way, an unrelated change.

Unreleased to this change.

> >  	switch (priv->chip_type) {
> > @@ -1330,8 +1332,12 @@ static u32 get_ftdi_divisor(struct tty_struct *tty,
> >  		if (div_value == -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;
> > @@ -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;
> 
> And please avoid using the ternary operator which tend to just hurt
> readability.
> 
> Looks like this needs to be refactored in some way.
> 
> > @@ -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);
> 
> Zero has a special meaning (B0).
> 
> >  	}
> >  	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;
> 
> Just use 9600 if you don't have an old termios. The termios rate should
> be valid in that case.

Ok

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

[Pali Rohár]

On Sunday 24 July 2022 14:41:19 Johan Hovold wrote:
> On Tue, Jul 12, 2022 at 01:53:06PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > 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 | 83 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> > 
> > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> > index 39e8c5d06157..838acce53e69 100644
> > --- a/drivers/usb/serial/ftdi_sio.c
> > +++ b/drivers/usb/serial/ftdi_sio.c
> > @@ -1167,6 +1167,23 @@ static int 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;
> > +	}
> > +}
> 
> This one should not be needed as sio only supports this discrete set of
> values in the first place.
> 
> >  static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
> >  {
> >  	unsigned short int divisor;
> > @@ -1189,15 +1206,33 @@ 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)
> 
> I believe "base" was used as a function name suffix in the inverse
> function (due to the additional base argument).

Yes, in the same way as it used in name of ftdi_232am_baud_base_to_divisor.

> > +{
> > +	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);
> > +}
> 
> I don't have the motivation to try to review these inverses right now.
> 
> Let's get the rest of the series in order first.
> 
> Johan

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

[Johan Hovold]

On Thu, Aug 18, 2022 at 04:11:40PM +0200, Pali Rohár wrote:
> On Sunday 24 July 2022 14:06:38 Johan Hovold wrote:
> > On Tue, Jul 12, 2022 at 01:53:02PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > > 
> > > 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 | 36 ++++++++++++++++++++---------------
> > >  1 file changed, 21 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> > > index 717b97f4e094..5b109714612f 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 int 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;
> > 
> > Please add a line break after the colons.
> > 
> > Adding another enum for the invalid value might be preferable.
> 
> Enum describe HW values, so it cannot be added to hw list.

Of course you can, but perhaps you'll figure out some other way to do
this.

> > > +	}
> > > +}
> > > +
> > >  static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
> > >  {
> > >  	unsigned short int divisor;
> > > @@ -1286,7 +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;
> > > -	u32 div_value = 0;
> > > +	int div_value = 0;
> > 
> > And don't change the type here.
> 
> This type change was explicitly asked during v1 review. v1 had u32.

Ok, but don't. This variable is used in other parts of this function.

> > >  	int div_okay = 1;
> > >  	int baud;
> > >  
> > > @@ -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) {

Looks like there's another bug here in the current driver as b300 is
defined as 0.

I'll fix that up separately first.

> > > +		div_value = ftdi_sio_baud_to_divisor(baud);
> > > +		if (div_value == -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;

Johan

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

[Johan Hovold]

On Thu, Aug 18, 2022 at 04:12:54PM +0200, Pali Rohár wrote:
> On Sunday 24 July 2022 14:20:58 Johan Hovold wrote:

> > > @@ -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;
> > 
> > Just use 9600 if you don't have an old termios. The termios rate should
> > be valid in that case.
> 
> Ok

Please trim your replies.

Johan

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

[Johan Hovold]

On Thu, Aug 18, 2022 at 04:09:52PM +0200, Pali Rohár wrote:
> On Sunday 24 July 2022 15:08:09 Johan Hovold wrote:
> > On Sun, Jul 24, 2022 at 02:59:08PM +0200, Pali Rohár wrote:
> > > On Sunday 24 July 2022 14:54:58 Johan Hovold wrote:
> > > > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote:
> > 
> > > > > What is updated is the real baudrate in c_ispeed and c_ospeed
> > > > > extensions.
> > > > > 
> > > > > It is really wrong? I thought that c_cflag should stay unchanged at
> > > > > B38400 when ASYNC_SPD_CUST is used.
> > > > 
> > > > Yeah, cflags stay unchanged, but you shouldn't touch those fields when
> > > > using the deprecated ASYNC_SPD_CUST hack.
> > > 
> > > Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed)
> > > should contain current real speed. What is the reason that c_*speed
> > > fields should have 38400 when ASYNC_SPD_CUST hack is set?
> > 
> > Because we shouldn't go adding new features built around the deprecated
> > ASYNC_SPD_CUST hack.
> 
> But this is not a new feature in the old deprecated hack. It for the
> new interface.

I think I understand what you're getting at, but no. Let's not add more
features built around ASYNC_SPD_CUST.

Johan

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

[Johan Hovold]

On Thu, Aug 18, 2022 at 04:17:38PM +0200, Pali Rohár wrote:
> On Sunday 24 July 2022 14:41:19 Johan Hovold wrote:
> > On Tue, Jul 12, 2022 at 01:53:06PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > > 
> > > 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 | 83 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 83 insertions(+)

> > >  static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
> > >  {
> > >  	unsigned short int divisor;
> > > @@ -1189,15 +1206,33 @@ 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)
> > 
> > I believe "base" was used as a function name suffix in the inverse
> > function (due to the additional base argument).
> 
> Yes, in the same way as it used in name of ftdi_232am_baud_base_to_divisor.

No, you used "base" as an infix here.

Johan

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

[Pali Rohár]

On Tuesday 13 September 2022 17:02:39 Johan Hovold wrote:
> On Thu, Aug 18, 2022 at 04:17:38PM +0200, Pali Rohár wrote:
> > On Sunday 24 July 2022 14:41:19 Johan Hovold wrote:
> > > On Tue, Jul 12, 2022 at 01:53:06PM +0200, Marek Behún wrote:
> > > > From: Pali Rohár <pali@kernel.org>
> > > > 
> > > > 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 | 83 +++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 83 insertions(+)
> 
> > > >  static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
> > > >  {
> > > >  	unsigned short int divisor;
> > > > @@ -1189,15 +1206,33 @@ 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)
> > > 
> > > I believe "base" was used as a function name suffix in the inverse
> > > function (due to the additional base argument).
> > 
> > Yes, in the same way as it used in name of ftdi_232am_baud_base_to_divisor.
> 
> No, you used "base" as an infix here.
> 
> Johan

Yes, of course, it is "in the middle" because I said I used it in the
same way as in ftdi_232am_baud_base_to_divisor.

ftdi_232am_baud_base_to_divisor converts input "baud" and input "base"
to return "divisor". Hence baud_base_to_divisor.

And so ftdi_232am_divisor_base_to_baud converts input "divisor" and
input "base" to return value "baud". Hence divisor_base_to_baud.

So what is the problem? I still have not caught.

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

[Pali Rohár]

On Tuesday 13 September 2022 16:49:01 Johan Hovold wrote:
> > > > @@ -1286,7 +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;
> > > > -	u32 div_value = 0;
> > > > +	int div_value = 0;
> > > 
> > > And don't change the type here.
> > 
> > This type change was explicitly asked during v1 review. v1 had u32.
> 
> Ok, but don't. This variable is used in other parts of this function.

As I said, I changed it just because I was explicitly asked for it
during v1 review. I'm just doing what I was asked.

Maybe... I can change it into union?

  union {
    s32 signed;
    u32 unsigned;
  } div_value;

I really do not see any other option how to achieve requirement that it
should be type of "int" with your another requirement that "do not do it".

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

[Johan Hovold]

On Tue, Sep 13, 2022 at 05:24:05PM +0200, Pali Rohár wrote:
> On Tuesday 13 September 2022 17:02:39 Johan Hovold wrote:
> > On Thu, Aug 18, 2022 at 04:17:38PM +0200, Pali Rohár wrote:
> > > On Sunday 24 July 2022 14:41:19 Johan Hovold wrote:

> > > > >  static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
> > > > >  {
> > > > >  	unsigned short int divisor;
> > > > > @@ -1189,15 +1206,33 @@ 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)
> > > > 
> > > > I believe "base" was used as a function name suffix in the inverse
> > > > function (due to the additional base argument).
> > > 
> > > Yes, in the same way as it used in name of ftdi_232am_baud_base_to_divisor.
> > 
> > No, you used "base" as an infix here.

> Yes, of course, it is "in the middle" because I said I used it in the
> same way as in ftdi_232am_baud_base_to_divisor.
> 
> ftdi_232am_baud_base_to_divisor converts input "baud" and input "base"
> to return "divisor". Hence baud_base_to_divisor.
> 
> And so ftdi_232am_divisor_base_to_baud converts input "divisor" and
> input "base" to return value "baud". Hence divisor_base_to_baud.
> 
> So what is the problem? I still have not caught.

Ok, I should have looked closer at the code before replying.

I believe I expected "base" to be used as a suffix in the current code
but that doesn't appear to be the case. So you're just following the
current style.

Johan

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

[Johan Hovold]

On Tue, Sep 13, 2022 at 05:30:51PM +0200, Pali Rohár wrote:
> On Tuesday 13 September 2022 16:49:01 Johan Hovold wrote:
> > > > > @@ -1286,7 +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;
> > > > > -	u32 div_value = 0;
> > > > > +	int div_value = 0;
> > > > 
> > > > And don't change the type here.
> > > 
> > > This type change was explicitly asked during v1 review. v1 had u32.
> > 
> > Ok, but don't. This variable is used in other parts of this function.
> 
> As I said, I changed it just because I was explicitly asked for it
> during v1 review. I'm just doing what I was asked.
> 
> Maybe... I can change it into union?
> 
>   union {
>     s32 signed;
>     u32 unsigned;
>   } div_value;
> 
> I really do not see any other option how to achieve requirement that it
> should be type of "int" with your another requirement that "do not do it".

I'm pretty sure I didn't review v1.

Perhaps you're referring to this:

	https://lore.kernel.org/all/Ysb3ORyUAPEOntqK@kroah.com/

But that doesn't mean you should change the type of div_value.

Johan

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

[Pali Rohár]

On Tuesday 13 September 2022 16:59:22 Johan Hovold wrote:
> On Thu, Aug 18, 2022 at 04:09:52PM +0200, Pali Rohár wrote:
> > On Sunday 24 July 2022 15:08:09 Johan Hovold wrote:
> > > On Sun, Jul 24, 2022 at 02:59:08PM +0200, Pali Rohár wrote:
> > > > On Sunday 24 July 2022 14:54:58 Johan Hovold wrote:
> > > > > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote:
> > > 
> > > > > > What is updated is the real baudrate in c_ispeed and c_ospeed
> > > > > > extensions.
> > > > > > 
> > > > > > It is really wrong? I thought that c_cflag should stay unchanged at
> > > > > > B38400 when ASYNC_SPD_CUST is used.
> > > > > 
> > > > > Yeah, cflags stay unchanged, but you shouldn't touch those fields when
> > > > > using the deprecated ASYNC_SPD_CUST hack.
> > > > 
> > > > Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed)
> > > > should contain current real speed. What is the reason that c_*speed
> > > > fields should have 38400 when ASYNC_SPD_CUST hack is set?
> > > 
> > > Because we shouldn't go adding new features built around the deprecated
> > > ASYNC_SPD_CUST hack.
> > 
> > But this is not a new feature in the old deprecated hack. It for the
> > new interface.
> 
> I think I understand what you're getting at, but no. Let's not add more
> features built around ASYNC_SPD_CUST.
> 
> Johan

Seems that you did not understand the point. So I will try to explain it
again. This is not a new feature for _old_ ASYNC_SPD_CUST. This is the
fix for the _new_ TCGETS2 API, to ensure that driver will always returns
corrects values in c_*speed fields. If driver is not going to fix this
_new_ TCGETS2 API then there is _NO_ point to use this new API in
userspace and it is better to stick with the old ASYNC_SPD_CUST. And
this is the current userspace state. So based on your input, it is the
time to deprecate TCGETS2?

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

[Johan Hovold]

On Wed, Sep 14, 2022 at 10:48:31AM +0200, Pali Rohár wrote:
> On Tuesday 13 September 2022 16:59:22 Johan Hovold wrote:
> > On Thu, Aug 18, 2022 at 04:09:52PM +0200, Pali Rohár wrote:
> > > On Sunday 24 July 2022 15:08:09 Johan Hovold wrote:
> > > > On Sun, Jul 24, 2022 at 02:59:08PM +0200, Pali Rohár wrote:
> > > > > On Sunday 24 July 2022 14:54:58 Johan Hovold wrote:
> > > > > > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote:
> > > > 
> > > > > > > What is updated is the real baudrate in c_ispeed and c_ospeed
> > > > > > > extensions.
> > > > > > > 
> > > > > > > It is really wrong? I thought that c_cflag should stay unchanged at
> > > > > > > B38400 when ASYNC_SPD_CUST is used.
> > > > > > 
> > > > > > Yeah, cflags stay unchanged, but you shouldn't touch those fields when
> > > > > > using the deprecated ASYNC_SPD_CUST hack.
> > > > > 
> > > > > Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed)
> > > > > should contain current real speed. What is the reason that c_*speed
> > > > > fields should have 38400 when ASYNC_SPD_CUST hack is set?
> > > > 
> > > > Because we shouldn't go adding new features built around the deprecated
> > > > ASYNC_SPD_CUST hack.
> > > 
> > > But this is not a new feature in the old deprecated hack. It for the
> > > new interface.
> > 
> > I think I understand what you're getting at, but no. Let's not add more
> > features built around ASYNC_SPD_CUST.

> Seems that you did not understand the point. So I will try to explain it
> again. This is not a new feature for _old_ ASYNC_SPD_CUST. This is the
> fix for the _new_ TCGETS2 API, to ensure that driver will always returns
> corrects values in c_*speed fields. If driver is not going to fix this
> _new_ TCGETS2 API then there is _NO_ point to use this new API in
> userspace and it is better to stick with the old ASYNC_SPD_CUST. And
> this is the current userspace state. So based on your input, it is the
> time to deprecate TCGETS2?

Stop being silly. As I've said repeatedly, we don't care about
ASYNC_SPD_CUST. Just return 38400 regardless of whatever magic happens
behind the scenes with the TIOCSSERIAL ioctl.

Johan

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

[Pali Rohár]

On Wednesday 14 September 2022 10:58:48 Johan Hovold wrote:
> On Wed, Sep 14, 2022 at 10:48:31AM +0200, Pali Rohár wrote:
> > On Tuesday 13 September 2022 16:59:22 Johan Hovold wrote:
> > > On Thu, Aug 18, 2022 at 04:09:52PM +0200, Pali Rohár wrote:
> > > > On Sunday 24 July 2022 15:08:09 Johan Hovold wrote:
> > > > > On Sun, Jul 24, 2022 at 02:59:08PM +0200, Pali Rohár wrote:
> > > > > > On Sunday 24 July 2022 14:54:58 Johan Hovold wrote:
> > > > > > > On Sun, Jul 24, 2022 at 02:33:51PM +0200, Pali Rohár wrote:
> > > > > 
> > > > > > > > What is updated is the real baudrate in c_ispeed and c_ospeed
> > > > > > > > extensions.
> > > > > > > > 
> > > > > > > > It is really wrong? I thought that c_cflag should stay unchanged at
> > > > > > > > B38400 when ASYNC_SPD_CUST is used.
> > > > > > > 
> > > > > > > Yeah, cflags stay unchanged, but you shouldn't touch those fields when
> > > > > > > using the deprecated ASYNC_SPD_CUST hack.
> > > > > > 
> > > > > > Hm... Why? I thought that new extended fields (c_ispeed and c_ospeed)
> > > > > > should contain current real speed. What is the reason that c_*speed
> > > > > > fields should have 38400 when ASYNC_SPD_CUST hack is set?
> > > > > 
> > > > > Because we shouldn't go adding new features built around the deprecated
> > > > > ASYNC_SPD_CUST hack.
> > > > 
> > > > But this is not a new feature in the old deprecated hack. It for the
> > > > new interface.
> > > 
> > > I think I understand what you're getting at, but no. Let's not add more
> > > features built around ASYNC_SPD_CUST.
> 
> > Seems that you did not understand the point. So I will try to explain it
> > again. This is not a new feature for _old_ ASYNC_SPD_CUST. This is the
> > fix for the _new_ TCGETS2 API, to ensure that driver will always returns
> > corrects values in c_*speed fields. If driver is not going to fix this
> > _new_ TCGETS2 API then there is _NO_ point to use this new API in
> > userspace and it is better to stick with the old ASYNC_SPD_CUST. And
> > this is the current userspace state. So based on your input, it is the
> > time to deprecate TCGETS2?
> 
> Stop being silly. As I've said repeatedly, we don't care about
> ASYNC_SPD_CUST. Just return 38400 regardless of whatever magic happens
> behind the scenes with the TIOCSSERIAL ioctl.
> 
> Johan

I'm not silly here. Look, those APIs are for userspace. And if userspace
application cannot use this new TCGETS2 API (for more reasons) then they
stick with the old one TIOCSSERIAL. And your inputs just say that it is
not a good idea to switch TCGETS2 as this API stay broken in some
drivers. Silly is the one who do not see (or do not want to see it;
because of own API perfectionism) the reasons why new "proposed API" is
still not (widely) used and applications stick with TCGETS + TIOCSSERIAL.

That is why I'm asking, it is time to starting deprecating TCGETS2 and
create for example TCGETS3? Only just few application use TCGETS2, so
deprecation of TCGETS2 can be done _now_ without pain as this API is not
widely used.

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

[Johan Hovold]

On Wed, Sep 14, 2022 at 11:10:06AM +0200, Pali Rohár wrote:
> On Wednesday 14 September 2022 10:58:48 Johan Hovold wrote:
> > On Wed, Sep 14, 2022 at 10:48:31AM +0200, Pali Rohár wrote:

> > > Seems that you did not understand the point. So I will try to explain it
> > > again. This is not a new feature for _old_ ASYNC_SPD_CUST. This is the
> > > fix for the _new_ TCGETS2 API, to ensure that driver will always returns
> > > corrects values in c_*speed fields. If driver is not going to fix this
> > > _new_ TCGETS2 API then there is _NO_ point to use this new API in
> > > userspace and it is better to stick with the old ASYNC_SPD_CUST. And
> > > this is the current userspace state. So based on your input, it is the
> > > time to deprecate TCGETS2?
> > 
> > Stop being silly. As I've said repeatedly, we don't care about
> > ASYNC_SPD_CUST. Just return 38400 regardless of whatever magic happens
> > behind the scenes with the TIOCSSERIAL ioctl.

> I'm not silly here. Look, those APIs are for userspace. And if userspace
> application cannot use this new TCGETS2 API (for more reasons) then they
> stick with the old one TIOCSSERIAL. And your inputs just say that it is
> not a good idea to switch TCGETS2 as this API stay broken in some
> drivers. Silly is the one who do not see (or do not want to see it;
> because of own API perfectionism) the reasons why new "proposed API" is
> still not (widely) used and applications stick with TCGETS + TIOCSSERIAL.
> 
> That is why I'm asking, it is time to starting deprecating TCGETS2 and
> create for example TCGETS3? Only just few application use TCGETS2, so
> deprecation of TCGETS2 can be done _now_ without pain as this API is not
> widely used.

You're trying to keep the ASYNC_SPD hack alive by forcing drivers to
take it into consideration for TCGETS2. Just stop using the former and
switch to using BOTHER. And if something is missing in user space for
that, then fix that.

Johan

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

[Pali Rohár]

On Wednesday 14 September 2022 11:18:03 Johan Hovold wrote:
> On Wed, Sep 14, 2022 at 11:10:06AM +0200, Pali Rohár wrote:
> > On Wednesday 14 September 2022 10:58:48 Johan Hovold wrote:
> > > On Wed, Sep 14, 2022 at 10:48:31AM +0200, Pali Rohár wrote:
> 
> > > > Seems that you did not understand the point. So I will try to explain it
> > > > again. This is not a new feature for _old_ ASYNC_SPD_CUST. This is the
> > > > fix for the _new_ TCGETS2 API, to ensure that driver will always returns
> > > > corrects values in c_*speed fields. If driver is not going to fix this
> > > > _new_ TCGETS2 API then there is _NO_ point to use this new API in
> > > > userspace and it is better to stick with the old ASYNC_SPD_CUST. And
> > > > this is the current userspace state. So based on your input, it is the
> > > > time to deprecate TCGETS2?
> > > 
> > > Stop being silly. As I've said repeatedly, we don't care about
> > > ASYNC_SPD_CUST. Just return 38400 regardless of whatever magic happens
> > > behind the scenes with the TIOCSSERIAL ioctl.
> 
> > I'm not silly here. Look, those APIs are for userspace. And if userspace
> > application cannot use this new TCGETS2 API (for more reasons) then they
> > stick with the old one TIOCSSERIAL. And your inputs just say that it is
> > not a good idea to switch TCGETS2 as this API stay broken in some
> > drivers. Silly is the one who do not see (or do not want to see it;
> > because of own API perfectionism) the reasons why new "proposed API" is
> > still not (widely) used and applications stick with TCGETS + TIOCSSERIAL.
> > 
> > That is why I'm asking, it is time to starting deprecating TCGETS2 and
> > create for example TCGETS3? Only just few application use TCGETS2, so
> > deprecation of TCGETS2 can be done _now_ without pain as this API is not
> > widely used.
> 
> You're trying to keep the ASYNC_SPD hack alive by forcing drivers to
> take it into consideration for TCGETS2. Just stop using the former and
> switch to using BOTHER. And if something is missing in user space for
> that, then fix that.
> 
> Johan

And what is the point of switching to BOTHER/TCGETS2 if some drivers
do not return _correct_ values?