Fix UART NS8250 detecting parity and framing errors

[smaryus_gmail.com]

Summary

UART has a register LSR, line status register. Every time there is a read on this register the error bits are cleared. The current implementation reads the LSR register to detect if data was received uart_core.c - uart_intr -> ns8250_bus_ipend and then reads data from the DATA register ns8250_bus_receive. Into the ns8250_bus_receive, LSR is read again to determine if there were any errors (parity, frame). At this point the error bit is cleared by the previews LSR read. As a result parity and frame errors are never detected. See [1] for more details.

The suggested solution is to add an extra member into the ns8250 structure to store the LSR value for later use, so that the byte that was read from the DATA register is in sync with the LSR register.

[1]: https://en.wikibooks.org/wiki/Serial_Programming/8250_UART_Programming#Line_Status_Register

Test Plan

1. Configure UART using TTY to calculate parity errors and not to ignore them. From C++ this can be made like this:

struct termios tio;
tcgetattr(fd, &tio);
cfmakeraw(&tio);
tio.c_cflag &= ~PARODD;         // select odd parity
tio.c_cflag |= PARENB;         // enable parity generation
tio.c_cflag |= CSIZE | CS8;
tio.c_iflag |= INPCK;          // enable parity checking
tio.c_iflag |= PARMRK;         // enable in-band marking 
tio.c_iflag &= ~IGNPAR;         // make sure input parity errors are not ignored
tcsetattr(fd, TCSANOW, &tio)

2. Send data on the UART with wrong parity.
3. When reading TTY at each parity error instead on receiving 1 byte are received 3 bytes: 0xFF 0x00 the_received_byte. In TTY 0xFF+0x00 represent parity error.

[emaste]

CC folks either in IRC discussion or in person at Waterloo Hackathon 2019

[imp]

I don't like the notion of having THE SAVED lsr. if we read it, we should OR in the bits to saved LSR and then remove bits from that word as we deal with them. Otherwise, it feels too racy to me.

sys/dev/uart/uart_dev_ns8250.c

93

why do we clear it here? Does the hardware clear it? this seems like exactly the case where we'd *NOT* want to clear it.

163

Why clear it here?

217

I'd be tempted to have a ns8250_clear_saved_lsr() function instead. It would be fewer style violations and easier to grok.

365

I'd be tempted to have a read_lsr here.

Also, why aren't we |= the bits and then clearing them as we process them?

375

why read it twice, here and below. I don't understand that. It adds a lot of inb's to the critical path.

715–722

here we're dealing with the lsr bits. We should CLEAR them as we add stuff to ipend.

[smaryus_gmail.com]

I only saved the LSR flags so I can later use it into the read to detect parity errors.
I don't know if saving LSR is useful in other places except for the parity errors. So maybe it would be enough to save the error bits from LSR,, because it looks like everything else works fine in driver.
What do you think it might be useful to save entire LSR and clear bits when needed, or just fix this parity error and rename to reflect for what is used? I don't know what will be the implication, but but we can also change into uart_intr to call a new function to handle the parity errors, in the same way like for overflow and then we don't save anymore the LSR internaly?

Thanks for review and comments.

sys/dev/uart/uart_dev_ns8250.c

93

Maybe it makes more sense to be moved into the while on line 83, but save the value for the else on line 82 if (lsr & (LSR_BI|LSR_FE|LSR_PE)
The hardware clears the error bits every time there is a uart_getreg(bas, REG_LSR).
Also if there is a uart_getreg(bas, REG_DATA) then there must also be a LSR read, if not the errors flags are not anymore in sync with the data.

163

I'll move this into the if (what & UART_DRAIN_RECEIVER), it might make more sense to clear it only then.
But there I think is needed, because there is a uart_getreg(bas, REG_LSR) follow by uart_getreg(bas, REG_DATA) , to be set to 0 or to the latest LSR value. But since LSR is read until there is no data then saving the value it might not make sense, since it is used only into then reading.

217

Yes, I'll make a function.

365

Yes, it can be a function to read LSR and save it if is needed into the lsr_saved_flags and be called only where is needed.
Because there are places where LSR is read but the value doesn't need to be save.

Regarding |= as far as I saw the only problem with LSR is when data is read for parity errors. So I've thought not to complicate the logic more and break something else, since everything else works fine.

375

I'll change this and call the future function that will read & save LSR.

715–722

Ok, I'll clear them.

[smaryus_gmail.com]

Based on the comments from the previous patch I've made some changes to how LSR errors are stored:

• instead of saving the entire LSR register, only saving the error bits that are clear by the UART. In this way the current implementation remains the same and only update the code in places where the value from bit errors is needed, eg. ns8250_bus_receive.
• created two functions, one to clear and one to update the save LSR error bits.
• clear the saved error bits after they are used or after a read on REG_DATA took place.