Hello Paul, first of all thanks for your cooperation on this ancient topic. It's very appreciated. And oh, I failed to Cc the NXP people. I added them to Cc:, maybe one of them can add something valuable to the discussion. 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 :-\ > > 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.) 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 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 drop the topic, just stop using fsl8250_handle_irq() without feeling sad. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
