2022-07-13 13:20:24

by VAMSHI GAJJELA

Subject: [PATCH v3] serial: 8250_dw: Avoid pslverr on reading empty receiver fifo

From: VAMSHI GAJJELA <[email protected]>

With PSLVERR_RESP_EN parameter set to 1, the device generates an error
response when an attempt to read an empty RBR with FIFO enabled.

This happens when LCR writes are ignored when UART is busy.
dw8250_check_lcr() in retries to update LCR, invokes dw8250_force_idle()
to clear and reset FIFO and eventually reads UART_RX causing the error.

Avoid this by not reading RBR/UART_RX when no data is available.

Signed-off-by: VAMSHI GAJJELA <[email protected]>
---
v3:
- check lsr based on FIFO enablement
v2:
- update as per review comments (re-format comments, xmas tree ordering)
drivers/tty/serial/8250/8250_dw.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index f57bbd32ef11..7573904579f6 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -82,8 +82,21 @@ static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value)
static void dw8250_force_idle(struct uart_port *p)
{
struct uart_8250_port *up = up_to_u8250p(p);
+ unsigned int lsr;

serial8250_clear_and_reinit_fifos(up);
+
+ /*
+ * With PSLVERR_RESP_EN parameter set to 1, the device generates an
+ * error response when an attempt to read an empty RBR with FIFO
+ * enabled.
+ */
+ if (up->fcr & UART_FCR_ENABLE_FIFO) {
+ lsr = p->serial_in(p, UART_LSR);
+ if (!(lsr & UART_LSR_DR))
+ return;
+ }
+
(void)p->serial_in(p, UART_RX);
}

--
2.37.0.144.g8ac04bfd2-goog

2022-07-13 16:54:04

by Andy Shevchenko

Subject: Re: [PATCH v3] serial: 8250_dw: Avoid pslverr on reading empty receiver fifo

On Wed, Jul 13, 2022 at 06:47:22PM +0530, Vamshi Gajjela wrote:
> From: VAMSHI GAJJELA <[email protected]>
>
> With PSLVERR_RESP_EN parameter set to 1, the device generates an error
> response when an attempt to read an empty RBR with FIFO enabled.
>
> This happens when LCR writes are ignored when UART is busy.
> dw8250_check_lcr() in retries to update LCR, invokes dw8250_force_idle()
> to clear and reset FIFO and eventually reads UART_RX causing the error.
>
> Avoid this by not reading RBR/UART_RX when no data is available.

From code perspective looks good,
Reviewed-by: Andy Shevchenko <[email protected]>

but maybe Ilpo or others have something to add based on the practical use.

> Signed-off-by: VAMSHI GAJJELA <[email protected]>
> ---
> v3:
> - check lsr based on FIFO enablement
> v2:
> - update as per review comments (re-format comments, xmas tree ordering)
> drivers/tty/serial/8250/8250_dw.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index f57bbd32ef11..7573904579f6 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -82,8 +82,21 @@ static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value)
> static void dw8250_force_idle(struct uart_port *p)
> {
> struct uart_8250_port *up = up_to_u8250p(p);
> + unsigned int lsr;
>
> serial8250_clear_and_reinit_fifos(up);
> +
> + /*
> + * With PSLVERR_RESP_EN parameter set to 1, the device generates an
> + * error response when an attempt to read an empty RBR with FIFO
> + * enabled.
> + */
> + if (up->fcr & UART_FCR_ENABLE_FIFO) {
> + lsr = p->serial_in(p, UART_LSR);
> + if (!(lsr & UART_LSR_DR))
> + return;
> + }
> +
> (void)p->serial_in(p, UART_RX);
> }
>
> --
> 2.37.0.144.g8ac04bfd2-goog
>

--
With Best Regards,
Andy Shevchenko