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

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

[Josh Boyer]

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_notice instead.  Users will
no longer have their boot process uglified by the kernel reminding
us that firmware can and often is broken when the 'quiet' kernel
parameter is specified.  Ironic, considering BGRT is supposed to
make boot pretty to begin with.

Signed-off-by: Josh Boyer <jwboyer-rxtnV0ftBwyoClj4AeEUq9i2O/JbrIOy@public.gmane.org>
---

v2: Switch to using pr_notice instead of pr_debug

 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..6a2f5691b1ab 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_notice("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_notice("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_notice("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_notice("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_notice("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_notice("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_notice("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_notice("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_notice("Ignoring BGRT: failed to map image memory\n");
 		kfree(bgrt_image);
 		bgrt_image = NULL;
 		return;
-- 
2.5.5

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

[Josh Triplett]

On Tue, May 03, 2016 at 01:47:04PM -0400, 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_notice instead.  Users will
> no longer have their boot process uglified by the kernel reminding
> us that firmware can and often is broken when the 'quiet' kernel
> parameter is specified.  Ironic, considering BGRT is supposed to
> make boot pretty to begin with.
> 
> Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>

Thank you for the updated patch.

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> ---
> 
> v2: Switch to using pr_notice instead of pr_debug
> 
>  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..6a2f5691b1ab 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_notice("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_notice("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_notice("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_notice("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_notice("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_notice("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_notice("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_notice("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_notice("Ignoring BGRT: failed to map image memory\n");
>  		kfree(bgrt_image);
>  		bgrt_image = NULL;
>  		return;
> -- 
> 2.5.5
> 

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

[Matt Fleming]

On Tue, 03 May, at 01:47:04PM, 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_notice instead.  Users will
> no longer have their boot process uglified by the kernel reminding
> us that firmware can and often is broken when the 'quiet' kernel
> parameter is specified.  Ironic, considering BGRT is supposed to
> make boot pretty to begin with.
> 
> Signed-off-by: Josh Boyer <jwboyer-rxtnV0ftBwyoClj4AeEUq9i2O/JbrIOy@public.gmane.org>
> ---
> 
> v2: Switch to using pr_notice instead of pr_debug
> 
>  arch/x86/platform/efi/efi-bgrt.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>

Applied. Thanks everyone!

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

[Josh Triplett]

On Tue, May 03, 2016 at 01:47:04PM -0400, 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_notice instead.  Users will
> no longer have their boot process uglified by the kernel reminding
> us that firmware can and often is broken when the 'quiet' kernel
> parameter is specified.  Ironic, considering BGRT is supposed to
> make boot pretty to begin with.
> 
> Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>

Whitespace nit below that I missed on my initial review.  I don't think
it's worth holding up the patch queue for.

> --- 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_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
>  		       bgrt_tab->header.length, sizeof(*bgrt_tab));

On this and other lines with continuations, the continuation line was
indented to match the 'pr_err('; changing that to 'pr_notice(' makes
that continuation indentation no longer make sense.  Perhaps it should
change to a single tab, rather than attempting to line up with the start
of the first argument?

- Josh Triplett