Re: [PATCH] RTC: Add an alarm disable quirk

[John Stultz <john.stultz@linaro.org>]

On 07/18/2013 08:44 AM, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
>
> 41c7f7424259f ("rtc: Disable the alarm in the hardware (v2)") added the
> functionality to disable the RTC wake alarm when shutting down the box.
>
> However, there are at least two b0rked BIOSes we know about:
>
> https://bugzilla.novell.com/show_bug.cgi?id=812592
> https://bugzilla.novell.com/show_bug.cgi?id=805740

So first of all, thanks for digging in and generating a patch here! This 
issue has been on my list, but I've not been able to reproduce it and 
have just not had the time to chase it down remotely, so I really 
appreciate your efforts here!


> where, when wakeup alarm is enabled in the BIOS, the machine reboots
> automatically right after shutdown, regardless of what wakeup time is
> programmed.

So this doesn't quite make sense, since the wakeup alarm is being 
disabled on shutdown (and this patch is allowing the alarm interrupt to 
be left enabled). I assumed it was some sort of BIOS issue where any 
modification of the RTC_AIE bit caused the alarm irq line to be left 
high(or something like that) that triggered the immediate power-on on 
shutdown. But I've not been able to dig down on this.

So while I do want to make sure we resolve this issue for the affected 
users, I would like to better understand exactly what is wrong in the 
BIOS that causes this.


> Bisecting the issue lead to this patch so disable its functionality with
> a DMI quirk only for those boxes.
So from the one bug above I could read, it looks like the RTC wakeup 
alarm functionality is also disabled with this patch, no? Might want to 
document that clearly, so we understand the known side-effects of 
applying this. It might also be interesting to see if much older kernels 
(pre 2.6.38 - before the RTC rework landed) have this functionality 
issue as well. I suspect there has to be some way to make the hardware 
work properly.

>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Rabin Vincent <rabin.vincent@stericsson.com>
> ---
>   drivers/rtc/class.c     | 24 ++++++++++++++++++++++++
>   drivers/rtc/interface.c |  8 ++++++++
>   include/linux/rtc.h     |  1 +
>   3 files changed, 33 insertions(+)
>
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index 02426812bebc..f3006db26125 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -19,6 +19,8 @@
>   #include <linux/idr.h>
>   #include <linux/slab.h>
>   #include <linux/workqueue.h>
> +#include <linux/dmi.h>
> +#include <linux/mod_devicetable.h>
>   
>   #include "rtc-core.h"
>   
> @@ -26,6 +28,25 @@
>   static DEFINE_IDA(rtc_ida);
>   struct class *rtc_class;
>   
> +static int __init clear_disable_alarm(const struct dmi_system_id *id)
> +{
> +	rtc_disable_alarm = false;
> +	return 0;
> +}
> +
> +static const struct dmi_system_id rtc_quirks[] __initconst = {
> +	/* https://bugzilla.novell.com/show_bug.cgi?id=805740 */
Any chance that bugzilla bug can be made public (it apparently requires 
a login to read, where as the other bug doesn't).


> +	{
> +		.callback = clear_disable_alarm,
> +		.ident    = "IBM Truman",

"IBM Truman"?

> +		.matches  = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "4852570"),
> +		},
> +	},
> +	{}
> +};
> +

Also, this seems to only address one of the systems you described. Do we 
need a second quirk entry as well?

>   static void rtc_device_release(struct device *dev)
>   {
>   	struct rtc_device *rtc = to_rtc_device(dev);
> @@ -340,6 +361,9 @@ static int __init rtc_init(void)
>   	rtc_class->pm = RTC_CLASS_DEV_PM_OPS;
>   	rtc_dev_init();
>   	rtc_sysfs_init(rtc_class);
> +
> +	dmi_check_system(rtc_quirks);
> +

Since this issue so far only affects x86 systems, would it be smarter to 
move the dmi quirk to the actual RTC driver in drivers/rtc/rtc-cmos.c 
rather then leaving it in the RTC core?


>   	return 0;
>   }
>   
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 72c5cdbe0791..0d944d1c02b8 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -17,6 +17,11 @@
>   #include <linux/log2.h>
>   #include <linux/workqueue.h>
>   
> +/*
> + * Do not disable RTC alarm on shutdown - workaround for b0rked BIOSes.
> + */
> +bool rtc_disable_alarm = true;
> +
>   static int rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer);
>   static void rtc_timer_remove(struct rtc_device *rtc, struct rtc_timer *timer);
>   
> @@ -787,6 +792,9 @@ static void rtc_alarm_disable(struct rtc_device *rtc)
>   	if (!rtc->ops || !rtc->ops->alarm_irq_enable)
>   		return;
>   
> +	if (!rtc_disable_alarm)
> +		return;
> +
>   	rtc->ops->alarm_irq_enable(rtc->dev.parent, false);

I suspect the same logic could be better applied in cmos_alarm_irq_enable().

thanks again!
-john