[PATCH] Don't print an error on unsupported BGRT version.

[PATCH] Don't print an error on unsupported BGRT version.

[Peter Jones]

BGRT can legitimately be a different version from what we support, and
that's a problem with the driver not supporting something, not an error
that needs to be surfaced to the user.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/efi-bgrt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
index d7f997f..e568701 100644
--- a/arch/x86/platform/efi/efi-bgrt.c
+++ b/arch/x86/platform/efi/efi-bgrt.c
@@ -46,7 +46,7 @@ void __init efi_bgrt_init(void)
 		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;
 	}
-- 
2.4.3

Re: [PATCH] Don't print an error on unsupported BGRT version.

[Josh Triplett]

On Wed, Jul 22, 2015 at 12:40:53PM -0400, Peter Jones wrote:
> BGRT can legitimately be a different version from what we support, and
> that's a problem with the driver not supporting something, not an error
> that needs to be surfaced to the user.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Are you actually seeing this error on a real system?  Or is this just a
theoretical concern with some future version?

While I agree that this doesn't need to be pr_err, I don't think
pr_debug is appropriate either.  It may mean the driver needs updating,
or it may mean that a system in the wild is actually broken and has an
invalid version number.  I'd prefer at least pr_warn so that people see
it and report it, but I could live with pr_notice; that should hide it
from systems booting in quiet mode, while still having it in the log
even on non-debug kernels.

- Josh Triplett

>  arch/x86/platform/efi/efi-bgrt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
> index d7f997f..e568701 100644
> --- a/arch/x86/platform/efi/efi-bgrt.c
> +++ b/arch/x86/platform/efi/efi-bgrt.c
> @@ -46,7 +46,7 @@ void __init efi_bgrt_init(void)
>  		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;
>  	}
> -- 
> 2.4.3
> 

Re: [PATCH] Don't print an error on unsupported BGRT version.

[Peter Jones]

On Wed, Jul 22, 2015 at 09:53:57AM -0700, Josh Triplett wrote:
> On Wed, Jul 22, 2015 at 12:40:53PM -0400, Peter Jones wrote:
> > BGRT can legitimately be a different version from what we support, and
> > that's a problem with the driver not supporting something, not an error
> > that needs to be surfaced to the user.
> > 
> > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Are you actually seeing this error on a real system?  Or is this just a
> theoretical concern with some future version?

Yeah, I have several systems where I see it every boot - and with
"quiet", it's the only message I see.  In all cases the version is 0,
i.e. the code was written before it was included in the standard, and
it's a version the driver legitimately does not handle.  Admittedly
these are mostly machines for developing UEFI on (including the Intel
S1200RP board with the UDK2014 B2 firmware.)  But the fact is there are
systems you can *buy* which will show this as an error, and it really
isn't an error.  It's just an earlier version than we support.

> While I agree that this doesn't need to be pr_err, I don't think
> pr_debug is appropriate either.  It may mean the driver needs updating,
> or it may mean that a system in the wild is actually broken and has an
> invalid version number.  I'd prefer at least pr_warn so that people see
> it and report it, but I could live with pr_notice; that should hide it
> from systems booting in quiet mode, while still having it in the log
> even on non-debug kernels.

I'd be okay with pr_notice.  I'll send a follow-up.

-- 
        Peter

Re: [PATCH] Don't print an error on unsupported BGRT version.

[josh-iaAMLnmF4UmaiuxdJuQwMA]

On Wed, Jul 22, 2015 at 01:09:06PM -0400, Peter Jones wrote:
> On Wed, Jul 22, 2015 at 09:53:57AM -0700, Josh Triplett wrote:
> > On Wed, Jul 22, 2015 at 12:40:53PM -0400, Peter Jones wrote:
> > > BGRT can legitimately be a different version from what we support, and
> > > that's a problem with the driver not supporting something, not an error
> > > that needs to be surfaced to the user.
> > > 
> > > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > 
> > Are you actually seeing this error on a real system?  Or is this just a
> > theoretical concern with some future version?
> 
> Yeah, I have several systems where I see it every boot - and with
> "quiet", it's the only message I see.  In all cases the version is 0,
> i.e. the code was written before it was included in the standard, and
> it's a version the driver legitimately does not handle.  Admittedly
> these are mostly machines for developing UEFI on (including the Intel
> S1200RP board with the UDK2014 B2 firmware.)  But the fact is there are
> systems you can *buy* which will show this as an error, and it really
> isn't an error.  It's just an earlier version than we support.

We should support version 0.  Is the structure layout and semantic the
same?  Could you submit a patch to handle version 0?

> > While I agree that this doesn't need to be pr_err, I don't think
> > pr_debug is appropriate either.  It may mean the driver needs updating,
> > or it may mean that a system in the wild is actually broken and has an
> > invalid version number.  I'd prefer at least pr_warn so that people see
> > it and report it, but I could live with pr_notice; that should hide it
> > from systems booting in quiet mode, while still having it in the log
> > even on non-debug kernels.
> 
> I'd be okay with pr_notice.  I'll send a follow-up.

If we can handle 0, I'd prefer to do that and then have a pr_warn for
versions other than 0 and 1.

- Josh Triplett

[PATCH] Don't print an error on unsupported BGRT version.

[Peter Jones]

BGRT can legitimately be a different version from what we support, and
that's a problem with the driver not supporting something, not an error
that needs to be surfaced to the user.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/efi-bgrt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
index d7f997f..e568701 100644
--- a/arch/x86/platform/efi/efi-bgrt.c
+++ b/arch/x86/platform/efi/efi-bgrt.c
@@ -46,7 +46,7 @@ void __init efi_bgrt_init(void)
 		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;
 	}
-- 
2.4.3

[PATCH] Don't print an error on unsupported BGRT version.

[Peter Jones]

BGRT can legitimately be a different version from what we support, and
that's a problem with the driver not supporting something, not an error
that needs to be surfaced to the user.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/efi-bgrt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
index d7f997f..e369162 100644
--- a/arch/x86/platform/efi/efi-bgrt.c
+++ b/arch/x86/platform/efi/efi-bgrt.c
@@ -46,7 +46,7 @@ void __init efi_bgrt_init(void)
 		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;
 	}
-- 
2.4.3

Re: [PATCH] Don't print an error on unsupported BGRT version.

[josh-iaAMLnmF4UmaiuxdJuQwMA]

On Wed, Jul 22, 2015 at 01:13:27PM -0400, Peter Jones wrote:
> BGRT can legitimately be a different version from what we support, and
> that's a problem with the driver not supporting something, not an error
> that needs to be surfaced to the user.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

As mentioned in my previous mail, please consider adding support for
version 0 instead, which would fix this issue in a better way.

If that's not possible, then sure, this patch is fine.  But ideally this
warning should tend to lead to a patch to the bgrt driver adding support
for the new version (or in this case the old version), rather than just
silencing the warning.

>  arch/x86/platform/efi/efi-bgrt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
> index d7f997f..e369162 100644
> --- a/arch/x86/platform/efi/efi-bgrt.c
> +++ b/arch/x86/platform/efi/efi-bgrt.c
> @@ -46,7 +46,7 @@ void __init efi_bgrt_init(void)
>  		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;
>  	}
> -- 
> 2.4.3
> 

Re: [PATCH] Don't print an error on unsupported BGRT version.

[Peter Jones]

On Wed, Jul 22, 2015 at 11:26:48AM -0700, josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org wrote:
> On Wed, Jul 22, 2015 at 01:13:27PM -0400, Peter Jones wrote:
> > BGRT can legitimately be a different version from what we support, and
> > that's a problem with the driver not supporting something, not an error
> > that needs to be surfaced to the user.
> > 
> > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> As mentioned in my previous mail, please consider adding support for
> version 0 instead, which would fix this issue in a better way.
> 
> If that's not possible, then sure, this patch is fine.  But ideally this
> warning should tend to lead to a patch to the bgrt driver adding support
> for the new version (or in this case the old version), rather than just
> silencing the warning.

Well, the inherent problem with version 0 is that it /isn't/
standardized - though on the machine in front of me (a retail Lenovo X1
Carbon), eyeballing it, it looks exactly the same.  I don't know how
we'd verify that none of the fields mean anything different.  My
suspicion is that some spec from before it was part of ACPI exists, or
maybe some earlier proposal to ACPI had version=0, but I have no access
to ACPI proposals from before they became part of UEFI.  The fact that
some retail machines seem to have version=0 makes me think it
/might/ be safe, but I can't really tell.  For all I know windows has
special-cased it in vendor supplied drivers that check the oem_id.

I've got something now that makes sense on one machine, and I need to
check out a few more machines, and then I'll send something along.

-- 
        Peter