[PATCH] [v2] fwts: skip the wake alarm suite if the feature is not implemented

[Ivan Hu]

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.

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/fwts-devel/attachments/20250725/95b53525/attachment-0001.html>