Re: [PATCH v2 3/3] serial: 8250: Set fifo timeout using uart_fifo_timeout()

From: Ilpo Järvinen
Date: Wed Apr 17 2024 - 04:13:57 EST


On Tue, 16 Apr 2024, Michael Pratt wrote:
> On Tuesday, April 16th, 2024 at 14:57, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> > > unsigned int status, tmout = 10000;
> > >
> > > - /* Wait up to 10ms for the character(s) to be sent. /
> > > + / Wait for a time relative to buffer size and baud */
> > > + if (up->fifo_enable && up->port.timeout)
> > > + tmout = jiffies_to_usecs(up->port.timeout);
> >
> >
> > Why do we still use that default? Can't we calculate timeout even for\
> > FIFO-less / FIFO-disabled devices?

Yes we definitely should be able to. Unfortunately these patches just keep
coming back not in the form that follows the review feedback, but they
come up their own way of doing things which is way worse and ignores the
given feedback.

> Maybe it's possible that there is some kind of rare case where the LSR register
> is not working or not configured properly for a device in which support
> is being worked on...without a timeout, that would result in an infinite loop.

"without a timeout" is not what Andy said. He said you should always have
a timeout, regardless of there being FIFO or not. And that timeout should
be derived in the same manner from baudrate and FIFO size (to address the
cases w/o FIFO, the fifosize should be lower bounded to 1 while
calculating the FIFO timeout).

> AFAIK, when everything is working properly, there is no such thing as needing
> a timeout for a uart device without fifo, as every single byte written would trigger
> an interrupt anyway.

While I agree the general principle, that this is backup that should not
even be needed, the statement is partly incorrect. We don't get interrupts
during console write because they're disabled. But LSR should still change
and allow progress without the backup timeout.

--
i.