[PATCH 0/7] tty: max310x: Simplify the code and fix a few bugs

[PATCH 0/7] tty: max310x: Simplify the code and fix a few bugs

[Serge Semin]

I started using this driver two years ago in kernek 4.4 and then in kernel
4.9. It didn't go well from the very beginning due to my platform
peculiarities: DW SPI core with hardware CS and relatively slow MIPS-based
SoC. This patchset is intended to fix some of the problems I found out
during the max310x driver utilization with max14830 device.

First of all it was discovered, that workqueue API isn't optimally used.
Work context isn't re-entrant by design, so the mutex used to guard the
TX-method is redundant. schedule_work() method is also created in a way
the work item is scheduled only if it isn't pending. Patch 1 concerns all
these fixes. Seeing the similar container_of(uart_port) is used three
times in the driver, the patch 2 introduces a macro to_max310x_port() to
get a pointer to corresponding struct max310x_one. This is the code
simplification and is going to be used in the following patches.

It was found out, that batch read and write methods used buffers allocated
on the kernel stack. Since they might be utilized by SPI controllers for
DMA it might be unsafe on some platforms. Patch 3 provides a dedicated
kmalloced buffers for this.

The baud-rate calculator function didn't work correct for all the possible
baud-rates requested within a pre-defined input reference frequency.
Instead an algo fully compliant with datasheet divisor formulae is
implemented in patch 4.

Patches 5 and 6 are created to fix some rs485 issues. Particularly the
rs485 mode is configured on the port startup if it's enabled. And seeing
the mode2 register provides a way to enable/disable the echo-suppression
in RS485 mode, it is used to implement the SER_RS485_RX_DURING_TX flag
support.

Finally it was discovered that in case if inbound hardware FIFO
experienced overflow, a lot of '\0' characters inserted into the
flip-buffer as a character of the RX-FIFO overrun. It isn't quite correct
since the overflow happened only after the last character had been
received. Patch 7 is dedicated to push only a single RX-FIFO overrun
character in this case.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>


Serge Semin (7):
  tty: max310x: Simplify tx-work item code
  tty: max310x: Introduce max310x_one port macro-wrapper
  tty: max310x: Don't pass stacked buffers to SPI
  tty: max310x: Fix invalid baudrate divisors calculator
  tty: max310x: Add rx-during-tx rs485 flag support
  tty: max310x: Optionally enable rs485 on startup
  tty: max310x: Split uart characters insertion loop

 drivers/tty/serial/max310x.c | 157 +++++++++++++++++++++--------------
 1 file changed, 95 insertions(+), 62 deletions(-)

-- 
2.21.0

[PATCH 1/7] tty: max310x: Simplify tx-work item code

[Serge Semin]

Since cmwq introduction in the kernel, workqueues've been turned into
non-reentrant execution contexts [1]. It means any work item is
guaranteed to be executed by at most one worker system-wide at any
given time. Since tx-handler max310x_handle_tx() is called by a
single work item we don't need it to be self-protected by the mutex.
We also don't need to check the tx work item pending state before
scheduling it (which in the first place was racy btw), since cmwq will
make sure to reschedule the item if it wasn't pending at the moment of
schedule_work() call.

[1] Documentation/core-api/workqueue.rst

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/tty/serial/max310x.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 450ba6d7996c..4ee805862b68 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -263,7 +263,6 @@ struct max310x_one {
 struct max310x_port {
 	struct max310x_devtype	*devtype;
 	struct regmap		*regmap;
-	struct mutex		mutex;
 	struct clk		*clk;
 #ifdef CONFIG_GPIOLIB
 	struct gpio_chip	gpio;
@@ -768,8 +767,7 @@ static void max310x_start_tx(struct uart_port *port)
 {
 	struct max310x_one *one = container_of(port, struct max310x_one, port);
 
-	if (!work_pending(&one->tx_work))
-		schedule_work(&one->tx_work);
+	schedule_work(&one->tx_work);
 }
 
 static irqreturn_t max310x_port_irq(struct max310x_port *s, int portno)
@@ -826,14 +824,11 @@ static irqreturn_t max310x_ist(int irq, void *dev_id)
 	return IRQ_RETVAL(handled);
 }
 
-static void max310x_wq_proc(struct work_struct *ws)
+static void max310x_tx_proc(struct work_struct *ws)
 {
 	struct max310x_one *one = container_of(ws, struct max310x_one, tx_work);
-	struct max310x_port *s = dev_get_drvdata(one->port.dev);
 
-	mutex_lock(&s->mutex);
 	max310x_handle_tx(&one->port);
-	mutex_unlock(&s->mutex);
 }
 
 static unsigned int max310x_tx_empty(struct uart_port *port)
@@ -1269,8 +1264,6 @@ static int max310x_probe(struct device *dev, struct max310x_devtype *devtype,
 	uartclk = max310x_set_ref_clk(dev, s, freq, xtal);
 	dev_dbg(dev, "Reference clock set to %i Hz\n", uartclk);
 
-	mutex_init(&s->mutex);
-
 	for (i = 0; i < devtype->nr; i++) {
 		unsigned int line;
 
@@ -1298,7 +1291,7 @@ static int max310x_probe(struct device *dev, struct max310x_devtype *devtype,
 		/* Clear IRQ status register */
 		max310x_port_read(&s->p[i].port, MAX310X_IRQSTS_REG);
 		/* Initialize queue for start TX */
-		INIT_WORK(&s->p[i].tx_work, max310x_wq_proc);
+		INIT_WORK(&s->p[i].tx_work, max310x_tx_proc);
 		/* Initialize queue for changing LOOPBACK mode */
 		INIT_WORK(&s->p[i].md_work, max310x_md_proc);
 		/* Initialize queue for changing RS485 mode */
@@ -1350,8 +1343,6 @@ static int max310x_probe(struct device *dev, struct max310x_devtype *devtype,
 		}
 	}
 
-	mutex_destroy(&s->mutex);
-
 out_clk:
 	clk_disable_unprepare(s->clk);
 
@@ -1372,7 +1363,6 @@ static int max310x_remove(struct device *dev)
 		s->devtype->power(&s->p[i].port, 0);
 	}
 
-	mutex_destroy(&s->mutex);
 	clk_disable_unprepare(s->clk);
 
 	return 0;
-- 
2.21.0

[PATCH 2/7] tty: max310x: Introduce max310x_one port macro-wrapper

[Serge Semin]

uart_port structure instance is embedded into the max310x_one
super-structure, which is accessed by some of the uart-port callback
methods. In order to improve the callback's code readability lets
define the to_max310x_port() wrapper which just translates the passed
uart_port pointer to the max310x_one one. It is also going to be
handy in future commits.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/tty/serial/max310x.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 4ee805862b68..527f1476c24a 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -259,6 +259,8 @@ struct max310x_one {
 	struct work_struct	md_work;
 	struct work_struct	rs_work;
 };
+#define to_max310x_port(_port) \
+	container_of(_port, struct max310x_one, port)
 
 struct max310x_port {
 	struct max310x_devtype	*devtype;
@@ -765,7 +767,7 @@ static void max310x_handle_tx(struct uart_port *port)
 
 static void max310x_start_tx(struct uart_port *port)
 {
-	struct max310x_one *one = container_of(port, struct max310x_one, port);
+	struct max310x_one *one = to_max310x_port(port);
 
 	schedule_work(&one->tx_work);
 }
@@ -858,7 +860,7 @@ static void max310x_md_proc(struct work_struct *ws)
 
 static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
-	struct max310x_one *one = container_of(port, struct max310x_one, port);
+	struct max310x_one *one = to_max310x_port(port);
 
 	schedule_work(&one->md_work);
 }
@@ -981,7 +983,7 @@ static void max310x_rs_proc(struct work_struct *ws)
 static int max310x_rs485_config(struct uart_port *port,
 				struct serial_rs485 *rs485)
 {
-	struct max310x_one *one = container_of(port, struct max310x_one, port);
+	struct max310x_one *one = to_max310x_port(port);
 
 	if ((rs485->delay_rts_before_send > 0x0f) ||
 	    (rs485->delay_rts_after_send > 0x0f))
-- 
2.21.0

[PATCH 3/7] tty: max310x: Don't pass stacked buffers to SPI

[Serge Semin]

SPI transfer tx/rx buffers must be DMA-safe and the structure
documentation clearly states this. Data declared on the system stack isn't
DMA-safe [1]. Instead at least kernel memory should be used for the
buffers. In order to fix this here we can create the buffers at the device
probing stage and use them without any synchronization, since batch
read/write methods are called from non-reentrant contexts - either from
rx-event IRQ threaded handler or from the tx workqueue item.

[1] Documentation/DMA-API-HOWTO.txt

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/tty/serial/max310x.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 527f1476c24a..0e3dc89c459b 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -258,6 +258,10 @@ struct max310x_one {
 	struct work_struct	tx_work;
 	struct work_struct	md_work;
 	struct work_struct	rs_work;
+
+	u8 wr_header;
+	u8 rd_header;
+	u8 rx_buf[MAX310X_FIFO_SIZE];
 };
 #define to_max310x_port(_port) \
 	container_of(_port, struct max310x_one, port)
@@ -608,11 +612,11 @@ static int max310x_set_ref_clk(struct device *dev, struct max310x_port *s,
 
 static void max310x_batch_write(struct uart_port *port, u8 *txbuf, unsigned int len)
 {
-	u8 header[] = { (port->iobase + MAX310X_THR_REG) | MAX310X_WRITE_BIT };
+	struct max310x_one *one = to_max310x_port(port);
 	struct spi_transfer xfer[] = {
 		{
-			.tx_buf = &header,
-			.len = sizeof(header),
+			.tx_buf = &one->wr_header,
+			.len = sizeof(one->wr_header),
 		}, {
 			.tx_buf = txbuf,
 			.len = len,
@@ -623,11 +627,11 @@ static void max310x_batch_write(struct uart_port *port, u8 *txbuf, unsigned int
 
 static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int len)
 {
-	u8 header[] = { port->iobase + MAX310X_RHR_REG };
+	struct max310x_one *one = to_max310x_port(port);
 	struct spi_transfer xfer[] = {
 		{
-			.tx_buf = &header,
-			.len = sizeof(header),
+			.tx_buf = &one->rd_header,
+			.len = sizeof(one->rd_header),
 		}, {
 			.rx_buf = rxbuf,
 			.len = len,
@@ -638,8 +642,8 @@ static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int l
 
 static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
 {
+	struct max310x_one *one = to_max310x_port(port);
 	unsigned int sts, ch, flag, i;
-	u8 buf[MAX310X_FIFO_SIZE];
 
 	if (port->read_status_mask == MAX310X_LSR_RXOVR_BIT) {
 		/* We are just reading, happily ignoring any error conditions.
@@ -654,7 +658,7 @@ static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
 		 * */
 
 		sts = max310x_port_read(port, MAX310X_LSR_IRQSTS_REG);
-		max310x_batch_read(port, buf, rxlen);
+		max310x_batch_read(port, one->rx_buf, rxlen);
 
 		port->icount.rx += rxlen;
 		flag = TTY_NORMAL;
@@ -666,7 +670,8 @@ static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
 		}
 
 		for (i = 0; i < rxlen; ++i) {
-			uart_insert_char(port, sts, MAX310X_LSR_RXOVR_BIT, buf[i], flag);
+			uart_insert_char(port, sts, MAX310X_LSR_RXOVR_BIT,
+					 one->rx_buf[i], flag);
 		}
 
 	} else {
@@ -1298,6 +1303,10 @@ static int max310x_probe(struct device *dev, struct max310x_devtype *devtype,
 		INIT_WORK(&s->p[i].md_work, max310x_md_proc);
 		/* Initialize queue for changing RS485 mode */
 		INIT_WORK(&s->p[i].rs_work, max310x_rs_proc);
+		/* Initialize SPI-transfer buffers */
+		s->p[i].wr_header = (s->p[i].port.iobase + MAX310X_THR_REG) |
+				    MAX310X_WRITE_BIT;
+		s->p[i].rd_header = (s->p[i].port.iobase + MAX310X_RHR_REG);
 
 		/* Register port */
 		ret = uart_add_one_port(&max310x_uart, &s->p[i].port);
-- 
2.21.0

[PATCH 4/7] tty: max310x: Fix invalid baudrate divisors calculator

[Serge Semin]

Current calculator doesn't do it' job quite correct. First of all the
max310x baud-rates generator supports the divisor being less than 16.
In this case the x2/x4 modes can be used to double or quadruple
the reference frequency. But the current baud-rate setter function
just filters all these modes out by the first condition and setups
these modes only if there is a clocks-baud division remainder. The former
doesn't seem right at all, since enabling the x2/x4 modes causes the line
noise tolerance reduction and should be only used as a last resort to
enable a requested too high baud-rate.

Finally the fraction is supposed to be calculated from D = Fref/(c*baud)
formulae, but not from D % 16, which causes the precision loss. So to speak
the current baud-rate calculator code works well only if the baud perfectly
fits to the uart reference input frequency.

Lets fix the calculator by implementing the algo fully compliant with
the fractional baud-rate generator described in the datasheet:
D = Fref / (c*baud), where c={16,8,4} is the x1/x2/x4 rate mode
respectively, Fref - reference input frequency. The divisor fraction is
calculated from the same formulae, but making sure it is found with a
resolution of 0.0625 (four bits).

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/tty/serial/max310x.c | 51 ++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 0e3dc89c459b..ca044f96c5cc 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -501,37 +501,48 @@ static bool max310x_reg_precious(struct device *dev, unsigned int reg)
 
 static int max310x_set_baud(struct uart_port *port, int baud)
 {
-	unsigned int mode = 0, clk = port->uartclk, div = clk / baud;
+	unsigned int mode = 0, div = 0, frac = 0, c = 0, F = 0;
 
-	/* Check for minimal value for divider */
-	if (div < 16)
-		div = 16;
-
-	if (clk % baud && (div / 16) < 0x8000) {
+	/*
+	 * Calculate the integer divisor first. Select a proper mode
+	 * in case if the requested baud is too high for the pre-defined
+	 * clocks frequency.
+	 */
+	div = port->uartclk / baud;
+	if (div < 8) {
+		/* Mode x4 */
+		c = 4;
+		mode = MAX310X_BRGCFG_4XMODE_BIT;
+	} else if (div < 16) {
 		/* Mode x2 */
+		c = 8;
 		mode = MAX310X_BRGCFG_2XMODE_BIT;
-		clk = port->uartclk * 2;
-		div = clk / baud;
-
-		if (clk % baud && (div / 16) < 0x8000) {
-			/* Mode x4 */
-			mode = MAX310X_BRGCFG_4XMODE_BIT;
-			clk = port->uartclk * 4;
-			div = clk / baud;
-		}
+	} else {
+		c = 16;
 	}
 
-	max310x_port_write(port, MAX310X_BRGDIVMSB_REG, (div / 16) >> 8);
-	max310x_port_write(port, MAX310X_BRGDIVLSB_REG, div / 16);
-	max310x_port_write(port, MAX310X_BRGCFG_REG, (div % 16) | mode);
+	/* Calculate the divisor in accordance with the fraction coefficient */
+	div /= c;
+	F = c*baud;
+
+	/* Calculate the baud rate fraction */
+	if (div > 0)
+		frac = (16*(port->uartclk % F)) / F;
+	else
+		div = 1;
+
+	max310x_port_write(port, MAX310X_BRGDIVMSB_REG, div >> 8);
+	max310x_port_write(port, MAX310X_BRGDIVLSB_REG, div);
+	max310x_port_write(port, MAX310X_BRGCFG_REG, frac | mode);
 
-	return DIV_ROUND_CLOSEST(clk, div);
+	/* Return the actual baud rate we just programmed */
+	return (16*port->uartclk) / (c*(16*div + frac));
 }
 
 static int max310x_update_best_err(unsigned long f, long *besterr)
 {
 	/* Use baudrate 115200 for calculate error */
-	long err = f % (115200 * 16);
+	long err = f % (460800 * 16);
 
 	if ((*besterr < 0) || (*besterr > err)) {
 		*besterr = err;
-- 
2.21.0

[PATCH 5/7] tty: max310x: Add rx-during-tx rs485 flag support

[Serge Semin]

The driver currently sets the echo suppression bit by default when rs485
is enabled. Naturally it disables any data retrieval in rs485 mode while
RTSn is pushed up. The receiver gate (RX_) can be enabled just by clearing
(or not setting) the EchoSuprs bit of mode2 register. So by setting or
clearing the bit we implement the SER_RS485_RX_DURING_TX rs485 flag
support.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/tty/serial/max310x.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index ca044f96c5cc..2255300404bd 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -975,25 +975,23 @@ static void max310x_set_termios(struct uart_port *port,
 static void max310x_rs_proc(struct work_struct *ws)
 {
 	struct max310x_one *one = container_of(ws, struct max310x_one, rs_work);
-	unsigned int val;
+	unsigned int delay, mode1 = 0, mode2 = 0;
 
-	val = (one->port.rs485.delay_rts_before_send << 4) |
+	delay = (one->port.rs485.delay_rts_before_send << 4) |
 		one->port.rs485.delay_rts_after_send;
-	max310x_port_write(&one->port, MAX310X_HDPIXDELAY_REG, val);
+	max310x_port_write(&one->port, MAX310X_HDPIXDELAY_REG, delay);
 
 	if (one->port.rs485.flags & SER_RS485_ENABLED) {
-		max310x_port_update(&one->port, MAX310X_MODE1_REG,
-				MAX310X_MODE1_TRNSCVCTRL_BIT,
-				MAX310X_MODE1_TRNSCVCTRL_BIT);
-		max310x_port_update(&one->port, MAX310X_MODE2_REG,
-				MAX310X_MODE2_ECHOSUPR_BIT,
-				MAX310X_MODE2_ECHOSUPR_BIT);
-	} else {
-		max310x_port_update(&one->port, MAX310X_MODE1_REG,
-				MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
-		max310x_port_update(&one->port, MAX310X_MODE2_REG,
-				MAX310X_MODE2_ECHOSUPR_BIT, 0);
+		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+
+		if (!(one->port.rs485.flags & SER_RS485_RX_DURING_TX))
+			mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
 	}
+
+	max310x_port_update(&one->port, MAX310X_MODE1_REG,
+			MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
+	max310x_port_update(&one->port, MAX310X_MODE2_REG,
+			MAX310X_MODE2_ECHOSUPR_BIT, mode2);
 }
 
 static int max310x_rs485_config(struct uart_port *port,
@@ -1005,7 +1003,8 @@ static int max310x_rs485_config(struct uart_port *port,
 	    (rs485->delay_rts_after_send > 0x0f))
 		return -ERANGE;
 
-	rs485->flags &= SER_RS485_RTS_ON_SEND | SER_RS485_ENABLED;
+	rs485->flags &= SER_RS485_RTS_ON_SEND | SER_RS485_RX_DURING_TX |
+			SER_RS485_ENABLED;
 	memset(rs485->padding, 0, sizeof(rs485->padding));
 	port->rs485 = *rs485;
 
-- 
2.21.0

[PATCH 6/7] tty: max310x: Optionally enable rs485 on startup

[Serge Semin]

UART port might be pre-configured with rs485 enabled flag at the
time of the port starting up process. In this case we need to
have the hardware rs485-related registers initialized in accordance
with the rs485 flags and settings provided by the configs descriptor.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/tty/serial/max310x.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 2255300404bd..36943f6c198c 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1030,6 +1030,22 @@ static int max310x_startup(struct uart_port *port)
 	max310x_port_update(port, MAX310X_MODE2_REG,
 			    MAX310X_MODE2_FIFORST_BIT, 0);
 
+	/* Configure mode1/mode2 to have rs485/rs232 enabled at startup */
+	val = (clamp(port->rs485.delay_rts_before_send, 0U, 15U) << 4) |
+		clamp(port->rs485.delay_rts_after_send, 0U, 15U);
+	max310x_port_write(port, MAX310X_HDPIXDELAY_REG, val);
+
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		max310x_port_update(port, MAX310X_MODE1_REG,
+				    MAX310X_MODE1_TRNSCVCTRL_BIT,
+				    MAX310X_MODE1_TRNSCVCTRL_BIT);
+
+		if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
+			max310x_port_update(port, MAX310X_MODE2_REG,
+					    MAX310X_MODE2_ECHOSUPR_BIT,
+					    MAX310X_MODE2_ECHOSUPR_BIT);
+	}
+
 	/* Configure flow control levels */
 	/* Flow control halt level 96, resume level 48 */
 	max310x_port_write(port, MAX310X_FLOWLVL_REG,
-- 
2.21.0

[PATCH 7/7] tty: max310x: Split uart characters insertion loop

[Serge Semin]

Batch read mode doesn't check any conditions or flags except the Rx
overflow one. But it may only happen after the last character is pushed
into the RHR register. In this case we shouldn't push all the read
characters with overrun flag set, but only the last one caused the
FIFO overflow. This commit splits the characters retrieval loop into
two parts. First one is ordinary intsert-chars procedure without taking
the overrun status into account. Second part inserts the last character
checking whether the overrun happened and pushing a '\0' character with
TTY_OVERRUN flag to a flip-buffer.

If we left the loop the way it was the '\0' character would be inserted
after each character retrieved at the overrun occasion.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/tty/serial/max310x.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 36943f6c198c..81b2413c3da4 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -680,10 +680,16 @@ static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
 			port->icount.overrun++;
 		}
 
-		for (i = 0; i < rxlen; ++i) {
-			uart_insert_char(port, sts, MAX310X_LSR_RXOVR_BIT,
-					 one->rx_buf[i], flag);
-		}
+		for (i = 0; i < (rxlen - 1); ++i)
+			uart_insert_char(port, sts, 0, one->rx_buf[i], flag);
+
+		/*
+		 * Handle the overrun case for the last character only, since
+		 * the RxFIFO overflow happens after it is pushed to the FIFO
+		 * tail.
+		 */
+		uart_insert_char(port, sts, MAX310X_LSR_RXOVR_BIT,
+				 one->rx_buf[rxlen], flag);
 
 	} else {
 		if (unlikely(rxlen >= port->fifosize)) {
-- 
2.21.0

Re: [PATCH 0/7] tty: max310x: Simplify the code and fix a few bugs

[Greg Kroah-Hartman]

On Tue, May 14, 2019 at 01:14:08PM +0300, Serge Semin wrote:
> I started using this driver two years ago in kernek 4.4 and then in kernel
> 4.9. It didn't go well from the very beginning due to my platform
> peculiarities: DW SPI core with hardware CS and relatively slow MIPS-based
> SoC. This patchset is intended to fix some of the problems I found out
> during the max310x driver utilization with max14830 device.
> 
> First of all it was discovered, that workqueue API isn't optimally used.
> Work context isn't re-entrant by design, so the mutex used to guard the
> TX-method is redundant. schedule_work() method is also created in a way
> the work item is scheduled only if it isn't pending. Patch 1 concerns all
> these fixes. Seeing the similar container_of(uart_port) is used three
> times in the driver, the patch 2 introduces a macro to_max310x_port() to
> get a pointer to corresponding struct max310x_one. This is the code
> simplification and is going to be used in the following patches.
> 
> It was found out, that batch read and write methods used buffers allocated
> on the kernel stack. Since they might be utilized by SPI controllers for
> DMA it might be unsafe on some platforms. Patch 3 provides a dedicated
> kmalloced buffers for this.
> 
> The baud-rate calculator function didn't work correct for all the possible
> baud-rates requested within a pre-defined input reference frequency.
> Instead an algo fully compliant with datasheet divisor formulae is
> implemented in patch 4.
> 
> Patches 5 and 6 are created to fix some rs485 issues. Particularly the
> rs485 mode is configured on the port startup if it's enabled. And seeing
> the mode2 register provides a way to enable/disable the echo-suppression
> in RS485 mode, it is used to implement the SER_RS485_RX_DURING_TX flag
> support.
> 
> Finally it was discovered that in case if inbound hardware FIFO
> experienced overflow, a lot of '\0' characters inserted into the
> flip-buffer as a character of the RX-FIFO overrun. It isn't quite correct
> since the overflow happened only after the last character had been
> received. Patch 7 is dedicated to push only a single RX-FIFO overrun
> character in this case.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

Nice cleanups, all now applied, thanks!

greg k-h