[PATCH] x86: honor ACPI FADT flag indicating absence of a CMOS RTC

[PATCH] x86: honor ACPI FADT flag indicating absence of a CMOS RTC

[Jan Beulich]

We shouldn't be creating a corresponding platform device in that case.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

---
 arch/x86/kernel/rtc.c |    5 +++++
 1 file changed, 5 insertions(+)

--- 3.12-rc5/arch/x86/kernel/rtc.c
+++ 3.12-rc5-x86-ACPI-no-RTC/arch/x86/kernel/rtc.c
@@ -192,6 +192,11 @@ static __init int add_rtc_cmos(void)
 	if (mrst_identify_cpu())
 		return -ENODEV;
 
+#ifdef CONFIG_ACPI
+	if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)
+		return -ENODEV;
+#endif
+
 	platform_device_register(&rtc_device);
 	dev_info(&rtc_device.dev,
 		 "registered platform RTC device (no PNP device found)\n");

Re: [PATCH] x86: honor ACPI FADT flag indicating absence of a CMOS RTC

[Ingo Molnar]

* Jan Beulich <JBeulich@suse.com> wrote:

> We shouldn't be creating a corresponding platform device in that case.

There's a sad lack of context in the changelog, how was it found, does 
this address any problem/bug observed in practice, etc?

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> ---
>  arch/x86/kernel/rtc.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> --- 3.12-rc5/arch/x86/kernel/rtc.c
> +++ 3.12-rc5-x86-ACPI-no-RTC/arch/x86/kernel/rtc.c
> @@ -192,6 +192,11 @@ static __init int add_rtc_cmos(void)
>  	if (mrst_identify_cpu())
>  		return -ENODEV;
>  
> +#ifdef CONFIG_ACPI
> +	if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)
> +		return -ENODEV;
> +#endif

It might also be prudent to emit a KERN_INFO line telling that we don't 
create the device - so that people who suddenly see unexpected breakage or 
change in behavior have a chance to see what we've done?

Thanks,

	Ingo

Re: [PATCH] x86: honor ACPI FADT flag indicating absence of a CMOS RTC

[Jan Beulich]

>>> On 18.10.13 at 08:29, Ingo Molnar <mingo@kernel.org> wrote:
> * Jan Beulich <JBeulich@suse.com> wrote:
> 
>> We shouldn't be creating a corresponding platform device in that case.
> 
> There's a sad lack of context in the changelog, how was it found, does 
> this address any problem/bug observed in practice, etc?

This was a result from code review in Xen hypervisor code in
the context of ACPI 5 work there, which lead me to check whether
Linux would honor that flag. No known issue in practice so far.
Once the below got clarified, I can certainly extend the description
in an eventual resubmission (albeit I would have though that fixing
the not honoring of a firmware flag should speak for itself).

>> +#ifdef CONFIG_ACPI
>> +	if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)
>> +		return -ENODEV;
>> +#endif
> 
> It might also be prudent to emit a KERN_INFO line telling that we don't 
> create the device - so that people who suddenly see unexpected breakage or 
> change in behavior have a chance to see what we've done?

There are so many other -ENODEV return paths here which don't
emit messages that this seemed inappropriate, the more that a
message _is_ being issued if the platform RTC device does get
registered (and hence one could judge by the message
disappearing between before and after the patch got applied).

Jan

Re: [PATCH] x86: honor ACPI FADT flag indicating absence of a CMOS RTC

[Ingo Molnar]

* Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 18.10.13 at 08:29, Ingo Molnar <mingo@kernel.org> wrote:
> > * Jan Beulich <JBeulich@suse.com> wrote:
> > 
> >> We shouldn't be creating a corresponding platform device in that case.
> > 
> > There's a sad lack of context in the changelog, how was it found, does 
> > this address any problem/bug observed in practice, etc?
> 
> This was a result from code review in Xen hypervisor code in the context 
> of ACPI 5 work there, which lead me to check whether Linux would honor 
> that flag. No known issue in practice so far. Once the below got 
> clarified, I can certainly extend the description in an eventual 
> resubmission (albeit I would have though that fixing the not honoring of 
> a firmware flag should speak for itself).

I thought we've been through this before, firmware related changes never 
'speak for themselves'!

The quality and effect of firmware flags varies wildly (sometimes they are 
good and avoid problems, sometimes they are crap and cause problems) and 
it's important to know the full context.

So in this area there's no such thing as being overly verbose in a 
changelog.

> >> +#ifdef CONFIG_ACPI
> >> +	if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)
> >> +		return -ENODEV;
> >> +#endif
> > 
> > It might also be prudent to emit a KERN_INFO line telling that we don't 
> > create the device - so that people who suddenly see unexpected breakage or 
> > change in behavior have a chance to see what we've done?
> 
> There are so many other -ENODEV return paths here which don't
> emit messages that this seemed inappropriate, the more that a
> message _is_ being issued if the platform RTC device does get
> registered (and hence one could judge by the message
> disappearing between before and after the patch got applied).

I'm trying to be conservative and protect users against potential bad side 
effects of such a change. It's routine. We can mark it as temporary and 
zap it a few kernel releases later.

Thanks,

	Ingo

[PATCH v2] x86: honor ACPI FADT flag indicating absence of a CMOS RTC

[Jan Beulich]

Even though the omission was found only during code review (originally
in the Xen hypervisor, looking through ACPI v5 flags and their meanings
and uses), we shouldn't be creating a corresponding platform device in
that case.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Emit a log message in this case, as requested by Ingo.
---
 arch/x86/kernel/rtc.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- 3.12-rc6/arch/x86/kernel/rtc.c
+++ 3.12-rc6-x86-ACPI-no-RTC/arch/x86/kernel/rtc.c
@@ -192,6 +192,14 @@ static __init int add_rtc_cmos(void)
 	if (mrst_identify_cpu())
 		return -ENODEV;
 
+#ifdef CONFIG_ACPI
+	if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) {
+		/* This warning can likely go away again in a year or two. */
+		pr_info("ACPI: not registering RTC platform device\n");
+		return -ENODEV;
+	}
+#endif
+
 	platform_device_register(&rtc_device);
 	dev_info(&rtc_device.dev,
 		 "registered platform RTC device (no PNP device found)\n");

[tip:timers/core] x86/time: Honor ACPI FADT flag indicating absence of a CMOS RTC

[tip-bot for Jan Beulich]

Commit-ID:  ee5872befc9324fa4c2583c24d7ee7120314a2b7
Gitweb:     http://git.kernel.org/tip/ee5872befc9324fa4c2583c24d7ee7120314a2b7
Author:     Jan Beulich <JBeulich@suse.com>
AuthorDate: Mon, 21 Oct 2013 09:31:57 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 26 Oct 2013 13:36:00 +0200

x86/time: Honor ACPI FADT flag indicating absence of a CMOS RTC

Even though the omission was found only during code review
(originally in the Xen hypervisor, looking through ACPI v5 flags
and their meanings and uses), we shouldn't be creating a
corresponding platform device in that case.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Cc: John Stultz <john.stultz@linaro.org>
Link: http://lkml.kernel.org/r/5265029D02000078000FC4D2@nat28.tlf.novell.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/rtc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 0aa2939..5b9dd44 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -192,6 +192,14 @@ static __init int add_rtc_cmos(void)
 	if (mrst_identify_cpu())
 		return -ENODEV;
 
+#ifdef CONFIG_ACPI
+	if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) {
+		/* This warning can likely go away again in a year or two. */
+		pr_info("ACPI: not registering RTC platform device\n");
+		return -ENODEV;
+	}
+#endif
+
 	platform_device_register(&rtc_device);
 	dev_info(&rtc_device.dev,
 		 "registered platform RTC device (no PNP device found)\n");