* [PATCH] ACPI: add a dummy ACPI RTC driver to handle BIOS's accesses to registers delcared in OperationRegion. @ 2013-01-16 3:15 Alex Hung 2013-01-16 23:02 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Alex Hung @ 2013-01-16 3:15 UTC (permalink / raw) To: alex.hung, lenb, rjw, linux-acpi This is to fix acpica returns an error and terminate AML execution as soon as BIOS tries to access ACPI's RTC registers. BugLink: http://launchpad.net/bugs/1065066 Signed-off-by: Alex Hung <alex.hung@canonical.com> --- drivers/acpi/Kconfig | 9 +++ drivers/acpi/Makefile | 1 + drivers/acpi/acpi_rtc.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+) create mode 100644 drivers/acpi/acpi_rtc.c diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 38c5078..fb90397 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -181,6 +181,15 @@ config ACPI_DOCK This driver supports ACPI-controlled docking stations and removable drive bays such as the IBM Ultrabay and the Dell Module Bay. +config ACPI_RTC + tristate "RTC" + default m + help + This driver supports an ACPI RTC device. It enables BIOS to read and + to write ACPI RTC registers declared in an OperationRegion with + RegionSpace as SYSTEMCMOS. This is required if BIOS needs to access + RTC registers during run-time such as a number of HP laptops. + config ACPI_I2C def_tristate I2C depends on I2C diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 2a4502b..383c62b 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -71,6 +71,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS) += ec_sys.o obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o obj-$(CONFIG_ACPI_BGRT) += bgrt.o obj-$(CONFIG_ACPI_I2C) += acpi_i2c.o +obj-$(CONFIG_ACPI_RTC) += acpi_rtc.o # processor has its own "processor." module_param namespace processor-y := processor_driver.o processor_throttling.o diff --git a/drivers/acpi/acpi_rtc.c b/drivers/acpi/acpi_rtc.c new file mode 100644 index 0000000..0ddcfc8 --- /dev/null +++ b/drivers/acpi/acpi_rtc.c @@ -0,0 +1,140 @@ +/* + * acpi_rtc - ACPI RTC Driver + * + * Copyright (C) 2013 Alex Hung <alex.hung@canonical.com> + * + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or (at + * your option) any later version. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA. + * + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/pci.h> +#include <linux/acpi.h> +#include <acpi/acpi_bus.h> +#include <acpi/acpi_drivers.h> +#include <linux/miscdevice.h> + +#define pr_fmt(fmt) "acpi_rtc: " fmt + +MODULE_LICENSE("GPL"); +MODULE_ALIAS("acpi*:PNP0B00:*"); + +static const struct acpi_device_id rtc_device_ids[] = { + {"PNP0B00", 0}, + {"", 0}, +}; + +static acpi_status acpi_acpi_handle_locate_callback(acpi_handle handle, + u32 level, void *context, void **return_value) +{ + struct acpi_device *dev = context; + dev->handle = handle; + + return AE_CTRL_TERMINATE; +} + +static acpi_status +acpi_rtc_space_handler(u32 function, acpi_physical_address address, + u32 bits, u64 *value64, + void *handler_context, void *region_context) +{ + return AE_OK; +} + +static int rtc_install_handlers(struct acpi_device *rtc_dev) +{ + acpi_status status; + status = acpi_install_address_space_handler(rtc_dev->handle, + ACPI_ADR_SPACE_CMOS, + &acpi_rtc_space_handler, + NULL, rtc_dev); + if (ACPI_FAILURE(status)) { + pr_info("Fail to install ACPI RTC handler\n"); + return AE_ERROR; + } + + return 0; +} + +static int acpi_rtc_add(struct acpi_device *device) +{ + int ret; + acpi_status status; + acpi_handle rtc_dev; + + status = acpi_get_devices(rtc_device_ids[0].id, + acpi_acpi_handle_locate_callback, + &rtc_dev, &rtc_dev); + if (!ACPI_SUCCESS(status)) + return AE_NOT_FOUND; + + ret = rtc_install_handlers((struct acpi_device *) &rtc_dev); + + return ret; +} + +static int acpi_rtc_remove(struct acpi_device *device, int type) +{ + acpi_status status; + status = acpi_remove_address_space_handler(device->handle, + ACPI_ADR_SPACE_CMOS, + &acpi_rtc_space_handler); + if (!ACPI_SUCCESS(status)) + return AE_ERROR; + + return AE_OK; +} + +static struct acpi_driver acpi_rtc_driver = { + .name = "rtc", + .class = "ACPI_RTC_CLASS", + .ids = rtc_device_ids, + .ops = { + .add = acpi_rtc_add, + .remove = acpi_rtc_remove, + }, +}; + +static int __init rtc_init(void) +{ + int err; + + pr_info("Initializing ACPI RTC module\n"); + err = acpi_bus_register_driver(&acpi_rtc_driver); + if (err) { + pr_err("Unable to register acpi driver.\n"); + goto error_acpi_register; + } + + return 0; + +error_acpi_register: + + return err; +} + +static void __exit rtc_exit(void) +{ + pr_info("Exiting ACPI RTC module\n"); + acpi_bus_unregister_driver(&acpi_rtc_driver); +} + +module_init(rtc_init); +module_exit(rtc_exit); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI: add a dummy ACPI RTC driver to handle BIOS's accesses to registers delcared in OperationRegion. 2013-01-16 3:15 [PATCH] ACPI: add a dummy ACPI RTC driver to handle BIOS's accesses to registers delcared in OperationRegion Alex Hung @ 2013-01-16 23:02 ` Rafael J. Wysocki 2013-01-17 3:44 ` Alex Hung 0 siblings, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2013-01-16 23:02 UTC (permalink / raw) To: Alex Hung, Moore, Robert; +Cc: lenb, linux-acpi On Wednesday, January 16, 2013 11:15:25 AM Alex Hung wrote: > This is to fix acpica returns an error and terminate AML execution as > soon as BIOS tries to access ACPI's RTC registers. > > BugLink: http://launchpad.net/bugs/1065066 Can you please describe the problem in more detail in the changelog itself? Providing a link to the bug report doesn't explain why you think this fix is the best possible one. Which quite frankly I'm not sure of. > Signed-off-by: Alex Hung <alex.hung@canonical.com> > --- > drivers/acpi/Kconfig | 9 +++ > drivers/acpi/Makefile | 1 + > drivers/acpi/acpi_rtc.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 150 insertions(+) > create mode 100644 drivers/acpi/acpi_rtc.c > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index 38c5078..fb90397 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -181,6 +181,15 @@ config ACPI_DOCK > This driver supports ACPI-controlled docking stations and removable > drive bays such as the IBM Ultrabay and the Dell Module Bay. > > +config ACPI_RTC > + tristate "RTC" > + default m > + help > + This driver supports an ACPI RTC device. It enables BIOS to read and > + to write ACPI RTC registers declared in an OperationRegion with > + RegionSpace as SYSTEMCMOS. This is required if BIOS needs to access > + RTC registers during run-time such as a number of HP laptops. > + > config ACPI_I2C > def_tristate I2C > depends on I2C > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index 2a4502b..383c62b 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -71,6 +71,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS) += ec_sys.o > obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o > obj-$(CONFIG_ACPI_BGRT) += bgrt.o > obj-$(CONFIG_ACPI_I2C) += acpi_i2c.o > +obj-$(CONFIG_ACPI_RTC) += acpi_rtc.o > > # processor has its own "processor." module_param namespace > processor-y := processor_driver.o processor_throttling.o > diff --git a/drivers/acpi/acpi_rtc.c b/drivers/acpi/acpi_rtc.c > new file mode 100644 > index 0000000..0ddcfc8 > --- /dev/null > +++ b/drivers/acpi/acpi_rtc.c > @@ -0,0 +1,140 @@ > +/* > + * acpi_rtc - ACPI RTC Driver > + * > + * Copyright (C) 2013 Alex Hung <alex.hung@canonical.com> > + * > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or (at > + * your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, write to the Free Software Foundation, Inc., > + * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA. > + * > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/pci.h> > +#include <linux/acpi.h> > +#include <acpi/acpi_bus.h> > +#include <acpi/acpi_drivers.h> > +#include <linux/miscdevice.h> > + > +#define pr_fmt(fmt) "acpi_rtc: " fmt > + > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("acpi*:PNP0B00:*"); > + > +static const struct acpi_device_id rtc_device_ids[] = { > + {"PNP0B00", 0}, > + {"", 0}, > +}; > + > +static acpi_status acpi_acpi_handle_locate_callback(acpi_handle handle, > + u32 level, void *context, void **return_value) > +{ > + struct acpi_device *dev = context; > + dev->handle = handle; > + > + return AE_CTRL_TERMINATE; > +} > + > +static acpi_status > +acpi_rtc_space_handler(u32 function, acpi_physical_address address, > + u32 bits, u64 *value64, > + void *handler_context, void *region_context) > +{ > + return AE_OK; > +} > + > +static int rtc_install_handlers(struct acpi_device *rtc_dev) > +{ > + acpi_status status; > + status = acpi_install_address_space_handler(rtc_dev->handle, > + ACPI_ADR_SPACE_CMOS, > + &acpi_rtc_space_handler, > + NULL, rtc_dev); > + if (ACPI_FAILURE(status)) { > + pr_info("Fail to install ACPI RTC handler\n"); > + return AE_ERROR; > + } > + > + return 0; > +} > + > +static int acpi_rtc_add(struct acpi_device *device) > +{ > + int ret; > + acpi_status status; > + acpi_handle rtc_dev; > + > + status = acpi_get_devices(rtc_device_ids[0].id, > + acpi_acpi_handle_locate_callback, > + &rtc_dev, &rtc_dev); > + if (!ACPI_SUCCESS(status)) > + return AE_NOT_FOUND; > + > + ret = rtc_install_handlers((struct acpi_device *) &rtc_dev); > + > + return ret; > +} > + > +static int acpi_rtc_remove(struct acpi_device *device, int type) > +{ > + acpi_status status; > + status = acpi_remove_address_space_handler(device->handle, > + ACPI_ADR_SPACE_CMOS, > + &acpi_rtc_space_handler); > + if (!ACPI_SUCCESS(status)) > + return AE_ERROR; > + > + return AE_OK; > +} > + > +static struct acpi_driver acpi_rtc_driver = { > + .name = "rtc", > + .class = "ACPI_RTC_CLASS", > + .ids = rtc_device_ids, > + .ops = { > + .add = acpi_rtc_add, > + .remove = acpi_rtc_remove, > + }, > +}; > + > +static int __init rtc_init(void) > +{ > + int err; > + > + pr_info("Initializing ACPI RTC module\n"); > + err = acpi_bus_register_driver(&acpi_rtc_driver); > + if (err) { > + pr_err("Unable to register acpi driver.\n"); > + goto error_acpi_register; > + } > + > + return 0; > + > +error_acpi_register: > + > + return err; > +} > + > +static void __exit rtc_exit(void) > +{ > + pr_info("Exiting ACPI RTC module\n"); > + acpi_bus_unregister_driver(&acpi_rtc_driver); > +} > + > +module_init(rtc_init); > +module_exit(rtc_exit); So all it does is to install an empty address space handler for the CMOS address space, right? Bob, I wonder what you think about that? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI: add a dummy ACPI RTC driver to handle BIOS's accesses to registers delcared in OperationRegion. 2013-01-16 23:02 ` Rafael J. Wysocki @ 2013-01-17 3:44 ` Alex Hung 2013-01-17 5:12 ` Moore, Robert 0 siblings, 1 reply; 5+ messages in thread From: Alex Hung @ 2013-01-17 3:44 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Moore, Robert, lenb, linux-acpi On 01/17/2013 07:02 AM, Rafael J. Wysocki wrote: > On Wednesday, January 16, 2013 11:15:25 AM Alex Hung wrote: >> This is to fix acpica returns an error and terminate AML execution as >> soon as BIOS tries to access ACPI's RTC registers. >> >> BugLink: http://launchpad.net/bugs/1065066 > > Can you please describe the problem in more detail in the changelog itself? > Providing a link to the bug report doesn't explain why you think this fix > is the best possible one. Which quite frankly I'm not sure of. > Hi, Sure. It is found that BIOS declared a RTC device with an OperationRegion as below (1). When the BIOS executes accesses to the registers such as in Method _Q33 in an EC (2), the kernel outputs messages (3) because there is no handler for this OperationRegion. The patch is to create a handler to avoid the errors in AML's execution. (1) Device (RTC) { Name (_HID, EisaId ("PNP0B00")) ... OperationRegion (CMS0, SystemCMOS, Zero, 0x40) Field (CMS0, ByteAcc, NoLock, Preserve) { RTSE, 8, Offset (0x02), RTMN, 8, Offset (0x04), RTHR, 8, Offset (0x06), RTDY, 8, RTDE, 8 } } (2) Method (_Q33, 0, NotSerialized) { Store (^^RTC.RTMN, Local0) FromBCD (Local0, Local0) Store (^^RTC.RTHR, Local1) FromBCD (Local1, Local1) Store (^^RTC.RTDY, Local2) Store (^^RTC.RTSE, Local3) ... } (3) [ 5553.247507] ACPI Error: No handler for Region [CMS0] (ffff88023503b2d0) [SystemCMOS] (20121018/evregion-376) [ 5553.247526] ACPI Error: Region SystemCMOS (ID=5) has no handler (20121018/exfldio-305) [ 5553.247545] ACPI Error: Method parse/execution failed [\_SB_.PCI0.LPCB.EC0_._Q33] (Node ffff8802350609d8), AE_NOT_EXIST (20121018/psparse-537) [ 5553.247603] Failed to execute _Q33 >> Signed-off-by: Alex Hung <alex.hung@canonical.com> >> --- >> drivers/acpi/Kconfig | 9 +++ >> drivers/acpi/Makefile | 1 + >> drivers/acpi/acpi_rtc.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 150 insertions(+) >> create mode 100644 drivers/acpi/acpi_rtc.c >> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig >> index 38c5078..fb90397 100644 >> --- a/drivers/acpi/Kconfig >> +++ b/drivers/acpi/Kconfig >> @@ -181,6 +181,15 @@ config ACPI_DOCK >> This driver supports ACPI-controlled docking stations and removable >> drive bays such as the IBM Ultrabay and the Dell Module Bay. >> >> +config ACPI_RTC >> + tristate "RTC" >> + default m >> + help >> + This driver supports an ACPI RTC device. It enables BIOS to read and >> + to write ACPI RTC registers declared in an OperationRegion with >> + RegionSpace as SYSTEMCMOS. This is required if BIOS needs to access >> + RTC registers during run-time such as a number of HP laptops. >> + >> config ACPI_I2C >> def_tristate I2C >> depends on I2C >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >> index 2a4502b..383c62b 100644 >> --- a/drivers/acpi/Makefile >> +++ b/drivers/acpi/Makefile >> @@ -71,6 +71,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS) += ec_sys.o >> obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o >> obj-$(CONFIG_ACPI_BGRT) += bgrt.o >> obj-$(CONFIG_ACPI_I2C) += acpi_i2c.o >> +obj-$(CONFIG_ACPI_RTC) += acpi_rtc.o >> >> # processor has its own "processor." module_param namespace >> processor-y := processor_driver.o processor_throttling.o >> diff --git a/drivers/acpi/acpi_rtc.c b/drivers/acpi/acpi_rtc.c >> new file mode 100644 >> index 0000000..0ddcfc8 >> --- /dev/null >> +++ b/drivers/acpi/acpi_rtc.c >> @@ -0,0 +1,140 @@ >> +/* >> + * acpi_rtc - ACPI RTC Driver >> + * >> + * Copyright (C) 2013 Alex Hung <alex.hung@canonical.com> >> + * >> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or (at >> + * your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, but >> + * WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License along >> + * with this program; if not, write to the Free Software Foundation, Inc., >> + * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA. >> + * >> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/init.h> >> +#include <linux/pci.h> >> +#include <linux/acpi.h> >> +#include <acpi/acpi_bus.h> >> +#include <acpi/acpi_drivers.h> >> +#include <linux/miscdevice.h> >> + >> +#define pr_fmt(fmt) "acpi_rtc: " fmt >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_ALIAS("acpi*:PNP0B00:*"); >> + >> +static const struct acpi_device_id rtc_device_ids[] = { >> + {"PNP0B00", 0}, >> + {"", 0}, >> +}; >> + >> +static acpi_status acpi_acpi_handle_locate_callback(acpi_handle handle, >> + u32 level, void *context, void **return_value) >> +{ >> + struct acpi_device *dev = context; >> + dev->handle = handle; >> + >> + return AE_CTRL_TERMINATE; >> +} >> + >> +static acpi_status >> +acpi_rtc_space_handler(u32 function, acpi_physical_address address, >> + u32 bits, u64 *value64, >> + void *handler_context, void *region_context) >> +{ >> + return AE_OK; >> +} >> + >> +static int rtc_install_handlers(struct acpi_device *rtc_dev) >> +{ >> + acpi_status status; >> + status = acpi_install_address_space_handler(rtc_dev->handle, >> + ACPI_ADR_SPACE_CMOS, >> + &acpi_rtc_space_handler, >> + NULL, rtc_dev); >> + if (ACPI_FAILURE(status)) { >> + pr_info("Fail to install ACPI RTC handler\n"); >> + return AE_ERROR; >> + } >> + >> + return 0; >> +} >> + >> +static int acpi_rtc_add(struct acpi_device *device) >> +{ >> + int ret; >> + acpi_status status; >> + acpi_handle rtc_dev; >> + >> + status = acpi_get_devices(rtc_device_ids[0].id, >> + acpi_acpi_handle_locate_callback, >> + &rtc_dev, &rtc_dev); >> + if (!ACPI_SUCCESS(status)) >> + return AE_NOT_FOUND; >> + >> + ret = rtc_install_handlers((struct acpi_device *) &rtc_dev); >> + >> + return ret; >> +} >> + >> +static int acpi_rtc_remove(struct acpi_device *device, int type) >> +{ >> + acpi_status status; >> + status = acpi_remove_address_space_handler(device->handle, >> + ACPI_ADR_SPACE_CMOS, >> + &acpi_rtc_space_handler); >> + if (!ACPI_SUCCESS(status)) >> + return AE_ERROR; >> + >> + return AE_OK; >> +} >> + >> +static struct acpi_driver acpi_rtc_driver = { >> + .name = "rtc", >> + .class = "ACPI_RTC_CLASS", >> + .ids = rtc_device_ids, >> + .ops = { >> + .add = acpi_rtc_add, >> + .remove = acpi_rtc_remove, >> + }, >> +}; >> + >> +static int __init rtc_init(void) >> +{ >> + int err; >> + >> + pr_info("Initializing ACPI RTC module\n"); >> + err = acpi_bus_register_driver(&acpi_rtc_driver); >> + if (err) { >> + pr_err("Unable to register acpi driver.\n"); >> + goto error_acpi_register; >> + } >> + >> + return 0; >> + >> +error_acpi_register: >> + >> + return err; >> +} >> + >> +static void __exit rtc_exit(void) >> +{ >> + pr_info("Exiting ACPI RTC module\n"); >> + acpi_bus_unregister_driver(&acpi_rtc_driver); >> +} >> + >> +module_init(rtc_init); >> +module_exit(rtc_exit); > > So all it does is to install an empty address space handler for the CMOS > address space, right? > > Bob, I wonder what you think about that? > > Rafael > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] ACPI: add a dummy ACPI RTC driver to handle BIOS's accesses to registers delcared in OperationRegion. 2013-01-17 3:44 ` Alex Hung @ 2013-01-17 5:12 ` Moore, Robert 2013-01-17 13:34 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Moore, Robert @ 2013-01-17 5:12 UTC (permalink / raw) To: Alex Hung, Rafael J. Wysocki; +Cc: lenb@kernel.org, linux-acpi@vger.kernel.org It is correct that ACPICA does not install a default handler for the SystemCMOS address space. However, it is conceivable that we could add one. It is a simple thing to do. Bob > -----Original Message----- > From: Alex Hung [mailto:alex.hung@canonical.com] > Sent: Wednesday, January 16, 2013 7:45 PM > To: Rafael J. Wysocki > Cc: Moore, Robert; lenb@kernel.org; linux-acpi@vger.kernel.org > Subject: Re: [PATCH] ACPI: add a dummy ACPI RTC driver to handle BIOS's > accesses to registers delcared in OperationRegion. > > On 01/17/2013 07:02 AM, Rafael J. Wysocki wrote: > > On Wednesday, January 16, 2013 11:15:25 AM Alex Hung wrote: > >> This is to fix acpica returns an error and terminate AML execution as > >> soon as BIOS tries to access ACPI's RTC registers. > >> > >> BugLink: http://launchpad.net/bugs/1065066 > > > > Can you please describe the problem in more detail in the changelog > itself? > > Providing a link to the bug report doesn't explain why you think this > > fix is the best possible one. Which quite frankly I'm not sure of. > > > > Hi, > > Sure. > > It is found that BIOS declared a RTC device with an OperationRegion as > below (1). When the BIOS executes accesses to the registers such as in > Method _Q33 in an EC (2), the kernel outputs messages (3) because there is > no handler for this OperationRegion. The patch is to create a handler to > avoid the errors in AML's execution. > > (1) > Device (RTC) > { > Name (_HID, EisaId ("PNP0B00")) > ... > OperationRegion (CMS0, SystemCMOS, Zero, 0x40) > Field (CMS0, ByteAcc, NoLock, Preserve) > { > RTSE, 8, > Offset (0x02), > RTMN, 8, > Offset (0x04), > RTHR, 8, > Offset (0x06), > RTDY, 8, > RTDE, 8 > } > } > > (2) > Method (_Q33, 0, NotSerialized) > { > Store (^^RTC.RTMN, Local0) > FromBCD (Local0, Local0) > Store (^^RTC.RTHR, Local1) > FromBCD (Local1, Local1) > Store (^^RTC.RTDY, Local2) > Store (^^RTC.RTSE, Local3) > ... > } > > (3) > [ 5553.247507] ACPI Error: No handler for Region [CMS0] > (ffff88023503b2d0) [SystemCMOS] (20121018/evregion-376) > [ 5553.247526] ACPI Error: Region SystemCMOS (ID=5) has no handler > (20121018/exfldio-305) > [ 5553.247545] ACPI Error: Method parse/execution failed > [\_SB_.PCI0.LPCB.EC0_._Q33] (Node ffff8802350609d8), AE_NOT_EXIST > (20121018/psparse-537) > [ 5553.247603] Failed to execute _Q33 > > > >> Signed-off-by: Alex Hung <alex.hung@canonical.com> > >> --- > >> drivers/acpi/Kconfig | 9 +++ > >> drivers/acpi/Makefile | 1 + > >> drivers/acpi/acpi_rtc.c | 140 > +++++++++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 150 insertions(+) > >> create mode 100644 drivers/acpi/acpi_rtc.c > >> > >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > >> index 38c5078..fb90397 100644 > >> --- a/drivers/acpi/Kconfig > >> +++ b/drivers/acpi/Kconfig > >> @@ -181,6 +181,15 @@ config ACPI_DOCK > >> This driver supports ACPI-controlled docking stations and > removable > >> drive bays such as the IBM Ultrabay and the Dell Module Bay. > >> > >> +config ACPI_RTC > >> + tristate "RTC" > >> + default m > >> + help > >> + This driver supports an ACPI RTC device. It enables BIOS to read > and > >> + to write ACPI RTC registers declared in an OperationRegion with > >> + RegionSpace as SYSTEMCMOS. This is required if BIOS needs to > access > >> + RTC registers during run-time such as a number of HP laptops. > >> + > >> config ACPI_I2C > >> def_tristate I2C > >> depends on I2C > >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > >> index 2a4502b..383c62b 100644 > >> --- a/drivers/acpi/Makefile > >> +++ b/drivers/acpi/Makefile > >> @@ -71,6 +71,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS) += ec_sys.o > >> obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o > >> obj-$(CONFIG_ACPI_BGRT) += bgrt.o > >> obj-$(CONFIG_ACPI_I2C) += acpi_i2c.o > >> +obj-$(CONFIG_ACPI_RTC) += acpi_rtc.o > >> > >> # processor has its own "processor." module_param namespace > >> processor-y := processor_driver.o processor_throttling.o > >> diff --git a/drivers/acpi/acpi_rtc.c b/drivers/acpi/acpi_rtc.c > >> new file mode 100644 > >> index 0000000..0ddcfc8 > >> --- /dev/null > >> +++ b/drivers/acpi/acpi_rtc.c > >> @@ -0,0 +1,140 @@ > >> +/* > >> + * acpi_rtc - ACPI RTC Driver > >> + * > >> + * Copyright (C) 2013 Alex Hung <alex.hung@canonical.com> > >> + * > >> + * > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >> + * > >> + * This program is free software; you can redistribute it and/or > modify > >> + * it under the terms of the GNU General Public License as published > by > >> + * the Free Software Foundation; either version 2 of the License, or > (at > >> + * your option) any later version. > >> + * > >> + * This program is distributed in the hope that it will be useful, > but > >> + * WITHOUT ANY WARRANTY; without even the implied warranty of > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > >> + * General Public License for more details. > >> + * > >> + * You should have received a copy of the GNU General Public License > along > >> + * with this program; if not, write to the Free Software Foundation, > Inc., > >> + * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA. > >> + * > >> + * > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >> + */ > >> + > >> +#include <linux/kernel.h> > >> +#include <linux/module.h> > >> +#include <linux/init.h> > >> +#include <linux/pci.h> > >> +#include <linux/acpi.h> > >> +#include <acpi/acpi_bus.h> > >> +#include <acpi/acpi_drivers.h> > >> +#include <linux/miscdevice.h> > >> + > >> +#define pr_fmt(fmt) "acpi_rtc: " fmt > >> + > >> +MODULE_LICENSE("GPL"); > >> +MODULE_ALIAS("acpi*:PNP0B00:*"); > >> + > >> +static const struct acpi_device_id rtc_device_ids[] = { > >> + {"PNP0B00", 0}, > >> + {"", 0}, > >> +}; > >> + > >> +static acpi_status acpi_acpi_handle_locate_callback(acpi_handle > handle, > >> + u32 level, void *context, void **return_value) > >> +{ > >> + struct acpi_device *dev = context; > >> + dev->handle = handle; > >> + > >> + return AE_CTRL_TERMINATE; > >> +} > >> + > >> +static acpi_status > >> +acpi_rtc_space_handler(u32 function, acpi_physical_address address, > >> + u32 bits, u64 *value64, > >> + void *handler_context, void *region_context) > >> +{ > >> + return AE_OK; > >> +} > >> + > >> +static int rtc_install_handlers(struct acpi_device *rtc_dev) > >> +{ > >> + acpi_status status; > >> + status = acpi_install_address_space_handler(rtc_dev->handle, > >> + ACPI_ADR_SPACE_CMOS, > >> + &acpi_rtc_space_handler, > >> + NULL, rtc_dev); > >> + if (ACPI_FAILURE(status)) { > >> + pr_info("Fail to install ACPI RTC handler\n"); > >> + return AE_ERROR; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static int acpi_rtc_add(struct acpi_device *device) > >> +{ > >> + int ret; > >> + acpi_status status; > >> + acpi_handle rtc_dev; > >> + > >> + status = acpi_get_devices(rtc_device_ids[0].id, > >> + acpi_acpi_handle_locate_callback, > >> + &rtc_dev, &rtc_dev); > >> + if (!ACPI_SUCCESS(status)) > >> + return AE_NOT_FOUND; > >> + > >> + ret = rtc_install_handlers((struct acpi_device *) &rtc_dev); > >> + > >> + return ret; > >> +} > >> + > >> +static int acpi_rtc_remove(struct acpi_device *device, int type) > >> +{ > >> + acpi_status status; > >> + status = acpi_remove_address_space_handler(device->handle, > >> + ACPI_ADR_SPACE_CMOS, > >> + &acpi_rtc_space_handler); > >> + if (!ACPI_SUCCESS(status)) > >> + return AE_ERROR; > >> + > >> + return AE_OK; > >> +} > >> + > >> +static struct acpi_driver acpi_rtc_driver = { > >> + .name = "rtc", > >> + .class = "ACPI_RTC_CLASS", > >> + .ids = rtc_device_ids, > >> + .ops = { > >> + .add = acpi_rtc_add, > >> + .remove = acpi_rtc_remove, > >> + }, > >> +}; > >> + > >> +static int __init rtc_init(void) > >> +{ > >> + int err; > >> + > >> + pr_info("Initializing ACPI RTC module\n"); > >> + err = acpi_bus_register_driver(&acpi_rtc_driver); > >> + if (err) { > >> + pr_err("Unable to register acpi driver.\n"); > >> + goto error_acpi_register; > >> + } > >> + > >> + return 0; > >> + > >> +error_acpi_register: > >> + > >> + return err; > >> +} > >> + > >> +static void __exit rtc_exit(void) > >> +{ > >> + pr_info("Exiting ACPI RTC module\n"); > >> + acpi_bus_unregister_driver(&acpi_rtc_driver); > >> +} > >> + > >> +module_init(rtc_init); > >> +module_exit(rtc_exit); > > > > So all it does is to install an empty address space handler for the CMOS > > address space, right? > > > > Bob, I wonder what you think about that? > > > > Rafael > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI: add a dummy ACPI RTC driver to handle BIOS's accesses to registers delcared in OperationRegion. 2013-01-17 5:12 ` Moore, Robert @ 2013-01-17 13:34 ` Rafael J. Wysocki 0 siblings, 0 replies; 5+ messages in thread From: Rafael J. Wysocki @ 2013-01-17 13:34 UTC (permalink / raw) To: Moore, Robert; +Cc: Alex Hung, lenb@kernel.org, linux-acpi@vger.kernel.org On Thursday, January 17, 2013 05:12:24 AM Moore, Robert wrote: > It is correct that ACPICA does not install a default handler for the SystemCMOS address space. > > However, it is conceivable that we could add one. It is a simple thing to do. I think that would be better than adding it the way this patch does. :-) Alternatively, we can just add an empty handler during the Linux-specific initialization. Thanks, Rafael > > -----Original Message----- > > From: Alex Hung [mailto:alex.hung@canonical.com] > > Sent: Wednesday, January 16, 2013 7:45 PM > > To: Rafael J. Wysocki > > Cc: Moore, Robert; lenb@kernel.org; linux-acpi@vger.kernel.org > > Subject: Re: [PATCH] ACPI: add a dummy ACPI RTC driver to handle BIOS's > > accesses to registers delcared in OperationRegion. > > > > On 01/17/2013 07:02 AM, Rafael J. Wysocki wrote: > > > On Wednesday, January 16, 2013 11:15:25 AM Alex Hung wrote: > > >> This is to fix acpica returns an error and terminate AML execution as > > >> soon as BIOS tries to access ACPI's RTC registers. > > >> > > >> BugLink: http://launchpad.net/bugs/1065066 > > > > > > Can you please describe the problem in more detail in the changelog > > itself? > > > Providing a link to the bug report doesn't explain why you think this > > > fix is the best possible one. Which quite frankly I'm not sure of. > > > > > > > Hi, > > > > Sure. > > > > It is found that BIOS declared a RTC device with an OperationRegion as > > below (1). When the BIOS executes accesses to the registers such as in > > Method _Q33 in an EC (2), the kernel outputs messages (3) because there is > > no handler for this OperationRegion. The patch is to create a handler to > > avoid the errors in AML's execution. > > > > (1) > > Device (RTC) > > { > > Name (_HID, EisaId ("PNP0B00")) > > ... > > OperationRegion (CMS0, SystemCMOS, Zero, 0x40) > > Field (CMS0, ByteAcc, NoLock, Preserve) > > { > > RTSE, 8, > > Offset (0x02), > > RTMN, 8, > > Offset (0x04), > > RTHR, 8, > > Offset (0x06), > > RTDY, 8, > > RTDE, 8 > > } > > } > > > > (2) > > Method (_Q33, 0, NotSerialized) > > { > > Store (^^RTC.RTMN, Local0) > > FromBCD (Local0, Local0) > > Store (^^RTC.RTHR, Local1) > > FromBCD (Local1, Local1) > > Store (^^RTC.RTDY, Local2) > > Store (^^RTC.RTSE, Local3) > > ... > > } > > > > (3) > > [ 5553.247507] ACPI Error: No handler for Region [CMS0] > > (ffff88023503b2d0) [SystemCMOS] (20121018/evregion-376) > > [ 5553.247526] ACPI Error: Region SystemCMOS (ID=5) has no handler > > (20121018/exfldio-305) > > [ 5553.247545] ACPI Error: Method parse/execution failed > > [\_SB_.PCI0.LPCB.EC0_._Q33] (Node ffff8802350609d8), AE_NOT_EXIST > > (20121018/psparse-537) > > [ 5553.247603] Failed to execute _Q33 > > > > > > >> Signed-off-by: Alex Hung <alex.hung@canonical.com> > > >> --- > > >> drivers/acpi/Kconfig | 9 +++ > > >> drivers/acpi/Makefile | 1 + > > >> drivers/acpi/acpi_rtc.c | 140 > > +++++++++++++++++++++++++++++++++++++++++++++++ > > >> 3 files changed, 150 insertions(+) > > >> create mode 100644 drivers/acpi/acpi_rtc.c > > >> > > >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > > >> index 38c5078..fb90397 100644 > > >> --- a/drivers/acpi/Kconfig > > >> +++ b/drivers/acpi/Kconfig > > >> @@ -181,6 +181,15 @@ config ACPI_DOCK > > >> This driver supports ACPI-controlled docking stations and > > removable > > >> drive bays such as the IBM Ultrabay and the Dell Module Bay. > > >> > > >> +config ACPI_RTC > > >> + tristate "RTC" > > >> + default m > > >> + help > > >> + This driver supports an ACPI RTC device. It enables BIOS to read > > and > > >> + to write ACPI RTC registers declared in an OperationRegion with > > >> + RegionSpace as SYSTEMCMOS. This is required if BIOS needs to > > access > > >> + RTC registers during run-time such as a number of HP laptops. > > >> + > > >> config ACPI_I2C > > >> def_tristate I2C > > >> depends on I2C > > >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > > >> index 2a4502b..383c62b 100644 > > >> --- a/drivers/acpi/Makefile > > >> +++ b/drivers/acpi/Makefile > > >> @@ -71,6 +71,7 @@ obj-$(CONFIG_ACPI_EC_DEBUGFS) += ec_sys.o > > >> obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o > > >> obj-$(CONFIG_ACPI_BGRT) += bgrt.o > > >> obj-$(CONFIG_ACPI_I2C) += acpi_i2c.o > > >> +obj-$(CONFIG_ACPI_RTC) += acpi_rtc.o > > >> > > >> # processor has its own "processor." module_param namespace > > >> processor-y := processor_driver.o processor_throttling.o > > >> diff --git a/drivers/acpi/acpi_rtc.c b/drivers/acpi/acpi_rtc.c > > >> new file mode 100644 > > >> index 0000000..0ddcfc8 > > >> --- /dev/null > > >> +++ b/drivers/acpi/acpi_rtc.c > > >> @@ -0,0 +1,140 @@ > > >> +/* > > >> + * acpi_rtc - ACPI RTC Driver > > >> + * > > >> + * Copyright (C) 2013 Alex Hung <alex.hung@canonical.com> > > >> + * > > >> + * > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > >> + * > > >> + * This program is free software; you can redistribute it and/or > > modify > > >> + * it under the terms of the GNU General Public License as published > > by > > >> + * the Free Software Foundation; either version 2 of the License, or > > (at > > >> + * your option) any later version. > > >> + * > > >> + * This program is distributed in the hope that it will be useful, > > but > > >> + * WITHOUT ANY WARRANTY; without even the implied warranty of > > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > >> + * General Public License for more details. > > >> + * > > >> + * You should have received a copy of the GNU General Public License > > along > > >> + * with this program; if not, write to the Free Software Foundation, > > Inc., > > >> + * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA. > > >> + * > > >> + * > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > >> + */ > > >> + > > >> +#include <linux/kernel.h> > > >> +#include <linux/module.h> > > >> +#include <linux/init.h> > > >> +#include <linux/pci.h> > > >> +#include <linux/acpi.h> > > >> +#include <acpi/acpi_bus.h> > > >> +#include <acpi/acpi_drivers.h> > > >> +#include <linux/miscdevice.h> > > >> + > > >> +#define pr_fmt(fmt) "acpi_rtc: " fmt > > >> + > > >> +MODULE_LICENSE("GPL"); > > >> +MODULE_ALIAS("acpi*:PNP0B00:*"); > > >> + > > >> +static const struct acpi_device_id rtc_device_ids[] = { > > >> + {"PNP0B00", 0}, > > >> + {"", 0}, > > >> +}; > > >> + > > >> +static acpi_status acpi_acpi_handle_locate_callback(acpi_handle > > handle, > > >> + u32 level, void *context, void **return_value) > > >> +{ > > >> + struct acpi_device *dev = context; > > >> + dev->handle = handle; > > >> + > > >> + return AE_CTRL_TERMINATE; > > >> +} > > >> + > > >> +static acpi_status > > >> +acpi_rtc_space_handler(u32 function, acpi_physical_address address, > > >> + u32 bits, u64 *value64, > > >> + void *handler_context, void *region_context) > > >> +{ > > >> + return AE_OK; > > >> +} > > >> + > > >> +static int rtc_install_handlers(struct acpi_device *rtc_dev) > > >> +{ > > >> + acpi_status status; > > >> + status = acpi_install_address_space_handler(rtc_dev->handle, > > >> + ACPI_ADR_SPACE_CMOS, > > >> + &acpi_rtc_space_handler, > > >> + NULL, rtc_dev); > > >> + if (ACPI_FAILURE(status)) { > > >> + pr_info("Fail to install ACPI RTC handler\n"); > > >> + return AE_ERROR; > > >> + } > > >> + > > >> + return 0; > > >> +} > > >> + > > >> +static int acpi_rtc_add(struct acpi_device *device) > > >> +{ > > >> + int ret; > > >> + acpi_status status; > > >> + acpi_handle rtc_dev; > > >> + > > >> + status = acpi_get_devices(rtc_device_ids[0].id, > > >> + acpi_acpi_handle_locate_callback, > > >> + &rtc_dev, &rtc_dev); > > >> + if (!ACPI_SUCCESS(status)) > > >> + return AE_NOT_FOUND; > > >> + > > >> + ret = rtc_install_handlers((struct acpi_device *) &rtc_dev); > > >> + > > >> + return ret; > > >> +} > > >> + > > >> +static int acpi_rtc_remove(struct acpi_device *device, int type) > > >> +{ > > >> + acpi_status status; > > >> + status = acpi_remove_address_space_handler(device->handle, > > >> + ACPI_ADR_SPACE_CMOS, > > >> + &acpi_rtc_space_handler); > > >> + if (!ACPI_SUCCESS(status)) > > >> + return AE_ERROR; > > >> + > > >> + return AE_OK; > > >> +} > > >> + > > >> +static struct acpi_driver acpi_rtc_driver = { > > >> + .name = "rtc", > > >> + .class = "ACPI_RTC_CLASS", > > >> + .ids = rtc_device_ids, > > >> + .ops = { > > >> + .add = acpi_rtc_add, > > >> + .remove = acpi_rtc_remove, > > >> + }, > > >> +}; > > >> + > > >> +static int __init rtc_init(void) > > >> +{ > > >> + int err; > > >> + > > >> + pr_info("Initializing ACPI RTC module\n"); > > >> + err = acpi_bus_register_driver(&acpi_rtc_driver); > > >> + if (err) { > > >> + pr_err("Unable to register acpi driver.\n"); > > >> + goto error_acpi_register; > > >> + } > > >> + > > >> + return 0; > > >> + > > >> +error_acpi_register: > > >> + > > >> + return err; > > >> +} > > >> + > > >> +static void __exit rtc_exit(void) > > >> +{ > > >> + pr_info("Exiting ACPI RTC module\n"); > > >> + acpi_bus_unregister_driver(&acpi_rtc_driver); > > >> +} > > >> + > > >> +module_init(rtc_init); > > >> +module_exit(rtc_exit); > > > > > > So all it does is to install an empty address space handler for the CMOS > > > address space, right? > > > > > > Bob, I wonder what you think about that? > > > > > > Rafael > > > > > > > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-01-17 13:28 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-16 3:15 [PATCH] ACPI: add a dummy ACPI RTC driver to handle BIOS's accesses to registers delcared in OperationRegion Alex Hung 2013-01-16 23:02 ` Rafael J. Wysocki 2013-01-17 3:44 ` Alex Hung 2013-01-17 5:12 ` Moore, Robert 2013-01-17 13:34 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox