2022-03-21 21:07:09

by Pali Rohár

[permalink] [raw]
Subject: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

Support for UPF_SPD_* flags is currently broken in more drivers for two
reasons. First one is that uart_update_timeout() function does not
calculate timeout for UPF_SPD_CUST flag correctly. Second reason is that
userspace termios structre is modified by most drivers after each
TIOCSSERIAL or TCSETS call to discard activation of UPF_SPD_* flags.

Reproducer for these issues:

#include <termios.h>
#include <sys/ioctl.h>
#include <asm/ioctls.h>
#include <linux/serial.h>

static unsigned cdiv(unsigned a, unsigned b) {
if (!b) return b;
return (a + (b/2)) / b;
}

void set_active_spd_cust_baud(int fd, unsigned baudrate) {
struct serial_struct serial;
struct termios termios;
tcgetattr(fd, &termios);
ioctl(fd, TIOCGSERIAL, &serial);
serial.flags &= ~ASYNC_SPD_MASK;
serial.flags |= ASYNC_SPD_CUST;
serial.custom_divisor = cdiv(serial.baud_base, baudrate);
ioctl(fd, TIOCSSERIAL, &serial);
cfsetspeed(&termios, B38400);
tcsetattr(fd, TCSANOW, &termios);
}

int is_spd_cust_active(int fd) {
struct serial_struct serial;
struct termios termios;
tcgetattr(fd, &termios);
ioctl(fd, TIOCGSERIAL, &serial);
return
(serial.flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST &&
cfgetospeed(&termios) == B38400;
}

(error handling was ommited for simplification)

After calling set_active_spd_cust_baud() function SPD custom divisor
should be active and therefore is_spd_cust_active() should return true.

But it is not active (cfgetospeed does not return B38400) and this patch
series should fix it. I have tested it with 8250 driver.


Originally Johan Hovold reported that there may be issue with these
ASYNC_SPD_FLAGS in email:
https://lore.kernel.org/linux-serial/[email protected]/


Johan, Greg, could you please test these patches if there is not any
regression?


Pali Rohár (3):
serial: core: Document why UPF_SPD_CUST is not handled in
uart_get_baud_rate()
serial: core: Fix function uart_update_timeout() to handle
UPF_SPD_CUST flag
serial: Fix support for UPF_SPD_* flags in serial drivers

drivers/tty/serial/21285.c | 2 +-
drivers/tty/serial/8250/8250_mtk.c | 2 +-
drivers/tty/serial/8250/8250_omap.c | 2 +-
drivers/tty/serial/8250/8250_port.c | 2 +-
drivers/tty/serial/altera_uart.c | 2 +-
drivers/tty/serial/ar933x_uart.c | 2 +-
drivers/tty/serial/arc_uart.c | 2 +-
drivers/tty/serial/dz.c | 2 +-
drivers/tty/serial/imx.c | 3 +-
drivers/tty/serial/lantiq.c | 2 +-
drivers/tty/serial/lpc32xx_hs.c | 2 +-
drivers/tty/serial/men_z135_uart.c | 2 +-
drivers/tty/serial/mps2-uart.c | 2 +-
drivers/tty/serial/msm_serial.c | 2 +-
drivers/tty/serial/mvebu-uart.c | 2 +-
drivers/tty/serial/owl-uart.c | 2 +-
drivers/tty/serial/pch_uart.c | 2 +-
drivers/tty/serial/pic32_uart.c | 2 +-
drivers/tty/serial/rda-uart.c | 2 +-
drivers/tty/serial/rp2.c | 2 +-
drivers/tty/serial/sccnxp.c | 2 +-
drivers/tty/serial/serial-tegra.c | 2 +-
drivers/tty/serial/serial_core.c | 76 ++++++++++++++++++++++++++++-
drivers/tty/serial/sprd_serial.c | 2 +-
drivers/tty/serial/timbuart.c | 2 +-
drivers/tty/serial/vt8500_serial.c | 2 +-
drivers/tty/serial/xilinx_uartps.c | 2 +-
include/linux/serial_core.h | 2 +
28 files changed, 103 insertions(+), 28 deletions(-)

--
2.20.1


2022-03-21 21:20:36

by Pali Rohár

[permalink] [raw]
Subject: [PATCH 2/3] serial: core: Fix function uart_update_timeout() to handle UPF_SPD_CUST flag

Function uart_update_timeout() currently handles UPF_SPD_HI, UPF_SPD_VHI,
UPF_SPD_SHI and UPF_SPD_WARP flags thanks to how uart_get_baud_rate()
works and how should be used. uart_get_baud_rate() already translates these
4 special flags to real baud rate value and therefore uart_update_timeout()
is called with the correct baud rate argument.

What is not handled in this function is the last remaining flag
UPF_SPD_CUST. It is because uart_get_baud_rate() returns hardcoded value
38400 when UPF_SPD_CUST is set and in use.

Implement support for UPF_SPD_CUST in uart_update_timeout(), so
port->timeout is calculated also when UPF_SPD_CUST flag is set.

For calculation use base uart clock and custom divisor. This calculation
does not have to be precise for all UART hardware but this the best
approximation which can be done. It is for sure better than hardcoded baud
rate value 38400.

Signed-off-by: Pali Rohár <[email protected]>
---
drivers/tty/serial/serial_core.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index d8fc2616d62b..34e085a038fe 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -343,6 +343,16 @@ uart_update_timeout(struct uart_port *port, unsigned int cflag,
{
unsigned int size;

+ /*
+ * Old custom speed handling. See function uart_get_divisor()
+ * and description in uart_get_baud_rate() function.
+ * This calculation does not have to be precise for all UART
+ * hardware but it is the best approximation which we can do here
+ * as we do not know the exact baud rate.
+ */
+ if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST)
+ baud = DIV_ROUND_CLOSEST(port->uartclk, 16 * port->custom_divisor);
+
size = tty_get_frame_size(cflag) * port->fifosize;

/*
--
2.20.1

2022-03-21 22:39:42

by Pali Rohár

[permalink] [raw]
Subject: [PATCH 3/3] serial: Fix support for UPF_SPD_* flags in serial drivers

Most serial drivers do not handle UPF_SPD_* flags correctly. They use
uart_get_baud_rate() and uart_get_divisor() functions for retrieving baud
rate and divisor which correctly handle UPF_SPD_* flags and set correct
value to HW. But drivers do not propagate correct value to termios
structure as they call just tty_termios_encode_baud_rate() function which
completely ignores these UPF_SPD_* flags. So termios structure reported to
userspace does not match to UPF_SPD_* flags which were used for configuring
HW and also does not match ASYNC_SDP_* flags stored in serial_struct which
are reported to userspace.

Fix this issue by introducing a new function uart_set_baud_rate() which is
wrapper around tty_termios_encode_baud_rate() and which handles those
UPF_SPD_* flags correctly.

Replace all calls of tty_termios_encode_baud_rate() function which take an
argument from uart_get_baud_rate() function by this new function
uart_set_baud_rate().

This ensures that serial drivers which are using uart_get_baud_rate() will
correctly handle UPF_SPD_* flags.

Signed-off-by: Pali Rohár <[email protected]>
---
drivers/tty/serial/21285.c | 2 +-
drivers/tty/serial/8250/8250_mtk.c | 2 +-
drivers/tty/serial/8250/8250_omap.c | 2 +-
drivers/tty/serial/8250/8250_port.c | 2 +-
drivers/tty/serial/altera_uart.c | 2 +-
drivers/tty/serial/ar933x_uart.c | 2 +-
drivers/tty/serial/arc_uart.c | 2 +-
drivers/tty/serial/dz.c | 2 +-
drivers/tty/serial/imx.c | 3 +-
drivers/tty/serial/lantiq.c | 2 +-
drivers/tty/serial/lpc32xx_hs.c | 2 +-
drivers/tty/serial/men_z135_uart.c | 2 +-
drivers/tty/serial/mps2-uart.c | 2 +-
drivers/tty/serial/msm_serial.c | 2 +-
drivers/tty/serial/mvebu-uart.c | 2 +-
drivers/tty/serial/owl-uart.c | 2 +-
drivers/tty/serial/pch_uart.c | 2 +-
drivers/tty/serial/pic32_uart.c | 2 +-
drivers/tty/serial/rda-uart.c | 2 +-
drivers/tty/serial/rp2.c | 2 +-
drivers/tty/serial/sccnxp.c | 2 +-
drivers/tty/serial/serial-tegra.c | 2 +-
drivers/tty/serial/serial_core.c | 51 +++++++++++++++++++++++++++++
drivers/tty/serial/sprd_serial.c | 2 +-
drivers/tty/serial/timbuart.c | 2 +-
drivers/tty/serial/vt8500_serial.c | 2 +-
drivers/tty/serial/xilinx_uartps.c | 2 +-
include/linux/serial_core.h | 2 ++
28 files changed, 79 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/serial/21285.c b/drivers/tty/serial/21285.c
index 09baef4ccc39..4e4dbd58c581 100644
--- a/drivers/tty/serial/21285.c
+++ b/drivers/tty/serial/21285.c
@@ -265,7 +265,7 @@ serial21285_set_termios(struct uart_port *port, struct ktermios *termios,
baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/16);
quot = uart_get_divisor(port, baud);
b = port->uartclk / (16 * quot);
- tty_termios_encode_baud_rate(termios, b, b);
+ uart_set_baud_rate(port, termios, b);

switch (termios->c_cflag & CSIZE) {
case CS5:
diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
index fb65dc601b23..4a46a7f039d0 100644
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -406,7 +406,7 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
spin_unlock_irqrestore(&port->lock, flags);
/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);
}

static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 73e5f1dbd075..96c1df9ffd25 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -510,7 +510,7 @@ static void omap_8250_set_termios(struct uart_port *port,

/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);
}

/* same as 8250 except that we may have extra flow bits set in EFR */
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 5775cbff8f6e..05ca0152b932 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2878,7 +2878,7 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,

/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);
}
EXPORT_SYMBOL(serial8250_do_set_termios);

diff --git a/drivers/tty/serial/altera_uart.c b/drivers/tty/serial/altera_uart.c
index 7c5f4e966b59..ec352d53662a 100644
--- a/drivers/tty/serial/altera_uart.c
+++ b/drivers/tty/serial/altera_uart.c
@@ -185,7 +185,7 @@ static void altera_uart_set_termios(struct uart_port *port,

if (old)
tty_termios_copy_hw(termios, old);
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);

spin_lock_irqsave(&port->lock, flags);
uart_update_timeout(port, termios->c_cflag, baud);
diff --git a/drivers/tty/serial/ar933x_uart.c b/drivers/tty/serial/ar933x_uart.c
index 4379ca4842ae..48e8127d0e9d 100644
--- a/drivers/tty/serial/ar933x_uart.c
+++ b/drivers/tty/serial/ar933x_uart.c
@@ -355,7 +355,7 @@ static void ar933x_uart_set_termios(struct uart_port *port,
spin_unlock_irqrestore(&up->port.lock, flags);

if (tty_termios_baud_rate(new))
- tty_termios_encode_baud_rate(new, baud, baud);
+ uart_set_baud_rate(port, new, baud);
}

static void ar933x_uart_rx_chars(struct ar933x_uart_port *up)
diff --git a/drivers/tty/serial/arc_uart.c b/drivers/tty/serial/arc_uart.c
index 596217d10d5c..cc2a06832699 100644
--- a/drivers/tty/serial/arc_uart.c
+++ b/drivers/tty/serial/arc_uart.c
@@ -391,7 +391,7 @@ arc_serial_set_termios(struct uart_port *port, struct ktermios *new,

/* Don't rewrite B0 */
if (tty_termios_baud_rate(new))
- tty_termios_encode_baud_rate(new, baud, baud);
+ uart_set_baud_rate(port, new, baud);

uart_update_timeout(port, new->c_cflag, baud);

diff --git a/drivers/tty/serial/dz.c b/drivers/tty/serial/dz.c
index e9edabc5a211..8084966ba9f9 100644
--- a/drivers/tty/serial/dz.c
+++ b/drivers/tty/serial/dz.c
@@ -599,7 +599,7 @@ static void dz_set_termios(struct uart_port *uport, struct ktermios *termios,
baud = 9600;
bflag = DZ_B9600;
}
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(uport, termios, baud);
}
cflag |= bflag;

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 90f82e6c54e4..b56344b46459 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1729,8 +1729,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
tdiv64 = sport->port.uartclk;
tdiv64 *= num;
do_div(tdiv64, denom * 16 * div);
- tty_termios_encode_baud_rate(termios,
- (speed_t)tdiv64, (speed_t)tdiv64);
+ uart_set_baud_rate(port, termios, (speed_t)tdiv64);

num -= 1;
denom -= 1;
diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index 497b334bc845..38b2f7fa80ff 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -502,7 +502,7 @@ lqasc_set_termios(struct uart_port *port,

/* Don't rewrite B0 */
if (tty_termios_baud_rate(new))
- tty_termios_encode_baud_rate(new, baud, baud);
+ uart_set_baud_rate(port, new, baud);

uart_update_timeout(port, cflag, baud);
}
diff --git a/drivers/tty/serial/lpc32xx_hs.c b/drivers/tty/serial/lpc32xx_hs.c
index b199d7859961..e6f9166be0c7 100644
--- a/drivers/tty/serial/lpc32xx_hs.c
+++ b/drivers/tty/serial/lpc32xx_hs.c
@@ -530,7 +530,7 @@ static void serial_lpc32xx_set_termios(struct uart_port *port,

/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);
}

static const char *serial_lpc32xx_type(struct uart_port *port)
diff --git a/drivers/tty/serial/men_z135_uart.c b/drivers/tty/serial/men_z135_uart.c
index 9acae5f8fc32..e3a83bca9991 100644
--- a/drivers/tty/serial/men_z135_uart.c
+++ b/drivers/tty/serial/men_z135_uart.c
@@ -713,7 +713,7 @@ static void men_z135_set_termios(struct uart_port *port,

spin_lock_irq(&port->lock);
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);

bd_reg = uart_freq / (4 * baud);
iowrite32(bd_reg, port->membase + MEN_Z135_BAUD_REG);
diff --git a/drivers/tty/serial/mps2-uart.c b/drivers/tty/serial/mps2-uart.c
index 587b42f754cb..117a7afcd088 100644
--- a/drivers/tty/serial/mps2-uart.c
+++ b/drivers/tty/serial/mps2-uart.c
@@ -383,7 +383,7 @@ mps2_uart_set_termios(struct uart_port *port, struct ktermios *termios,
spin_unlock_irqrestore(&port->lock, flags);

if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);
}

static const char *mps2_uart_type(struct uart_port *port)
diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index fcef7a961430..789dfeb286bf 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -1262,7 +1262,7 @@ static void msm_set_termios(struct uart_port *port, struct ktermios *termios,
baud = uart_get_baud_rate(port, termios, old, 300, 4000000);
baud = msm_set_baud_rate(port, baud, &flags);
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);

/* calculate parity */
mr = msm_read(port, UART_MR2);
diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
index ab226da75f7b..7bd9579855fa 100644
--- a/drivers/tty/serial/mvebu-uart.c
+++ b/drivers/tty/serial/mvebu-uart.c
@@ -515,7 +515,7 @@ static void mvebu_uart_set_termios(struct uart_port *port,
baud = uart_get_baud_rate(port, old, NULL,
min_baud, max_baud);
} else {
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);
uart_update_timeout(port, termios->c_cflag, baud);
}

diff --git a/drivers/tty/serial/owl-uart.c b/drivers/tty/serial/owl-uart.c
index 91f1eb0058d7..8d70cdbd4a73 100644
--- a/drivers/tty/serial/owl-uart.c
+++ b/drivers/tty/serial/owl-uart.c
@@ -387,7 +387,7 @@ static void owl_uart_set_termios(struct uart_port *port,

/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);

port->read_status_mask |= OWL_UART_STAT_RXER;
if (termios->c_iflag & INPCK)
diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index f0351e6f0ef6..53f2e9c09d8d 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -1412,7 +1412,7 @@ static void pch_uart_set_termios(struct uart_port *port,
pch_uart_set_mctrl(&priv->port, priv->port.mctrl);
/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);

out:
spin_unlock(&port->lock);
diff --git a/drivers/tty/serial/pic32_uart.c b/drivers/tty/serial/pic32_uart.c
index 0a12fb11e698..286045456a1c 100644
--- a/drivers/tty/serial/pic32_uart.c
+++ b/drivers/tty/serial/pic32_uart.c
@@ -596,7 +596,7 @@ static void pic32_uart_set_termios(struct uart_port *port,
uart_update_timeout(port, new->c_cflag, baud);

if (tty_termios_baud_rate(new))
- tty_termios_encode_baud_rate(new, baud, baud);
+ uart_set_baud_rate(port, termios, baud);

/* enable uart */
pic32_uart_en_and_unmask(port);
diff --git a/drivers/tty/serial/rda-uart.c b/drivers/tty/serial/rda-uart.c
index d550d8fa2fab..c08c597cda86 100644
--- a/drivers/tty/serial/rda-uart.c
+++ b/drivers/tty/serial/rda-uart.c
@@ -318,7 +318,7 @@ static void rda_uart_set_termios(struct uart_port *port,

/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);

/* update the per-port timeout */
uart_update_timeout(port, termios->c_cflag, baud);
diff --git a/drivers/tty/serial/rp2.c b/drivers/tty/serial/rp2.c
index 6689d8add8f7..a12e9a97905b 100644
--- a/drivers/tty/serial/rp2.c
+++ b/drivers/tty/serial/rp2.c
@@ -382,7 +382,7 @@ static void rp2_uart_set_termios(struct uart_port *port,
baud_div = uart_get_divisor(port, baud);

if (tty_termios_baud_rate(new))
- tty_termios_encode_baud_rate(new, baud, baud);
+ uart_set_baud_rate(port, new, baud);

spin_lock_irqsave(&port->lock, flags);

diff --git a/drivers/tty/serial/sccnxp.c b/drivers/tty/serial/sccnxp.c
index 10cc16a71f26..141dc0ebb688 100644
--- a/drivers/tty/serial/sccnxp.c
+++ b/drivers/tty/serial/sccnxp.c
@@ -714,7 +714,7 @@ static void sccnxp_set_termios(struct uart_port *port,

/* Report actual baudrate back to core */
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);

/* Enable RX & TX */
sccnxp_port_write(port, SCCNXP_CR_REG, CR_RX_ENABLE | CR_TX_ENABLE);
diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 45e2e4109acd..876aa57dbcd0 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -1360,7 +1360,7 @@ static void tegra_uart_set_termios(struct uart_port *u,
return;
}
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(u, termios, baud);
spin_lock_irqsave(&u->lock, flags);

/* Flow control */
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 34e085a038fe..113db02be87e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -477,6 +477,57 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,

EXPORT_SYMBOL(uart_get_baud_rate);

+void
+uart_set_baud_rate(struct uart_port *port, struct ktermios *termios, unsigned int baud)
+{
+ upf_t flags = port->flags & UPF_SPD_MASK;
+ unsigned int close = baud / 50;
+ unsigned int altbaud = 0;
+
+ switch (flags) {
+ case UPF_SPD_HI:
+ altbaud = 57600;
+ break;
+ case UPF_SPD_VHI:
+ altbaud = 115200;
+ break;
+ case UPF_SPD_SHI:
+ altbaud = 230400;
+ break;
+ case UPF_SPD_WARP:
+ altbaud = 460800;
+ break;
+ }
+
+ /*
+ * UPF_SPD_* port flags are in use when B38400 is set in termios.
+ * Let termios baudrate set to B38400 value when new baudrate is
+ * in 2% tolerance (same tolerance as in tty_termios_encode_baud_rate).
+ * For UPF_SPD_CUST flag it is required to be this function called with
+ * baud = 38400 and then real baudrate depends only on custom_divisor.
+ */
+ if (tty_termios_baud_rate(termios) == 38400) {
+ if (altbaud && baud - close >= altbaud && baud + close <= altbaud) {
+ altbaud = baud;
+ baud = 38400;
+ } else if (flags == UPF_SPD_CUST && baud == 38400) {
+ /* See uart_update_timeout() about this calculation */
+ altbaud = DIV_ROUND_CLOSEST(port->uartclk, 16 * port->custom_divisor);
+ }
+ }
+
+ tty_termios_encode_baud_rate(termios, baud, baud);
+
+ /*
+ * If UPF_SPD_* port flags are active and in use then store into
+ * TCGETS2 c_*speed fields real baudrate.
+ */
+ if (baud == 38400 && altbaud)
+ termios->c_ispeed = termios->c_ospeed = altbaud;
+}
+
+EXPORT_SYMBOL(uart_set_baud_rate);
+
/**
* uart_get_divisor - return uart clock divisor
* @port: uart_port structure describing the port.
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 9a7ae6384edf..17b0cadd31e6 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -867,7 +867,7 @@ static void sprd_set_termios(struct uart_port *port,

/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);
}

static const char *sprd_type(struct uart_port *port)
diff --git a/drivers/tty/serial/timbuart.c b/drivers/tty/serial/timbuart.c
index 08941eabe7b1..c41377d218e7 100644
--- a/drivers/tty/serial/timbuart.c
+++ b/drivers/tty/serial/timbuart.c
@@ -294,7 +294,7 @@ static void timbuart_set_termios(struct uart_port *port,
up initially */
if (old)
tty_termios_copy_hw(termios, old);
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);

spin_lock_irqsave(&port->lock, flags);
iowrite8((u8)bindex, port->membase + TIMBUART_BAUDRATE);
diff --git a/drivers/tty/serial/vt8500_serial.c b/drivers/tty/serial/vt8500_serial.c
index e15b2bf69904..2e5d9b52e478 100644
--- a/drivers/tty/serial/vt8500_serial.c
+++ b/drivers/tty/serial/vt8500_serial.c
@@ -369,7 +369,7 @@ static void vt8500_set_termios(struct uart_port *port,
baud = uart_get_baud_rate(port, termios, old, 900, 921600);
baud = vt8500_set_baud_rate(port, baud);
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);

/* calculate parity */
lcr = vt8500_read(&vt8500_port->uart, VT8500_URLCR);
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index d5e243908d9f..300108c9ba7e 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -714,7 +714,7 @@ static void cdns_uart_set_termios(struct uart_port *port,
baud = uart_get_baud_rate(port, termios, old, minbaud, maxbaud);
baud = cdns_uart_set_baud_rate(port, baud);
if (tty_termios_baud_rate(termios))
- tty_termios_encode_baud_rate(termios, baud, baud);
+ uart_set_baud_rate(port, termios, baud);

/* Update the per-port timeout. */
uart_update_timeout(port, termios->c_cflag, baud);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index c58cc142d23f..a0f736be5645 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -330,6 +330,8 @@ void uart_update_timeout(struct uart_port *port, unsigned int cflag,
unsigned int uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
struct ktermios *old, unsigned int min,
unsigned int max);
+void uart_set_baud_rate(struct uart_port *port, struct ktermios *termios,
+ unsigned int baud);
unsigned int uart_get_divisor(struct uart_port *port, unsigned int baud);

/* Base timer interval for polling */
--
2.20.1

2022-03-22 14:47:59

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Mon, Mar 21, 2022 at 11:07 PM Pali Rohár <[email protected]> wrote:
>
> Support for UPF_SPD_* flags is currently broken in more drivers for two
> reasons. First one is that uart_update_timeout() function does not

the uart_update_timeout()

> calculate timeout for UPF_SPD_CUST flag correctly. Second reason is that
> userspace termios structre is modified by most drivers after each

structure

...

> (error handling was ommited for simplification)

omitted

> After calling set_active_spd_cust_baud() function SPD custom divisor
> should be active and therefore is_spd_cust_active() should return true.
>
> But it is not active (cfgetospeed does not return B38400) and this patch
> series should fix it. I have tested it with 8250 driver.

drivers

> Originally Johan Hovold reported that there may be issue with these
> ASYNC_SPD_FLAGS in email:
> https://lore.kernel.org/linux-serial/[email protected]/
>
>
> Johan, Greg, could you please test these patches if there is not any
> regression?

I'm wondering why we are still supporting this ugly hack?
Doesn't BOTHER work for you?

I would rather expect to have this removed completely.

--
With Best Regards,
Andy Shevchenko

2022-03-22 21:16:03

by Pali Rohár

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Tuesday 22 March 2022 16:29:08 Andy Shevchenko wrote:
> On Mon, Mar 21, 2022 at 11:07 PM Pali Rohár <[email protected]> wrote:
> >
> > Support for UPF_SPD_* flags is currently broken in more drivers for two
> > reasons. First one is that uart_update_timeout() function does not
>
> the uart_update_timeout()
>
> > calculate timeout for UPF_SPD_CUST flag correctly. Second reason is that
> > userspace termios structre is modified by most drivers after each
>
> structure
>
> ...
>
> > (error handling was ommited for simplification)
>
> omitted
>
> > After calling set_active_spd_cust_baud() function SPD custom divisor
> > should be active and therefore is_spd_cust_active() should return true.
> >
> > But it is not active (cfgetospeed does not return B38400) and this patch
> > series should fix it. I have tested it with 8250 driver.
>
> drivers
>
> > Originally Johan Hovold reported that there may be issue with these
> > ASYNC_SPD_FLAGS in email:
> > https://lore.kernel.org/linux-serial/[email protected]/
> >
> >
> > Johan, Greg, could you please test these patches if there is not any
> > regression?
>
> I'm wondering why we are still supporting this ugly hack?
> Doesn't BOTHER work for you?

Johan pointed in above mentioned patch that it would break
ASYNC_SPD_FLAGS. So I have looked at how are ASYNC_SPD_FLAGS implemented
to ensure that they would work correctly...

> I would rather expect to have this removed completely.

Well, if somebody is going to remove it, I have no objections. But I
understood that ASYNC_SPD_FLAGS should be still supported...

2022-04-22 18:01:06

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Tue, Mar 22, 2022 at 04:29:08PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 21, 2022 at 11:07 PM Pali Roh?r <[email protected]> wrote:
> >
> > Support for UPF_SPD_* flags is currently broken in more drivers for two
> > reasons. First one is that uart_update_timeout() function does not
>
> the uart_update_timeout()
>
> > calculate timeout for UPF_SPD_CUST flag correctly. Second reason is that
> > userspace termios structre is modified by most drivers after each
>
> structure
>
> ...
>
> > (error handling was ommited for simplification)
>
> omitted
>
> > After calling set_active_spd_cust_baud() function SPD custom divisor
> > should be active and therefore is_spd_cust_active() should return true.
> >
> > But it is not active (cfgetospeed does not return B38400) and this patch
> > series should fix it. I have tested it with 8250 driver.
>
> drivers
>
> > Originally Johan Hovold reported that there may be issue with these
> > ASYNC_SPD_FLAGS in email:
> > https://lore.kernel.org/linux-serial/[email protected]/
> >
> >
> > Johan, Greg, could you please test these patches if there is not any
> > regression?
>
> I'm wondering why we are still supporting this ugly hack?
> Doesn't BOTHER work for you?

Yes, I too do not want to add more support for these old flags. If they
have not been working, let's not add support for them as obviously no
one is using them. Let's try to remove them if at all possible.

thanks,

greg k-h

2022-07-07 08:50:34

by Pali Rohár

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Friday 22 April 2022 16:28:06 Greg Kroah-Hartman wrote:
> On Tue, Mar 22, 2022 at 04:29:08PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 21, 2022 at 11:07 PM Pali Rohár <[email protected]> wrote:
> > >
> > > Support for UPF_SPD_* flags is currently broken in more drivers for two
> > > reasons. First one is that uart_update_timeout() function does not
> >
> > the uart_update_timeout()
> >
> > > calculate timeout for UPF_SPD_CUST flag correctly. Second reason is that
> > > userspace termios structre is modified by most drivers after each
> >
> > structure
> >
> > ...
> >
> > > (error handling was ommited for simplification)
> >
> > omitted
> >
> > > After calling set_active_spd_cust_baud() function SPD custom divisor
> > > should be active and therefore is_spd_cust_active() should return true.
> > >
> > > But it is not active (cfgetospeed does not return B38400) and this patch
> > > series should fix it. I have tested it with 8250 driver.
> >
> > drivers
> >
> > > Originally Johan Hovold reported that there may be issue with these
> > > ASYNC_SPD_FLAGS in email:
> > > https://lore.kernel.org/linux-serial/[email protected]/
> > >
> > >
> > > Johan, Greg, could you please test these patches if there is not any
> > > regression?
> >
> > I'm wondering why we are still supporting this ugly hack?
> > Doesn't BOTHER work for you?
>
> Yes, I too do not want to add more support for these old flags. If they
> have not been working, let's not add support for them as obviously no
> one is using them. Let's try to remove them if at all possible.

Well, it works partially. For more drivers SET method is working, but
GET method returns incorrect value. If your userspace application is
written in a way that does not retrieve from kernel current settings
then it has big probability that application works.

So, do you really want to remove support for these old flags completely?
That would of course break above applications.

Note that usage of BOTHER is problematic and in most cases highly
impossible if you are using glibc libc.so. BOTHER is incompatible with
glibc header files and so you can either include BOTHER/linux termios
file (exclusive) OR glibc header files.

New version of tcsetattr and ioctl_tty manpages would have documented
how to use BOTHER (it is currently in the manpages git).

Currently the only known option how to use BOTHER is to completely
reimplement all functions from "termios.h", provide custom "termios.h"
header file (and not use glibc termios.h nor any file which it includes)
and statically link this reimplementation into final application.

So in most cases BOTHER is not alternative to those old SPD flags even
for modern applications.

> thanks,
>
> greg k-h

2022-07-08 13:14:21

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Thu, Jul 07, 2022 at 10:48:40AM +0200, Pali Roh?r wrote:
> On Friday 22 April 2022 16:28:06 Greg Kroah-Hartman wrote:
> > On Tue, Mar 22, 2022 at 04:29:08PM +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 21, 2022 at 11:07 PM Pali Roh?r <[email protected]> wrote:
> > > >
> > > > Support for UPF_SPD_* flags is currently broken in more drivers for two
> > > > reasons. First one is that uart_update_timeout() function does not
> > >
> > > the uart_update_timeout()
> > >
> > > > calculate timeout for UPF_SPD_CUST flag correctly. Second reason is that
> > > > userspace termios structre is modified by most drivers after each
> > >
> > > structure
> > >
> > > ...
> > >
> > > > (error handling was ommited for simplification)
> > >
> > > omitted
> > >
> > > > After calling set_active_spd_cust_baud() function SPD custom divisor
> > > > should be active and therefore is_spd_cust_active() should return true.
> > > >
> > > > But it is not active (cfgetospeed does not return B38400) and this patch
> > > > series should fix it. I have tested it with 8250 driver.
> > >
> > > drivers
> > >
> > > > Originally Johan Hovold reported that there may be issue with these
> > > > ASYNC_SPD_FLAGS in email:
> > > > https://lore.kernel.org/linux-serial/[email protected]/
> > > >
> > > >
> > > > Johan, Greg, could you please test these patches if there is not any
> > > > regression?
> > >
> > > I'm wondering why we are still supporting this ugly hack?
> > > Doesn't BOTHER work for you?
> >
> > Yes, I too do not want to add more support for these old flags. If they
> > have not been working, let's not add support for them as obviously no
> > one is using them. Let's try to remove them if at all possible.
>
> Well, it works partially. For more drivers SET method is working, but
> GET method returns incorrect value. If your userspace application is
> written in a way that does not retrieve from kernel current settings
> then it has big probability that application works.

I do not understand, sorry, what do you mean by this?

And as you are responding to a months-old thread, I am totally lost, and
don't even know what the patch here was...

> So, do you really want to remove support for these old flags completely?
> That would of course break above applications.

I'm not saying remove them, I'm saying let us not add any more
dependancies on them in order to keep new applications from ever wanting
to use them.

> Note that usage of BOTHER is problematic and in most cases highly
> impossible if you are using glibc libc.so. BOTHER is incompatible with
> glibc header files and so you can either include BOTHER/linux termios
> file (exclusive) OR glibc header files.

I thought that was all fixed up in newer versions of glibc. Is that
really still the case in the latest release?

> New version of tcsetattr and ioctl_tty manpages would have documented
> how to use BOTHER (it is currently in the manpages git).
>
> Currently the only known option how to use BOTHER is to completely
> reimplement all functions from "termios.h", provide custom "termios.h"
> header file (and not use glibc termios.h nor any file which it includes)
> and statically link this reimplementation into final application.
>
> So in most cases BOTHER is not alternative to those old SPD flags even
> for modern applications.

Using the kernel's .h file should be easy enough here for them, right?
And again, I thought glibc was fixed, what about other libc versions?

thanks,

greg k-h

2022-07-08 13:30:34

by Pali Rohár

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Friday 08 July 2022 15:07:43 Greg Kroah-Hartman wrote:
> On Thu, Jul 07, 2022 at 10:48:40AM +0200, Pali Rohár wrote:
> > On Friday 22 April 2022 16:28:06 Greg Kroah-Hartman wrote:
> > > On Tue, Mar 22, 2022 at 04:29:08PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Mar 21, 2022 at 11:07 PM Pali Rohár <[email protected]> wrote:
> > > > >
> > > > > Support for UPF_SPD_* flags is currently broken in more drivers for two
> > > > > reasons. First one is that uart_update_timeout() function does not
> > > >
> > > > the uart_update_timeout()
> > > >
> > > > > calculate timeout for UPF_SPD_CUST flag correctly. Second reason is that
> > > > > userspace termios structre is modified by most drivers after each
> > > >
> > > > structure
> > > >
> > > > ...
> > > >
> > > > > (error handling was ommited for simplification)
> > > >
> > > > omitted
> > > >
> > > > > After calling set_active_spd_cust_baud() function SPD custom divisor
> > > > > should be active and therefore is_spd_cust_active() should return true.
> > > > >
> > > > > But it is not active (cfgetospeed does not return B38400) and this patch
> > > > > series should fix it. I have tested it with 8250 driver.
> > > >
> > > > drivers
> > > >
> > > > > Originally Johan Hovold reported that there may be issue with these
> > > > > ASYNC_SPD_FLAGS in email:
> > > > > https://lore.kernel.org/linux-serial/[email protected]/
> > > > >
> > > > >
> > > > > Johan, Greg, could you please test these patches if there is not any
> > > > > regression?
> > > >
> > > > I'm wondering why we are still supporting this ugly hack?
> > > > Doesn't BOTHER work for you?
> > >
> > > Yes, I too do not want to add more support for these old flags. If they
> > > have not been working, let's not add support for them as obviously no
> > > one is using them. Let's try to remove them if at all possible.
> >
> > Well, it works partially. For more drivers SET method is working, but
> > GET method returns incorrect value. If your userspace application is
> > written in a way that does not retrieve from kernel current settings
> > then it has big probability that application works.
>
> I do not understand, sorry, what do you mean by this?

I mean that SET methods are working, GET methods not. In this case SET
done via ioctl(TIOCSSERIAL) and GET via ioctl(TIOCGSERIAL).

> And as you are responding to a months-old thread, I am totally lost, and
> don't even know what the patch here was...
>
> > So, do you really want to remove support for these old flags completely?
> > That would of course break above applications.
>
> I'm not saying remove them, I'm saying let us not add any more
> dependancies on them in order to keep new applications from ever wanting
> to use them.

Last time you wrote to remove them. Now saying not to remove them. So I
do not understand you now.

> > Note that usage of BOTHER is problematic and in most cases highly
> > impossible if you are using glibc libc.so. BOTHER is incompatible with
> > glibc header files and so you can either include BOTHER/linux termios
> > file (exclusive) OR glibc header files.
>
> I thought that was all fixed up in newer versions of glibc. Is that
> really still the case in the latest release?

In which version you though that it was fixed? Last time when I checked
it (half year ago) all these issues were there still present. Now I
briefly grepped glib git source code and seems that nothing was changed,
no support for BOTHER and neither ABI changes. I also looked into
changelong of last 3 released glibc versions there is neither no
information about it.

> > New version of tcsetattr and ioctl_tty manpages would have documented
> > how to use BOTHER (it is currently in the manpages git).
> >
> > Currently the only known option how to use BOTHER is to completely
> > reimplement all functions from "termios.h", provide custom "termios.h"
> > header file (and not use glibc termios.h nor any file which it includes)
> > and statically link this reimplementation into final application.
> >
> > So in most cases BOTHER is not alternative to those old SPD flags even
> > for modern applications.
>
> Using the kernel's .h file should be easy enough here for them, right?

No. Because structures and defines conflicts with glibc structures and
defines. That is why (ABI incompatible) reimplementation is needed.

> And again, I thought glibc was fixed, what about other libc versions?

musl does not support BOTHER too. And I do not know other libc versions
which are used in production.

> thanks,
>
> greg k-h

2022-07-08 14:19:07

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Fri, Jul 08, 2022 at 03:26:21PM +0200, Pali Roh?r wrote:
> On Friday 08 July 2022 15:07:43 Greg Kroah-Hartman wrote:
> > On Thu, Jul 07, 2022 at 10:48:40AM +0200, Pali Roh?r wrote:
> > > On Friday 22 April 2022 16:28:06 Greg Kroah-Hartman wrote:
> > > > On Tue, Mar 22, 2022 at 04:29:08PM +0200, Andy Shevchenko wrote:
> > > > > On Mon, Mar 21, 2022 at 11:07 PM Pali Roh?r <[email protected]> wrote:
> > > > > >
> > > > > > Support for UPF_SPD_* flags is currently broken in more drivers for two
> > > > > > reasons. First one is that uart_update_timeout() function does not
> > > > >
> > > > > the uart_update_timeout()
> > > > >
> > > > > > calculate timeout for UPF_SPD_CUST flag correctly. Second reason is that
> > > > > > userspace termios structre is modified by most drivers after each
> > > > >
> > > > > structure
> > > > >
> > > > > ...
> > > > >
> > > > > > (error handling was ommited for simplification)
> > > > >
> > > > > omitted
> > > > >
> > > > > > After calling set_active_spd_cust_baud() function SPD custom divisor
> > > > > > should be active and therefore is_spd_cust_active() should return true.
> > > > > >
> > > > > > But it is not active (cfgetospeed does not return B38400) and this patch
> > > > > > series should fix it. I have tested it with 8250 driver.
> > > > >
> > > > > drivers
> > > > >
> > > > > > Originally Johan Hovold reported that there may be issue with these
> > > > > > ASYNC_SPD_FLAGS in email:
> > > > > > https://lore.kernel.org/linux-serial/[email protected]/
> > > > > >
> > > > > >
> > > > > > Johan, Greg, could you please test these patches if there is not any
> > > > > > regression?
> > > > >
> > > > > I'm wondering why we are still supporting this ugly hack?
> > > > > Doesn't BOTHER work for you?
> > > >
> > > > Yes, I too do not want to add more support for these old flags. If they
> > > > have not been working, let's not add support for them as obviously no
> > > > one is using them. Let's try to remove them if at all possible.
> > >
> > > Well, it works partially. For more drivers SET method is working, but
> > > GET method returns incorrect value. If your userspace application is
> > > written in a way that does not retrieve from kernel current settings
> > > then it has big probability that application works.
> >
> > I do not understand, sorry, what do you mean by this?
>
> I mean that SET methods are working, GET methods not. In this case SET
> done via ioctl(TIOCSSERIAL) and GET via ioctl(TIOCGSERIAL).
>
> > And as you are responding to a months-old thread, I am totally lost, and
> > don't even know what the patch here was...
> >
> > > So, do you really want to remove support for these old flags completely?
> > > That would of course break above applications.
> >
> > I'm not saying remove them, I'm saying let us not add any more
> > dependancies on them in order to keep new applications from ever wanting
> > to use them.
>
> Last time you wrote to remove them. Now saying not to remove them. So I
> do not understand you now.

I'm sorry, I am totally lost. How about starting over and resubmitting
the changes you want and we can go from there.

greg k-h

2022-07-08 14:30:00

by Pali Rohár

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Friday 08 July 2022 15:51:01 Greg Kroah-Hartman wrote:
> On Fri, Jul 08, 2022 at 03:26:21PM +0200, Pali Rohár wrote:
> > On Friday 08 July 2022 15:07:43 Greg Kroah-Hartman wrote:
> > > On Thu, Jul 07, 2022 at 10:48:40AM +0200, Pali Rohár wrote:
> > > > On Friday 22 April 2022 16:28:06 Greg Kroah-Hartman wrote:
> > > > > On Tue, Mar 22, 2022 at 04:29:08PM +0200, Andy Shevchenko wrote:
> > > > > > On Mon, Mar 21, 2022 at 11:07 PM Pali Rohár <[email protected]> wrote:
> > > > > > >
> > > > > > > Support for UPF_SPD_* flags is currently broken in more drivers for two
> > > > > > > reasons. First one is that uart_update_timeout() function does not
> > > > > >
> > > > > > the uart_update_timeout()
> > > > > >
> > > > > > > calculate timeout for UPF_SPD_CUST flag correctly. Second reason is that
> > > > > > > userspace termios structre is modified by most drivers after each
> > > > > >
> > > > > > structure
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > (error handling was ommited for simplification)
> > > > > >
> > > > > > omitted
> > > > > >
> > > > > > > After calling set_active_spd_cust_baud() function SPD custom divisor
> > > > > > > should be active and therefore is_spd_cust_active() should return true.
> > > > > > >
> > > > > > > But it is not active (cfgetospeed does not return B38400) and this patch
> > > > > > > series should fix it. I have tested it with 8250 driver.
> > > > > >
> > > > > > drivers
> > > > > >
> > > > > > > Originally Johan Hovold reported that there may be issue with these
> > > > > > > ASYNC_SPD_FLAGS in email:
> > > > > > > https://lore.kernel.org/linux-serial/[email protected]/
> > > > > > >
> > > > > > >
> > > > > > > Johan, Greg, could you please test these patches if there is not any
> > > > > > > regression?
> > > > > >
> > > > > > I'm wondering why we are still supporting this ugly hack?
> > > > > > Doesn't BOTHER work for you?
> > > > >
> > > > > Yes, I too do not want to add more support for these old flags. If they
> > > > > have not been working, let's not add support for them as obviously no
> > > > > one is using them. Let's try to remove them if at all possible.
> > > >
> > > > Well, it works partially. For more drivers SET method is working, but
> > > > GET method returns incorrect value. If your userspace application is
> > > > written in a way that does not retrieve from kernel current settings
> > > > then it has big probability that application works.
> > >
> > > I do not understand, sorry, what do you mean by this?
> >
> > I mean that SET methods are working, GET methods not. In this case SET
> > done via ioctl(TIOCSSERIAL) and GET via ioctl(TIOCGSERIAL).
> >
> > > And as you are responding to a months-old thread, I am totally lost, and
> > > don't even know what the patch here was...
> > >
> > > > So, do you really want to remove support for these old flags completely?
> > > > That would of course break above applications.
> > >
> > > I'm not saying remove them, I'm saying let us not add any more
> > > dependancies on them in order to keep new applications from ever wanting
> > > to use them.
> >
> > Last time you wrote to remove them. Now saying not to remove them. So I
> > do not understand you now.
>
> I'm sorry, I am totally lost.

So look what you have wrote? Who is lost here is me.

> How about starting over and resubmitting
> the changes you want and we can go from there.
>
> greg k-h

What to resubmit? I do not understand you. In case you lost emails or
accidentally removed them, you can look at them in archive, not? I hope
that you do not want me to copy+paste all existing patches with all your
quotes on them which you wrote into new emails.

2022-07-08 15:56:57

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Fri, Jul 8, 2022 at 4:20 PM Pali Rohár <[email protected]> wrote:
> On Friday 08 July 2022 15:51:01 Greg Kroah-Hartman wrote:
> > On Fri, Jul 08, 2022 at 03:26:21PM +0200, Pali Rohár wrote:
> > > On Friday 08 July 2022 15:07:43 Greg Kroah-Hartman wrote:
> > > > On Thu, Jul 07, 2022 at 10:48:40AM +0200, Pali Rohár wrote:
> > > > > On Friday 22 April 2022 16:28:06 Greg Kroah-Hartman wrote:

...

> > > > I'm not saying remove them, I'm saying let us not add any more
> > > > dependancies on them in order to keep new applications from ever wanting
> > > > to use them.
> > >
> > > Last time you wrote to remove them. Now saying not to remove them. So I
> > > do not understand you now.

There was a _new_ addition of the ugly SPD_CUST, that's what I believe
Greg opposes to. And I support that.

> > I'm sorry, I am totally lost.
>
> So look what you have wrote? Who is lost here is me.
>
> > How about starting over and resubmitting
> > the changes you want and we can go from there.
>
> What to resubmit? I do not understand you. In case you lost emails or
> accidentally removed them, you can look at them in archive, not? I hope
> that you do not want me to copy+paste all existing patches with all your
> quotes on them which you wrote into new emails.

That change that adds the new user of SPD_CUST?

In any case the best summary about BOTHER I ever read is this [1] (and
an initial steps in picocom [2]). And I believe that instead of
SPD_CUST we should get rid (or at least minimize) the problems with
BOTHER in user space.

[1]: https://github.com/npat-efault/picocom/blob/master/termios2.txt
[2]: https://github.com/jmesmon/picocom/issues/2



--
With Best Regards,
Andy Shevchenko

2022-07-08 16:07:08

by Pali Rohár

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Friday 08 July 2022 17:42:03 Andy Shevchenko wrote:
> On Fri, Jul 8, 2022 at 4:20 PM Pali Rohár <[email protected]> wrote:
> > On Friday 08 July 2022 15:51:01 Greg Kroah-Hartman wrote:
> > > On Fri, Jul 08, 2022 at 03:26:21PM +0200, Pali Rohár wrote:
> > > > On Friday 08 July 2022 15:07:43 Greg Kroah-Hartman wrote:
> > > > > On Thu, Jul 07, 2022 at 10:48:40AM +0200, Pali Rohár wrote:
> > > > > > On Friday 22 April 2022 16:28:06 Greg Kroah-Hartman wrote:
>
> ...
>
> > > > > I'm not saying remove them, I'm saying let us not add any more
> > > > > dependancies on them in order to keep new applications from ever wanting
> > > > > to use them.
> > > >
> > > > Last time you wrote to remove them. Now saying not to remove them. So I
> > > > do not understand you now.
>
> There was a _new_ addition of the ugly SPD_CUST, that's what I believe
> Greg opposes to. And I support that.

Which addition? I do not understand you. There was not any new driver
with introduction of SPD support.

> > > I'm sorry, I am totally lost.
> >
> > So look what you have wrote? Who is lost here is me.
> >
> > > How about starting over and resubmitting
> > > the changes you want and we can go from there.
> >
> > What to resubmit? I do not understand you. In case you lost emails or
> > accidentally removed them, you can look at them in archive, not? I hope
> > that you do not want me to copy+paste all existing patches with all your
> > quotes on them which you wrote into new emails.
>
> That change that adds the new user of SPD_CUST?

What you are talking about? Which user?

> In any case the best summary about BOTHER I ever read is this [1] (and
> an initial steps in picocom [2]).

Is not that example in manpage enough?

> And I believe that instead of
> SPD_CUST we should get rid (or at least minimize) the problems with
> BOTHER in user space.

I looked into archives and seems that glibc people are not interested in
this area. And I'm not going to spend time on another project which seems
to be useless.

> [1]: https://github.com/npat-efault/picocom/blob/master/termios2.txt
> [2]: https://github.com/jmesmon/picocom/issues/2
>
>
>
> --
> With Best Regards,
> Andy Shevchenko

2022-07-08 16:42:04

by Pali Rohár

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Friday 08 July 2022 18:09:07 Andy Shevchenko wrote:
> On Fri, Jul 8, 2022 at 5:54 PM Pali Rohár <[email protected]> wrote:
> > On Friday 08 July 2022 17:42:03 Andy Shevchenko wrote:
> > > On Fri, Jul 8, 2022 at 4:20 PM Pali Rohár <[email protected]> wrote:
> > > > On Friday 08 July 2022 15:51:01 Greg Kroah-Hartman wrote:
> > > > > On Fri, Jul 08, 2022 at 03:26:21PM +0200, Pali Rohár wrote:
> > > > > > On Friday 08 July 2022 15:07:43 Greg Kroah-Hartman wrote:
> > > > > > > On Thu, Jul 07, 2022 at 10:48:40AM +0200, Pali Rohár wrote:
> > > > > > > > On Friday 22 April 2022 16:28:06 Greg Kroah-Hartman wrote:
> > >
> > > ...
> > >
> > > > > > > I'm not saying remove them, I'm saying let us not add any more
> > > > > > > dependancies on them in order to keep new applications from ever wanting
> > > > > > > to use them.
> > > > > >
> > > > > > Last time you wrote to remove them. Now saying not to remove them. So I
> > > > > > do not understand you now.
> > >
> > > There was a _new_ addition of the ugly SPD_CUST, that's what I believe
> > > Greg opposes to. And I support that.
> >
> > Which addition? I do not understand you. There was not any new driver
> > with introduction of SPD support.
>
> You stated that SPD_CUST is broken in some drivers, so you are trying
> to fix a broken ugly hack. Why? Instead of making it rot and be
> removed eventually, you pump life into frankenstein.

Firstly I got rejection of my other patches because they does not handle
SDP_CUST correctly. So I decided to look at those issues and fix it via
helper function which can be easily reused in all drivers. So helper
function wrap all "ugly" hacks. Then I got reaction that SDP should be
removed. Then I got another reaction that that "I'm not saying to remove
them" and another another reaction why to be removed eventually.

So how should I interpret this? I'm feeling that you are just trying to
annoy people with this "do this", "do opposite", "do again it", "do
again opposite"...

> > > > > I'm sorry, I am totally lost.
> > > >
> > > > So look what you have wrote? Who is lost here is me.
> > > >
> > > > > How about starting over and resubmitting
> > > > > the changes you want and we can go from there.
> > > >
> > > > What to resubmit? I do not understand you. In case you lost emails or
> > > > accidentally removed them, you can look at them in archive, not? I hope
> > > > that you do not want me to copy+paste all existing patches with all your
> > > > quotes on them which you wrote into new emails.
> > >
> > > That change that adds the new user of SPD_CUST?
> >
> > What you are talking about? Which user?
>
> This I missed, I was thinking that you are talking about a new user,
> now I read briefly and it seems that it's about an existing user.
> Anyway, that change I suppose.
>
> > > In any case the best summary about BOTHER I ever read is this [1] (and
> > > an initial steps in picocom [2]).
> >
> > Is not that example in manpage enough?
>
> Dunno.
> Can you point it out to me? I can't find it quickly.

Argh... Have you read emails to which you wrote reply? So copy+paste
relevant part from my previous email just for you:

"New version of tcsetattr and ioctl_tty manpages would have documented
how to use BOTHER (it is currently in the manpages git)."

Plus in past I also pointed to the extended version of that example from
manpage which is currently in my repo on github:
https://github.com/pali/linux-baudrate.git

> > > And I believe that instead of
> > > SPD_CUST we should get rid (or at least minimize) the problems with
> > > BOTHER in user space.
> >
> > I looked into archives and seems that glibc people are not interested in
> > this area. And I'm not going to spend time on another project which seems
> > to be useless.
>
> So why should the kernel suffer if it already provides something good
> for the user and user space ignores that?

Because it is unusable? API which standard linux userspace applications
cannot use is useless. And for application develop it does not matter if
issue is in kernel part of API or userspace part of API. At the end
would be use used.

With whole this discussion I have feeling that there correct way is just
to use SDP flags in userspace as there is no interest in fixing BOTHER's
c_ospeed and c_ispeed in kernel drivers and it was rejected just because
of not handling SDP flags correctly.

> > > [1]: https://github.com/npat-efault/picocom/blob/master/termios2.txt
> > > [2]: https://github.com/jmesmon/picocom/issues/2
>
> --
> With Best Regards,
> Andy Shevchenko

2022-07-08 16:46:18

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Fri, Jul 8, 2022 at 5:54 PM Pali Rohár <[email protected]> wrote:
> On Friday 08 July 2022 17:42:03 Andy Shevchenko wrote:
> > On Fri, Jul 8, 2022 at 4:20 PM Pali Rohár <[email protected]> wrote:
> > > On Friday 08 July 2022 15:51:01 Greg Kroah-Hartman wrote:
> > > > On Fri, Jul 08, 2022 at 03:26:21PM +0200, Pali Rohár wrote:
> > > > > On Friday 08 July 2022 15:07:43 Greg Kroah-Hartman wrote:
> > > > > > On Thu, Jul 07, 2022 at 10:48:40AM +0200, Pali Rohár wrote:
> > > > > > > On Friday 22 April 2022 16:28:06 Greg Kroah-Hartman wrote:
> >
> > ...
> >
> > > > > > I'm not saying remove them, I'm saying let us not add any more
> > > > > > dependancies on them in order to keep new applications from ever wanting
> > > > > > to use them.
> > > > >
> > > > > Last time you wrote to remove them. Now saying not to remove them. So I
> > > > > do not understand you now.
> >
> > There was a _new_ addition of the ugly SPD_CUST, that's what I believe
> > Greg opposes to. And I support that.
>
> Which addition? I do not understand you. There was not any new driver
> with introduction of SPD support.

You stated that SPD_CUST is broken in some drivers, so you are trying
to fix a broken ugly hack. Why? Instead of making it rot and be
removed eventually, you pump life into frankenstein.

> > > > I'm sorry, I am totally lost.
> > >
> > > So look what you have wrote? Who is lost here is me.
> > >
> > > > How about starting over and resubmitting
> > > > the changes you want and we can go from there.
> > >
> > > What to resubmit? I do not understand you. In case you lost emails or
> > > accidentally removed them, you can look at them in archive, not? I hope
> > > that you do not want me to copy+paste all existing patches with all your
> > > quotes on them which you wrote into new emails.
> >
> > That change that adds the new user of SPD_CUST?
>
> What you are talking about? Which user?

This I missed, I was thinking that you are talking about a new user,
now I read briefly and it seems that it's about an existing user.
Anyway, that change I suppose.

> > In any case the best summary about BOTHER I ever read is this [1] (and
> > an initial steps in picocom [2]).
>
> Is not that example in manpage enough?

Dunno.
Can you point it out to me? I can't find it quickly.

> > And I believe that instead of
> > SPD_CUST we should get rid (or at least minimize) the problems with
> > BOTHER in user space.
>
> I looked into archives and seems that glibc people are not interested in
> this area. And I'm not going to spend time on another project which seems
> to be useless.

So why should the kernel suffer if it already provides something good
for the user and user space ignores that?

> > [1]: https://github.com/npat-efault/picocom/blob/master/termios2.txt
> > [2]: https://github.com/jmesmon/picocom/issues/2

--
With Best Regards,
Andy Shevchenko

2022-07-08 17:49:16

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 0/3] serial: Fix support for UPF_SPD_* flags

On Fri, Jul 8, 2022 at 6:25 PM Pali Rohár <[email protected]> wrote:
>
> On Friday 08 July 2022 18:09:07 Andy Shevchenko wrote:
> > On Fri, Jul 8, 2022 at 5:54 PM Pali Rohár <[email protected]> wrote:
> > > On Friday 08 July 2022 17:42:03 Andy Shevchenko wrote:
> > > > On Fri, Jul 8, 2022 at 4:20 PM Pali Rohár <[email protected]> wrote:
> > > > > On Friday 08 July 2022 15:51:01 Greg Kroah-Hartman wrote:
> > > > > > On Fri, Jul 08, 2022 at 03:26:21PM +0200, Pali Rohár wrote:
> > > > > > > On Friday 08 July 2022 15:07:43 Greg Kroah-Hartman wrote:
> > > > > > > > On Thu, Jul 07, 2022 at 10:48:40AM +0200, Pali Rohár wrote:
> > > > > > > > > On Friday 22 April 2022 16:28:06 Greg Kroah-Hartman wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > I'm not saying remove them, I'm saying let us not add any more
> > > > > > > > dependancies on them in order to keep new applications from ever wanting
> > > > > > > > to use them.
> > > > > > >
> > > > > > > Last time you wrote to remove them. Now saying not to remove them. So I
> > > > > > > do not understand you now.
> > > >
> > > > There was a _new_ addition of the ugly SPD_CUST, that's what I believe
> > > > Greg opposes to. And I support that.
> > >
> > > Which addition? I do not understand you. There was not any new driver
> > > with introduction of SPD support.
> >
> > You stated that SPD_CUST is broken in some drivers, so you are trying
> > to fix a broken ugly hack. Why? Instead of making it rot and be
> > removed eventually, you pump life into frankenstein.
>
> Firstly I got rejection of my other patches because they does not handle
> SDP_CUST correctly. So I decided to look at those issues and fix it via
> helper function which can be easily reused in all drivers. So helper
> function wrap all "ugly" hacks. Then I got reaction that SDP should be
> removed. Then I got another reaction that that "I'm not saying to remove
> them" and another another reaction why to be removed eventually.
>
> So how should I interpret this? I'm feeling that you are just trying to
> annoy people with this "do this", "do opposite", "do again it", "do
> again opposite"...

Ask someone who makes a decision. I wrote just my p.o.v. on the
"problem". I think there is no problem with SPD_CUST, it should be
oblivionized.

> > > > > > I'm sorry, I am totally lost.
> > > > >
> > > > > So look what you have wrote? Who is lost here is me.
> > > > >
> > > > > > How about starting over and resubmitting
> > > > > > the changes you want and we can go from there.
> > > > >
> > > > > What to resubmit? I do not understand you. In case you lost emails or
> > > > > accidentally removed them, you can look at them in archive, not? I hope
> > > > > that you do not want me to copy+paste all existing patches with all your
> > > > > quotes on them which you wrote into new emails.
> > > >
> > > > That change that adds the new user of SPD_CUST?
> > >
> > > What you are talking about? Which user?
> >
> > This I missed, I was thinking that you are talking about a new user,
> > now I read briefly and it seems that it's about an existing user.
> > Anyway, that change I suppose.
> >
> > > > In any case the best summary about BOTHER I ever read is this [1] (and
> > > > an initial steps in picocom [2]).
> > >
> > > Is not that example in manpage enough?
> >
> > Dunno.
> > Can you point it out to me? I can't find it quickly.
>
> Argh... Have you read emails to which you wrote reply? So copy+paste
> relevant part from my previous email just for you:
>
> "New version of tcsetattr and ioctl_tty manpages would have documented
> how to use BOTHER (it is currently in the manpages git)."

I do not know the "manpages git" URL. Neither its hosting. kernel.org?
And then? It took time for you to just write something instead of helping me.
Whatever. I found the commits.

> Plus in past I also pointed to the extended version of that example from
> manpage which is currently in my repo on github:
> https://github.com/pali/linux-baudrate.git
>
> > > > And I believe that instead of
> > > > SPD_CUST we should get rid (or at least minimize) the problems with
> > > > BOTHER in user space.
> > >
> > > I looked into archives and seems that glibc people are not interested in
> > > this area. And I'm not going to spend time on another project which seems
> > > to be useless.
> >
> > So why should the kernel suffer if it already provides something good
> > for the user and user space ignores that?
>
> Because it is unusable? API which standard linux userspace applications
> cannot use is useless. And for application develop it does not matter if
> issue is in kernel part of API or userspace part of API. At the end
> would be use used.

Then help make it happen?

> With whole this discussion I have feeling that there correct way is just
> to use SDP flags in userspace as there is no interest in fixing BOTHER's
> c_ospeed and c_ispeed in kernel drivers and it was rejected just because
> of not handling SDP flags correctly.

I'm puzzled who asked you about SPD_CUST implementation... It.is.an.ugly.hack.

--
With Best Regards,
Andy Shevchenko