Re: [PATCH] serial: 8250_fsl: Don't report FE, PE and OE twice

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 9825 bytes --]

On Thu, 12 May 2022, Uwe Kleine-König wrote:

> Hello Paul,
> 
> On Thu, May 12, 2022 at 11:46:21AM -0400, Paul Gortmaker wrote:
> > [Re: [PATCH] serial: 8250_fsl: Don't report FE, PE and OE twice] On 12/05/2022 (Thu 08:17) Uwe Kleine-König wrote:
> > > On Wed, May 11, 2022 at 09:29:10PM -0400, Paul Gortmaker wrote:
> > > > [[PATCH] serial: 8250_fsl: Don't report FE, PE and OE twice] On 11/05/2022 (Wed 11:32) Uwe Kleine-König wrote:
> > > > 
> > > > > Some Freescale 8250 implementations have the problem that a single long
> > > > > break results in one irq per character frame time. The code in
> > > > > fsl8250_handle_irq() that is supposed to handle that uses the BI bit in
> > > > > lsr_saved_flags to detect such a situation and then skip the second
> > > > > received character. However it also stores other error bits and so after
> > > > > a single frame error the character received in the next irq handling is
> > > > > passed to the upper layer with a frame error, too.
> > > > > 
> > > > > To weaken this problem restrict saving LSR to only the BI bit.
> > > > 
> > > > But what is missing is just what "this problem" is - what applications
> > > > are broken and how?  What are the symptoms?  This is a 15+ year old
> > > > platform and so one has to ask why this is just being seen now.
> > > 
> > > The problem is "However it also stores other error bits and so after a
> > > single frame error the character received in the next irq handling is
> > > passed to the upper layer with a frame error, too." which is just the
> > > sentence before. I hoped this would be understandable :-\
> > 
> > I was trying to get you to describe symptoms at a higher level - as I
> > said above, at the application level - what were you using that wasn't
> > working that led you down the path to investigate this?   Transfering
> > data wasn't reaching the expected max for baud rate, or serial console
> > was showing lags and dropped characters, or ...?
> 
> The situation where the problem was noticed is: The 8313 is supposed to
> periodically receive a burst of a small (and fixed) number of
> characters. In the field it sometimes happend that there was a peak on
> the data line between two such telegrams which the UART interpreted as a
> character with a parity error. After that the first character of the
> next telegram wasn't received in userspace, because the driver claimed
> it was received with another parity error. So effectively a dropped
> character.
> 
> > The false positive error bits description is fine, but it isn't
> > something that a person in the future could read and then say "Oh I'm
> > having the same problem - I should backport that!"
> > 
> > > > > Note however that the handling is still broken:
> > > > > 
> > > > >  - lsr_saved_flags is updated using orig_lsr which is the LSR content
> > > > >    for the first received char, but there might be more in the FIFO, so
> > > > >    a character is thrown away that is received later and not necessarily
> > > > >    the one following the break.
> > > > >  - The doubled break might be the 2nd and 3rd char in the FIFO, so the
> > > > >    workaround doesn't catch these, because serial8250_rx_chars() doesn't
> > > > >    handle the workaround.
> > > > >  - lsr_saved_flags might have set UART_LSR_BI at the entry of
> > > > >    fsl8250_handle_irq() which doesn't originate from
> > > > >    fsl8250_handle_irq()'s "up->lsr_saved_flags |= orig_lsr &
> > > > >    UART_LSR_BI;" but from e.g. from serial8250_tx_empty().
> > > > >  - For a long or a short break this isn't about two characters, but more
> > > > >    or only a single one.
> > > > 
> > > > I've long since flushed the context required to parse the above, sorry.
> > > > But the part where it says "is still broken" stands out to me.
> > > 
> > > The current state is (assuming the errata is accurate and I understood
> > > it correctly): 
> > >  - You get a problem for sure if there is a frame error (because the
> > >    next good char is thrown away).
> > >  - You get a problem for sure if there is a parity error (because the
> > >    next good char is thrown away).
> > >  - You get a problem for sure if there was an overflow (because the
> > >    first good char after the overflow is thrown away).
> > >  - The code is racy for break handling. In some unlikely cases the break
> > >    workaround is applied wrongly.
> > > 
> > > (Where "thrown away" is really: passed to the tty layer with an error
> > > indication, which depending on tty settings results in dropping the
> > > character or passing it on to userspace.)
> > > 
> > > My patch only fixes the first three issues. A part of the reason for the
> > > uncomplete fix is that I don't have a platform that requires the workaround.
> > > (I thought I had, but it doesn't show the described behaviour and
> > > instead behaves nicely, i.e. one irq per break and no stray bits are
> > > set.)
> > 
> > I was hoping that with the full description of the issue from 12+ years
> > ago that you'd be able to reproduce it on your platform with the WAR disabled.
> > I take it that you tried and SysRQ still worked fine?
> 
> I think I did. I have to plan a bit of continous time to reverify.
> 
> > I also found a copy of an earlier proposed fix from 2010 on patchworks:
> > http://patchwork.ozlabs.org/project/linuxppc-dev/patch/20100301212324.GA1738@windriver.com/
> > 
> > Maybe there are some additional details in there of interest?
> > 
> > I wonder if some other intervening change in that wide time span happens
> > to mask the issue?  Who knows.  I'm not sure if you are that interested;
> > enough to go try an old kernel to find out...
> > 
> > > That the patch I did is correct is quite obvious for me. Currently the
> > > fsl8250_handle_irq() function sets the bits BI, OE, FE and PE in
> > 
> > If I recall correctly, it just clears BI - are you sure it sets bits?
> 
> Not explicitly, but it does
> 
> 	orig_lsr = up->port.serial_in(&up->port, UART_LSR);
> 	...
> 	up->lsr_saved_flags = orig_lsr;
> 
> So whatever error bit is read on function entry is reused for the first
> char in the next irq run.

Yes, it is clearly leaking extra flags (if those are set) both in the case 
of break workaround and without it.

> > > lsr_saved_flags, but only evaluates BI for the workaround. The commit
> > > that introduced that only talks about BI, the mentioned erratum also
> > > only mentions BI.
> > > 
> > > I can try to make the commit log more convincing. Or if the ability to
> > > test this code on an affected platform is declared a requirement, I will
> > 
> > I'm not in any position to declare any requirements - just that when you
> > are bit-bashing to work around some "black box" silicon errata, any
> > changes might impact whether the WAR is still working or not.

C code is not "some black box". On the next irq, only BI in 
lsr_saved_flags is looked at by the driver, that can be seen from the C 
code, no need to look at the errata.

And then the C code also tells on the next next irq, the other bits (if 
any were set) are taken into use for a real character, which is 
undesirable (= BUG!).

> > Your change alters lsr_saved_flags for *every* event, even when no breaks
> > or workarounds have been in play.  I'm not sure what that might trigger.

Indeed, fixing a bug alters behavior such that the bug no longer occurs :-).

Or are you saying that leaking old FE, PE and OE into the next char 
using lsr_saved_flags when no break nor workaround isn't in the play
is an event that should _not_ be altered???

If no extra flags are set, the proposed change is no-op.

Maybe Uwe's fix could be scoped down to clear only FE, PE and OE if one 
really wants to make a minimal fix? That would leave (mainly) DR out of it 
which could impact the behavior a little (the difference seems a bit 
theoretic to me but it is there)?

-- 
 i.

> > > drop the topic, just stop using fsl8250_handle_irq() without feeling sad.
> > 
> > That might be the best option in the end but I did notice something else
> > you might want to consider.  I believe the fsl8250_handle_irq() was just
> > a copy of the generic serial8250_handle_irq() as it was in 2011, with
> > the single block of code inserted for the WAR:
> > 
> > +       /* This is the WAR; if last event was BRK, then read and return */
> > +       if (unlikely(up->lsr_saved_flags & UART_LSR_BI)) {
> > +               up->lsr_saved_flags &= ~UART_LSR_BI;
> > +               port->serial_in(port, UART_RX);
> > +               spin_unlock_irqrestore(&up->port.lock, flags);
> > +               return 1;
> > +       }
> > 
> > Of course as we all know - when you copy something, you risk being left
> > behind when the original gets updated.  I just took a look at today's
> > generic 8250 one -- "git blame drivers/tty/serial/8250/8250_port.c" and
> > there are changes that probably have left fsl8250_handle_irq() being
> > left behind.  A bit more detective work would be required to see
> > changes prior to the refactoring in the 2015 commit of b6830f6df891.
> > 
> > It probably would be worthwhile to return fsl8250_handle_irq() to be the
> > "equivalent" of serial8250_handle_irq() + WAR as it was originally.  It
> > would be hard to argue against mainlining such changes - they are table
> > stakes.  And who knows, with a bit of luck it might solve your issue too?
> > 
> 
> Yeah, I already looked into these and part of my plan to fix the
> workaround was to bring fsl8250_handle_irq() on par with the generic irq
> handler routine. Effectively there isn't missing much.
> 
> > Of couse that is more effort than to just stop using the workaround, so I
> > wouldn't blame you at all if you decided to go that route.
> 
> Will discuss that with my customer how much effort to put into this.
> 
> Best regards
> Uwe
> 
>