Re: [PATCH] x86/efi-bgrt: Switch all pr_err() to pr_debug() for invalid BGRT

[MÃshe van der Sterre]

On 04/27/2016 03:56 PM, Josh Boyer wrote:
> On Wed, Apr 27, 2016 at 9:26 AM, MÃshe van der Sterre <me@xxxxxxxx> wrote:
> > (additionally CC-ing Josh Triplett)
> Thanks for doing so.  I completely forgot.
>
> > On 04/27/2016 02:50 PM, Josh Boyer wrote:
> > > The promise of pretty boot splashes from firmware via BGRT was at
> > > best only that; a promise.  The kernel diligently checks to make
> > > sure the BGRT data firmware gives it is valid, and dutifully warns
> > > the user when it isn't.  However, it does so via the pr_err log
> > > level which seems unnecessary.  The user cannot do anything about
> > > this and there really isn't an error on the part of Linux to
> > > correct.
> > >
> > > This lowers the log level by using pr_debug instead.  Users will
> > > no longer have their boot process uglified by the kernel reminding
> > > us that firmware can and often is broken.  Ironic, considering
> > > BGRT is supposed to make boot pretty to begin with.
> > Hi Josh Boyer,
> >
> > Are you seeing these errors somewhere? I recently fixed the error "Ignoring
> We have a user that reports seeing:
>
> "Ignoring BGRT: Invalid version 0 (expected 1)"
>
> on a Lenovo T430 machine.  We've had a few other scattered reports on
> various machine types since BGRT went into the kernel as well.
Ok. With this information, I think pr_debug is indeed better.
> > BGRT: invalid status 0 (expected 1)" because Linux apparently interpreted
> > that part of the specification differently than others.
> > If that's the error you are seeing, perhaps your problem is already solved
> > in recent kernels? (fixed in commit 66dbe99)
> >
> > Personally I agree that BGRT messages should not annoy actual users of
> > production firmwares.
> > However I also agree with the previous consensus that these checks (for
> > actual spec violations) should remain pr_err unless some production firmware
> > is triggering them. What do you think?
> Production firmware is literally the only firmware end users will ever
> see.  I don't see much point in leaving scary error messages in the
> kernel to complain about things the user has no chance of fixing or in
> almost all cases even reporting to people who could fix it.
In principle I can understand the wish to show big scary error messages 
to firmware developers doing it wrong.

With that said:
The patch looks good to me, but Josh Triplett and Matt Fleming their 
opinions might be better informed than mine.

> josh
>
> > > Signed-off-by: Josh Boyer <jwboyer@xxxxxxxxxxxxxxxxx>
> > > ---
> > >    arch/x86/platform/efi/efi-bgrt.c | 18 +++++++++---------
> > >    1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/x86/platform/efi/efi-bgrt.c
> > > b/arch/x86/platform/efi/efi-bgrt.c
> > > index a2433817c987..6f70d2ac8029 100644
> > > --- a/arch/x86/platform/efi/efi-bgrt.c
> > > +++ b/arch/x86/platform/efi/efi-bgrt.c
> > > @@ -43,40 +43,40 @@ void __init efi_bgrt_init(void)
> > >                  return;
> > >          if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
> > > -               pr_err("Ignoring BGRT: invalid length %u (expected
> > > %zu)\n",
> > > +               pr_debug("Ignoring BGRT: invalid length %u (expected
> > > %zu)\n",
> > >                         bgrt_tab->header.length, sizeof(*bgrt_tab));
> > >                  return;
> > >          }
> > >          if (bgrt_tab->version != 1) {
> > > -               pr_err("Ignoring BGRT: invalid version %u (expected 1)\n",
> > > +               pr_debug("Ignoring BGRT: invalid version %u (expected
> > > 1)\n",
> > >                         bgrt_tab->version);
> > >                  return;
> > >          }
> > >          if (bgrt_tab->status & 0xfe) {
> > > -               pr_err("Ignoring BGRT: reserved status bits are non-zero
> > > %u\n",
> > > +               pr_debug("Ignoring BGRT: reserved status bits are non-zero
> > > %u\n",
> > >                         bgrt_tab->status);
> > >                  return;
> > >          }
> > >          if (bgrt_tab->image_type != 0) {
> > > -               pr_err("Ignoring BGRT: invalid image type %u (expected
> > > 0)\n",
> > > +               pr_debug("Ignoring BGRT: invalid image type %u (expected
> > > 0)\n",
> > >                         bgrt_tab->image_type);
> > >                  return;
> > >          }
> > >          if (!bgrt_tab->image_address) {
> > > -               pr_err("Ignoring BGRT: null image address\n");
> > > +               pr_debug("Ignoring BGRT: null image address\n");
> > >                  return;
> > >          }
> > >          image = memremap(bgrt_tab->image_address, sizeof(bmp_header),
> > > MEMREMAP_WB);
> > >          if (!image) {
> > > -               pr_err("Ignoring BGRT: failed to map image header
> > > memory\n");
> > > +               pr_debug("Ignoring BGRT: failed to map image header
> > > memory\n");
> > >                  return;
> > >          }
> > >          memcpy(&bmp_header, image, sizeof(bmp_header));
> > >          memunmap(image);
> > >          if (bmp_header.id != 0x4d42) {
> > > -               pr_err("Ignoring BGRT: Incorrect BMP magic number 0x%x
> > > (expected 0x4d42)\n",
> > > +               pr_debug("Ignoring BGRT: Incorrect BMP magic number 0x%x
> > > (expected 0x4d42)\n",
> > >                          bmp_header.id);
> > >                  return;
> > >          }
> > > @@ -84,14 +84,14 @@ void __init efi_bgrt_init(void)
> > >          bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL | __GFP_NOWARN);
> > >          if (!bgrt_image) {
> > > -               pr_err("Ignoring BGRT: failed to allocate memory for image
> > > (wanted %zu bytes)\n",
> > > +               pr_debug("Ignoring BGRT: failed to allocate memory for
> > > image (wanted %zu bytes)\n",
> > >                         bgrt_image_size);
> > >                  return;
> > >          }
> > >          image = memremap(bgrt_tab->image_address, bmp_header.size,
> > > MEMREMAP_WB);
> > >          if (!image) {
> > > -               pr_err("Ignoring BGRT: failed to map image memory\n");
> > > +               pr_debug("Ignoring BGRT: failed to map image memory\n");
> > >                  kfree(bgrt_image);
> > >                  bgrt_image = NULL;
> > >                  return;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html