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 for Android: free password hash cracker in your pocket

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

Message-ID: <Z2raSoNcj0xlbiWF@smile.fi.intel.com>
Date: Tue, 24 Dec 2024 17:59:06 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: John Ogness <john.ogness@...utronix.de>
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 Fri, Dec 20, 2024 at 12:56:31PM +0106, John Ogness wrote:
> 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...

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