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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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