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

[Ilpo Järvinen]

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.