On Wed, May 24, 2023 at 04:02:58PM +0300, Ilpo Järvinen wrote:
> On Wed, 24 May 2023, Uwe Kleine-König wrote:
>
> > At least on MPC83xx (but I suspect also on other implementations) it can
> > happen that after irqs are disabled but before the FIFOs are cleared a
> > character is received. Resetting the FIFO throws away that character,
> > but doesn't clear the IIR.RDI event that signals that there is read data
> > available.
> >
> > Read from RX to clear IIR.RDI and throw away the result.
> >
> > This fixes a infinite loop after the above described race happened: The
> > irq handler (here: fsl8250_handle_irq()) triggers, but LSR.DR isn't set,
> > so RX isn't read and the irq handler returns just to be called again.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > ---
> > drivers/tty/serial/8250/8250_port.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index fe8d79c4ae95..8b47ec000922 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -556,6 +556,18 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
> > serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO |
> > UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
> > serial_out(p, UART_FCR, 0);
> > +
> > + /*
> > + * When doing rs485 on MPC8313 it can happen that in the short
> > + * window when switching from RX to TX there is a character in
> > + * the RX FIFO which isn't processed any more and then discarded
> > + * in here by clearing the FIFO. In that case IIR signals an RX
> > + * timeout once irqs are reenabled (i.e. in
> > + * serial8250_em485_stop_tx()) but LSR shows no pending event.
> > + * The RX timeout event can only be cleared by reading RX. Do
> > + * this here before reenabling the FIFO and irqs.
> > + */
> > + serial_port_in(&p->port, UART_RX);
> > }
> > }
>
> This solution has too wide impact, I think.
What is the impact? After the FIFO is reset reading the RX register
shouldn't matter?
> It should be made driver specific.
I'm not a big fan, the 8250 driver is already very fragmented.
> Can't you read IIR to see if the event indication is there before
> doing this UART_RX read?
Assuming reading IIR and reading RX take approx the same amount of
time, I don't see an upside of checking IIR first?!
> Maybe add an UART specific function for fifo clearing/reset.
See above. And with the theory that reading RX doesn't hurt after the
FIFO was just cleared, adding this to generic code has the upside that
other variants that might have the same issue get fixed, too.
> I've long wondered this related thing:
>
> Does anyone have idea why serial8250_clear_and_reinit_fifos() and
> serial8250_clear_fifos() are separate, what is the benefit of not setting
> FCR back to up->fcr? That is that intermediate FCR <= 0 really required
> for the FIFO reset sequence or is it just an artifact of how the code is
> structured into those two functions.
>
> It might make sense to drop serial8250_clear_and_reinit_fifos() and
> change serial8250_clear_fifos() into something like this (depending on
> the answers):
>
> static void serial8250_clear_fifos(struct uart_8250_port *p, bool disable)
> {
> if (p->capabilities & UART_CAP_FIFO) {
> serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO);
> serial_out(p, UART_FCR, UART_FCR_ENABLE_FIFO |
> UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
> serial_out(p, UART_FCR, disable ? 0 : p->fcr);
> }
> }
I'd say this should work. Apart from skipping
serial_out(p, UART_FCR, 0);
it has the side effect of skipping
serial_out(p, UART_FCR, p->fcr);
if !(p->capabilities & UART_CAP_FIFO). That shouldn't matter though.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
