Re: [External] Re: [PATCH v5 2/4] serial: 8250: avoid potential PSLVERR issue

Ilpo Järvinen
2025-05-12T10:27:22Z

[-- Attachment #1: Type: text/plain, Size: 7890 bytes --]

On Mon, 12 May 2025, yunhui cui wrote:

> Hi Ilpo,
>
> On Tue, May 6, 2025 at 8:00 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Tue, 6 May 2025, Yunhui Cui wrote:
> >
> > > Failure to check the UART_LSR_DR before reading UART_RX, or the
> > > non-atomic nature of clearing the FIFO and reading UART_RX, poses
> > > potential risks that could lead to PSLVERR.
> >
> > Don't expect reader to know the condition how PSLVERR is triggered. I know
> > it's worded out in the other patch but also explain it here.
> >
> > You're only explaining problem and missing what this patch does to solve
> > the problem.
> >
> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > ---
> > > drivers/tty/serial/8250/8250.h | 13 +++++++++
> > > drivers/tty/serial/8250/8250_port.c | 43 +++++++++++++++--------------
> > > 2 files changed, 35 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > > index b861585ca02a..6f97ff3a197d 100644
> > > --- a/drivers/tty/serial/8250/8250.h
> > > +++ b/drivers/tty/serial/8250/8250.h
> > > @@ -162,6 +162,19 @@ static inline u16 serial_lsr_in(struct uart_8250_port *up)
> > > return lsr;
> > > }
> > >
> > > +/*
> > > + * To avoid PSLVERR, check UART_LSR_DR in UART_LSR before
> > > + * reading UART_RX.
> > > + */
> > > +static inline void serial8250_discard_data(struct uart_8250_port *up)
> > > +{
> > > + u16 lsr;
> > > +
> > > + lsr = serial_in(up, UART_LSR);
> > > + if (lsr & UART_LSR_DR)
> > > + serial_in(up, UART_RX);
> > > +}
> > > +
> > > /*
> > > * For the 16C950
> > > */
> > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > > index a913135d5217..1666b965f6a0 100644
> > > --- a/drivers/tty/serial/8250/8250_port.c
> > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > @@ -1357,9 +1357,8 @@ static void autoconfig_irq(struct uart_8250_port *up)
> > > /* Synchronize UART_IER access against the console. */
> > > uart_port_lock_irq(port);
> > > serial_out(up, UART_IER, UART_IER_ALL_INTR);
> > > + serial8250_discard_data(up);
> > > uart_port_unlock_irq(port);
> > > - serial_in(up, UART_LSR);
> > > - serial_in(up, UART_RX);
> > > serial_in(up, UART_IIR);
> > > serial_in(up, UART_MSR);
> > > serial_out(up, UART_TX, 0xFF);
> > > @@ -2137,25 +2136,22 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
> > > static int serial8250_get_poll_char(struct uart_port *port)
> > > {
> > > struct uart_8250_port *up = up_to_u8250p(port);
> > > - int status;
> > > + int status = NO_POLL_CHAR;
> > > u16 lsr;
> > > + unsigned long flags;
> > >
> > > serial8250_rpm_get(up);
> > >
> > > + uart_port_lock_irqsave(port, &flags);
> > > lsr = serial_port_in(port, UART_LSR);
> > > + if (lsr & UART_LSR_DR)
> > > + status = serial_port_in(port, UART_RX);
> > > + uart_port_unlock_irqrestore(port, flags);
> > >
> > > - if (!(lsr & UART_LSR_DR)) {
> > > - status = NO_POLL_CHAR;
> > > - goto out;
> > > - }
> > > -
> > > - status = serial_port_in(port, UART_RX);
> > > -out:
> > > serial8250_rpm_put(up);
> > > return status;
> >
> > Not a problem that originates from you, but IMO calling this variable
> > "status" is quite misleading when it is the character (or NO_POLL_CHAR
> > is no character is present).
> >
> > > }
> > >
> > > -
> > > static void serial8250_put_poll_char(struct uart_port *port,
> > > unsigned char c)
> > > {
> > > @@ -2264,13 +2260,17 @@ int serial8250_do_startup(struct uart_port *port)
> > > * Clear the FIFO buffers and disable them.
> > > * (they will be reenabled in set_termios())
> > > */
> > > + uart_port_lock_irqsave(port, &flags);
> > > serial8250_clear_fifos(up);
> > >
> > > /*
> > > - * Clear the interrupt registers.
> > > + * Read UART_RX to clear interrupts (e.g., Character Timeout).
> > > + * No data on UART_IIR_RX_TIMEOUT, UART_LSR_DR won't set.
> > > + * FIFO disabled, read UART_RX without LSR check, no PSLVERR.
> >
> > I don't understand what the last two lines mean and I don't see the
> > connection to the code that is below the comment either, could you try to
> > rephrase the comment.
>
> The original intention was to check UART_LSR_DR first when reading
> UART_RX. However, the purpose of serial_port_in(port, UART_RX) here is
> to clear the interrupt, such as the interrupt caused by RX_TIMEOUT.

I understood the first sentence in the comment but the rest of it is very
cryptic and has many grammar issues too. Also, the extent of passive voice
there makes it hard to know who does what (UART / kernel).

> The logic for clearing the interrupt in the interrupt handling
> function of RX_TIMEOUT is !UART_LSR_DR. And to avoid PSLVERR, we need
> to check UART_LSR_DR first. To meet the requirements of both, the FIFO
> needs to be disabled.

The grammar is so broken, it failed to convey that message.

> Therefore, we should put serial8250_clear_fifos() and the execution of
> serial_port_in(port, UART_RX) without checking UART_LSR_DR under the
> port->lock.
>
> >
> > > */
> > > serial_port_in(port, UART_LSR);
> > > serial_port_in(port, UART_RX);
> > > + uart_port_unlock_irqrestore(port, flags);
> > > serial_port_in(port, UART_IIR);
> > > serial_port_in(port, UART_MSR);
> > >
> > > @@ -2429,15 +2429,14 @@ int serial8250_do_startup(struct uart_port *port)
> > > }
> > >
> > > dont_test_tx_en:
> > > - uart_port_unlock_irqrestore(port, flags);
> > >
> > > /*
> > > * Clear the interrupt registers again for luck, and clear the
> > > * saved flags to avoid getting false values from polling
> > > * routines or the previous session.
> > > */
> > > - serial_port_in(port, UART_LSR);
> > > - serial_port_in(port, UART_RX);
> > > + serial8250_discard_data(up);
> > > + uart_port_unlock_irqrestore(port, flags);
> > > serial_port_in(port, UART_IIR);
> > > serial_port_in(port, UART_MSR);
> > > up->lsr_saved_flags = 0;
> > > @@ -2519,7 +2518,6 @@ void serial8250_do_shutdown(struct uart_port *port)
> > > port->mctrl &= ~TIOCM_OUT2;
> > >
> > > serial8250_set_mctrl(port, port->mctrl);
> > > - uart_port_unlock_irqrestore(port, flags);
> > >
> > > /*
> > > * Disable break condition and FIFOs
> > > @@ -2527,6 +2525,14 @@ void serial8250_do_shutdown(struct uart_port *port)
> > > serial_port_out(port, UART_LCR,
> > > serial_port_in(port, UART_LCR) & ~UART_LCR_SBC);
> > > serial8250_clear_fifos(up);
> > > + /*
> > > + * Read data port to reset things, and then unlink from
> > > + * the IRQ chain.
> > > + * Since reading UART_RX clears interrupts, doing so with
> > > + * FIFO disabled won't trigger PSLVERR.
> > > + */
> > > + serial_port_in(port, UART_RX);
> > > + uart_port_unlock_irqrestore(port, flags);
> > >
> > > #ifdef CONFIG_SERIAL_8250_RSA
> > > /*
> > > @@ -2535,11 +2541,6 @@ void serial8250_do_shutdown(struct uart_port *port)
> > > disable_rsa(up);
> > > #endif
> > >
> > > - /*
> > > - * Read data port to reset things, and then unlink from
> > > - * the IRQ chain.
> > > - */
> > > - serial_port_in(port, UART_RX);
> > > serial8250_rpm_put(up);
> > >
> > > up->ops->release_irq(up);
> > >
> >
> > --
> > i.
> >
>
> Thanks,
> Yunhui
>

--
i.