diff mbox series

[v4,2/4] serial: 8250: introduce serial8250_discard_data()

Message ID 20250425062425.68761-2-cuiyunhui@bytedance.com
State New
Headers show
Series None | expand

Commit Message

yunhui cui April 25, 2025, 6:24 a.m. UTC 
To prevent triggering PSLVERR, it is necessary to check whether the
UART_LSR_DR bit of UART_LSR is set before reading UART_RX.
Ensure atomicity of UART_LSR and UART_RX, put serial8250_discard_data()
under port->lock.

Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
 drivers/tty/serial/8250/8250.h      | 15 +++++++++++
 drivers/tty/serial/8250/8250_port.c | 42 ++++++++++++++---------------
 2 files changed, 36 insertions(+), 21 deletions(-)

Comments

yunhui cui May 6, 2025, 10:38 a.m. UTC | #1 
Hi js,

On Fri, Apr 25, 2025 at 2:49 PM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 25. 04. 25, 8:24, Yunhui Cui wrote:
> > To prevent triggering PSLVERR, it is necessary to check whether the
> > UART_LSR_DR bit of UART_LSR is set before reading UART_RX.
> > Ensure atomicity of UART_LSR and UART_RX, put serial8250_discard_data()
> > under port->lock.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > ---
> >   drivers/tty/serial/8250/8250.h      | 15 +++++++++++
> >   drivers/tty/serial/8250/8250_port.c | 42 ++++++++++++++---------------
> >   2 files changed, 36 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > index b861585ca02a..5a106cf88207 100644
> > --- a/drivers/tty/serial/8250/8250.h
> > +++ b/drivers/tty/serial/8250/8250.h
> > @@ -162,6 +162,21 @@ 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 unsigned int serial8250_discard_data(struct uart_8250_port *up)
> > +{
> > +     u16 lsr;
>
> Is lsr really 16-bit?

The data book I have shows a 32 - bit width, but actually only the
lower 8 bits are valid. Using u16 is fine, and the current code all
uses u16 lsr.

>
> > +
> > +     lsr = serial_in(up, UART_LSR);
> > +     if (lsr & UART_LSR_DR)
> > +             return serial_in(up, UART_RX);
>
> Why does a discard function return a value at all?

Update in next version.

>
> > +
> > +     return 0;
> > +}
> > +
> >   /*
> >    * For the 16C950
> >    */
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index a913135d5217..802ac50357c0 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> ...
> > @@ -2137,25 +2136,21 @@ 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;
> >
> >       serial8250_rpm_get(up);
> >
> > +     uart_port_lock_irqsave(port, &flags);
> >       lsr = serial_port_in(port, UART_LSR);
> > -
> > -     if (!(lsr & UART_LSR_DR)) {
> > -             status = NO_POLL_CHAR;
> > -             goto out;
> > -     }
> > -
> > -     status = serial_port_in(port, UART_RX);
> > +     if ((lsr & UART_LSR_DR))
>
> Too many parentheses.

Okay.

>
> > +             status = serial_port_in(port, UART_RX);
> > +     uart_port_unlock_irqrestore(port, flags);
>
> thanks,
> --
> js
> suse labs

Thanks,
Yunhui
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index b861585ca02a..5a106cf88207 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -162,6 +162,21 @@  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 unsigned int serial8250_discard_data(struct uart_8250_port *up)
+{
+	u16 lsr;
+
+	lsr = serial_in(up, UART_LSR);
+	if (lsr & UART_LSR_DR)
+		return serial_in(up, UART_RX);
+
+	return 0;
+}
+
 /*
  * For the 16C950
  */
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index a913135d5217..802ac50357c0 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,21 @@  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;
 
 	serial8250_rpm_get(up);
 
+	uart_port_lock_irqsave(port, &flags);
 	lsr = serial_port_in(port, UART_LSR);
-
-	if (!(lsr & UART_LSR_DR)) {
-		status = NO_POLL_CHAR;
-		goto out;
-	}
-
-	status = serial_port_in(port, UART_RX);
+	if ((lsr & UART_LSR_DR))
+		status = serial_port_in(port, UART_RX);
+	uart_port_unlock_irqrestore(port, flags);
 out:
 	serial8250_rpm_put(up);
 	return status;
 }
 
-
 static void serial8250_put_poll_char(struct uart_port *port,
 			 unsigned char c)
 {
@@ -2264,13 +2259,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.
 	 */
 	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 +2428,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 +2517,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 +2524,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 +2540,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);