Re: [PATCH 3/3] serial: Improve detection of duplicate serial ports

[Benjamin Herrenschmidt]

On Fri, 2022-12-23 at 12:48 +1100, Benjamin Herrenschmidt wrote:
> We currently rely on some pretty fragile comparison by name to
> identify wether a serial port being configured is identical

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

(Sorry about that ... blame christmas :-)

> ---
>  grub-core/term/arc/serial.c      |  4 +++
>  grub-core/term/ieee1275/serial.c |  4 +++
>  grub-core/term/ns8250.c          | 16 +++++++++
>  grub-core/term/serial.c          | 57 +++++++++++++-----------------
> --
>  include/grub/serial.h            |  4 +++
>  5 files changed, 51 insertions(+), 34 deletions(-)
> 
> diff --git a/grub-core/term/arc/serial.c b/grub-
> core/term/arc/serial.c
> index 651f814ee..487aa1b30 100644
> --- a/grub-core/term/arc/serial.c
> +++ b/grub-core/term/arc/serial.c
> @@ -106,6 +106,10 @@ grub_arcserial_add_port (const char *path)
>    struct grub_serial_port *port;
>    grub_err_t err;
>  
> +  FOR_SERIAL_PORTS (port)
> +    if (grub_strcmp(path, port->name) == 0)
> +      return port;
> +
>    port = grub_zalloc (sizeof (*port));
>    if (!port)
>      return NULL;
> diff --git a/grub-core/term/ieee1275/serial.c b/grub-
> core/term/ieee1275/serial.c
> index b4aa9d5c5..afbe8dcda 100644
> --- a/grub-core/term/ieee1275/serial.c
> +++ b/grub-core/term/ieee1275/serial.c
> @@ -227,6 +227,10 @@ add_port (struct ofserial_hash_ent *ent)
>    if (!ent->shortest)
>      return NULL;
>  
> +  FOR_SERIAL_PORTS (port)
> +    if (port->ent == ent)
> +      return port;
> +
>    port = grub_zalloc (sizeof (*port));
>    if (!port)
>      return NULL;
> diff --git a/grub-core/term/ns8250.c b/grub-core/term/ns8250.c
> index 1eb6a30b6..074bd08ca 100644
> --- a/grub-core/term/ns8250.c
> +++ b/grub-core/term/ns8250.c
> @@ -364,6 +364,14 @@ grub_serial_ns8250_add_port (grub_port_t port,
> struct grub_serial_config *config
>          return &com_ports[i];
>        }
>  
> +  FOR_SERIAL_PORTS (p)
> +    if (!p->mmio && p->port == port)
> +      {
> +        if (config != NULL)
> +          grub_serial_port_configure (p, config);
> +        return p;
> +      }
> +
>    grub_outb (0x5a, port + UART_SR);
>    if (grub_inb (port + UART_SR) != 0x5a)
>      return NULL;
> @@ -409,6 +417,14 @@ grub_serial_ns8250_add_mmio (grub_addr_t addr,
> unsigned int acc_size,
>          return &com_ports[i];
>        }
>  
> +  FOR_SERIAL_PORTS (p)
> +    if (p->mmio && p->mmio_base == addr)
> +      {
> +        if (config != NULL)
> +          grub_serial_port_configure (p, config);
> +        return p;
> +      }
> +
>    p = grub_malloc (sizeof (*p));
>    if (p == NULL)
>      return NULL;
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index 275dd61af..e214d773b 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -37,8 +37,6 @@
>  
>  GRUB_MOD_LICENSE ("GPLv3+");
>  
> -#define FOR_SERIAL_PORTS(var) FOR_LIST_ELEMENTS((var),
> (grub_serial_ports))
> -
>  enum
>    {
>      OPTION_UNIT,
> @@ -65,7 +63,7 @@ static const struct grub_arg_option options[] =
>    {0, 0, 0, 0, 0, 0}
>  };
>  
> -static struct grub_serial_port *grub_serial_ports;
> +struct grub_serial_port *grub_serial_ports;
>  
>  struct grub_serial_output_state
>  {
> @@ -147,26 +145,30 @@ grub_serial_find (const char *name)
>  {
>    struct grub_serial_port *port;
>  
> +  /*
> +   * First look for an exact match by name, this will take care of
> +   * things like "com0" which have already been created and that
> +   * this function cannot re-create.
> +   */
>    FOR_SERIAL_PORTS (port)
>      if (grub_strcmp (port->name, name) == 0)
> -      break;
> +      return port;
>  
>  #if (defined(__mips__) || defined (__i386__) || defined
> (__x86_64__)) && !defined(GRUB_MACHINE_EMU) &&
> !defined(GRUB_MACHINE_ARC)
> -  if (!port && grub_strncmp (name, "port", sizeof ("port") - 1) == 0
> +  if (grub_strncmp (name, "port", sizeof ("port") - 1) == 0
>        && grub_isxdigit (name [sizeof ("port") - 1]))
>      {
>        port = grub_serial_ns8250_add_port (grub_strtoul (&name[sizeof
> ("port") - 1],
>                                                         0, 16),
> NULL);
> -      if (port == NULL)
> -        return NULL;
> +      if (port != NULL)
> +        return port;
>      }
> -  if (!port && grub_strncmp (name, "mmio,", sizeof ("mmio,") - 1) ==
> 0
> +  if (grub_strncmp (name, "mmio,", sizeof ("mmio,") - 1) == 0
>        && grub_isxdigit (name [sizeof ("mmio,") - 1]))
>      {
>        const char *p1, *p = &name[sizeof ("mmio,") - 1];
>        grub_addr_t addr = grub_strtoul (p, &p1, 16);
>        unsigned int acc_size = 1;
> -      unsigned int nlen = p1 - p;
>  
>        /*
>         * If we reach here, we know there's a digit after "mmio,", so
> @@ -203,45 +205,32 @@ grub_serial_find (const char *name)
>              grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Incorrect MMIO
> access size"));
>            }
>  
> -      /*
> -       * Specifying the access size is optional an
> grub_serial_ns8250_add_mmio()
> -       * will not add it to the name. So the original loop trying to
> match an
> -       * existing port above might have missed this one. Let's do
> another
> -       * search ignoring the access size part of the string. At this
> point
> -       * nlen contains the length of the name up to the end of the
> address.
> -       */
> -      FOR_SERIAL_PORTS (port)
> -        if (grub_strncmp (port->name, name, nlen) == 0) {
> -          break;
> -        }
> -
>        port = grub_serial_ns8250_add_mmio (addr, acc_size, NULL);
> -      if (port == NULL)
> -        return NULL;
> +      if (port != NULL)
> +        return port;
>      }
> -  if (!port && grub_strcmp (name, "auto") == 0)
> +  if (grub_strcmp (name, "auto") == 0)
>      {
>        /* Look for an SPCR if any. If not, default to com0 */
>        port = grub_ns8250_spcr_init ();
> -      if (port == NULL)
> -        {
> -          FOR_SERIAL_PORTS (port)
> -            if (grub_strcmp (port->name, "com0") == 0)
> -              break;
> -       }
> +      if (port != NULL)
> +        return port;
> +      FOR_SERIAL_PORTS (port)
> +        if (grub_strcmp (port->name, "com0") == 0)
> +          return port;
>      }
>  #endif
>  
>  #ifdef GRUB_MACHINE_IEEE1275
> -  if (!port && grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/")
> - 1) == 0)
> +  if (grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) ==
> 0)
>      {
>        port = grub_ofserial_add_port (&name[sizeof ("ieee1275/") -
> 1]);
> -      if (port == NULL)
> -        return NULL;
> +      if (port != NULL)
> +        return port;
>      }
>  #endif
>  
> -  return port;
> +  return NULL;
>  }
>  
>  static grub_err_t
> diff --git a/include/grub/serial.h b/include/grub/serial.h
> index 17ee0f08b..ce3ec10ec 100644
> --- a/include/grub/serial.h
> +++ b/include/grub/serial.h
> @@ -219,4 +219,8 @@ extern void grub_serial_init (void);
>  extern void grub_serial_fini (void);
>  #endif
>  
> +extern struct grub_serial_port *grub_serial_ports;
> +#define FOR_SERIAL_PORTS(var) FOR_LIST_ELEMENTS((var),
> (grub_serial_ports))
> +
> +
>  #endif