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

[Daniel Kiper]

On Fri, Dec 02, 2022 at 10:05:26AM +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 | 31 ++++++++++++++++++++++++++++++-
>  2 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index bbebc2aef..a25235636 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4167,8 +4167,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
> @@ -4184,6 +4200,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 f57fe45ef..96a5ad725 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -164,6 +164,35 @@ grub_serial_find (const char *name)
>       if (grub_strcmp (port->name, name) == 0)
>         break;
>      }
> +  if (!port && grub_memcmp (name, "mmio,", sizeof ("mmio,") - 1) == 0

I think grub_strncmp() in string context would be more appropriate.
Please replace grub_memcmp() with grub_strncmp() where possible.

> +      && grub_isxdigit (name [sizeof ("mmio,") - 1]))
> +    {
> +      const char *p1, *p = &name[sizeof ("mmio,") - 1];
> +      grub_addr_t addr = grub_strtoul (p, &p1, 16);

You blindly assume grub_strtoul() will not fail. It is not true.
Please take a look at commit ac8a37dda (net/http: Allow use of
non-standard TCP/IP ports) how properly detect grub_strtoul() failures.

> +      unsigned int acc_size = 1;
> +      unsigned int nlen = p1 - p;

Please add empty line here.

> +      if (p1[0] == '.')
> +        switch(p1[1])
> +       {
> +       case 'w':
> +          acc_size = 2;
> +          break;
> +       case 'l':
> +          acc_size = 3;
> +          break;
> +       case 'q':
> +          acc_size = 4;
> +          break;

Does not compiler complain there is no default here? I think I saw such
warnings somewhere when it is missing.

> +          }
> +      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 */
> @@ -212,7 +241,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_memcmp (state[OPTION_PORT].arg, "mmio,", 5) == 0)

s/grub_memcmp/grub_strncmp/

Daniel