[PATCH] x86: enable rtc-efi

[PATCH] x86: enable rtc-efi

[Jan Beulich]

Besides a Kconfig change this just requires creating a respective
platform device.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: dann frazier <dannf@hp.com>

---
 arch/x86/platform/efi/efi.c |   16 ++++++++++++++++
 drivers/rtc/Kconfig         |    2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

--- 3.0-rc6/arch/x86/platform/efi/efi.c
+++ 3.0-rc6-x86-EFI-RTC/arch/x86/platform/efi/efi.c
@@ -31,6 +31,7 @@
 #include <linux/efi.h>
 #include <linux/bootmem.h>
 #include <linux/memblock.h>
+#include <linux/platform_device.h>
 #include <linux/spinlock.h>
 #include <linux/uaccess.h>
 #include <linux/time.h>
@@ -679,6 +680,21 @@ void __init efi_enter_virtual_mode(void)
 	kfree(new_memmap);
 }
 
+static struct platform_device rtc_efi_dev = {
+	.name = "rtc-efi",
+	.id = -1,
+};
+
+static int __init rtc_init(void)
+{
+	if (efi_enabled && platform_device_register(&rtc_efi_dev) < 0)
+		printk(KERN_ERR "unable to register rtc device...\n");
+
+	/* not necessarily an error */
+	return 0;
+}
+arch_initcall(rtc_init);
+
 /*
  * Convenience functions to obtain memory types and attributes
  */
--- 3.0-rc6/drivers/rtc/Kconfig
+++ 3.0-rc6-x86-EFI-RTC/drivers/rtc/Kconfig
@@ -556,7 +556,7 @@ config RTC_DRV_DS1742
 
 config RTC_DRV_EFI
 	tristate "EFI RTC"
-	depends on IA64
+	depends on EFI
 	help
 	  If you say yes here you will get support for the EFI
 	  Real Time Clock.

Re: [PATCH] x86: enable rtc-efi

[Matthew Garrett]

On Tue, Jul 05, 2011 at 08:38:51AM +0100, Jan Beulich wrote:
> Besides a Kconfig change this just requires creating a respective
> platform device.

"During runtime, if a PC-AT CMOS device is present in the platform the 
caller must synchronize access to the device before calling GetTime()."

This shouldn't be enabled on PC hardware without locking to avoid races 
with direct ahrdware access.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86: enable rtc-efi

[Matthew Garrett]

On Tue, Jul 05, 2011 at 08:38:51AM +0100, Jan Beulich wrote:
> Besides a Kconfig change this just requires creating a respective
> platform device.

Couple of other issues:

> +static struct platform_device rtc_efi_dev = {
> +	.name = "rtc-efi",
> +	.id = -1,
> +};

You haven't removed this from the IA64 code. Isn't that going to result 
in registering the same device twice?

> +static int __init rtc_init(void)
> +{
> +	if (efi_enabled && platform_device_register(&rtc_efi_dev) < 0)
> +		printk(KERN_ERR "unable to register rtc device...\n");

Ought to -ENODEV if efi isn't present? The error message should also be 
more verbose.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86: enable rtc-efi

[Jan Beulich]

>>> On 05.07.11 at 20:29, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, Jul 05, 2011 at 08:38:51AM +0100, Jan Beulich wrote:
>> Besides a Kconfig change this just requires creating a respective
>> platform device.
> 
> Couple of other issues:
> 
>> +static struct platform_device rtc_efi_dev = {
>> +	.name = "rtc-efi",
>> +	.id = -1,
>> +};
> 
> You haven't removed this from the IA64 code. Isn't that going to result 
> in registering the same device twice?

How would code under arch/ia64/ and code under arch/x86/ ever
manage to register the same device twice?

>> +static int __init rtc_init(void)
>> +{
>> +	if (efi_enabled && platform_device_register(&rtc_efi_dev) < 0)
>> +		printk(KERN_ERR "unable to register rtc device...\n");
> 
> Ought to -ENODEV if efi isn't present? The error message should also be 
> more verbose.

I would agree on the -ENODEV part, but the error message is what
the original ia64 code (which I simply copied over and modified
slightly) has, so if a change is needed it ought to be done there first
I would think. Additionally I can't really see what meaningful addition
to the error message you envision.

Jan

Re: [PATCH] x86: enable rtc-efi

[Matthew Garrett]

On Wed, Jul 06, 2011 at 08:09:45AM +0100, Jan Beulich wrote:
> >>> On 05.07.11 at 20:29, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > You haven't removed this from the IA64 code. Isn't that going to result 
> > in registering the same device twice?
> 
> How would code under arch/ia64/ and code under arch/x86/ ever
> manage to register the same device twice?

Gah. Sorry, I misread and thought thsi was in the generic code too.

> >> +static int __init rtc_init(void)
> >> +{
> >> +	if (efi_enabled && platform_device_register(&rtc_efi_dev) < 0)
> >> +		printk(KERN_ERR "unable to register rtc device...\n");
> > 
> > Ought to -ENODEV if efi isn't present? The error message should also be 
> > more verbose.
> 
> I would agree on the -ENODEV part, but the error message is what
> the original ia64 code (which I simply copied over and modified
> slightly) has, so if a change is needed it ought to be done there first
> I would think. Additionally I can't really see what meaningful addition
> to the error message you envision.

EFI is the only way to access the rtc on IA64, whereas on x86 you'll 
almost certainly also have direct hardware access as well. Make it clear 
which rtc device you're unable to register.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86: enable rtc-efi

[Matthew Garrett]

On Fri, Jul 08, 2011 at 07:46:35AM +0100, Jan Beulich wrote:

>    Having thought some more about this, I don't think this should be
>    handled in the RTC driver. Instead, arch/x86/platform/efi/efi.c ought
>    to be handling this - also for all other eventual callers of the time
>    related calls. In other words, the bug has been existing before my
>    patch (and it just exposes it by introducing callers of the incompletely
>    implemented backing functions).

I'm in the process of removing this code from arch/x86, since it's 
functionally identical to the IA64 version. That's probably going to 
complicate this approach a little, but you're right that it makes sense 
for the locking to be at the backend rather than in rtc-efi.

-- 
Matthew Garrett | mjg59@srcf.ucam.org