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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <84ed22h8u0.fsf@jogness.linutronix.de>
Date: Fri, 20 Dec 2024 12:56:31 +0106
From: John Ogness <john.ogness@...utronix.de>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby
 <jirislaby@...nel.org>, Esben Haabendal <esben@...nix.com>,
 linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org, Arnd Bergmann
 <arnd@...db.de>, Rengarajan S <rengarajan.s@...rochip.com>, Bart Van
 Assche <bvanassche@....org>, Niklas Schnelle <schnelle@...ux.ibm.com>,
 Serge Semin <fancer.lancer@...il.com>, Lino Sanfilippo
 <l.sanfilippo@...bus.com>, Peter Collingbourne <pcc@...gle.com>
Subject: Re: [PATCH tty-next v1 1/4] serial: 8250: Use @ier bits to
 determine if Rx is stopped

On 2024-12-16, Andy Shevchenko <andriy.shevchenko@...ux.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