[PATCH tty-next v1 0/4] serial: 8250: Fix LSR masking

[PATCH tty-next v1 0/4] serial: 8250: Fix LSR masking

[John Ogness]

Hi,

During the review process of my 8250 nbcon series, Petr Mladek
mentioned [0] that it was odd that the console driver clears
UART_LSR_DR from @read_status_mask but never sets it.

Since there is literally zero documentation on the
driver-specific fields @read_status_mask and
@ignore_status_mask, I embarked on a journey to figure out what
these fields are for and how they are supposed to be used.

My quest took me back to Linux 1.1.60, where there was the
first significant change in the purpose of these fields. That
purpose was then reverted in Linux 2.1.8, but some of the
pieces were forgotten. Over the years it seems no one really
noticed as these bogus pieces hung around and were even
expanded upon.

And yes, I uncovered a subtle bug that has been around longer
than git.

This series cleans up the usage for the @read_status_mask field
and adds some documentation so that future developers will know
what this field is actually for. And the series also fixes the
subtle bug.

Note that since the 8250 was the original serial driver and was
copy/pasted as a basis for many later serial drivers, the issue
may exist in other drivers as well.

[0] https://lore.kernel.org/lkml/ZyuOX4VVbfAFhMfV@pathway.suse.cz

John Ogness (4):
  serial: 8250: Use @ier bits to determine if Rx is stopped
  serial: 8250: Do not set UART_LSR_THRE in @read_status_mask
  serial: 8250: Never adjust UART_LSR_DR in @read_status_mask
  serial: 8250: Explain the role of @read_status_mask

 drivers/tty/serial/8250/8250_core.c |  1 -
 drivers/tty/serial/8250/8250_omap.c |  9 +++++++--
 drivers/tty/serial/8250/8250_port.c | 11 ++++++++---
 3 files changed, 15 insertions(+), 6 deletions(-)


base-commit: 30691a59c85c48575b04e849f675660fd8060cad
-- 
2.39.5

[PATCH tty-next v1 1/4] serial: 8250: Use @ier bits to determine if Rx is stopped

[John Ogness]

Commit f19c3f6c8109 ("serial: 8250_port: Don't service RX FIFO if
throttled") uses @read_status_mask (bit UART_LSR_DR) to determine
if Rx has been stopped. However, the bit UART_LSR_DR is not
managed properly in @read_status_mask for all Rx stop/start
situations and is therefore not suitable for this purpose.

Use the UART_IER_RLSI and UART_IER_RDI bits in @ier instead, as
this is already common in 8250-variants and drivers.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 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 1ea52fce9bf1..4d742bfb7e9a 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1931,7 +1931,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 	 */
 	if (!(status & (UART_LSR_FIFOE | UART_LSR_BRK_ERROR_BITS)) &&
 	    (port->status & (UPSTAT_AUTOCTS | UPSTAT_AUTORTS)) &&
-	    !(port->read_status_mask & UART_LSR_DR))
+	    !(up->ier & (UART_IER_RLSI | UART_IER_RDI)))
 		skip_rx = true;
 
 	if (status & (UART_LSR_DR | UART_LSR_BI) && !skip_rx) {
-- 
2.39.5

Re: [PATCH tty-next v1 1/4] serial: 8250: Use @ier bits to determine if Rx is stopped

[Andy Shevchenko]

On Mon, Dec 16, 2024 at 06:18:41PM +0106, John Ogness wrote:
> Commit f19c3f6c8109 ("serial: 8250_port: Don't service RX FIFO if
> throttled") uses @read_status_mask (bit UART_LSR_DR) to determine
> if Rx has been stopped. However, the bit UART_LSR_DR is not
> managed properly in @read_status_mask for all Rx stop/start
> situations and is therefore not suitable for this purpose.
> 
> Use the UART_IER_RLSI and UART_IER_RDI bits in @ier instead, as
> this is already common in 8250-variants and drivers.

Hmm... IER is Interrupt Enable Register, so it has been programmed to the value
we control, on the opposite the LSR is Line Status Register and defines status
on the line at the moment of reading. Can you elaborate how your change is correct
substitute?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH tty-next v1 1/4] serial: 8250: Use @ier bits to determine if Rx is stopped

[Andy Shevchenko]

On Mon, Dec 16, 2024 at 10:31:06PM +0200, Andy Shevchenko wrote:
> On Mon, Dec 16, 2024 at 06:18:41PM +0106, John Ogness wrote:
> > Commit f19c3f6c8109 ("serial: 8250_port: Don't service RX FIFO if
> > throttled") uses @read_status_mask (bit UART_LSR_DR) to determine
> > if Rx has been stopped. However, the bit UART_LSR_DR is not
> > managed properly in @read_status_mask for all Rx stop/start
> > situations and is therefore not suitable for this purpose.
> > 
> > Use the UART_IER_RLSI and UART_IER_RDI bits in @ier instead, as
> > this is already common in 8250-variants and drivers.
> 
> Hmm... IER is Interrupt Enable Register, so it has been programmed to the value
> we control, on the opposite the LSR is Line Status Register and defines status
> on the line at the moment of reading. Can you elaborate how your change is correct
> substitute?

Additionally the common IRQ handler may be called at last in the custom ones
and hence potentially the value of saved IER might be different to what the
actual register is programmed to.

Besides that I don't remember all of the mysterious ways of DMA. May it affect
the value of IER and when we swtich from DMA to PIO and vice versa we get an
incorrect value in the saved variable?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH tty-next v1 1/4] serial: 8250: Use @ier bits to determine if Rx is stopped

[John Ogness]

On 2024-12-16, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>> > Use the UART_IER_RLSI and UART_IER_RDI bits in @ier instead, as
>> > this is already common in 8250-variants and drivers.
>> 
>> Hmm... IER is Interrupt Enable Register, so it has been programmed to the value
>> we control, on the opposite the LSR is Line Status Register and defines status
>> on the line at the moment of reading. Can you elaborate how your change is correct
>> substitute?

The change subsitutes @ier usage for @read_status_mask usage. Both are
programmed values that we control. Note that the code being replaced
does _not_ care about the Line Status Register. It is only looking at
the bit in the mask.

Everywhere that UART_LSR_DR is set/cleared in @read_status_mask,
UART_IER_RLSI and UART_IER_RDI are also set/cleared in @ier.

Also, there are plenty of examples where the RLSI and RDI bits of @ier
are used to determine if Rx is enabled. Here are the examples from the
8250 variants...

In fsl8250_handle_irq() we see that the @ier condition is used for
deciding if Rx is enabled:

        /* Process incoming characters first */
        if ((lsr & (UART_LSR_DR | UART_LSR_BI)) &&
            (up->ier & (UART_IER_RLSI | UART_IER_RDI))) {
                lsr = serial8250_rx_chars(up, lsr);
        }

Ditto for brcmuart_hrtimer_func():

        /* re-enable receive unless upper layer has disabled it */
        if ((up->ier & (UART_IER_RLSI | UART_IER_RDI)) ==
            (UART_IER_RLSI | UART_IER_RDI)) {

Ditto for fsl8250_handle_irq():

                if (up->ier & (UART_IER_RLSI | UART_IER_RDI)) {
                        port->ops->stop_rx(port);

Ditto for omap8250_irq():

                if (up->ier & (UART_IER_RLSI | UART_IER_RDI)) {
                        port->ops->stop_rx(port);

> Additionally the common IRQ handler may be called at last in the custom ones
> and hence potentially the value of saved IER might be different to what the
> actual register is programmed to.

There is only 1 place where they do not match: serial8250_do_startup()

        /*
         * Set the IER shadow for rx interrupts but defer actual interrupt
         * enable until after the FIFOs are enabled; otherwise, an already-
         * active sender can swamp the interrupt handler with "too much work".
         */
        up->ier = UART_IER_RLSI | UART_IER_RDI;

The IER hardware register contains 0 here.

This comes from commit ee3ad90be5ec ("serial: 8250: Defer interrupt
enable until fifos enabled").

But since IER is 0, there will be no interrupt to land in any handlers
leading to serial8250_handle_irq().

> Besides that I don't remember all of the mysterious ways of DMA. May it affect
> the value of IER and when we swtich from DMA to PIO and vice versa we get an
> incorrect value in the saved variable?

The change being made in this patch is only related to the Rx FIFO
throttling when hardware flow control is enabled. The "feature" was
added by commit f19c3f6c810 ("serial: 8250_port: Don't service RX FIFO
if throttled").

For the omap variant this worked because the omap variant also updates
the @read_status_mask when unthrottling (no other variant does that).

What confuses me is in 8250_omap.c:__dma_rx_complete() where there is:

        __dma_rx_do_complete(p);
        if (!priv->throttled) {
                p->ier |= UART_IER_RLSI | UART_IER_RDI;
                serial_out(p, UART_IER, p->ier);
                if (!(priv->habit & UART_HAS_EFR2))
                        omap_8250_rx_dma(p);
        }

I would expect to see:

        __dma_rx_do_complete(p);
        if (!priv->throttled) {
                p->ier |= UART_IER_RLSI | UART_IER_RDI;
                p->port.read_status_mask |= UART_LSR_DR;
                serial_out(p, UART_IER, p->ier);
        }

But perhaps that is a bug. In fact, it would be exactly the bug that
this patch is fixing because there are many places where
@read_status_mask does not mirror Rx enable/disable status (because that
is not the correct use of @read_status_mask).

John

Re: [PATCH tty-next v1 1/4] serial: 8250: Use @ier bits to determine if Rx is stopped

[Andy Shevchenko]

On Fri, Dec 20, 2024 at 12:56:31PM +0106, John Ogness wrote:
> On 2024-12-16, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >> > Use the UART_IER_RLSI and UART_IER_RDI bits in @ier instead, as
> >> > this is already common in 8250-variants and drivers.
> >> 
> >> Hmm... IER is Interrupt Enable Register, so it has been programmed to the value
> >> we control, on the opposite the LSR is Line Status Register and defines status
> >> on the line at the moment of reading. Can you elaborate how your change is correct
> >> substitute?
> 
> The change subsitutes @ier usage for @read_status_mask usage. Both are
> programmed values that we control. Note that the code being replaced
> does _not_ care about the Line Status Register. It is only looking at
> the bit in the mask.
> 
> Everywhere that UART_LSR_DR is set/cleared in @read_status_mask,
> UART_IER_RLSI and UART_IER_RDI are also set/cleared in @ier.
> 
> Also, there are plenty of examples where the RLSI and RDI bits of @ier
> are used to determine if Rx is enabled. Here are the examples from the
> 8250 variants...

Okay, so perhaps a small summary to the commit message that both values are of
our control and hence there is no real event parsing is done in the original
case.

...

> > Additionally the common IRQ handler may be called at last in the custom ones
> > and hence potentially the value of saved IER might be different to what the
> > actual register is programmed to.
> 
> There is only 1 place where they do not match: serial8250_do_startup()
> 
>         /*
>          * Set the IER shadow for rx interrupts but defer actual interrupt
>          * enable until after the FIFOs are enabled; otherwise, an already-
>          * active sender can swamp the interrupt handler with "too much work".
>          */
>         up->ier = UART_IER_RLSI | UART_IER_RDI;
> 
> The IER hardware register contains 0 here.
> 
> This comes from commit ee3ad90be5ec ("serial: 8250: Defer interrupt
> enable until fifos enabled").
> 
> But since IER is 0, there will be no interrupt to land in any handlers
> leading to serial8250_handle_irq().

It's still possible to get into the handler, note, we may be working with
shared IRQs. There is a bit 0 in IIR that has to be checked to see if the
interrupt from the UART in question or something else.

...

> > Besides that I don't remember all of the mysterious ways of DMA. May it affect
> > the value of IER and when we swtich from DMA to PIO and vice versa we get an
> > incorrect value in the saved variable?
> 
> The change being made in this patch is only related to the Rx FIFO
> throttling when hardware flow control is enabled. The "feature" was
> added by commit f19c3f6c810 ("serial: 8250_port: Don't service RX FIFO
> if throttled").
> 
> For the omap variant this worked because the omap variant also updates
> the @read_status_mask when unthrottling (no other variant does that).
> 
> What confuses me is in 8250_omap.c:__dma_rx_complete() where there is:
> 
>         __dma_rx_do_complete(p);
>         if (!priv->throttled) {
>                 p->ier |= UART_IER_RLSI | UART_IER_RDI;
>                 serial_out(p, UART_IER, p->ier);
>                 if (!(priv->habit & UART_HAS_EFR2))
>                         omap_8250_rx_dma(p);
>         }
> 
> I would expect to see:
> 
>         __dma_rx_do_complete(p);
>         if (!priv->throttled) {
>                 p->ier |= UART_IER_RLSI | UART_IER_RDI;
>                 p->port.read_status_mask |= UART_LSR_DR;
>                 serial_out(p, UART_IER, p->ier);
>         }
> 
> But perhaps that is a bug. In fact, it would be exactly the bug that
> this patch is fixing because there are many places where
> @read_status_mask does not mirror Rx enable/disable status (because that
> is not the correct use of @read_status_mask).

Yeah, it may be that somebody (Tony?) cam shed a bit of light here...

-- 
With Best Regards,
Andy Shevchenko

[PATCH tty-next v1 2/4] serial: 8250: Do not set UART_LSR_THRE in @read_status_mask

[John Ogness]

Since Linux 2.1.8 @read_status_mask is no longer used as a
general control of which bits are used from the LSR register.
Instead it has become an additional mask applied to
@ignore_status_mask. Since UART_LSR_THRE is never set for
@ignore_status_mask, it serves no purpose to set it for
@read_status_mask. In fact, it propagates the misconception
that @read_status_mask can be used as a general mask for LSR
bits.

Do not set UART_LSR_THRE for @read_status_mask.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c | 2 +-
 drivers/tty/serial/8250/8250_port.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 9eb9aa766811..5290aed76a5e 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -412,7 +412,7 @@ static void omap_8250_set_termios(struct uart_port *port,
 	 */
 	uart_update_timeout(port, termios->c_cflag, baud);
 
-	up->port.read_status_mask = UART_LSR_OE | UART_LSR_THRE | UART_LSR_DR;
+	up->port.read_status_mask = UART_LSR_OE | UART_LSR_DR;
 	if (termios->c_iflag & INPCK)
 		up->port.read_status_mask |= UART_LSR_FE | UART_LSR_PE;
 	if (termios->c_iflag & (IGNBRK | PARMRK))
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 4d742bfb7e9a..a5c1b069c67b 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2786,7 +2786,7 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	 */
 	uart_update_timeout(port, termios->c_cflag, baud);
 
-	port->read_status_mask = UART_LSR_OE | UART_LSR_THRE | UART_LSR_DR;
+	port->read_status_mask = UART_LSR_OE | UART_LSR_DR;
 	if (termios->c_iflag & INPCK)
 		port->read_status_mask |= UART_LSR_FE | UART_LSR_PE;
 	if (termios->c_iflag & (IGNBRK | BRKINT | PARMRK))
-- 
2.39.5

Re: [PATCH tty-next v1 2/4] serial: 8250: Do not set UART_LSR_THRE in @read_status_mask

[Andy Shevchenko]

On Mon, Dec 16, 2024 at 06:18:42PM +0106, John Ogness wrote:
> Since Linux 2.1.8 @read_status_mask is no longer used as a
> general control of which bits are used from the LSR register.

The curious ones may add history/history.git to their repo and run
`git show 2.1.8 -- drivers/char/serial.c` to see how it was done.

> Instead it has become an additional mask applied to
> @ignore_status_mask. Since UART_LSR_THRE is never set for
> @ignore_status_mask, it serves no purpose to set it for
> @read_status_mask. In fact, it propagates the misconception
> that @read_status_mask can be used as a general mask for LSR
> bits.

-- 
With Best Regards,
Andy Shevchenko

[PATCH tty-next v1 3/4] serial: 8250: Never adjust UART_LSR_DR in @read_status_mask

[John Ogness]

The CREAD feature of termios is implemented by setting/clearing
UART_LSR_DR within @ignore_status_mask. For this feature to
function correctly, it requires that UART_LSR_DR is never
masked (unset) in @read_status_mask so that uart_insert_char()
can properly drop the character if CREAD is disabled.

Currently there are code paths that clear/set UART_LSR_DR from
@read_status_mask at times. This appears to be a relic from
Linux 1.1.60, where @read_status_mask could be used to mask
all UART_LSR reading. However, since Linux 2.1.8 that is no
longer the case. Now if UART_LSR_DR is cleared from
@read_status_mask, received characters may not be dropped
even though CREAD is disabled.

This can be seen when:

- CREAD is disabled (UART_LSR_DR is set in @ignore_status_mask)
- LSR has an error bit set from UART_LSR_BRK_ERROR_BITS
  (and that error is not ignored via @ignore_status_mask)
- UART_LSR_DR is cleared in @read_status_mask

In this case characters will be inserted into the tty buffer
even though they should be ignored.

Remove all setting/clearing of UART_LSR_DR for
@read_status_mask except for its initialization.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_core.c | 1 -
 drivers/tty/serial/8250/8250_omap.c | 1 -
 drivers/tty/serial/8250/8250_port.c | 1 -
 3 files changed, 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 5f9f06911795..2b70e82dffeb 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -675,7 +675,6 @@ static void serial_8250_overrun_backoff_work(struct work_struct *work)
 
 	uart_port_lock_irqsave(port, &flags);
 	up->ier |= UART_IER_RLSI | UART_IER_RDI;
-	up->port.read_status_mask |= UART_LSR_DR;
 	serial_out(up, UART_IER, up->ier);
 	uart_port_unlock_irqrestore(port, flags);
 }
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 5290aed76a5e..10144fcc0363 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -838,7 +838,6 @@ static void omap_8250_unthrottle(struct uart_port *port)
 	if (up->dma)
 		up->dma->rx_dma(up);
 	up->ier |= UART_IER_RLSI | UART_IER_RDI;
-	port->read_status_mask |= UART_LSR_DR;
 	serial_out(up, UART_IER, up->ier);
 	uart_port_unlock_irqrestore(port, flags);
 
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index a5c1b069c67b..3b9dc2bb06eb 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1390,7 +1390,6 @@ static void serial8250_stop_rx(struct uart_port *port)
 	serial8250_rpm_get(up);
 
 	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
-	up->port.read_status_mask &= ~UART_LSR_DR;
 	serial_port_out(port, UART_IER, up->ier);
 
 	serial8250_rpm_put(up);
-- 
2.39.5

[PATCH tty-next v1 4/4] serial: 8250: Explain the role of @read_status_mask

[John Ogness]

The role of @read_status_mask has changed over time and seems
to still cause confusion. This can be expected since there is
zero documentation about this driver-specific variable.

Add comments to the initialization of @read_status_mask to
clarify its role.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c | 6 ++++++
 drivers/tty/serial/8250/8250_port.c | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 10144fcc0363..42b4aa56b902 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -412,6 +412,12 @@ static void omap_8250_set_termios(struct uart_port *port,
 	 */
 	uart_update_timeout(port, termios->c_cflag, baud);
 
+	/*
+	 * Specify which conditions may be considered for error
+	 * handling and the ignoring of characters. The actual
+	 * ignoring of characters only occurs if the bit is set
+	 * in @ignore_status_mask as well.
+	 */
 	up->port.read_status_mask = UART_LSR_OE | UART_LSR_DR;
 	if (termios->c_iflag & INPCK)
 		up->port.read_status_mask |= UART_LSR_FE | UART_LSR_PE;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3b9dc2bb06eb..1a65d3e5f3c0 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2785,6 +2785,12 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	 */
 	uart_update_timeout(port, termios->c_cflag, baud);
 
+	/*
+	 * Specify which conditions may be considered for error
+	 * handling and the ignoring of characters. The actual
+	 * ignoring of characters only occurs if the bit is set
+	 * in @ignore_status_mask as well.
+	 */
 	port->read_status_mask = UART_LSR_OE | UART_LSR_DR;
 	if (termios->c_iflag & INPCK)
 		port->read_status_mask |= UART_LSR_FE | UART_LSR_PE;
-- 
2.39.5