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

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

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?

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?

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