Re: [PATCH] efi: remove the rtc-wakeup capability from default value

[Feng Tang <feng.tang@linux.alibaba.com>]

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