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: 9686 bytes --]

On Tue, 24 May 2022, Uwe Kleine-König wrote:

> On Tue, May 24, 2022 at 07:01:18PM +0300, Ilpo Järvinen wrote:
> > 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)?
> 
> Is this an Ack for my patch?

It wasn't but now that I took a further look into it I've managed to 
prove myself that there are no changes at all in the _interactions with 
the device_ so Paul's concerns on making subtle changes are not valid!
The reason is as follows:

1) If BI is not set in lsr_saved_flags, the old lsr_saved_flags is 
recalled no earlier than in serial8250_read_char() at the point where the 
code is already past the only serial_in() there.

2) If BI is set in lsr_saved_flags, fsl8250_handle_irq() first clears BI 
and returns due to the workaround. On the next next irq, the old 
lsr_saved_flags is again not recalled before past the only serial_in() 
serial8250_read_char().

3) If execution does not even enter serial8250_read_char(), the old 
lsr_saved_flags value is not used at all and lsr_saved_flags is 
overwritten.

serial8250_read_char() only does bookkeeping and higher-layer stuff with 
the bits from lsr_saved_flags which does not impact how the kernel 
interacts with the device! 

Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.