[PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

We've started getting reports of users seeing Machine Check Exceptions
when booting their Samsung laptops in UEFI mode,

     https://bugzilla.kernel.org/show_bug.cgi?id=47121

This module seems to be the culprit as it's grovelling around in the
0xf0000 region which has no mapping in either the e820 or EFI memory
maps on the affected machines.

Reported-by: Alessandro Crismani <alessandro.crismani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reported-by: Mikhail Bakhterev <mike.bakhterev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Patrick H <kernel-0BLq+tI11CLk1uMJSBkQmQ@public.gmane.org>
Cc: H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
Cc: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Corentin Chary <corentincj-EjuBZuxMvz2sTnJN9+BGXg@public.gmane.org>
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/platform/x86/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index c86bae8..0fe5200 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -717,7 +717,7 @@ config XO15_EBOOK
 
 config SAMSUNG_LAPTOP
 	tristate "Samsung Laptop driver"
-	depends on X86
+	depends on X86 && !EFI
 	depends on RFKILL || RFKILL = n
 	depends on BACKLIGHT_CLASS_DEVICE
 	select LEDS_CLASS
-- 
1.7.11.7

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Corentin Chary]

On Sun, Nov 4, 2012 at 5:35 PM, Matt Fleming <matt@console-pimps.org> wrote:
> From: Matt Fleming <matt.fleming@intel.com>
>
> We've started getting reports of users seeing Machine Check Exceptions
> when booting their Samsung laptops in UEFI mode,
>
>      https://bugzilla.kernel.org/show_bug.cgi?id=47121
>
> This module seems to be the culprit as it's grovelling around in the
> 0xf0000 region which has no mapping in either the e820 or EFI memory
> maps on the affected machines.
>
> Reported-by: Alessandro Crismani <alessandro.crismani@gmail.com>
> Reported-by: Mikhail Bakhterev <mike.bakhterev@gmail.com>
> Tested-by: Patrick H <kernel@feystorm.net>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Matthew Garrett <mjg@redhat.com>
> Cc: Corentin Chary <corentincj@iksaif.net>
> Cc: stable@vger.kernel.org
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  drivers/platform/x86/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index c86bae8..0fe5200 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -717,7 +717,7 @@ config XO15_EBOOK
>
>  config SAMSUNG_LAPTOP
>         tristate "Samsung Laptop driver"
> -       depends on X86
> +       depends on X86 && !EFI
>         depends on RFKILL || RFKILL = n
>         depends on BACKLIGHT_CLASS_DEVICE
>         select LEDS_CLASS
> --
> 1.7.11.7
>

Acked-by: Corentin Chary <corentin.chary@gmail.com>

Matthew could you merge this in your tree ? Thanks.

Matt, any idea if SABI doesn't exist on these machines, or is just
mapped somewhere else ?
By any luck, could Samsung tell you how to safely detect SABI aware machines ?

--
Corentin Chary
http://xf.iksaif.net

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Alan Cox]

> Acked-by: Corentin Chary <corentin.chary-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

This is totally bogus and prevents users build a kernel which can work in
either mode. As such its a regression.

Do the detection check at runtime. If it was booted via EFI then don't
grovel in places you shouldn't. Indeed its possible EFI should reserve
those memory regions ?

Alan

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Corentin Chary]

On Sun, Nov 4, 2012 at 7:37 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> Acked-by: Corentin Chary <corentin.chary@gmail.com>
>
> This is totally bogus and prevents users build a kernel which can work in
> either mode. As such its a regression.

Arg.. Sorry for that, I didn't realized that CONFIG_EFI=y was not
something rare these days.

> Do the detection check at runtime. If it was booted via EFI then don't
> grovel in places you shouldn't. Indeed its possible EFI should reserve
> those memory regions ?

I wonder how the windows driver works in this case.. Maybe they use
something completly different, and the SABI interface is still there
because nobody removed/disabled it ? In this case it's probably not a
good idea to use it on these machines since the implementation is
likely to be completly broken.

--
Corentin Chary
http://xf.iksaif.net

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Greg KH]

On Mon, Nov 05, 2012 at 09:12:01AM +0000, Corentin Chary wrote:
> On Sun, Nov 4, 2012 at 7:37 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >> Acked-by: Corentin Chary <corentin.chary@gmail.com>
> >
> > This is totally bogus and prevents users build a kernel which can work in
> > either mode. As such its a regression.
> 
> Arg.. Sorry for that, I didn't realized that CONFIG_EFI=y was not
> something rare these days.
> 
> > Do the detection check at runtime. If it was booted via EFI then don't
> > grovel in places you shouldn't. Indeed its possible EFI should reserve
> > those memory regions ?
> 
> I wonder how the windows driver works in this case.. Maybe they use
> something completly different, and the SABI interface is still there
> because nobody removed/disabled it ? In this case it's probably not a
> good idea to use it on these machines since the implementation is
> likely to be completly broken.

Odds are, the windows driver just isn't even loaded on the newer
machines, as ACPI works just fine for this.  But, we don't have the
option of shipping custom systems for different laptops like Samsung
does, so we have to probe for this somehow.

Initally we were looking at the DMI strings for specific laptop models,
but that got annoying as we had to keep adding new models.  So we now
just check the memory locations for all Samsung laptops, which was
working fine.

What is the problem if we try to access this memory on UEFI machines?
What is the error that is caused?

Is there any "this_is_a_uefi_system()" type call drivers can make to
just opt-out if that call is true?

thanks,

greg k-h

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Matt Fleming]

On Mon, 2012-11-05 at 11:30 +0100, Greg KH wrote:
> Odds are, the windows driver just isn't even loaded on the newer
> machines, as ACPI works just fine for this.  But, we don't have the
> option of shipping custom systems for different laptops like Samsung
> does, so we have to probe for this somehow.
> 
> Initally we were looking at the DMI strings for specific laptop models,
> but that got annoying as we had to keep adding new models.  So we now
> just check the memory locations for all Samsung laptops, which was
> working fine.
> 
> What is the problem if we try to access this memory on UEFI machines?
> What is the error that is caused?

Machine Check Exceptions are generated.

> Is there any "this_is_a_uefi_system()" type call drivers can make to
> just opt-out if that call is true?

There is the 'efi_enabled' variable, but it doesn't strictly mean
"this_is_a_uefi_system()", it actually means "Do we have EFI runtime
services?". The whole thing is a bit of a mess and I'm planning on
cleaning it up this week.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Alan Cox]

> There is the 'efi_enabled' variable, but it doesn't strictly mean
> "this_is_a_uefi_system()", it actually means "Do we have EFI runtime
> services?". The whole thing is a bit of a mess and I'm planning on
> cleaning it up this week.


As far as I can understand it we should be reserving those areas on a
UEFI booted machine and just marking them as busy so that they are not
available to any drivers to go peering into.

Alan

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Matt Fleming]

On Mon, 2012-11-05 at 12:07 +0000, Alan Cox wrote:
> > There is the 'efi_enabled' variable, but it doesn't strictly mean
> > "this_is_a_uefi_system()", it actually means "Do we have EFI runtime
> > services?". The whole thing is a bit of a mess and I'm planning on
> > cleaning it up this week.
> 
> 
> As far as I can understand it we should be reserving those areas on a
> UEFI booted machine and just marking them as busy so that they are not
> available to any drivers to go peering into.

Yes, that would be the best way to resolve this going forward.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Matt Fleming]

On Sun, 2012-11-04 at 19:37 +0000, Alan Cox wrote:
> > Acked-by: Corentin Chary <corentin.chary@gmail.com>
> 
> This is totally bogus and prevents users build a kernel which can work in
> either mode. As such its a regression.
> 
> Do the detection check at runtime. If it was booted via EFI then don't
> grovel in places you shouldn't. Indeed its possible EFI should reserve
> those memory regions ?

The kernel would have to reserve the gaps in the memory mappings since
there is no mapping in the EFI memory map for 0xf0000. There is no
support for that currently AFAIK.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Matt Fleming]

On Sun, 2012-11-04 at 17:44 +0000, Corentin Chary wrote:
> On Sun, Nov 4, 2012 at 5:35 PM, Matt Fleming <matt@console-pimps.org> wrote:
> > From: Matt Fleming <matt.fleming@intel.com>
> >
> > We've started getting reports of users seeing Machine Check Exceptions
> > when booting their Samsung laptops in UEFI mode,
> >
> >      https://bugzilla.kernel.org/show_bug.cgi?id=47121
> >
> > This module seems to be the culprit as it's grovelling around in the
> > 0xf0000 region which has no mapping in either the e820 or EFI memory
> > maps on the affected machines.
> >
> > Reported-by: Alessandro Crismani <alessandro.crismani@gmail.com>
> > Reported-by: Mikhail Bakhterev <mike.bakhterev@gmail.com>
> > Tested-by: Patrick H <kernel@feystorm.net>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > Cc: Matthew Garrett <mjg@redhat.com>
> > Cc: Corentin Chary <corentincj@iksaif.net>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> > ---
> >  drivers/platform/x86/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index c86bae8..0fe5200 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -717,7 +717,7 @@ config XO15_EBOOK
> >
> >  config SAMSUNG_LAPTOP
> >         tristate "Samsung Laptop driver"
> > -       depends on X86
> > +       depends on X86 && !EFI
> >         depends on RFKILL || RFKILL = n
> >         depends on BACKLIGHT_CLASS_DEVICE
> >         select LEDS_CLASS
> > --
> > 1.7.11.7
> >
> 
> Acked-by: Corentin Chary <corentin.chary@gmail.com>
> 
> Matthew could you merge this in your tree ? Thanks.
> 
> Matt, any idea if SABI doesn't exist on these machines, or is just
> mapped somewhere else ?
> By any luck, could Samsung tell you how to safely detect SABI aware machines ?

I don't know whether or not SABI exists on these machines and I don't
have one of them to try and figure that out, sorry.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Jonathan Nieder]

Matt Fleming wrote:

> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -717,7 +717,7 @@ config XO15_EBOOK
> 
>  config SAMSUNG_LAPTOP
>  	tristate "Samsung Laptop driver"
> -	depends on X86
> +	depends on X86 && !EFI

That means distros would just not get the samsung-laptop driver.
Is there a runtime check that could be used instead?

Thanks,
Jonathan

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Matt Fleming]

On Sun, 2012-11-04 at 09:47 -0800, Jonathan Nieder wrote:
> Matt Fleming wrote:
> 
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -717,7 +717,7 @@ config XO15_EBOOK
> > 
> >  config SAMSUNG_LAPTOP
> >  	tristate "Samsung Laptop driver"
> > -	depends on X86
> > +	depends on X86 && !EFI
> 
> That means distros would just not get the samsung-laptop driver.
> Is there a runtime check that could be used instead?

Well, the closest thing we have at the moment is the 'efi_enabled'
variable, but that doesn't actually mean "We were booted from EFI?", it
means "Do we have EFI runtime services?", and that's not a broad enough
check for this case. We don't have access to the EFI runtime services
when a 64-bit kernel is booted from 32-bit EFI firmware or vice-versa.
Notably the chromebooks use this scheme. And seeing as Samsung make
chromebooks, I'm not convinced we won't hit that case.

But yeah, you've got a valid point. Clearly we need a way to check this
at runtime. I'll repsin this patch.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Greg KH]

On Sun, Nov 04, 2012 at 05:35:06PM +0000, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@intel.com>
> 
> We've started getting reports of users seeing Machine Check Exceptions
> when booting their Samsung laptops in UEFI mode,
> 
>      https://bugzilla.kernel.org/show_bug.cgi?id=47121
> 
> This module seems to be the culprit as it's grovelling around in the
> 0xf0000 region which has no mapping in either the e820 or EFI memory
> maps on the affected machines.

So, does this mean that if we try to ioremap a memory location in the
kernel, like this driver is, it will not fail, but, when accessing the
memory, bad things happen?

That's not good, shouldn't the call to ioremap_nocache() have failed
originally?  That sounds like a core EFI/platform bug here, and one that
you might run into other places.

Shouldn't fixing that be the real fix?


thanks,

greg k-h

Re: [PATCH] samsung-laptop: Disable if CONFIG_EFI=y

[Matt Fleming]

On Mon, 2012-11-05 at 11:37 +0100, Greg KH wrote:
> On Sun, Nov 04, 2012 at 05:35:06PM +0000, Matt Fleming wrote:
> > From: Matt Fleming <matt.fleming@intel.com>
> > 
> > We've started getting reports of users seeing Machine Check Exceptions
> > when booting their Samsung laptops in UEFI mode,
> > 
> >      https://bugzilla.kernel.org/show_bug.cgi?id=47121
> > 
> > This module seems to be the culprit as it's grovelling around in the
> > 0xf0000 region which has no mapping in either the e820 or EFI memory
> > maps on the affected machines.
> 
> So, does this mean that if we try to ioremap a memory location in the
> kernel, like this driver is, it will not fail, but, when accessing the
> memory, bad things happen?

Yes. I'm not sure exactly what sequence of memory accesses is causing
the MCEs because the above bug report states that the MCEs were frequent
but intermittent and I don't have any hardware to reproduce this on.

> That's not good, shouldn't the call to ioremap_nocache() have failed
> originally?  That sounds like a core EFI/platform bug here, and one that
> you might run into other places.

I think it's an x86 bug. AFAIK it's entirely possible to ioremap
arbitrary memory addresses whether or not the firmware/BIOS knows about
them (by which I mean, includes a mapping in the e820 map). It's just
that traditionally the firmware didn't throw MCEs when you peeked at
some part of the address map. The < 1MB region of the x86 address map
also fills me with visions of voodoo.

Jacob Shin and Yinghai Lu have been working on some patches that avoid
mapping the holes (the address regions for which there is no entry in
the e820 memmap) and I think that could be used to address this problem.

> Shouldn't fixing that be the real fix?

Absolutely, and it was always my intention to fix this properly so that
I didn't have the same bug reported to me over and over again. However,
initially I was more concerned about writing a fix that was good enough
and small enough to be marked for stable. Messing with the kernel memory
map, particularly for EFI systems, has been fraught with peril in the
past.

-- 
Matt Fleming, Intel Open Source Technology Center