[6/6] tty: serial: amba-pl011: Parse bits option as 5, 6, 7 or 8 in _get_options

  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(-)
  

On Thu, 26 Oct 2023, Théo Lebrun wrote:

> 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(-)
> 
> 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 */

0x60 needs to be replaced with a named define!
  

Hi,

On Thu Oct 26, 2023 at 1:13 PM CEST, Ilpo Järvinen wrote:
> On Thu, 26 Oct 2023, Théo Lebrun wrote:
> > 
> > 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 */
>
> 0x60 needs to be replaced with a named define!

Fixed locally for the next revision, thanks!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
  

On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> 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>

With Ilpo's comment fixed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
  

Hello,

On Thu Oct 26, 2023 at 3:48 PM CEST, Linus Walleij wrote:
> On Thu, Oct 26, 2023 at 12:41 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
>
> > 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>
>
> With Ilpo's comment fixed:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

It's been fixed locally. Thank you for your review Linus!

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
  

On Thu, 26 Oct 2023 12:41:23 +0200
Théo Lebrun <theo.lebrun@bootlin.com> wrote:

Hi,
I would change the commit title to better indicate that you add support
for bits 5 and 6, which was missing.

Maybe "Add support for 5 and 6 bits in..." ?

> pl011_console_get_options() gets called to retrieve currently configured
> options from the registers. Previously, LCRH_TX.WLEN was being parsed

It took me some time to understand your explanation :) Maybe change
to:

"Previously, only 7 or 8 bits were supported."

> as either 7 or 8 (fallback). Hardware supports values from 5 to 8

Add bits:

"5 to 8 bits..."

And indicate that this patch adds support for 5 and 6 bits.


> 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(-)
> 
> 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 */

Capital "F" -> "From...".

And add "bits" -> "From 5 to 8 bits..."

Hugo.


>  
>  	ibrd = pl011_read(uap, REG_IBRD);
>  	fbrd = pl011_read(uap, REG_FBRD);
> 
> -- 
> 2.41.0
>
  

Hello,

On Thu Oct 26, 2023 at 4:53 PM CEST, Hugo Villeneuve wrote:
> On Thu, 26 Oct 2023 12:41:23 +0200
> Théo Lebrun <theo.lebrun@bootlin.com> wrote:
>
> Hi,
> I would change the commit title to better indicate that you add support
> for bits 5 and 6, which was missing.
>
> Maybe "Add support for 5 and 6 bits in..." ?
>
> > pl011_console_get_options() gets called to retrieve currently configured
> > options from the registers. Previously, LCRH_TX.WLEN was being parsed
>
> It took me some time to understand your explanation :) Maybe change
> to:
>
> "Previously, only 7 or 8 bits were supported."
>
> > as either 7 or 8 (fallback). Hardware supports values from 5 to 8
>
> Add bits:
>
> "5 to 8 bits..."
>
> And indicate that this patch adds support for 5 and 6 bits.

I agree the whole commit message is unclear. Let's rewrite it. What do
you think of the following:

   tty: serial: amba-pl011: Allow parsing word length of 5/6 bits at console setup

   If no options are given at console setup, we parse hardware register
   LCRH_TX.WLEN for bits per word. We compare the value to the 7 bits
   value (UART01x_LCRH_WLEN_7). If the hardware is configured for 5, 6
   or 8 bits per word, we fallback to 8 bits.

   Change that behavior to parse the whole range available: from 5 to 8
   bits per word.

Note that we don't add support for 5/6 bits, we only update the parsing
of the regs (if no options are passed at setup) to reflect the current
hardware config. The behavior will be different only if the inherited
value (from reset/bootloader) is 5 or 6: previously we guessed 8 bits
word length, now we guess the right value.

What's your opinion on this new commit message?

Thanks!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
  

On Tue, Oct 31, 2023 at 10:35:29AM +0100, Théo Lebrun wrote:
> Hello,
> 
> On Thu Oct 26, 2023 at 4:53 PM CEST, Hugo Villeneuve wrote:
> > On Thu, 26 Oct 2023 12:41:23 +0200
> > Théo Lebrun <theo.lebrun@bootlin.com> wrote:
> >
> > Hi,
> > I would change the commit title to better indicate that you add support
> > for bits 5 and 6, which was missing.
> >
> > Maybe "Add support for 5 and 6 bits in..." ?
> >
> > > pl011_console_get_options() gets called to retrieve currently configured
> > > options from the registers. Previously, LCRH_TX.WLEN was being parsed
> >
> > It took me some time to understand your explanation :) Maybe change
> > to:
> >
> > "Previously, only 7 or 8 bits were supported."
> >
> > > as either 7 or 8 (fallback). Hardware supports values from 5 to 8
> >
> > Add bits:
> >
> > "5 to 8 bits..."
> >
> > And indicate that this patch adds support for 5 and 6 bits.
> 
> I agree the whole commit message is unclear. Let's rewrite it. What do
> you think of the following:
> 
>    tty: serial: amba-pl011: Allow parsing word length of 5/6 bits at console setup
> 
>    If no options are given at console setup, we parse hardware register
>    LCRH_TX.WLEN for bits per word. We compare the value to the 7 bits
>    value (UART01x_LCRH_WLEN_7). If the hardware is configured for 5, 6
>    or 8 bits per word, we fallback to 8 bits.
> 
>    Change that behavior to parse the whole range available: from 5 to 8
>    bits per word.
> 
> Note that we don't add support for 5/6 bits, we only update the parsing
> of the regs (if no options are passed at setup) to reflect the current
> hardware config. The behavior will be different only if the inherited
> value (from reset/bootloader) is 5 or 6: previously we guessed 8 bits
> word length, now we guess the right value.
> 
> What's your opinion on this new commit message?

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.
  

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:04:11PM +0100, Théo Lebrun wrote:
> 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?

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.)

> 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.

Well, honestly I don't think it needs a comment _if_ one thinks about
what these sizes mean for what is supposed to be a console displaying
ASCII characters. It feels to me like pointing out the obvious, and
would be on the level of teaching people how to suck eggs... but then
again, maybe there are times when people need to be taught how to
suck eggs...

So yes, add a comment if you think it's a good idea, but should that
comment be replicated in almost every driver or should it be documented
elsewhere?
  

On Tue, 31 Oct 2023 10:35:29 +0100
Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Hello,
> 
> On Thu Oct 26, 2023 at 4:53 PM CEST, Hugo Villeneuve wrote:
> > On Thu, 26 Oct 2023 12:41:23 +0200
> > Théo Lebrun <theo.lebrun@bootlin.com> wrote:
> >
> > Hi,
> > I would change the commit title to better indicate that you add support
> > for bits 5 and 6, which was missing.
> >
> > Maybe "Add support for 5 and 6 bits in..." ?
> >
> > > pl011_console_get_options() gets called to retrieve currently configured
> > > options from the registers. Previously, LCRH_TX.WLEN was being parsed
> >
> > It took me some time to understand your explanation :) Maybe change
> > to:
> >
> > "Previously, only 7 or 8 bits were supported."
> >
> > > as either 7 or 8 (fallback). Hardware supports values from 5 to 8
> >
> > Add bits:
> >
> > "5 to 8 bits..."
> >
> > And indicate that this patch adds support for 5 and 6 bits.
> 
> I agree the whole commit message is unclear. Let's rewrite it. What do
> you think of the following:
> 
>    tty: serial: amba-pl011: Allow parsing word length of 5/6 bits at console setup
> 
>    If no options are given at console setup, we parse hardware register
>    LCRH_TX.WLEN for bits per word. We compare the value to the 7 bits
>    value (UART01x_LCRH_WLEN_7). If the hardware is configured for 5, 6
>    or 8 bits per word, we fallback to 8 bits.
> 
>    Change that behavior to parse the whole range available: from 5 to 8
>    bits per word.
> 
> Note that we don't add support for 5/6 bits, we only update the parsing
> of the regs (if no options are passed at setup) to reflect the current
> hardware config. The behavior will be different only if the inherited
> value (from reset/bootloader) is 5 or 6: previously we guessed 8 bits
> word length, now we guess the right value.
> 
> What's your opinion on this new commit message?

Hi,
that's fine with me.

Hugo.
  

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
  

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.

> What makes us suppose at probe that it must be a console?

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.

TTYs under Linux have a standard initial set of parameters at boot -
9600 baud, 8 bits, etc. The exception to this is if a serial port *is
being used* as a serial console, where these settings are overriden by
the serial console configuration. The reason for that is... imagine
the chaos that would occur if:

- Your boot loader configures (through user configuration) the serial
  port for 115200 baud.
- The boot loader loads the kernel and passes control to it, with
  a command line specifying that this serial port is to be used for
  the serial console, but not specifying any parameters.
- The kernel boots, and starts outputting messages at 115200 baud.
- Userspace starts running, opens /dev/console, and switches the port
  to 9600 baud. Now you have utter garbage being received from the
  serial console.

So, the serial console is special in that regard.

Now, when we configure the serial console, we attempt to:

1) parse the options provided on the console= line to set the serial
port appropriately, or
2) if there are no options, then we attempt to set the serial port to
something sane *for use as a serial console*, which uses ASCII protocol
not some random binary protocol. 5 and 6 bits make no sense here.
  

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