* [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