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

[Benjamin Herrenschmidt]

On Wed, 2022-12-21 at 15:11 +0100, Daniel Kiper wrote:
> > +  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.

Same comment as patch 3, this is a pre-existing construct (look a few
lines above in the original code:

  if (!port && grub_memcmp (name, "port", sizeof ("port") - 1) == 0
      && grub_isxdigit (name [sizeof ("port") - 1]))
    {

I'll fix them all in the respun patch.

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

Indeed, that's not great, I'll fix.

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

I'm pretty sure it didn't but it might depend on a specific gcc 
version, I'll fix.

Cheers,
Ben.