drivers/firmware/efi/efi.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
The kernel selftest of rtc reported a error on an ARM server:
RUN rtc.alarm_alm_set ...
rtctest.c:262:alarm_alm_set:Alarm time now set to 17:31:36.
rtctest.c:267:alarm_alm_set:Expected -1 (-1) != rc (-1)
alarm_alm_set: Test terminated by assertion
FAIL rtc.alarm_alm_set
not ok 5 rtc.alarm_alm_set
The root cause is, the unerlying EFI firmware doesn't support wakeup
service (get/set alarm), while it doesn't have the efi 'RT_PROP'
table either. The current code logic will claim efi supports these
runtime service capability by default, and let following 'RT_PROP'
table parsing to correct it, if that table exists.
This issue was reproduced on ARM server from another verndor, and not
reproudce on one x86 server (Icelake). All these 3 platforms don't have
'RT_PROP' tables, so they are all claimed to support alarm service,
but x86 server uses real CMOS RTC device instead rtc-efi device, and
passes the test.
So remove the wakeup/alarm capability from default value, and setup
the capability bits according to the 'RT_PROP' table parsing.
Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com>
---
drivers/firmware/efi/efi.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index e57bff702b5f..7cf35376a2f7 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -789,6 +789,17 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
}
}
+ /*
+ * After bootup, the runtime_supported_mask was set to be capable of
+ * all features, which could be kind of too optimistici. In real
+ * world, many platforms don't support advanced RTC wakeup runtime
+ * service, while they don't provide RT_PROPERTY table either, which
+ * led to rtc-wakeup capability being worngly claimed.
+ *
+ * So remove the wakeup capbility from default value, and let the
+ * RT_PROPERTY do the judge.
+ */
+ efi.runtime_supported_mask &= ~EFI_RT_SUPPORTED_WAKEUP_SERVICES;
if (rt_prop != EFI_INVALID_TABLE_ADDR) {
efi_rt_properties_table_t *tbl;
--
2.43.5On Wed, 9 Jul 2025 at 20:35, Feng Tang <feng.tang@linux.alibaba.com> wrote:
>
> The kernel selftest of rtc reported a error on an ARM server:
>
> RUN rtc.alarm_alm_set ...
> rtctest.c:262:alarm_alm_set:Alarm time now set to 17:31:36.
> rtctest.c:267:alarm_alm_set:Expected -1 (-1) != rc (-1)
> alarm_alm_set: Test terminated by assertion
> FAIL rtc.alarm_alm_set
> not ok 5 rtc.alarm_alm_set
>
> The root cause is, the unerlying EFI firmware doesn't support wakeup
> service (get/set alarm), while it doesn't have the efi 'RT_PROP'
> table either. The current code logic will claim efi supports these
> runtime service capability by default, and let following 'RT_PROP'
> table parsing to correct it, if that table exists.
>
> This issue was reproduced on ARM server from another verndor, and not
> reproudce on one x86 server (Icelake). All these 3 platforms don't have
> 'RT_PROP' tables, so they are all claimed to support alarm service,
> but x86 server uses real CMOS RTC device instead rtc-efi device, and
> passes the test.
>
> So remove the wakeup/alarm capability from default value, and setup
> the capability bits according to the 'RT_PROP' table parsing.
>
What does this achieve? The test result is accurate, as the platform
violates the spec by not implementing the RTC wakeup services, and not
setting the RT_PROP table bits accordingly.
What do we gain by pretending that the platform is not broken, and
lying about it?
> Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com>
> ---
> drivers/firmware/efi/efi.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index e57bff702b5f..7cf35376a2f7 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -789,6 +789,17 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
> }
> }
>
> + /*
> + * After bootup, the runtime_supported_mask was set to be capable of
> + * all features, which could be kind of too optimistici. In real
> + * world, many platforms don't support advanced RTC wakeup runtime
> + * service, while they don't provide RT_PROPERTY table either, which
> + * led to rtc-wakeup capability being worngly claimed.
> + *
> + * So remove the wakeup capbility from default value, and let the
> + * RT_PROPERTY do the judge.
> + */
> + efi.runtime_supported_mask &= ~EFI_RT_SUPPORTED_WAKEUP_SERVICES;
> if (rt_prop != EFI_INVALID_TABLE_ADDR) {
> efi_rt_properties_table_t *tbl;
>
Doesn't this break the RTC wakeup services on platforms that do
implement them, and don't expose a RT_PROP table?
On Wed, Jul 09, 2025 at 08:42:24PM +1000, Ard Biesheuvel wrote:
> On Wed, 9 Jul 2025 at 20:35, Feng Tang <feng.tang@linux.alibaba.com> wrote:
> >
> > The kernel selftest of rtc reported a error on an ARM server:
> >
> > RUN rtc.alarm_alm_set ...
> > rtctest.c:262:alarm_alm_set:Alarm time now set to 17:31:36.
> > rtctest.c:267:alarm_alm_set:Expected -1 (-1) != rc (-1)
> > alarm_alm_set: Test terminated by assertion
> > FAIL rtc.alarm_alm_set
> > not ok 5 rtc.alarm_alm_set
> >
> > The root cause is, the unerlying EFI firmware doesn't support wakeup
> > service (get/set alarm), while it doesn't have the efi 'RT_PROP'
> > table either. The current code logic will claim efi supports these
> > runtime service capability by default, and let following 'RT_PROP'
> > table parsing to correct it, if that table exists.
> >
> > This issue was reproduced on ARM server from another verndor, and not
> > reproudce on one x86 server (Icelake). All these 3 platforms don't have
> > 'RT_PROP' tables, so they are all claimed to support alarm service,
> > but x86 server uses real CMOS RTC device instead rtc-efi device, and
> > passes the test.
> >
> > So remove the wakeup/alarm capability from default value, and setup
> > the capability bits according to the 'RT_PROP' table parsing.
> >
>
> What does this achieve? The test result is accurate, as the platform
> violates the spec by not implementing the RTC wakeup services, and not
> setting the RT_PROP table bits accordingly.
>
> What do we gain by pretending that the platform is not broken, and
> lying about it?
I don't have much experience with EFI, so I might be totally wrong. I
don't think not providing the RT_PROP table is 'broken', that's why I
tried to borrow platforms from different vendors to do the check, which
all have no this table.
For platform which have no 'RT_PROP' tables (seems to be not a rare case),
claiming them support all efi runtime service may be kind of risky.
>
> > Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com>
> > ---
> > drivers/firmware/efi/efi.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index e57bff702b5f..7cf35376a2f7 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -789,6 +789,17 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
> > }
> > }
> >
> > + /*
> > + * After bootup, the runtime_supported_mask was set to be capable of
> > + * all features, which could be kind of too optimistici. In real
> > + * world, many platforms don't support advanced RTC wakeup runtime
> > + * service, while they don't provide RT_PROPERTY table either, which
> > + * led to rtc-wakeup capability being worngly claimed.
> > + *
> > + * So remove the wakeup capbility from default value, and let the
> > + * RT_PROPERTY do the judge.
> > + */
> > + efi.runtime_supported_mask &= ~EFI_RT_SUPPORTED_WAKEUP_SERVICES;
> > if (rt_prop != EFI_INVALID_TABLE_ADDR) {
> > efi_rt_properties_table_t *tbl;
> >
>
> Doesn't this break the RTC wakeup services on platforms that do
> implement them, and don't expose a RT_PROP table?
You are right, there is such risk.
Thanks,
Feng
On Wed, 9 Jul 2025 at 21:00, Feng Tang <feng.tang@linux.alibaba.com> wrote: > > On Wed, Jul 09, 2025 at 08:42:24PM +1000, Ard Biesheuvel wrote: > > On Wed, 9 Jul 2025 at 20:35, Feng Tang <feng.tang@linux.alibaba.com> wrote: > > > > > > The kernel selftest of rtc reported a error on an ARM server: > > > > > > RUN rtc.alarm_alm_set ... > > > rtctest.c:262:alarm_alm_set:Alarm time now set to 17:31:36. > > > rtctest.c:267:alarm_alm_set:Expected -1 (-1) != rc (-1) > > > alarm_alm_set: Test terminated by assertion > > > FAIL rtc.alarm_alm_set > > > not ok 5 rtc.alarm_alm_set > > > > > > The root cause is, the unerlying EFI firmware doesn't support wakeup > > > service (get/set alarm), while it doesn't have the efi 'RT_PROP' > > > table either. The current code logic will claim efi supports these > > > runtime service capability by default, and let following 'RT_PROP' > > > table parsing to correct it, if that table exists. > > > > > > This issue was reproduced on ARM server from another verndor, and not > > > reproudce on one x86 server (Icelake). All these 3 platforms don't have > > > 'RT_PROP' tables, so they are all claimed to support alarm service, > > > but x86 server uses real CMOS RTC device instead rtc-efi device, and > > > passes the test. > > > > > > So remove the wakeup/alarm capability from default value, and setup > > > the capability bits according to the 'RT_PROP' table parsing. > > > > > > > What does this achieve? The test result is accurate, as the platform > > violates the spec by not implementing the RTC wakeup services, and not > > setting the RT_PROP table bits accordingly. > > > > What do we gain by pretending that the platform is not broken, and > > lying about it? > > I don't have much experience with EFI, so I might be totally wrong. I > don't think not providing the RT_PROP table is 'broken', that's why I > tried to borrow platforms from different vendors to do the check, which > all have no this table. > > For platform which have no 'RT_PROP' tables (seems to be not a rare case), > claiming them support all efi runtime service may be kind of risky. > It is the other way around. The UEFI spec mandates that all runtime services are implemented, unless a RT_PROP table is provided.
Add Alexandre Belloni for his view on rtc-efi driver On Thu, Jul 10, 2025 at 09:33:19AM +1000, Ard Biesheuvel wrote: > On Wed, 9 Jul 2025 at 21:00, Feng Tang <feng.tang@linux.alibaba.com> wrote: > > > > On Wed, Jul 09, 2025 at 08:42:24PM +1000, Ard Biesheuvel wrote: > > > On Wed, 9 Jul 2025 at 20:35, Feng Tang <feng.tang@linux.alibaba.com> wrote: > > > > > > > > The kernel selftest of rtc reported a error on an ARM server: > > > > > > > > RUN rtc.alarm_alm_set ... > > > > rtctest.c:262:alarm_alm_set:Alarm time now set to 17:31:36. > > > > rtctest.c:267:alarm_alm_set:Expected -1 (-1) != rc (-1) > > > > alarm_alm_set: Test terminated by assertion > > > > FAIL rtc.alarm_alm_set > > > > not ok 5 rtc.alarm_alm_set > > > > > > > > The root cause is, the unerlying EFI firmware doesn't support wakeup > > > > service (get/set alarm), while it doesn't have the efi 'RT_PROP' > > > > table either. The current code logic will claim efi supports these > > > > runtime service capability by default, and let following 'RT_PROP' > > > > table parsing to correct it, if that table exists. > > > > > > > > This issue was reproduced on ARM server from another verndor, and not > > > > reproudce on one x86 server (Icelake). All these 3 platforms don't have > > > > 'RT_PROP' tables, so they are all claimed to support alarm service, > > > > but x86 server uses real CMOS RTC device instead rtc-efi device, and > > > > passes the test. > > > > > > > > So remove the wakeup/alarm capability from default value, and setup > > > > the capability bits according to the 'RT_PROP' table parsing. > > > > > > > > > > What does this achieve? The test result is accurate, as the platform > > > violates the spec by not implementing the RTC wakeup services, and not > > > setting the RT_PROP table bits accordingly. > > > > > > What do we gain by pretending that the platform is not broken, and > > > lying about it? > > > > I don't have much experience with EFI, so I might be totally wrong. I > > don't think not providing the RT_PROP table is 'broken', that's why I > > tried to borrow platforms from different vendors to do the check, which > > all have no this table. > > > > For platform which have no 'RT_PROP' tables (seems to be not a rare case), > > claiming them support all efi runtime service may be kind of risky. > > > > It is the other way around. The UEFI spec mandates that all runtime > services are implemented, unless a RT_PROP table is provided. Thanks for the explaination! Yes, it's fair to claim the uefi implementation on the 2 ARM servers 'broken' :) I talked with some firmware developers. They said the rtc-alarm service could be implemented, while the difficult part is how to notify OS. I submitted a request for a correct RT_PROP table. Meanwhile, given there are quite some platforms (All ARM server I can access) don't have the table and not support rtc wakeup service, I'm thinking of adding some runtime check for the service in rtc-efi driver, something like: --- diff --git a/drivers/rtc/rtc-efi.c b/drivers/rtc/rtc-efi.c index fa8bf82df948..7ae948aebd11 100644 --- a/drivers/rtc/rtc-efi.c +++ b/drivers/rtc/rtc-efi.c @@ -259,6 +259,7 @@ static int __init efi_rtc_probe(struct platform_device *dev) struct rtc_device *rtc; efi_time_t eft; efi_time_cap_t cap; + efi_bool_t enabled, pending; /* First check if the RTC is usable */ if (efi.get_time(&eft, &cap) != EFI_SUCCESS) @@ -272,7 +273,8 @@ static int __init efi_rtc_probe(struct platform_device *dev) rtc->ops = &efi_rtc_ops; clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features); - if (efi_rt_services_supported(EFI_RT_SUPPORTED_WAKEUP_SERVICES)) + if (efi_rt_services_supported(EFI_RT_SUPPORTED_WAKEUP_SERVICES) && + efi.get_wakeup_time(&enabled, &pending, &eft) == EFI_SUCCESS) set_bit(RTC_FEATURE_ALARM_WAKEUP_ONLY, rtc->features); else clear_bit(RTC_FEATURE_ALARM, rtc->features); This works on one ARM server I can test kernel with. Any suggestions? Thanks, Feng
On Thu, 10 Jul 2025 at 17:24, Feng Tang <feng.tang@linux.alibaba.com> wrote: > > Add Alexandre Belloni for his view on rtc-efi driver > > On Thu, Jul 10, 2025 at 09:33:19AM +1000, Ard Biesheuvel wrote: > > On Wed, 9 Jul 2025 at 21:00, Feng Tang <feng.tang@linux.alibaba.com> wrote: > > > > > > On Wed, Jul 09, 2025 at 08:42:24PM +1000, Ard Biesheuvel wrote: > > > > On Wed, 9 Jul 2025 at 20:35, Feng Tang <feng.tang@linux.alibaba.com> wrote: > > > > > > > > > > The kernel selftest of rtc reported a error on an ARM server: > > > > > > > > > > RUN rtc.alarm_alm_set ... > > > > > rtctest.c:262:alarm_alm_set:Alarm time now set to 17:31:36. > > > > > rtctest.c:267:alarm_alm_set:Expected -1 (-1) != rc (-1) > > > > > alarm_alm_set: Test terminated by assertion > > > > > FAIL rtc.alarm_alm_set > > > > > not ok 5 rtc.alarm_alm_set > > > > > > > > > > The root cause is, the unerlying EFI firmware doesn't support wakeup > > > > > service (get/set alarm), while it doesn't have the efi 'RT_PROP' > > > > > table either. The current code logic will claim efi supports these > > > > > runtime service capability by default, and let following 'RT_PROP' > > > > > table parsing to correct it, if that table exists. > > > > > > > > > > This issue was reproduced on ARM server from another verndor, and not > > > > > reproudce on one x86 server (Icelake). All these 3 platforms don't have > > > > > 'RT_PROP' tables, so they are all claimed to support alarm service, > > > > > but x86 server uses real CMOS RTC device instead rtc-efi device, and > > > > > passes the test. > > > > > > > > > > So remove the wakeup/alarm capability from default value, and setup > > > > > the capability bits according to the 'RT_PROP' table parsing. > > > > > > > > > > > > > What does this achieve? The test result is accurate, as the platform > > > > violates the spec by not implementing the RTC wakeup services, and not > > > > setting the RT_PROP table bits accordingly. > > > > > > > > What do we gain by pretending that the platform is not broken, and > > > > lying about it? > > > > > > I don't have much experience with EFI, so I might be totally wrong. I > > > don't think not providing the RT_PROP table is 'broken', that's why I > > > tried to borrow platforms from different vendors to do the check, which > > > all have no this table. > > > > > > For platform which have no 'RT_PROP' tables (seems to be not a rare case), > > > claiming them support all efi runtime service may be kind of risky. > > > > > > > It is the other way around. The UEFI spec mandates that all runtime > > services are implemented, unless a RT_PROP table is provided. > > Thanks for the explaination! Yes, it's fair to claim the uefi implementation > on the 2 ARM servers 'broken' :) > > I talked with some firmware developers. They said the rtc-alarm service could > be implemented, while the difficult part is how to notify OS. I submitted a > request for a correct RT_PROP table. > > Meanwhile, given there are quite some platforms (All ARM server I can access) > don't have the table and not support rtc wakeup service, I'm thinking of adding > some runtime check for the service in rtc-efi driver, something like: > > --- > diff --git a/drivers/rtc/rtc-efi.c b/drivers/rtc/rtc-efi.c > index fa8bf82df948..7ae948aebd11 100644 > --- a/drivers/rtc/rtc-efi.c > +++ b/drivers/rtc/rtc-efi.c > @@ -259,6 +259,7 @@ static int __init efi_rtc_probe(struct platform_device *dev) > struct rtc_device *rtc; > efi_time_t eft; > efi_time_cap_t cap; > + efi_bool_t enabled, pending; > > /* First check if the RTC is usable */ > if (efi.get_time(&eft, &cap) != EFI_SUCCESS) > @@ -272,7 +273,8 @@ static int __init efi_rtc_probe(struct platform_device *dev) > > rtc->ops = &efi_rtc_ops; > clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features); > - if (efi_rt_services_supported(EFI_RT_SUPPORTED_WAKEUP_SERVICES)) > + if (efi_rt_services_supported(EFI_RT_SUPPORTED_WAKEUP_SERVICES) && > + efi.get_wakeup_time(&enabled, &pending, &eft) == EFI_SUCCESS) > set_bit(RTC_FEATURE_ALARM_WAKEUP_ONLY, rtc->features); > else > clear_bit(RTC_FEATURE_ALARM, rtc->features); > > This works on one ARM server I can test kernel with. Any suggestions? > I think this is fine - please send it as a proper patch with commit log etc.
© 2016 - 2025 Red Hat, Inc.