[PATCH] [v2] fwts: skip the wake alarm suite if the feature is not implemented
alkorv at posteo.uk
alkorv at posteo.uk
Fri Jul 25 13:26:34 UTC 2025
Thanks for taking a look at this!
On Friday, 25 July 2025 04:18:16 BST Ivan Hu wrote:
> Thanks for the patch.
>
> However, I believe this patch is not appropriate, as it might cause x86
> platforms to skip the test even when they actually have RTC issues. The
> skipping logic should be handled specifically within the wakealarm tests,
> and in fact, it is already addressed some there.
> #ifdef FWTS_ARCH_INTEL
> /* For x86 devices, this is considered a failure */
> fwts_failed(fw, LOG_LEVEL_MEDIUM, "NoWakeAlarmTest1",
> "Could not find an RTC with an alarm ioctl() interface.");
> fwts_advice(fw,
> "x86 devices generally should have an RTC wake alarm that "
> "is normally controlled by the RTC alarm ioctl() interface. This interface "
> "does not exist, so the wake alarm tests will be aborted.");
> return FWTS_ABORTED;
> #else
> fwts_log_info(fw,
> "non-x86 devices sometimes do not have an RTC wake alarm that "
> "is normally controlled by the RTC alarm ioctl() interface. This "
> "interface does not exist, so the wake alarm tests will be skipped.");
> return FWTS_SKIP;
> #endif
>
> As shown above, the current implementation already correctly differentiates
> the behavior between x86 and non-x86 platforms.
I don't think that the current implementation is entirely
correct: the device node /dev/rtc0 may be present while
the wake alarm may still be unimplemented. I believe
the problem is that fwts_wakealarm_get() uses the ioctl
RTC_ALM_READ that doesn't return an error if the wake
alarm is not implemented, this doesn't seem to be correct
either, however, it's the way the Linux kernel currently
behaves. Even if the ioctl returned an error, the tests 2-5
would still fail because the ioctl RTC_AIE_ON returns
an error if the wake alarm is not implemented.
OK, I've attempted to address your concern in the 3rd
version of the patch, please find it in the reply to your
message.
--
Sincerely yours,
Al Korv
>
> Cheers,
> Ivan
>
> On Sat, Jul 19, 2025 at 4:18 AM Al Korv <alkorv at posteo.uk> wrote:
> > The wake alarm may be unimplemented on a non-x86 platform thus
> > the calls to the RTC alarm interface may legitimately fail. Al-
> > though the wake alarm test suite appears to anticipate this and
> > skips the first test on a non-x86 platform if fwts_wakealarm_get()
> > fails the subsequent tests are still performed contrary to the error
> > message printed in the non-x86 branch of the routine wakealarm_test1();
> > that happens because the routine fwts_framework_run_test() stops exe-
> > cuting the suite only if its test returns FWTS_ABORTED or its init
> > callback returns FWTS_SKIP while neither happens in the wakealarm
> > suite, moreover neither option seems to be desirable in this case.
> >
> > The patch fixes the issue as follows:
> > * it introduces the new firmware feature FWTS_FW_FEATURE_WAKE_ALARM
> >
> > that is considered to be available if the field 'enabled' in the
> > struct rtc_wkalrm returned by the ioctl RTC_WKALM_RD invoked on
> > the RTC device node is set to 1;
> >
> > * it makes the suite wakealarm require the feature to run.
> >
> > Signed-off-by: Al Korv <alkorv at posteo.uk>
> > ---
> >
> > src/acpi/wakealarm/wakealarm.c | 2 +-
> > src/lib/include/fwts_firmware.h | 4 +++-
> > src/lib/src/fwts_firmware.c | 16 +++++++++++++++-
> > 3 files changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/acpi/wakealarm/wakealarm.c
> > b/src/acpi/wakealarm/wakealarm.c
> > index 89512540..e4f6e7bd 100644
> > --- a/src/acpi/wakealarm/wakealarm.c
> > +++ b/src/acpi/wakealarm/wakealarm.c
> > @@ -175,6 +175,6 @@ static fwts_framework_ops wakealarm_ops = {
> >
> > .minor_tests = wakealarm_tests
> >
> > };
> >
> > -FWTS_REGISTER("wakealarm", &wakealarm_ops, FWTS_TEST_ANYTIME,
> > FWTS_FLAG_BATCH | FWTS_FLAG_ROOT_PRIV)
> > +FWTS_REGISTER_FEATURES("wakealarm", &wakealarm_ops, FWTS_TEST_ANYTIME,
> > FWTS_FLAG_BATCH | FWTS_FLAG_ROOT_PRIV, FWTS_FW_FEATURE_WAKE_ALARM)
> >
> > #endif
> >
> > diff --git a/src/lib/include/fwts_firmware.h
> > b/src/lib/include/fwts_firmware.h
> > index 2ac97527..d821c2bf 100644
> > --- a/src/lib/include/fwts_firmware.h
> > +++ b/src/lib/include/fwts_firmware.h
> > @@ -33,9 +33,11 @@ typedef enum {
> >
> > FWTS_FW_FEATURE_ACPI = 0x1,
> > FWTS_FW_FEATURE_DEVICETREE = 0x2,
> > FWTS_FW_FEATURE_IPMI = 0x4,
> >
> > + FWTS_FW_FEATURE_WAKE_ALARM = 0x8,
> >
> > FWTS_FW_FEATURE_ALL = FWTS_FW_FEATURE_ACPI |
> >
> > FWTS_FW_FEATURE_DEVICETREE |
> >
> > - FWTS_FW_FEATURE_IPMI
> > + FWTS_FW_FEATURE_IPMI |
> > + FWTS_FW_FEATURE_WAKE_ALARM
> >
> > } fwts_firmware_feature;
> >
> > fwts_firmware_type fwts_firmware_detect(void);
> >
> > diff --git a/src/lib/src/fwts_firmware.c b/src/lib/src/fwts_firmware.c
> > index 735b0766..04e9802c 100644
> > --- a/src/lib/src/fwts_firmware.c
> > +++ b/src/lib/src/fwts_firmware.c
> > @@ -19,7 +19,9 @@
> >
> > #include <stdlib.h>
> > #include <sys/types.h>
> > #include <sys/stat.h>
> >
> > +#include <sys/ioctl.h>
> >
> > #include <unistd.h>
> >
> > +#include <fcntl.h>
> >
> > #include "fwts.h"
> >
> > @@ -65,6 +67,17 @@ int fwts_firmware_features(void)
> >
> > if (!stat("/dev/ipmi0", &statbuf))
> >
> > features |= FWTS_FW_FEATURE_IPMI;
> >
> > + if (!stat("/dev/rtc0", &statbuf)) {
> > + int fd = open("/dev/rtc0", O_RDWR);
> > + if (fd >= 0) {
> > + struct rtc_wkalrm wkalarm;
> > + int res = ioctl(fd, RTC_WKALM_RD, &wkalarm);
> > + if (res == 0 && wkalarm.enabled)
> > + features |= FWTS_FW_FEATURE_WAKE_ALARM;
> > + (void)close(fd);
> > + }
> > + }
> > +
> >
> > return features;
> >
> > }
> >
> > @@ -77,11 +90,12 @@ const char *fwts_firmware_feature_string(const
> > fwts_firmware_feature features)
> >
> > { FWTS_FW_FEATURE_ACPI, "ACPI" },
> > { FWTS_FW_FEATURE_DEVICETREE, "devicetree" },
> > { FWTS_FW_FEATURE_IPMI, "IPMI" },
> >
> > + { FWTS_FW_FEATURE_WAKE_ALARM, "Wake alarm" },
> >
> > };
> >
> > const int n = FWTS_ARRAY_SIZE(feature_names);
> > static const char sep[] = ", ";
> >
> > - static char str[60];
> > + static char str[70];
> >
> > size_t len;
> > char *p;
> > int i;
> >
> > --
> > 2.39.5
More information about the fwts-devel
mailing list