diff mbox

[RFC,1/2] Revert "rtc-cmos: Add an alarm disable quirk"

Message ID 1431077665-3493-1-git-send-email-adrianhuang0701@gmail.com
State Superseded
Headers show

Commit Message

Adrian Huang May 8, 2015, 9:34 a.m. UTC
Commit d5a1c7e3fc38 ("rtc-cmos: Add an alarm disable quirk") that
added a special quirk is not needed because PATCH [2/2] of this
patchset makes the kernel more robust:
rtc: restore the RTC alarm time to the configured alarm time in BIOS
Setup

Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
---
 drivers/rtc/rtc-cmos.c | 52 --------------------------------------------------
 1 file changed, 52 deletions(-)

Comments

Borislav Petkov May 18, 2015, 9:47 a.m. UTC | #1
On Fri, May 08, 2015 at 05:34:25PM +0800, Adrian Huang wrote:
> Commit d5a1c7e3fc38 ("rtc-cmos: Add an alarm disable quirk") that
> added a special quirk is not needed because PATCH [2/2] of this
> patchset makes the kernel more robust:
> rtc: restore the RTC alarm time to the configured alarm time in BIOS
> Setup
> 
> Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
> ---
>  drivers/rtc/rtc-cmos.c | 52 --------------------------------------------------
>  1 file changed, 52 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index a82556a..9754564 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -29,8 +29,6 @@
>   * other drivers and utilities on correctly configured systems.
>   */
>  
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> @@ -41,7 +39,6 @@
>  #include <linux/pm.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
> -#include <linux/dmi.h>
>  
>  /* this is for "generic access to PC-style RTC" using CMOS_READ/CMOS_WRITE */
>  #include <asm-generic/rtc.h>
> @@ -380,50 +377,6 @@ static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>  	return 0;
>  }
>  
> -/*
> - * Do not disable RTC alarm on shutdown - workaround for b0rked BIOSes.
> - */
> -static bool alarm_disable_quirk;
> -
> -static int __init set_alarm_disable_quirk(const struct dmi_system_id *id)
> -{
> -	alarm_disable_quirk = true;
> -	pr_info("BIOS has alarm-disable quirk - RTC alarms disabled\n");
> -	return 0;
> -}
> -
> -static const struct dmi_system_id rtc_quirks[] __initconst = {
> -	/* https://bugzilla.novell.com/show_bug.cgi?id=805740 */
> -	{
> -		.callback = set_alarm_disable_quirk,
> -		.ident    = "IBM Truman",
> -		.matches  = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "4852570"),
> -		},

So, your series really fix the reboot issue on this already notoriously
buggy box.

I did try to confirm it 5 times just to be sure and in all 5, the box
remained off.

We're testing another box currently which has the same issue.

Now, for the next version of your patches, I'd ask you to put this patch
second. I.e., you want to introduce the new fix first and *then* remove
the quirk as this way we have a window where no quirk will be in place
and possible bisection will be disturbed needlessly even though we can
help it.

> -	},
> -	/* https://bugzilla.novell.com/show_bug.cgi?id=812592 */
> -	{
> -		.callback = set_alarm_disable_quirk,
> -		.ident    = "Gigabyte GA-990XA-UD3",
> -		.matches  = {
> -			DMI_MATCH(DMI_SYS_VENDOR,
> -					"Gigabyte Technology Co., Ltd."),
> -			DMI_MATCH(DMI_PRODUCT_NAME, "GA-990XA-UD3"),
> -		},

Looking at the bugzilla entry above, that should be Diego's box.

Diego, would you be able to test these patches? They're a better fix
than what I did last year.

If you want me to prepare an openSUSE kernel for you, let me know and
I'll do one. I'd only need to know which distro.

Thanks.
Diego Ercolani May 18, 2015, 10 a.m. UTC | #2
Hello, sure I can.
In the meanwhile I upgraded my linux box to opensuse 13.2 with actual
kernel: 3.16.7-21-desktop
so the patch should be applied to that kernel, and please, as is passed
some time please point out the tests you want me to try

Diego

2015-05-18 11:47 GMT+02:00 Borislav Petkov <bp@suse.de>:

> On Fri, May 08, 2015 at 05:34:25PM +0800, Adrian Huang wrote:
> > Commit d5a1c7e3fc38 ("rtc-cmos: Add an alarm disable quirk") that
> > added a special quirk is not needed because PATCH [2/2] of this
> > patchset makes the kernel more robust:
> > rtc: restore the RTC alarm time to the configured alarm time in BIOS
> > Setup
> >
> > Signed-off-by: Adrian Huang <ahuang12@lenovo.com>
> > ---
> >  drivers/rtc/rtc-cmos.c | 52
> --------------------------------------------------
> >  1 file changed, 52 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> > index a82556a..9754564 100644
> > --- a/drivers/rtc/rtc-cmos.c
> > +++ b/drivers/rtc/rtc-cmos.c
> > @@ -29,8 +29,6 @@
> >   * other drivers and utilities on correctly configured systems.
> >   */
> >
> > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > -
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> > @@ -41,7 +39,6 @@
> >  #include <linux/pm.h>
> >  #include <linux/of.h>
> >  #include <linux/of_platform.h>
> > -#include <linux/dmi.h>
> >
> >  /* this is for "generic access to PC-style RTC" using
> CMOS_READ/CMOS_WRITE */
> >  #include <asm-generic/rtc.h>
> > @@ -380,50 +377,6 @@ static int cmos_set_alarm(struct device *dev,
> struct rtc_wkalrm *t)
> >       return 0;
> >  }
> >
> > -/*
> > - * Do not disable RTC alarm on shutdown - workaround for b0rked BIOSes.
> > - */
> > -static bool alarm_disable_quirk;
> > -
> > -static int __init set_alarm_disable_quirk(const struct dmi_system_id
> *id)
> > -{
> > -     alarm_disable_quirk = true;
> > -     pr_info("BIOS has alarm-disable quirk - RTC alarms disabled\n");
> > -     return 0;
> > -}
> > -
> > -static const struct dmi_system_id rtc_quirks[] __initconst = {
> > -     /* https://bugzilla.novell.com/show_bug.cgi?id=805740 */
> > -     {
> > -             .callback = set_alarm_disable_quirk,
> > -             .ident    = "IBM Truman",
> > -             .matches  = {
> > -                     DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> > -                     DMI_MATCH(DMI_PRODUCT_NAME, "4852570"),
> > -             },
>
> So, your series really fix the reboot issue on this already notoriously
> buggy box.
>
> I did try to confirm it 5 times just to be sure and in all 5, the box
> remained off.
>
> We're testing another box currently which has the same issue.
>
> Now, for the next version of your patches, I'd ask you to put this patch
> second. I.e., you want to introduce the new fix first and *then* remove
> the quirk as this way we have a window where no quirk will be in place
> and possible bisection will be disturbed needlessly even though we can
> help it.
>
> > -     },
> > -     /* https://bugzilla.novell.com/show_bug.cgi?id=812592 */
> > -     {
> > -             .callback = set_alarm_disable_quirk,
> > -             .ident    = "Gigabyte GA-990XA-UD3",
> > -             .matches  = {
> > -                     DMI_MATCH(DMI_SYS_VENDOR,
> > -                                     "Gigabyte Technology Co., Ltd."),
> > -                     DMI_MATCH(DMI_PRODUCT_NAME, "GA-990XA-UD3"),
> > -             },
>
> Looking at the bugzilla entry above, that should be Diego's box.
>
> Diego, would you be able to test these patches? They're a better fix
> than what I did last year.
>
> If you want me to prepare an openSUSE kernel for you, let me know and
> I'll do one. I'd only need to know which distro.
>
> Thanks.
>
> --
> Regards/Gruss,
>     Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --
>
Borislav Petkov May 18, 2015, 10:12 a.m. UTC | #3
Hi Diego,

On Mon, May 18, 2015 at 12:00:22PM +0200, Diego Ercolani wrote:
> Hello, sure I can.

cool, thanks! :-)

> In the meanwhile I upgraded my linux box to opensuse 13.2 with actual
> kernel: 3.16.7-21-desktop

Ok, I'll prepare one.

> so the patch should be applied to that kernel, and please, as is passed
> some time please point out the tests you want me to try

Simply executing

"shutdown -h now"

should keep the box off.

You could also do the things you've tried when reporting the bug:

https://bugzilla.suse.com/show_bug.cgi?id=812592#c0

Anyway, thanks, I'll let you know.
Borislav Petkov May 18, 2015, 4:01 p.m. UTC | #4
On Mon, May 18, 2015 at 12:12:48PM +0200, Borislav Petkov wrote:
> > In the meanwhile I upgraded my linux box to opensuse 13.2 with actual
> > kernel: 3.16.7-21-desktop
> 
> Ok, I'll prepare one.

Ok, I've uploaded a test kernel here - kernel-desktop, 64-bit:

http://beta.suse.com/private/bpetkov/

Let me know if you have trouble testing it.

Thanks.
Diego Ercolani May 21, 2015, 8:31 a.m. UTC | #5
I'm sorry, I'm experiencing a new issue related to btrfs, so my system is
not usable now: https://bugzilla.opensuse.org/show_bug.cgi?id=931787 until
I found a solution

2015-05-18 18:01 GMT+02:00 Borislav Petkov <bp@suse.de>:

> On Mon, May 18, 2015 at 12:12:48PM +0200, Borislav Petkov wrote:
> > > In the meanwhile I upgraded my linux box to opensuse 13.2 with actual
> > > kernel: 3.16.7-21-desktop
> >
> > Ok, I'll prepare one.
>
> Ok, I've uploaded a test kernel here - kernel-desktop, 64-bit:
>
> http://beta.suse.com/private/bpetkov/
>
> Let me know if you have trouble testing it.
>
> Thanks.
>
> --
> Regards/Gruss,
>     Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --
>
Diego Ercolani May 21, 2015, 8:25 p.m. UTC | #6
I confirm that I can shutdon the linux box with the kernel you provided
here it is the bootlog

Thank you

2015-05-18 18:01 GMT+02:00 Borislav Petkov <bp@suse.de>:

> On Mon, May 18, 2015 at 12:12:48PM +0200, Borislav Petkov wrote:
> > > In the meanwhile I upgraded my linux box to opensuse 13.2 with actual
> > > kernel: 3.16.7-21-desktop
> >
> > Ok, I'll prepare one.
>
> Ok, I've uploaded a test kernel here - kernel-desktop, 64-bit:
>
> http://beta.suse.com/private/bpetkov/
>
> Let me know if you have trouble testing it.
>
> Thanks.
>
> --
> Regards/Gruss,
>     Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --
>
Borislav Petkov May 21, 2015, 8:52 p.m. UTC | #7
On Thu, May 21, 2015 at 10:25:01PM +0200, Diego Ercolani wrote:
> I confirm that I can shutdon the linux box with the kernel you provided
> here it is the bootlog

Cool, very nice. Thanks a lot for testing!

@Adrian: I think you can add Tested-by:'s to your v2.

Here are mine:

Acked-by: Borislav Petkov <bp@suse.de>
Tested-by: Borislav Petkov <bp@suse.de>

Egbert did test it on a bunch of machines too. Egbert, can you give your
Tested-by: too please?

Thanks Diego!
Thanks guys!
Egbert Eich May 21, 2015, 10:16 p.m. UTC | #8
Borislav Petkov writes:
 > On Thu, May 21, 2015 at 10:25:01PM +0200, Diego Ercolani wrote:
 > > I confirm that I can shutdon the linux box with the kernel you provided
 > > here it is the bootlog
 > 
 > Cool, very nice. Thanks a lot for testing!
 > 
 > @Adrian: I think you can add Tested-by:'s to your v2.
 > 
 > Here are mine:
 > 
 > Acked-by: Borislav Petkov <bp@suse.de>
 > Tested-by: Borislav Petkov <bp@suse.de>
 > 
 > Egbert did test it on a bunch of machines too. Egbert, can you give your
 > Tested-by: too please?


You bet!

Tested-by: Egbert Eich <eich@suse.de>

Cheers,
	Egbert.
Adrian Huang May 21, 2015, 11:44 p.m. UTC | #9
> Cool, very nice. Thanks a lot for testing!
>

Awesome, thanks to all you guys for testing. I'm really appreciated.

> @Adrian: I think you can add Tested-by:'s to your v2.
>

Sure. I'll send v2 out later with Borislav's Acked-by/Tested-by and
Egbert's Tested-by.

@Diego: May I have your Tested-by, too?

-- Adrian
Diego Ercolani May 22, 2015, 6:20 a.m. UTC | #10
Sure, only if I haven't to pay ;-)

Il giorno ven 22 mag 2015 01:44 Huang Adrian <adrianhuang0701@gmail.com> ha
scritto:

> > Cool, very nice. Thanks a lot for testing!
> >
>
> Awesome, thanks to all you guys for testing. I'm really appreciated.
>
> > @Adrian: I think you can add Tested-by:'s to your v2.
> >
>
> Sure. I'll send v2 out later with Borislav's Acked-by/Tested-by and
> Egbert's Tested-by.
>
> @Diego: May I have your Tested-by, too?
>
> -- Adrian
>
diff mbox

Patch

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index a82556a..9754564 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -29,8 +29,6 @@ 
  * other drivers and utilities on correctly configured systems.
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -41,7 +39,6 @@ 
 #include <linux/pm.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
-#include <linux/dmi.h>
 
 /* this is for "generic access to PC-style RTC" using CMOS_READ/CMOS_WRITE */
 #include <asm-generic/rtc.h>
@@ -380,50 +377,6 @@  static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	return 0;
 }
 
-/*
- * Do not disable RTC alarm on shutdown - workaround for b0rked BIOSes.
- */
-static bool alarm_disable_quirk;
-
-static int __init set_alarm_disable_quirk(const struct dmi_system_id *id)
-{
-	alarm_disable_quirk = true;
-	pr_info("BIOS has alarm-disable quirk - RTC alarms disabled\n");
-	return 0;
-}
-
-static const struct dmi_system_id rtc_quirks[] __initconst = {
-	/* https://bugzilla.novell.com/show_bug.cgi?id=805740 */
-	{
-		.callback = set_alarm_disable_quirk,
-		.ident    = "IBM Truman",
-		.matches  = {
-			DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
-			DMI_MATCH(DMI_PRODUCT_NAME, "4852570"),
-		},
-	},
-	/* https://bugzilla.novell.com/show_bug.cgi?id=812592 */
-	{
-		.callback = set_alarm_disable_quirk,
-		.ident    = "Gigabyte GA-990XA-UD3",
-		.matches  = {
-			DMI_MATCH(DMI_SYS_VENDOR,
-					"Gigabyte Technology Co., Ltd."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "GA-990XA-UD3"),
-		},
-	},
-	/* http://permalink.gmane.org/gmane.linux.kernel/1604474 */
-	{
-		.callback = set_alarm_disable_quirk,
-		.ident    = "Toshiba Satellite L300",
-		.matches  = {
-			DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
-			DMI_MATCH(DMI_PRODUCT_NAME, "Satellite L300"),
-		},
-	},
-	{}
-};
-
 static int cmos_alarm_irq_enable(struct device *dev, unsigned int enabled)
 {
 	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
@@ -432,9 +385,6 @@  static int cmos_alarm_irq_enable(struct device *dev, unsigned int enabled)
 	if (!is_valid_irq(cmos->irq))
 		return -EINVAL;
 
-	if (alarm_disable_quirk)
-		return 0;
-
 	spin_lock_irqsave(&rtc_lock, flags);
 
 	if (enabled)
@@ -1243,8 +1193,6 @@  static int __init cmos_init(void)
 			platform_driver_registered = true;
 	}
 
-	dmi_check_system(rtc_quirks);
-
 	if (retval == 0)
 		return 0;