On 16/05/2024 19:28:47-0700, Joseph Jang wrote: > In alarm_wkalm_set and alarm_wkalm_set_minute test, they use different > ioctl (RTC_ALM_SET/RTC_WKALM_SET) for alarm feature detection. They will > skip testing if RTC_ALM_SET/RTC_WKALM_SET ioctl returns an EINVAL error > code. This design may miss detecting real problems when the > efi.set_wakeup_time() return errors and then RTC_ALM_SET/RTC_WKALM_SET > ioctl returns an EINVAL error code with RTC_FEATURE_ALARM enabled. > > In order to make rtctest more explicit and robust, we propose to use > RTC_PARAM_GET ioctl interface to check rtc alarm feature state before > running alarm related tests. If the kernel does not support RTC_PARAM_GET > ioctl interface, we will fallback to check the presence of "alarm" in > /proc/driver/rtc. > > The rtctest requires the read permission on /dev/rtc0. The rtctest will > be skipped if the /dev/rtc0 is not readable. > This change as to be separated. Also, I'm not sure what happened with https://lore.kernel.org/all/20230717175251.54390-1-atulpant.linux@xxxxxxxxx/ > Requires commit 101ca8d05913b ("rtc: efi: Enable SET/GET WAKEUP services > as optional") > > Reviewed-by: Jeremy Szu <jszu@xxxxxxxxxx> > Reviewed-by: Matthew R. Ochs <mochs@xxxxxxxxxx> > Signed-off-by: Joseph Jang <jjang@xxxxxxxxxx> > --- > tools/testing/selftests/rtc/Makefile | 2 +- > tools/testing/selftests/rtc/rtctest.c | 72 +++++++++++++++++++-------- > 2 files changed, 53 insertions(+), 21 deletions(-) > > diff --git a/tools/testing/selftests/rtc/Makefile b/tools/testing/selftests/rtc/Makefile > index 55198ecc04db..6e3a98fb24ba 100644 > --- a/tools/testing/selftests/rtc/Makefile > +++ b/tools/testing/selftests/rtc/Makefile > @@ -1,5 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0 > -CFLAGS += -O3 -Wl,-no-as-needed -Wall > +CFLAGS += -O3 -Wl,-no-as-needed -Wall -I../../../../usr/include/ > LDLIBS += -lrt -lpthread -lm > > TEST_GEN_PROGS = rtctest > diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c > index 63ce02d1d5cc..aa47b17fbd1a 100644 > --- a/tools/testing/selftests/rtc/rtctest.c > +++ b/tools/testing/selftests/rtc/rtctest.c > @@ -8,6 +8,7 @@ > #include <errno.h> > #include <fcntl.h> > #include <linux/rtc.h> > +#include <stdbool.h> > #include <stdio.h> > #include <stdlib.h> > #include <sys/ioctl.h> > @@ -24,12 +25,17 @@ > #define READ_LOOP_SLEEP_MS 11 > > static char *rtc_file = "/dev/rtc0"; > +static char *rtc_procfs = "/proc/driver/rtc"; > > FIXTURE(rtc) { > int fd; > }; > > FIXTURE_SETUP(rtc) { > + if (access(rtc_file, R_OK) != 0) > + SKIP(return, "Skipping test since cannot access %s, perhaps miss sudo", > + rtc_file); > + > self->fd = open(rtc_file, O_RDONLY); > } > > @@ -82,6 +88,36 @@ static void nanosleep_with_retries(long ns) > } > } > > +static bool is_rtc_alarm_supported(int fd) > +{ > + struct rtc_param param = { 0 }; > + int rc; > + char buf[1024] = { 0 }; > + > + /* Validate kernel reflects unsupported RTC alarm state */ > + param.param = RTC_PARAM_FEATURES; > + param.index = 0; > + rc = ioctl(fd, RTC_PARAM_GET, ¶m); > + if (rc < 0) { > + /* Fallback to read rtc procfs */ > + fd = open(rtc_procfs, O_RDONLY); I think I was clear on the previous thread, no new users of the procfs interface. You can carry this n your own tree but that can't be upstream. > + if (fd != -1) { > + rc = read(fd, buf, sizeof(buf)); > + close(fd); > + > + /* Check for the presence of "alarm" in the buf */ > + if (strstr(buf, "alarm") == NULL) > + return false; > + } else > + return false; > + } else { > + if ((param.uvalue & _BITUL(RTC_FEATURE_ALARM)) == 0) > + return false; > + } > + > + return true; > +} > + > TEST_F_TIMEOUT(rtc, date_read_loop, READ_LOOP_DURATION_SEC + 2) { > int rc; > long iter_count = 0; > @@ -202,6 +238,9 @@ TEST_F(rtc, alarm_alm_set) { > SKIP(return, "Skipping test since %s does not exist", rtc_file); > ASSERT_NE(-1, self->fd); > > + if (!is_rtc_alarm_supported(self->fd)) > + SKIP(return, "Skipping test since alarms are not supported."); > + > rc = ioctl(self->fd, RTC_RD_TIME, &tm); > ASSERT_NE(-1, rc); > > @@ -209,11 +248,7 @@ TEST_F(rtc, alarm_alm_set) { > gmtime_r(&secs, (struct tm *)&tm); > > rc = ioctl(self->fd, RTC_ALM_SET, &tm); > - if (rc == -1) { > - ASSERT_EQ(EINVAL, errno); > - TH_LOG("skip alarms are not supported."); > - return; > - } > + ASSERT_NE(-1, rc); > > rc = ioctl(self->fd, RTC_ALM_READ, &tm); > ASSERT_NE(-1, rc); > @@ -260,6 +295,9 @@ TEST_F(rtc, alarm_wkalm_set) { > SKIP(return, "Skipping test since %s does not exist", rtc_file); > ASSERT_NE(-1, self->fd); > > + if (!is_rtc_alarm_supported(self->fd)) > + SKIP(return, "Skipping test since alarms are not supported."); > + > rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time); > ASSERT_NE(-1, rc); > > @@ -269,11 +307,7 @@ TEST_F(rtc, alarm_wkalm_set) { > alarm.enabled = 1; > > rc = ioctl(self->fd, RTC_WKALM_SET, &alarm); > - if (rc == -1) { > - ASSERT_EQ(EINVAL, errno); > - TH_LOG("skip alarms are not supported."); > - return; > - } > + ASSERT_NE(-1, rc); > > rc = ioctl(self->fd, RTC_WKALM_RD, &alarm); > ASSERT_NE(-1, rc); > @@ -312,6 +346,9 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) { > SKIP(return, "Skipping test since %s does not exist", rtc_file); > ASSERT_NE(-1, self->fd); > > + if (!is_rtc_alarm_supported(self->fd)) > + SKIP(return, "Skipping test since alarms are not supported."); > + > rc = ioctl(self->fd, RTC_RD_TIME, &tm); > ASSERT_NE(-1, rc); > > @@ -319,11 +356,7 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) { > gmtime_r(&secs, (struct tm *)&tm); > > rc = ioctl(self->fd, RTC_ALM_SET, &tm); > - if (rc == -1) { > - ASSERT_EQ(EINVAL, errno); > - TH_LOG("skip alarms are not supported."); > - return; > - } > + ASSERT_NE(-1, rc); > > rc = ioctl(self->fd, RTC_ALM_READ, &tm); > ASSERT_NE(-1, rc); > @@ -370,6 +403,9 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { > SKIP(return, "Skipping test since %s does not exist", rtc_file); > ASSERT_NE(-1, self->fd); > > + if (!is_rtc_alarm_supported(self->fd)) > + SKIP(return, "Skipping test since alarms are not supported."); > + > rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time); > ASSERT_NE(-1, rc); > > @@ -379,11 +415,7 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { > alarm.enabled = 1; > > rc = ioctl(self->fd, RTC_WKALM_SET, &alarm); > - if (rc == -1) { > - ASSERT_EQ(EINVAL, errno); > - TH_LOG("skip alarms are not supported."); > - return; > - } > + ASSERT_NE(-1, rc); > > rc = ioctl(self->fd, RTC_WKALM_RD, &alarm); > ASSERT_NE(-1, rc); > -- > 2.34.1 > -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
