On Fri, Dec 20, 2024 at 12:56:31PM +0106, John Ogness wrote:
> On 2024-12-16, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 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
