* [PATCH] efi: remove the rtc-wakeup capability from default value @ 2025-07-09 10:35 Feng Tang 2025-07-09 10:42 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Feng Tang @ 2025-07-09 10:35 UTC (permalink / raw) To: Ard Biesheuvel, linux-efi, linux-kernel; +Cc: Feng Tang 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.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] efi: remove the rtc-wakeup capability from default value 2025-07-09 10:35 [PATCH] efi: remove the rtc-wakeup capability from default value Feng Tang @ 2025-07-09 10:42 ` Ard Biesheuvel 2025-07-09 10:59 ` Feng Tang 0 siblings, 1 reply; 6+ messages in thread From: Ard Biesheuvel @ 2025-07-09 10:42 UTC (permalink / raw) To: Feng Tang; +Cc: linux-efi, linux-kernel 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? > 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] efi: remove the rtc-wakeup capability from default value 2025-07-09 10:42 ` Ard Biesheuvel @ 2025-07-09 10:59 ` Feng Tang 2025-07-09 23:33 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Feng Tang @ 2025-07-09 10:59 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: linux-efi, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] efi: remove the rtc-wakeup capability from default value 2025-07-09 10:59 ` Feng Tang @ 2025-07-09 23:33 ` Ard Biesheuvel 2025-07-10 7:24 ` Feng Tang 0 siblings, 1 reply; 6+ messages in thread From: Ard Biesheuvel @ 2025-07-09 23:33 UTC (permalink / raw) To: Feng Tang; +Cc: linux-efi, linux-kernel 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] efi: remove the rtc-wakeup capability from default value 2025-07-09 23:33 ` Ard Biesheuvel @ 2025-07-10 7:24 ` Feng Tang 2025-07-10 7:32 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Feng Tang @ 2025-07-10 7:24 UTC (permalink / raw) To: Ard Biesheuvel, Alexandre Belloni; +Cc: linux-efi, linux-kernel 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] efi: remove the rtc-wakeup capability from default value 2025-07-10 7:24 ` Feng Tang @ 2025-07-10 7:32 ` Ard Biesheuvel 0 siblings, 0 replies; 6+ messages in thread From: Ard Biesheuvel @ 2025-07-10 7:32 UTC (permalink / raw) To: Feng Tang; +Cc: Alexandre Belloni, linux-efi, linux-kernel 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-10 7:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-07-09 10:35 [PATCH] efi: remove the rtc-wakeup capability from default value Feng Tang 2025-07-09 10:42 ` Ard Biesheuvel 2025-07-09 10:59 ` Feng Tang 2025-07-09 23:33 ` Ard Biesheuvel 2025-07-10 7:24 ` Feng Tang 2025-07-10 7:32 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox