2022-07-12 13:31:18

by VAMSHI GAJJELA

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

From: VAMSHI GAJJELA <[email protected]>

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

This happens when LCR writes are ignored when UART is busy.
dw8250_check_lcr() in retries to updateLCR, invokes dw8250_force_idle()
to clear and reset fifo and eventually reads UART_RX causing pslverr.

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

Signed-off-by: VAMSHI GAJJELA <[email protected]>
---
drivers/tty/serial/8250/8250_dw.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index f57bbd32ef11..a83222839884 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -81,9 +81,19 @@ static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value)

static void dw8250_force_idle(struct uart_port *p)
{
+ unsigned int lsr;
struct uart_8250_port *up = up_to_u8250p(p);

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

--
2.37.0.144.g8ac04bfd2-goog

2022-07-12 13:33:13

by Andy Shevchenko

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

On Tue, Jul 12, 2022 at 3:16 PM Vamshi Gajjela <[email protected]> wrote:
>
> From: VAMSHI GAJJELA <[email protected]>
>
> With PSLVERR_RESP_EN parameter set to 1, device generates an error

the device

> response when an attempt to read empty RBR with FIFO enabled.

an empty

> This happens when LCR writes are ignored when UART is busy.
> dw8250_check_lcr() in retries to updateLCR, invokes dw8250_force_idle()
> to clear and reset fifo and eventually reads UART_RX causing pslverr.

fifo --> FIFO
pslverr --> the error

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

...

> + unsigned int lsr;
> struct uart_8250_port *up = up_to_u8250p(p);

Can we keep it ordered according to the reversed xmas tree?

...

> + /*
> + * With PSLVERR_RESP_EN parameter set to 1, device generates pslverr

the device
pslverr --> an

> + * error response when an attempt to read empty RBR with FIFO enabled

Missed period.

> + */
> + lsr = p->serial_in(p, UART_LSR);

The only caller of this function already has the lsr value, why you
can't (re)use it?

> + if ((up->fcr & UART_FCR_ENABLE_FIFO) && !(lsr & UART_LSR_DR))
> + return;

--
With Best Regards,
Andy Shevchenko

2022-07-13 08:20:09

by VAMSHI GAJJELA

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

On Tue, Jul 12, 2022 at 6:56 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Tue, Jul 12, 2022 at 3:16 PM Vamshi Gajjela <[email protected]> wrote:
> >
> > From: VAMSHI GAJJELA <[email protected]>
> >
> > With PSLVERR_RESP_EN parameter set to 1, device generates an error
>
> the device
ack
>
> > response when an attempt to read empty RBR with FIFO enabled.
>
> an empty
ack
>
> > This happens when LCR writes are ignored when UART is busy.
> > dw8250_check_lcr() in retries to updateLCR, invokes dw8250_force_idle()
> > to clear and reset fifo and eventually reads UART_RX causing pslverr.
>
> fifo --> FIFO
> pslverr --> the error
ack
>
> > Avoid this by not reading RBR/UART_RX when no data is available.
>
> ...
>
> > + unsigned int lsr;
> > struct uart_8250_port *up = up_to_u8250p(p);
>
> Can we keep it ordered according to the reversed xmas tree?
agree.
>
> ...
>
> > + /*
> > + * With PSLVERR_RESP_EN parameter set to 1, device generates pslverr
>
> the device
> pslverr --> an
ack.
>
> > + * error response when an attempt to read empty RBR with FIFO enabled
>
> Missed period.
ack
>
> > + */
> > + lsr = p->serial_in(p, UART_LSR);
>
> The only caller of this function already has the lsr value, why you
> can't (re)use it?
lsr is not read before, caller function (dw8250_check_lcr) reads lcr.
>
> > + if ((up->fcr & UART_FCR_ENABLE_FIFO) && !(lsr & UART_LSR_DR))
> > + return;
>
> --
> With Best Regards,
> Andy Shevchenko

2022-07-13 10:31:49

by Andy Shevchenko

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

On Wed, Jul 13, 2022 at 10:13 AM VAMSHI GAJJELA
<[email protected]> wrote:
> On Tue, Jul 12, 2022 at 6:56 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Tue, Jul 12, 2022 at 3:16 PM Vamshi Gajjela <[email protected]> wrote:

...

> > > + lsr = p->serial_in(p, UART_LSR);
> >
> > The only caller of this function already has the lsr value, why you
> > can't (re)use it?
> lsr is not read before, caller function (dw8250_check_lcr) reads lcr.

I see, thanks for elaboration.

--
With Best Regards,
Andy Shevchenko