| Message ID | 1387439053-8711-4-git-send-email-jlee@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Where did you find a platform with "no CMOS" set and a PNP RTC? I find the expect behavior in that case to be quite ambiguous and it is not at all clear to me that what you have here is the right thing. "Lee, Chun-Yi" <joeyli.kernel@gmail.com> wrote: >We should not acess CMOS address when CMOS RTC Not Present bit set in >FADT. The ee5872be patch didn't avoid rtc-cmos driver loaded when >system support >ACPI PNP PNP0B0* devices. >So this patch block the registion of rtc-cmos driver to avoid >user space access RTC through CMOS interface. > >Signed-off-by: Lee, Chun-Yi <jlee@suse.com> >--- > arch/x86/kernel/rtc.c | 20 ++++++++++++++++---- > drivers/rtc/rtc-cmos.c | 9 +++++++++ > 2 files changed, 25 insertions(+), 4 deletions(-) > >diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c >index ca9622a..9b6c568 100644 >--- a/arch/x86/kernel/rtc.c >+++ b/arch/x86/kernel/rtc.c >@@ -174,16 +174,27 @@ static __init int add_rtc_cmos(void) > { "PNP0b00", "PNP0b01", "PNP0b02", }; > struct pnp_dev *dev; > struct pnp_id *id; >- int i; >+ int i = 0; >+ bool found_pnp; > > pnp_for_each_dev(dev) { > for (id = dev->id; id; id = id->next) { > for (i = 0; i < ARRAY_SIZE(ids); i++) { >- if (compare_pnp_id(id, ids[i]) != 0) >- return 0; >+ if (compare_pnp_id(id, ids[i]) != 0) { >+ found_pnp = true; >+ goto found_pnp; >+ } > } > } > } >+ >+found_pnp: >+ if (found_pnp) { >+ if (acpi_gbl_FADT.header.revision >= 5 && >+ acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) >+ pr_err(FW_BUG "Found %s device but CMOS RTC Not Present flag >set\n", ids[i]); >+ return 0; >+ } > #endif > if (of_have_populated_dt()) > return 0; >@@ -193,7 +204,8 @@ static __init int add_rtc_cmos(void) > return -ENODEV; > > #ifdef CONFIG_ACPI >- if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) { >+ if (acpi_gbl_FADT.header.revision >= 5 && >+ 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; >diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c >index f148762..3a84ca9 100644 >--- a/drivers/rtc/rtc-cmos.c >+++ b/drivers/rtc/rtc-cmos.c >@@ -28,6 +28,9 @@ > * interrupts disabled, holding the global rtc_lock, to exclude those > * 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> >@@ -1144,6 +1147,12 @@ static int __init cmos_init(void) > { > int retval = 0; > >+ if (acpi_gbl_FADT.header.revision >= 5 && >+ acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) { >+ pr_info("ACPI CMOS RTC Not Present detected - not loading\n"); >+ return 0; >+ } >+ > #ifdef CONFIG_PNP > retval = pnp_register_driver(&cmos_pnp_driver); > if (retval == 0)
Hi hpa, ? ??2013-12-19 ? 06:38 -0800?H. Peter Anvin ??? > Where did you find a platform with "no CMOS" set and a PNP RTC? I find the expect behavior in that case to be quite ambiguous and it is not at all clear to me that what you have here is the right thing. Actually there doesn't have the box both with "No CMOS" and PNP device. I choice to totally block rtc-cmos driver when "No CMOS RTC" because the definition in ACPI spec: CMOS RTC Not Present If set, indicates that the CMOS RTC is either not implemented, or does not exist at the legacy addresses. OSPM uses the Control Method Time and Alarm Namespace device instead. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ It suggest us using ACPI TAD interface when this flag present. But, I agreed your point for this is ambiguous due to ACPI spec didn't clear define the relationship between PNP0B0x. Maybe we can do more detail check in cmos_init when "No CMOS RTC" set: + check if have ACPI TAD device, then block rtc-cmos + check if no ACPI TAD device, but have PNP0B0x, then we use PNP0b0x. > > "Lee, Chun-Yi" <joeyli.kernel@gmail.com> wrote: > >We should not acess CMOS address when CMOS RTC Not Present bit set in > >FADT. The ee5872be patch didn't avoid rtc-cmos driver loaded when > >system support > >ACPI PNP PNP0B0* devices. > >So this patch block the registion of rtc-cmos driver to avoid > >user space access RTC through CMOS interface. > > > >Signed-off-by: Lee, Chun-Yi <jlee@suse.com> > >--- > > arch/x86/kernel/rtc.c | 20 ++++++++++++++++---- > > drivers/rtc/rtc-cmos.c | 9 +++++++++ > > 2 files changed, 25 insertions(+), 4 deletions(-) > > ... > >--- a/drivers/rtc/rtc-cmos.c > >+++ b/drivers/rtc/rtc-cmos.c > >@@ -28,6 +28,9 @@ > > * interrupts disabled, holding the global rtc_lock, to exclude those > > * 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> > >@@ -1144,6 +1147,12 @@ static int __init cmos_init(void) > > { > > int retval = 0; > > > >+ if (acpi_gbl_FADT.header.revision >= 5 && > >+ acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) { > >+ pr_info("ACPI CMOS RTC Not Present detected - not loading\n"); > >+ return 0; > >+ } > >+ > > #ifdef CONFIG_PNP > > retval = pnp_register_driver(&cmos_pnp_driver); > > if (retval == 0) > Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/19/2013 07:54 PM, joeyli wrote: > Hi hpa, > > ? ??2013-12-19 ? 06:38 -0800?H. Peter Anvin ??? >> Where did you find a platform with "no CMOS" set and a PNP RTC? I find the expect behavior in that case to be quite ambiguous and it is not at all clear to me that what you have here is the right thing. > > Actually there doesn't have the box both with "No CMOS" and PNP device. > I choice to totally block rtc-cmos driver when "No CMOS RTC" because the > definition in ACPI spec: > > CMOS RTC Not Present > > If set, indicates that the CMOS RTC is either not implemented, or > does not exist at the legacy addresses. OSPM uses the Control > Method Time and Alarm Namespace device instead. > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > It suggest us using ACPI TAD interface when this flag present. But, I > agreed your point for this is ambiguous due to ACPI spec didn't clear > define the relationship between PNP0B0x. > > Maybe we can do more detail check in cmos_init when "No CMOS RTC" set: > + check if have ACPI TAD device, then block rtc-cmos > + check if no ACPI TAD device, but have PNP0B0x, then we use PNP0b0x. > I think the only thing we should use that bit for is to inhibit the last-resort probing of I/O ports 0x70-0x73... if at all. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c index ca9622a..9b6c568 100644 --- a/arch/x86/kernel/rtc.c +++ b/arch/x86/kernel/rtc.c @@ -174,16 +174,27 @@ static __init int add_rtc_cmos(void) { "PNP0b00", "PNP0b01", "PNP0b02", }; struct pnp_dev *dev; struct pnp_id *id; - int i; + int i = 0; + bool found_pnp; pnp_for_each_dev(dev) { for (id = dev->id; id; id = id->next) { for (i = 0; i < ARRAY_SIZE(ids); i++) { - if (compare_pnp_id(id, ids[i]) != 0) - return 0; + if (compare_pnp_id(id, ids[i]) != 0) { + found_pnp = true; + goto found_pnp; + } } } } + +found_pnp: + if (found_pnp) { + if (acpi_gbl_FADT.header.revision >= 5 && + acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) + pr_err(FW_BUG "Found %s device but CMOS RTC Not Present flag set\n", ids[i]); + return 0; + } #endif if (of_have_populated_dt()) return 0; @@ -193,7 +204,8 @@ static __init int add_rtc_cmos(void) return -ENODEV; #ifdef CONFIG_ACPI - if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) { + if (acpi_gbl_FADT.header.revision >= 5 && + 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; diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c index f148762..3a84ca9 100644 --- a/drivers/rtc/rtc-cmos.c +++ b/drivers/rtc/rtc-cmos.c @@ -28,6 +28,9 @@ * interrupts disabled, holding the global rtc_lock, to exclude those * 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> @@ -1144,6 +1147,12 @@ static int __init cmos_init(void) { int retval = 0; + if (acpi_gbl_FADT.header.revision >= 5 && + acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) { + pr_info("ACPI CMOS RTC Not Present detected - not loading\n"); + return 0; + } + #ifdef CONFIG_PNP retval = pnp_register_driver(&cmos_pnp_driver); if (retval == 0)
We should not acess CMOS address when CMOS RTC Not Present bit set in FADT. The ee5872be patch didn't avoid rtc-cmos driver loaded when system support ACPI PNP PNP0B0* devices. So this patch block the registion of rtc-cmos driver to avoid user space access RTC through CMOS interface. Signed-off-by: Lee, Chun-Yi <jlee@suse.com> --- arch/x86/kernel/rtc.c | 20 ++++++++++++++++---- drivers/rtc/rtc-cmos.c | 9 +++++++++ 2 files changed, 25 insertions(+), 4 deletions(-)