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

["Môshe van der Sterre" <me@moshe.nl>]

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@moshe.nl> 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@fedoraproject.org>
>>> ---
>>>    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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html