| Message ID | 20231026-mbly-uart-v1-6-9258eea297d3@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Cleanup AMBA PL011 driver | expand |
Hello, On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote: > There is no point in supporting 5 or 6 bits for console usage. Think > about it. What values are going to be sent over the console? It'll be > ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha > characters into control characters, punctuation and numbers. 5-bit > would be all control characters. > > So there's no point trying to do anything with 5 or 6 bits per byte, > and I decided we might as well take that as an error (or maybe a > case that the hardware has not been setup) and default to 8 bits per > byte. I see your point. Two things come to mind: - I added this parsing of 5/6 bits to be symmetrical with pl011_set_termios that handles 5/6 properly. Should pl011_set_termios be modified then? - If a value of 5 or 6 means the hardware has not been setup, shouldn't we ignore all other parsed values? If you decide to keep the current behavior, I'd be down to adding a comment to explicit this choice in pl011_console_get_options. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ------------------------------------------------------------------------
On Tue Oct 31, 2023 at 12:22 PM CET, Russell King (Oracle) wrote: > On Tue, Oct 31, 2023 at 12:04:11PM +0100, Théo Lebrun wrote: > > On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote: > > > There is no point in supporting 5 or 6 bits for console usage. Think > > > about it. What values are going to be sent over the console? It'll be > > > ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha > > > characters into control characters, punctuation and numbers. 5-bit > > > would be all control characters. > > > > > > So there's no point trying to do anything with 5 or 6 bits per byte, > > > and I decided we might as well take that as an error (or maybe a > > > case that the hardware has not been setup) and default to 8 bits per > > > byte. > > > > I see your point. Two things come to mind: > > > > - I added this parsing of 5/6 bits to be symmetrical with > > pl011_set_termios that handles 5/6 properly. Should pl011_set_termios > > be modified then? > > Why should it? Note that I said above about _console_ usage which is > what you were referring to - the early code that sets up the console > by either reading the current settings (so that we can transparently > use the UART when its handed over already setup by a boot loader). > > This is completely different to what happens once the kernel is running. > Userspace might very well have a reason to set 5 or 6 bits if it wants > to communicate with a device that uses those sizes. > > However, such a device won't be a console for the reasons I outlined > above (it will truncate the ASCII characters turning console messages > into garbage.) I'm not sure I get it. (1) We assume it is a console so it's ASCII so no reason to set to 5 or 6 bits per word. But (2) there might be a reason to set the UART to 5 or 6 bits, the userspace decides. How do the two interact? Say we boot to Linux, userspace configures to 6 bits because reasons and we reset. At second probe we see a config of 6 bits per word but assume that can't be logical, even though it is. What makes us suppose at probe that it must be a console? I won't die on a hill for this topic; we'll go the way you prefer! Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Tue Oct 31, 2023 at 3:05 PM CET, Russell King (Oracle) wrote: > On Tue, Oct 31, 2023 at 02:51:45PM +0100, Théo Lebrun wrote: > > On Tue Oct 31, 2023 at 12:22 PM CET, Russell King (Oracle) wrote: > > > On Tue, Oct 31, 2023 at 12:04:11PM +0100, Théo Lebrun wrote: > > > > On Tue Oct 31, 2023 at 11:11 AM CET, Russell King (Oracle) wrote: > > > > > There is no point in supporting 5 or 6 bits for console usage. Think > > > > > about it. What values are going to be sent over the console? It'll be > > > > > ASCII, which requires at _least_ 7-bit. 6-bit would turn alpha > > > > > characters into control characters, punctuation and numbers. 5-bit > > > > > would be all control characters. > > > > > > > > > > So there's no point trying to do anything with 5 or 6 bits per byte, > > > > > and I decided we might as well take that as an error (or maybe a > > > > > case that the hardware has not been setup) and default to 8 bits per > > > > > byte. > > > > > > > > I see your point. Two things come to mind: > > > > > > > > - I added this parsing of 5/6 bits to be symmetrical with > > > > pl011_set_termios that handles 5/6 properly. Should pl011_set_termios > > > > be modified then? > > > > > > Why should it? Note that I said above about _console_ usage which is > > > what you were referring to - the early code that sets up the console > > > by either reading the current settings (so that we can transparently > > > use the UART when its handed over already setup by a boot loader). > > > > > > This is completely different to what happens once the kernel is running. > > > Userspace might very well have a reason to set 5 or 6 bits if it wants > > > to communicate with a device that uses those sizes. > > > > > > However, such a device won't be a console for the reasons I outlined > > > above (it will truncate the ASCII characters turning console messages > > > into garbage.) > > > > I'm not sure I get it. (1) We assume it is a console so it's ASCII so no > > reason to set to 5 or 6 bits per word. But (2) there might be a reason > > to set the UART to 5 or 6 bits, the userspace decides. > > Precisely. > > > How do the two interact? Say we boot to Linux, userspace configures to 6 > > bits because reasons and we reset. At second probe we see a config of 6 > > bits per word but assume that can't be logical, even though it is. > > I think you're conflating "serial console" with "serial port". A > "serial port" can support multiple different formats, and in this case, > such as 5, 6, 7, and 8 bits. 5 and 6 bits are likely to be a specialised > application which uses a binary protocol, not ASCII. > > A "serial console" is one application of a "serial port" and a "serial > console" is used to send ASCII characters, not a binary protocol. That was all clear in my mind; I was missing the following bit: > Sorry, but no, we don't assume every serial port is a serial console. > Unless something has changed since I was involved with the serial > layer, **we only read the parameters from a serial port _if_ and only > if that port is being used as a serial console.** Thank you for the time you took; I'll get rid of the patch and send a V2 fixing nits for other patches. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 5774d48c7f16..b2062e4cbbab 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -2384,10 +2384,7 @@ static void pl011_console_get_options(struct uart_amba_port *uap, int *baud, *parity = 'o'; } - if ((lcr_h & 0x60) == UART01x_LCRH_WLEN_7) - *bits = 7; - else - *bits = 8; + *bits = FIELD_GET(0x60, lcr_h) + 5; /* from 5 to 8 inclusive */ ibrd = pl011_read(uap, REG_IBRD); fbrd = pl011_read(uap, REG_FBRD);
pl011_console_get_options() gets called to retrieve currently configured options from the registers. Previously, LCRH_TX.WLEN was being parsed as either 7 or 8 (fallback). Hardware supports values from 5 to 8 inclusive, which pl011_set_termios() exploits for example. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- drivers/tty/serial/amba-pl011.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)