[PATCH v3 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[PATCH v3 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[Ilpo Järvinen]

Not all LSR register flags are preserved across reads. Therefore, LSR
readers must store the non-preserved bits into lsr_save_flags.

This fix was initially mixed into feature commit f6f586102add ("serial:
8250: Handle UART without interrupt on TEMT using em485"). However,
that feature change had a flaw and it was reverted to make room for
simpler approach providing the same feature. The embedded fix got
reverted with the feature change.

Re-add the lsr_save_flags fix and properly mark it's a fix.

Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
Link: https://lore.kernel.org/all/1d6c31d-d194-9e6a-ddf9-5f29af829f3@linux.intel.com/T/#m1737eef986bd20cf19593e344cebd7b0244945fc
Co-developed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 4998799abae2..c5e0f925f4b6 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1511,6 +1511,8 @@ static inline void __stop_tx(struct uart_8250_port *p)
 		unsigned char lsr = serial_in(p, UART_LSR);
 		u64 stop_delay = 0;
 
+		p->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
+
 		if (!(lsr & UART_LSR_THRE))
 			return;
 		/*
-- 
2.30.2

[PATCH v3 2/6] serial: 8250: Create serial_lsr_in()

[Ilpo Järvinen]

LSR register readers need to be careful in order to not lose bits that
are not preserved across reads. Create a helper that takes care of
storing the non-preserved bits into lsr_save_flags.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250.h      | 20 ++++++++++++++++++++
 drivers/tty/serial/8250/8250_core.c |  3 +--
 drivers/tty/serial/8250/8250_port.c | 15 ++++-----------
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 3d9a7e539dea..b120da57c61f 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -123,6 +123,26 @@ static inline void serial_out(struct uart_8250_port *up, int offset, int value)
 	up->port.serial_out(&up->port, offset, value);
 }
 
+/**
+ *	serial_lsr_in - Read LSR register and preserve flags across reads
+ *	@up:	uart 8250 port
+ *
+ *	Read LSR register and handle saving non-preserved flags across reads.
+ *	The flags that are not preserved across reads are stored into
+ *	up->lsr_saved_flags.
+ *
+ *	Returns LSR value or'ed with the preserved flags (if any).
+ */
+static inline unsigned int serial_lsr_in(struct uart_8250_port *up)
+{
+	unsigned int lsr = up->lsr_saved_flags;
+
+	lsr |= serial_in(up, UART_LSR);
+	up->lsr_saved_flags = lsr & LSR_SAVE_FLAGS;
+
+	return lsr;
+}
+
 /*
  * For the 16C950
  */
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 12c5e5d0ce6d..90ddc8924811 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -276,8 +276,7 @@ static void serial8250_backup_timeout(struct timer_list *t)
 	 * the "Diva" UART used on the management processor on many HP
 	 * ia64 and parisc boxes.
 	 */
-	lsr = serial_in(up, UART_LSR);
-	up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
+	lsr = serial_lsr_in(up);
 	if ((iir & UART_IIR_NO_INT) && (up->ier & UART_IER_THRI) &&
 	    (!uart_circ_empty(&up->port.state->xmit) || up->port.x_char) &&
 	    (lsr & UART_LSR_THRE)) {
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index c5e0f925f4b6..ec5abeb638eb 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1508,11 +1508,9 @@ static inline void __stop_tx(struct uart_8250_port *p)
 	struct uart_8250_em485 *em485 = p->em485;
 
 	if (em485) {
-		unsigned char lsr = serial_in(p, UART_LSR);
+		unsigned char lsr = serial_lsr_in(p);
 		u64 stop_delay = 0;
 
-		p->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
-
 		if (!(lsr & UART_LSR_THRE))
 			return;
 		/*
@@ -1567,10 +1565,8 @@ static inline void __start_tx(struct uart_port *port)
 
 	if (serial8250_set_THRI(up)) {
 		if (up->bugs & UART_BUG_TXEN) {
-			unsigned char lsr;
+			unsigned char lsr = serial_lsr_in(up);
 
-			lsr = serial_in(up, UART_LSR);
-			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
 			if (lsr & UART_LSR_THRE)
 				serial8250_tx_chars(up);
 		}
@@ -2001,8 +1997,7 @@ static unsigned int serial8250_tx_empty(struct uart_port *port)
 	serial8250_rpm_get(up);
 
 	spin_lock_irqsave(&port->lock, flags);
-	lsr = serial_port_in(port, UART_LSR);
-	up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
+	lsr = serial_lsr_in(up);
 	spin_unlock_irqrestore(&port->lock, flags);
 
 	serial8250_rpm_put(up);
@@ -2078,9 +2073,7 @@ static void wait_for_lsr(struct uart_8250_port *up, int bits)
 
 	/* Wait up to 10ms for the character(s) to be sent. */
 	for (;;) {
-		status = serial_in(up, UART_LSR);
-
-		up->lsr_saved_flags |= status & LSR_SAVE_FLAGS;
+		status = serial_lsr_in(up);
 
 		if ((status & bits) == bits)
 			break;
-- 
2.30.2

[PATCH v3 3/6] serial: 8250: Get preserved flags using serial_lsr_in()

[Ilpo Järvinen]

serial8250_handle_irq() assumes it's the first to read LSR register.
However, there are 8250 drivers which perform LSR read in their own irq
handler prior to calling serial8250_handle_irq(). As not all flags are
preserved across LSR reads, use serial_lsr_in() helper to get all the
preserved flags.

This commit might fix other commits too besides the ones for DW UART
mentioned below. It's just not clear to me which of the other devices
clear some of the LSR flags on read. AFAIK, nobody has complained about
this problem (either against DW or other devices) so it might not have
that bad impact in the end.

Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA flow controlling devices")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index ec5abeb638eb..a0ea048eb2ad 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1916,7 +1916,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 
 	spin_lock_irqsave(&port->lock, flags);
 
-	status = serial_port_in(port, UART_LSR);
+	status = serial_lsr_in(up);
 
 	/*
 	 * If port is stopped and there are no error conditions in the
-- 
2.30.2

[PATCH v3 4/6] serial: 8250: Adjust misleading LSR related comment

[Ilpo Järvinen]

serial8250_rx_chars() has max_count based character limit. If it
triggers, the function returns old LSR value (and it has never returned
only flags which were not handled). Adjust the comment to match
behavior and warn about which flags can be depended on.

While I'd have moved LSR read before LSR read and used serial_lsr_in()
also here but I came across this old discussion about the topic:
  https://www.spinics.net/lists/linux-serial/msg20555.html
...so I left it as it is (it works as long as the callers only use
a subset of the LSR flags which holds true today).

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index a0ea048eb2ad..686891f1b2ca 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1782,9 +1782,12 @@ void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
 EXPORT_SYMBOL_GPL(serial8250_read_char);
 
 /*
- * serial8250_rx_chars: processes according to the passed in LSR
- * value, and returns the remaining LSR bits not handled
- * by this Rx routine.
+ * serial8250_rx_chars: Read characters. The first LSR value must be passed
+ * in.
+ *
+ * Returns LSR bits. The caller should rely only non-rx related LSR bits
+ * (such as THRE) because the LSR value might come from an already consumed
+ * character.
  */
 unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
 {
-- 
2.30.2

Re: [PATCH v3 4/6] serial: 8250: Adjust misleading LSR related comment

[Andy Shevchenko]

On Tue, Jun 7, 2022 at 11:16 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> serial8250_rx_chars() has max_count based character limit. If it
> triggers, the function returns old LSR value (and it has never returned

the old

> only flags which were not handled). Adjust the comment to match
> behavior and warn about which flags can be depended on.
>
> While I'd have moved LSR read before LSR read and used serial_lsr_in()
> also here but I came across this old discussion about the topic:

>   https://www.spinics.net/lists/linux-serial/msg20555.html

Can it be transformed to lore.kernel.org link? and maybe even moved as
BugLink tag?

> ...so I left it as it is (it works as long as the callers only use
> a subset of the LSR flags which holds true today).

With comments addressed,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index a0ea048eb2ad..686891f1b2ca 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1782,9 +1782,12 @@ void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
>  EXPORT_SYMBOL_GPL(serial8250_read_char);
>
>  /*
> - * serial8250_rx_chars: processes according to the passed in LSR
> - * value, and returns the remaining LSR bits not handled
> - * by this Rx routine.
> + * serial8250_rx_chars: Read characters. The first LSR value must be passed
> + * in.

While at it, I would do the following:
1) keep on one line;
2) replace : with -.

The idea behind is to easily convert to kernel doc in the future if
needed, or at least be consistent with kernel doc format.

> + *
> + * Returns LSR bits. The caller should rely only non-rx related LSR bits

rely only on

non-Rx

> + * (such as THRE) because the LSR value might come from an already consumed
> + * character.
>   */


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/6] serial: 8250: Adjust misleading LSR related comment

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1443 bytes --]

On Tue, 7 Jun 2022, Andy Shevchenko wrote:

> On Tue, Jun 7, 2022 at 11:16 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > only flags which were not handled). Adjust the comment to match
> > behavior and warn about which flags can be depended on.
> >
> > While I'd have moved LSR read before LSR read and used serial_lsr_in()
> > also here but I came across this old discussion about the topic:
> 
> >   https://www.spinics.net/lists/linux-serial/msg20555.html
> 
> Can it be transformed to lore.kernel.org link?

Unfortunately no, AFAICT. I tried searching but I couldn't find one 
from there (In fact, I tried even before submitting them because you 
previously told me I should use lore links instead).

More importantly though, it seems that the link was not the one I was 
supposed to use (I probably copy pasted the url from a wrong window while 
investigating all turns of it). This is the correct link:

  https://www.spinics.net/lists/linux-serial/msg16220.html

> and maybe even moved as BugLink tag?

What's BugLink? Our documentation isn't particularly helpful:

$ git grep BugLink
scripts/checkpatch.pl:              $line !~ /^\s*(?:Link|Patchwork|http|https|BugLink|base-commit):/i &&
$

...It seems mostly about bug trackers such as bugzilla, etc. based on git 
log so that tag seems not relevant.

I put the correct URL now into Link tag.

I'll resend tomorrow to give the dust some time to settle.

-- 
 i.

Re: [PATCH v3 4/6] serial: 8250: Adjust misleading LSR related comment

[Andy Shevchenko]

On Tue, Jun 07, 2022 at 03:11:32PM +0300, Ilpo Järvinen wrote:
> On Tue, 7 Jun 2022, Andy Shevchenko wrote:
> > On Tue, Jun 7, 2022 at 11:16 AM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:

...

> > >   https://www.spinics.net/lists/linux-serial/msg20555.html

> > Can it be transformed to lore.kernel.org link?
> 
> Unfortunately no, AFAICT. I tried searching but I couldn't find one
> from there (In fact, I tried even before submitting them because you
> previously told me I should use lore links instead).
> 
> More importantly though, it seems that the link was not the one I was
> supposed to use (I probably copy pasted the url from a wrong window while
> investigating all turns of it). This is the correct link:
> 
>   https://www.spinics.net/lists/linux-serial/msg16220.html

Indeed, that discussion by some reason is not in the lore archive.

...

> I put the correct URL now into Link tag.
> 
> I'll resend tomorrow to give the dust some time to settle.

Fine with me, thanks!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 5/6] serial: 8250_dw: Use serial_lsr_in() in dw8250_handle_irq()

[Ilpo Järvinen]

dw8250_handle_irq() reads LSR under a few conditions, convert both to
use serial_lsr_in() in order to preserve LSR flags properly across
reads.

Cc: Douglas Anderson <dianders@chromium.org>
Cc: Phil Edworthy <phil.edworthy@renesas.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA flow controlling devices")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index f57bbd32ef11..1fae45991812 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -253,7 +253,7 @@ static int dw8250_handle_irq(struct uart_port *p)
 	 */
 	if (!up->dma && rx_timeout) {
 		spin_lock_irqsave(&p->lock, flags);
-		status = p->serial_in(p, UART_LSR);
+		status = serial_lsr_in(up);
 
 		if (!(status & (UART_LSR_DR | UART_LSR_BI)))
 			(void) p->serial_in(p, UART_RX);
@@ -263,7 +263,7 @@ static int dw8250_handle_irq(struct uart_port *p)
 
 	/* Manually stop the Rx DMA transfer when acting as flow controller */
 	if (quirks & DW_UART_QUIRK_IS_DMA_FC && up->dma && up->dma->rx_running && rx_timeout) {
-		status = p->serial_in(p, UART_LSR);
+		status = serial_lsr_in(up);
 		if (status & (UART_LSR_DR | UART_LSR_BI)) {
 			dw8250_writel_ext(p, RZN1_UART_RDMACR, 0);
 			dw8250_writel_ext(p, DW_UART_DMASA, 1);
-- 
2.30.2

[PATCH v3 6/6] serial: 8250_dw: Store LSR into lsr_saved_flags in dw8250_tx_wait_empty()

[Ilpo Järvinen]

Make sure LSR flags are preserved in dw8250_tx_wait_empty(). This
function is called from a low-level out function and therefore cannot
call serial_lsr_in() as it would lead to infinite recursion.

It is borderline if the flags need to be saved here at all since this
code relates to writing LCR register which usually implies no important
characters should be arriving.

Fixes: 914eaf935ec7 ("serial: 8250_dw: Allow TX FIFO to drain before writing to UART_LCR")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 1fae45991812..4cc69bb612ab 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -122,12 +122,15 @@ static void dw8250_check_lcr(struct uart_port *p, int value)
 /* Returns once the transmitter is empty or we run out of retries */
 static void dw8250_tx_wait_empty(struct uart_port *p)
 {
+	struct uart_8250_port *up = up_to_u8250p(p);
 	unsigned int tries = 20000;
 	unsigned int delay_threshold = tries - 1000;
 	unsigned int lsr;
 
 	while (tries--) {
 		lsr = readb (p->membase + (UART_LSR << p->regshift));
+		up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
+
 		if (lsr & UART_LSR_TEMT)
 			break;
 
-- 
2.30.2