Re: [PATCH v2 8/8] serial: Add ability to specify MMIO ports via 'serial'

[Daniel Kiper]

Thank you for quick turn around!

Now patches looks much better but...

On Thu, Dec 22, 2022 at 05:12:57PM +1100, Benjamin Herrenschmidt wrote:
> From: Benjamin Herrenschmidt <benh@amazon.com>
>
> This adds the ability to explicitely add an MMIO based serial port
> via the 'serial' command. The syntax is:
>
> serial --port=mmio,<hex_address>{.b,.w,.l,.q}
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  docs/grub.texi          | 26 ++++++++++++++++++--
>  grub-core/term/serial.c | 53 +++++++++++++++++++++++++++++++++++++----
>  2 files changed, 72 insertions(+), 7 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index fd22a69fa..502ca2ef7 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4168,8 +4168,24 @@ Commands usable anywhere in the menu and in the 
> command-line.
>  @deffn Command serial [@option{--unit=unit}] [@option{--port=port}] 
> [@option{--speed=speed}] [@option{--word=word}] [@option{--parity=parity}] 
> [@option{--stop=stop}]
>  Initialize a serial device. @var{unit} is a number in the range 0-3
>  specifying which serial port to use; default is 0, which corresponds to
> -the port often called COM1. @var{port} is the I/O port where the UART
> -is to be found; if specified it takes precedence over @var{unit}.
> +the port often called COM1.
> +
> +@var{port} is the I/O port where the UART is to be found or, if prefixed
> +with @samp{mmio,}, the MMIO address of the UART. If specified it takes
> +precedence over @var{unit}.
> +
> +Additionally, an MMIO address can be suffixed with:
> +@itemize @bullet
> +@item
> +@samp{.b} for bytes access (default)
> +@item
> +@samp{.w} for 16-bit word access
> +@item
> +@samp{.l} for 32-bit long word access or
> +@item
> +@samp{.q} for 64-bit long long word access
> +@end itemize
> +
>  @var{speed} is the transmission speed; default is 9600. @var{word} and
>  @var{stop} are the number of data bits and stop bits. Data bits must
>  be in the range 5-8 and stop bits must be 1 or 2. Default is 8 data
> @@ -4185,6 +4201,12 @@ The serial port is not used as a communication channel 
> unless the
>  @command{terminal_input} or @command{terminal_output} command is used
>  (@pxref{terminal_input}, @pxref{terminal_output}).
>
> +Examples:
> +@example
> +serial --port=3f8 --speed=9600
> +serial --port=mmio,fefb0000.l --speed=115200
> +@end example
> +
>  See also @ref{Serial terminal}.
>  @end deffn
>
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index ed7b398b7..d374495cb 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -152,8 +152,8 @@ grub_serial_find (const char *name)
>        break;
>
>  #if (defined(__mips__) || defined (__i386__) || defined (__x86_64__)) && 
> !defined(GRUB_MACHINE_EMU) && !defined(GRUB_MACHINE_ARC)
> -  if (!port && grub_memcmp (name, "port", sizeof ("port") - 1) == 0
> -      && grub_isxdigit (name [sizeof ("port") - 1]))
> +  if (!port && grub_strncmp (name, "port", grub_strlen ("port")) == 0

Maybe I was a bit imprecise but I was thinking about converting
grub_memcmp() to grub_strncmp() in your patches/changes only.
Sorry about that.

If you want still to do that I would prefer if you do this in separate
patch. And please do not change "sizeof () - 1" to "grub_strlen ()".

In the worst case I am OK with a sentence in the commit message saying
you change grub_memcmp() to grub_strncmp() on the occasion. Otherwise it
is confusing to see such changes in the patch. Though separate patch
is preferred way to do this...

And please replace grub_memcmp() with grub_strncmp() and 4 with
'sizeof ("mmio") - 1' in patch #3.

> +      && grub_isxdigit (name [grub_strlen ("port")]))
>      {
>        name = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof 
> ("port") - 1],
>                                                       0, 16), NULL);
> @@ -164,6 +164,49 @@ grub_serial_find (const char *name)
>          if (grub_strcmp (port->name, name) == 0)
>             break;
>      }
> +  if (!port && grub_strncmp (name, "mmio,", grub_strlen ("mmio,")) == 0
> +      && grub_isxdigit (name [grub_strlen ("mmio,")]))
> +    {
> +      const char *p1, *p = &name[grub_strlen ("mmio,")];

s/grub_strlen ("mmio,")/sizeof ("mmio,") - 1/

> +      grub_addr_t addr = grub_strtoul (p, &p1, 16);
> +      unsigned int acc_size = 1;
> +      unsigned int nlen = p1 - p;
> +
> +      /* Check address validity and general syntax */
> +      if (addr == 0 || (p1[0] != '\0' && p1[0] != '.'))

This condition looks wrong. I think it should be

  if (*p == '\0' || (*p1 != '.' && *p1 != '\0') || addr == 0)

I think the most important thing is lack of "*p == '\0'". Am I right?
And I changed a logic a bit to make it more readable...

Additionally, maybe we should print an error here to give a hint to
a user what is wrong. The grub_error(..., N_()) is your friend...

> +        return NULL;
> +      if (p1[0] == '.')
> +        switch(p1[1])
> +          {
> +          case 'w':
> +            acc_size = 2;
> +            break;
> +          case 'l':
> +            acc_size = 3;
> +            break;
> +          case 'q':
> +            acc_size = 4;
> +            break;
> +          case 'b':
> +            acc_size = 1;
> +         /* fallthrough */
> +       default:
> +         /*
> +          * Should we abort for an unknown size ? Let's just default
> +          * to 1 byte, it would increase the chance that the user who
> +          * did a typo can actually see the console.
> +          */
> +         acc_size = 1;
> +          }
> +      name = grub_serial_ns8250_add_mmio (addr, acc_size, NULL);
> +      if (!name)
> +        return NULL;
> +
> +      FOR_SERIAL_PORTS (port)
> +        if (grub_strncmp (port->name, name, nlen) == 0) {
> +          break;
> +        }
> +    }
>    if (!port && grub_strcmp (name, "auto") == 0)
>      {
>        /* Look for an SPCR if any. If not, default to com0 */
> @@ -178,9 +221,9 @@ grub_serial_find (const char *name)
>  #endif
>
>  #ifdef GRUB_MACHINE_IEEE1275
> -  if (!port && grub_memcmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) == 
> 0)
> +  if (!port && grub_strncmp (name, "ieee1275/", grub_strlen ("ieee1275/")) 
> == 0)

Ditto.

>      {
> -      name = grub_ofserial_add_port (&name[sizeof ("ieee1275/") - 1]);
> +      name = grub_ofserial_add_port (&name[grub_strlen ("ieee1275/")]);
>        if (!name)
>       return NULL;
>
> @@ -212,7 +255,7 @@ grub_cmd_serial (grub_extcmd_context_t ctxt, int argc, 
> char **args)
>
>    if (state[OPTION_PORT].set)
>      {
> -      if (grub_memcmp (state[OPTION_PORT].arg, "mmio", 4) == 0)
> +      if (grub_strncmp (state[OPTION_PORT].arg, "mmio,", grub_strlen 
> ("mmio,")) == 0)

Ditto.

And if you fix this please fix coding style in 
grub_le_to_cpu*()/grub_cpu_to_le*()
calls in patch #7 too.

I hope I did not miss other issues this time...

Daniel