| Message ID | 20250506112321.61710-2-cuiyunhui@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,1/4] serial: 8250: fix panic due to PSLVERR | expand |
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. > */ > 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); >
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. 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. 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
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 >
Hi Ilpo, On Mon, May 12, 2025 at 6:27 PM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > 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. The purpose of serial_port_in(port, UART_RX) is to clear interrupts such as rx_timeout. In dw8250_handle_irq(), serial_port_in(p, UART_RX) is called when the LSR does not have the UART_LSR_DR bit set. To avoid PSLVERR when the FIFO is enabled, serial_in(up, UART_RX) should be called only when the LSR has the UART_LSR_DR bit set. These two logics are clearly contradictory. Therefore, both serial8250_clear_fifos() and serial_port_in(port, UART_RX) are placed under the protection of port->lock. If you believe this is not a potential issue, that's fine. I can remove this patch in the next patchset version. > > > 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. Thanks, Yunhui
On Mon, 12 May 2025, yunhui cui wrote: > Hi Ilpo, > > On Mon, May 12, 2025 at 6:27 PM Ilpo Järvinen > <ilpo.jarvinen@linux.intel.com> wrote: > > > > 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. > > The purpose of serial_port_in(port, UART_RX) is to clear interrupts > such as rx_timeout. In dw8250_handle_irq(), serial_port_in(p, UART_RX) > is called when the LSR does not have the UART_LSR_DR bit set. > > To avoid PSLVERR when the FIFO is enabled, serial_in(up, UART_RX) > should be called only when the LSR has the UART_LSR_DR bit set. > > These two logics are clearly contradictory. Therefore, both > serial8250_clear_fifos() and serial_port_in(port, UART_RX) are placed > under the protection of port->lock. > > If you believe this is not a potential issue, that's fine. I can > remove this patch in the next patchset version. No, my goal is not to get this removed from the patch series. I meant that the comment wording needs to be fixed for the next version such that it is understandable for those that are not deeply familiar with what is related to PSLVERR. Currently even I struggle to follow what's written into that comment (unless I read heavily between lines and base guesses on the extra knowledge I've about how this entire patchset relates to PSLVERR). > > > 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. > > Thanks, > Yunhui >
Hi Ilpo, On Mon, May 12, 2025 at 8:03 PM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > On Mon, 12 May 2025, yunhui cui wrote: > > > Hi Ilpo, > > > > On Mon, May 12, 2025 at 6:27 PM Ilpo Järvinen > > <ilpo.jarvinen@linux.intel.com> wrote: > > > > > > 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. > > > > The purpose of serial_port_in(port, UART_RX) is to clear interrupts > > such as rx_timeout. In dw8250_handle_irq(), serial_port_in(p, UART_RX) > > is called when the LSR does not have the UART_LSR_DR bit set. > > > > To avoid PSLVERR when the FIFO is enabled, serial_in(up, UART_RX) > > should be called only when the LSR has the UART_LSR_DR bit set. > > > > These two logics are clearly contradictory. Therefore, both > > serial8250_clear_fifos() and serial_port_in(port, UART_RX) are placed > > under the protection of port->lock. > > > > If you believe this is not a potential issue, that's fine. I can > > remove this patch in the next patchset version. > > No, my goal is not to get this removed from the patch series. > > I meant that the comment wording needs to be fixed for the next version > such that it is understandable for those that are not deeply familiar with > what is related to PSLVERR. Currently even I struggle to follow what's > written into that comment (unless I read heavily between lines and base > guesses on the extra knowledge I've about how this entire patchset relates > to PSLVERR). > I plan to change the commit message as follows: When the PSLVERR_RESP_EN parameter is set to 1, reading UART_RX while the FIFO is enabled and UART_LSR_DR is not set will generate a PSLVERR error. 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. PSLVERR is addressed through two methods. One is to introduce serial8250_discard_data() to check whether UART_LSR_DR is set before reading UART_RX, thus solving the PSLVERR issue when the FIFO is enabled. The other is to place FIFO clearing and reading of UART_RX under port->lock. The comment here will be changed as follows: To prevent PSLVERR, we can either disable the FIFO before reading UART_RX or read UART_RX only when UART_LSR_DR is set while the FIFO remains enabled. If using the latter approach to avoid PSLVERR, it creates a contradiction with the interrupt - clearing (see the rx_timeout handling in dw8250_handle_irq()). What do you think? > > > > 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. > > > > Thanks, > > Yunhui > > > > -- > i. Thanks, Yunhui
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; } - 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. */ 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);
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. 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(-)