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

[Benjamin Herrenschmidt]

On Thu, 2022-12-22 at 13:54 +0100, Daniel Kiper wrote:
> Thank you for quick turn around!
> 
> Now patches looks much better but...

Hehe :-)

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

Ok, that's fine, I wasn't sure what you wanted, it's easy enough for me
to respin and re-test.

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

I'll do a separate patch. I don't like sizeof () - 1 but it's indeed
more efficient at this point because grub_strlen() doesn't exploit
gcc's __builtin_strlen() (which would evaluate to a constant). So I'll
put them back in for now, but we should look at using more gcc
builtin's imho down the line.

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

Nah, I'll do a separate patch. I'll put it in the same series though.

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

Ah I missed that one, thanks.
> 
> > +      && 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/

Ack.

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

if *p is '\0' then addr is 0 (and *p1 is '\0'). But we wouldn't get
there in the first place because of the grub_isxdigit() in the previous
test. So we know *p is a digit at this point. In fact I don't even need
to test addr == 0, there could be universes where it's a valid MMIO
address :-) (not kidding, I've seen it, though nowhere we run grub
these days).

So the only thing we really care about at this point is the first non-
digit character which is *p1. It's either a dot or a '\0' for things to
be valild.

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

Right, it would go to whatever console is still active at this point
(probably BIOS console) which is probably not going to be terribly
useful for people who are tyring to use a serial port (for a reason ..
:-) but it won't hurt either.

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

Ack. I'll move the memcmp/strcmp change to a separate patch and leave
the sizeof alone

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

Oops :-)

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

No worries, I don't mind respinning. At least there is movement now :-)

I'll have new patches later today hopefully.

Cheers,
Ben.