[PATCH] Revert "x86/reboot, efi: Use EFI reboot for Acer TravelMate X514-51T"

[PATCH] Revert "x86/reboot, efi: Use EFI reboot for Acer TravelMate X514-51T"

[Jian-Hong Pan]

This reverts commit 0082517fa4bce073e7cf542633439f26538a14cc.

According to Acer's information, this reboot issue is fixed since 1.08
and newer BIOS. So, we can revert the quirk.

Fixes: 0082517fa4bc ("x86/reboot, efi: Use EFI reboot for Acer TravelMate X514-51T")
Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
---
 arch/x86/kernel/reboot.c | 21 ---------------------
 include/linux/efi.h      |  7 +------
 2 files changed, 1 insertion(+), 27 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 0cc7c0b106bb..92177ccd47f3 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -81,19 +81,6 @@ static int __init set_bios_reboot(const struct dmi_system_id *d)
 	return 0;
 }
 
-/*
- * Some machines don't handle the default ACPI reboot method and
- * require the EFI reboot method:
- */
-static int __init set_efi_reboot(const struct dmi_system_id *d)
-{
-	if (reboot_type != BOOT_EFI && !efi_runtime_disabled()) {
-		reboot_type = BOOT_EFI;
-		pr_info("%s series board detected. Selecting EFI-method for reboot.\n", d->ident);
-	}
-	return 0;
-}
-
 void __noreturn machine_real_restart(unsigned int type)
 {
 	local_irq_disable();
@@ -179,14 +166,6 @@ static const struct dmi_system_id reboot_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "AOA110"),
 		},
 	},
-	{	/* Handle reboot issue on Acer TravelMate X514-51T */
-		.callback = set_efi_reboot,
-		.ident = "Acer TravelMate X514-51T",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
-			DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate X514-51T"),
-		},
-	},
 
 	/* Apple */
 	{	/* Handle problems with rebooting on Apple MacBook5 */
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 7efd7072cca5..8375bbc6e739 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1529,12 +1529,7 @@ efi_status_t efi_parse_options(char const *cmdline);
 efi_status_t efi_setup_gop(struct screen_info *si, efi_guid_t *proto,
 			   unsigned long size);
 
-#ifdef CONFIG_EFI
-extern bool efi_runtime_disabled(void);
-#else
-static inline bool efi_runtime_disabled(void) { return true; }
-#endif
-
+bool efi_runtime_disabled(void);
 extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
 extern unsigned long efi_call_virt_save_flags(void);
 
-- 
2.25.1

Re: [PATCH] Revert "x86/reboot, efi: Use EFI reboot for Acer TravelMate X514-51T"

[Borislav Petkov]

On Thu, Mar 12, 2020 at 04:33:42PM +0800, Jian-Hong Pan wrote:
> This reverts commit 0082517fa4bce073e7cf542633439f26538a14cc.
> 
> According to Acer's information, this reboot issue is fixed since 1.08
> and newer BIOS. So, we can revert the quirk.

We can?

How do you know *everyone* affected will update their BIOS?

And what's the downside of keeping it?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] Revert "x86/reboot, efi: Use EFI reboot for Acer TravelMate X514-51T"

[Jian-Hong Pan]

Borislav Petkov <bp@alien8.de> 於 2020年3月12日 週四 下午6:46寫道：
>
> On Thu, Mar 12, 2020 at 04:33:42PM +0800, Jian-Hong Pan wrote:
> > This reverts commit 0082517fa4bce073e7cf542633439f26538a14cc.
> >
> > According to Acer's information, this reboot issue is fixed since 1.08
> > and newer BIOS. So, we can revert the quirk.
>
> We can?
>
> How do you know *everyone* affected will update their BIOS?
>
> And what's the downside of keeping it?

Good question!
How about add DMI_BIOS_VERSION for BIOS DMI_MATCH.  For example,
DMI_MATCH(DMI_BIOS_VERSION, "V1.0")

The BIOS versions listed on Acer website [1] are 1.04, 1.10 ...
The reboot issue is fixed since BIOS 1.08.  So, the only the string
like "V1.0*" should apply the quirk.

But, that will raise another question:  Since the original quirk works
for all Acer X514-51T and the quirk cannot be removed for older BIOS.
Why not keep only original matching items for all Acer X514-51T
laptops?

I am not sure which option is better.  Any comment?

[1] https://www.acer.com/ac/en/US/content/support-product/7889?b=1

BR,
Jian-Hong Pan

Re: [PATCH] Revert "x86/reboot, efi: Use EFI reboot for Acer TravelMate X514-51T"

[Borislav Petkov]

On Mon, Mar 16, 2020 at 05:17:56PM +0800, Jian-Hong Pan wrote:
> But, that will raise another question:  Since the original quirk works
> for all Acer X514-51T and the quirk cannot be removed for older BIOS.
> Why not keep only original matching items for all Acer X514-51T
> laptops?

What does the "original matching items" mean?

> I am not sure which option is better.  Any comment?

If you mean, "let's not do anything and fix it only when there's really
a need to fix anything", then yes, I agree.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] Revert "x86/reboot, efi: Use EFI reboot for Acer TravelMate X514-51T"

[Jian-Hong Pan]

Borislav Petkov <bp@alien8.de> 於 2020年3月16日 週一 下午6:01寫道：
>
> On Mon, Mar 16, 2020 at 05:17:56PM +0800, Jian-Hong Pan wrote:
> > But, that will raise another question:  Since the original quirk works
> > for all Acer X514-51T and the quirk cannot be removed for older BIOS.
> > Why not keep only original matching items for all Acer X514-51T
> > laptops?
>
> What does the "original matching items" mean?

I should make it more clearly.

The quirk's original matching items for Acer TravelMate X514-51T from
commit 0082517fa4bc ("x86/reboot, efi: Use EFI reboot for Acer
TravelMate X514-51T"):

        {       /* Handle reboot issue on Acer TravelMate X514-51T */
                .callback = set_efi_reboot,
                .ident = "Acer TravelMate X514-51T",
                .matches = {
                        DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
                        DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate X514-51T"),
                },
        },

These matching items make all Acer TravelMate X514-51Ts apply the quirk.

If BIOS version condition is added like:

        {       /* Handle reboot issue on Acer TravelMate X514-51T */
                .callback = set_efi_reboot,
                .ident = "Acer TravelMate X514-51T",
                .matches = {
                        DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
                        DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate X514-51T"),
                        DMI_MATCH(DMI_BIOS_VERSION, "V1.0"),
                },
        },

Then, only Acer TravelMate X514-51T with older BIOS (1.04 and before,
according BIOS version listed on Acer's website [1]) will apply the
quirk.
The one with newer BIOS's reboot type will be defined later by the codes.

[1] https://www.acer.com/ac/en/US/content/support-product/7889?b=1

> > I am not sure which option is better.  Any comment?
>
> If you mean, "let's not do anything and fix it only when there's really
> a need to fix anything", then yes, I agree.

Got it!

Thanks,
Jian-Hong Pan

Re: [PATCH] Revert "x86/reboot, efi: Use EFI reboot for Acer TravelMate X514-51T"

[Daniel Drake]

On Thu, Mar 12, 2020 at 6:46 PM Borislav Petkov <bp@alien8.de> wrote:
> How do you know *everyone* affected will update their BIOS?
>
> And what's the downside of keeping it?

It could indeed be kept without user-visible downside, and that would
be the normal case for quirks that work around BIOS bugs.

But I had two reasons for suggesting that Jian-Hong should send this
revert patch, which may be worth some consideration:

 1. This was working around a BIOS bug truly separate from Linux to
the point where it was a little questionable for Linux to put a quirk
in place. The original bug was that after Linux completed executing
the reboot code, the machine would reboot, the BIOS would start
loading, and then crash well before loading the OS. Presumably
crashing on some state that Linux left that was not reset in the
machine's reboot stage. The vendor later found the issue (something
TPM-related) and fixed the BIOS to avoid the crash.
 2. We normally receive these units before they go into mass
production, so there's a decent chance that production versions
already include this BIOS fix.

Based on that I was considering that the patch could be reverted for
cleanliness/ At the same time, I do not have strong feelings on this,
no issues if the quirk is left in place.

Daniel

Re: [PATCH] Revert "x86/reboot, efi: Use EFI reboot for Acer TravelMate X514-51T"

[Borislav Petkov]

On Fri, Mar 20, 2020 at 08:37:54PM +0800, Daniel Drake wrote:
> Based on that I was considering that the patch could be reverted for
> cleanliness/ At the same time, I do not have strong feelings on this,
> no issues if the quirk is left in place.

Oh, I'd take the revert on cleanliness grounds any day of the week. But
if that broken fw has snuck out and someone comes and complains that it
wouldn't reboot properly, then I'd have to revert the revert.

And looking at that reboot_dmi_table[] of who's who of broken BIOSes,
one entry less ain't gonna make it prettier.

:-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette