| Message ID | 20241021032213.1915224-1-jjang@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/2] selftest: rtc: Add to check rtc alarm status for alarm related test | expand |
On 20/10/2024 20:22:13-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 error number of > (RTC_ALM_SET/RTC_WKALM_SET) ioctl call for alarm feature detection. > > Requires commit 101ca8d05913b ("rtc: efi: Enable SET/GET WAKEUP services > as optional") > > Reviewed-by: Koba Ko <kobak@nvidia.com> > Reviewed-by: Matthew R. Ochs <mochs@nvidia.com> > Signed-off-by: Joseph Jang <jjang@nvidia.com> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > Changes in v2: > - Changed to use $(top_srcdir) instead of hardcoding the path. > > tools/testing/selftests/rtc/Makefile | 2 +- > tools/testing/selftests/rtc/rtctest.c | 64 +++++++++++++++++++++++++++ > 2 files changed, 65 insertions(+), 1 deletion(-) > > 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$(top_srcdir)/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..2b12497eb30d 100644 > --- a/tools/testing/selftests/rtc/rtctest.c > +++ b/tools/testing/selftests/rtc/rtctest.c > @@ -25,6 +25,12 @@ > > static char *rtc_file = "/dev/rtc0"; > > +enum rtc_alarm_state { > + RTC_ALARM_UNKNOWN, > + RTC_ALARM_ENABLED, > + RTC_ALARM_DISABLED, > +}; > + > FIXTURE(rtc) { > int fd; > }; > @@ -82,6 +88,24 @@ static void nanosleep_with_retries(long ns) > } > } > > +static enum rtc_alarm_state get_rtc_alarm_state(int fd) > +{ > + struct rtc_param param = { 0 }; > + int rc; > + > + /* 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) > + return RTC_ALARM_UNKNOWN; > + > + if ((param.uvalue & _BITUL(RTC_FEATURE_ALARM)) == 0) > + return RTC_ALARM_DISABLED; > + > + return RTC_ALARM_ENABLED; > +} > + > TEST_F_TIMEOUT(rtc, date_read_loop, READ_LOOP_DURATION_SEC + 2) { > int rc; > long iter_count = 0; > @@ -197,11 +221,16 @@ TEST_F(rtc, alarm_alm_set) { > fd_set readfds; > time_t secs, new; > int rc; > + enum rtc_alarm_state alarm_state = RTC_ALARM_UNKNOWN; > > if (self->fd == -1 && errno == ENOENT) > SKIP(return, "Skipping test since %s does not exist", rtc_file); > ASSERT_NE(-1, self->fd); > > + alarm_state = get_rtc_alarm_state(self->fd); > + if (alarm_state == RTC_ALARM_DISABLED) > + SKIP(return, "Skipping test since alarms are not supported."); > + > rc = ioctl(self->fd, RTC_RD_TIME, &tm); > ASSERT_NE(-1, rc); > > @@ -210,6 +239,11 @@ TEST_F(rtc, alarm_alm_set) { > > rc = ioctl(self->fd, RTC_ALM_SET, &tm); > if (rc == -1) { > + /* > + * Report error if rtc alarm was enabled. Fallback to check ioctl > + * error number if rtc alarm state is unknown. > + */ > + ASSERT_EQ(RTC_ALARM_UNKNOWN, alarm_state); > ASSERT_EQ(EINVAL, errno); > TH_LOG("skip alarms are not supported."); > return; > @@ -255,11 +289,16 @@ TEST_F(rtc, alarm_wkalm_set) { > fd_set readfds; > time_t secs, new; > int rc; > + enum rtc_alarm_state alarm_state = RTC_ALARM_UNKNOWN; > > if (self->fd == -1 && errno == ENOENT) > SKIP(return, "Skipping test since %s does not exist", rtc_file); > ASSERT_NE(-1, self->fd); > > + alarm_state = get_rtc_alarm_state(self->fd); > + if (alarm_state == RTC_ALARM_DISABLED) > + SKIP(return, "Skipping test since alarms are not supported."); > + > rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time); > ASSERT_NE(-1, rc); > > @@ -270,6 +309,11 @@ TEST_F(rtc, alarm_wkalm_set) { > > rc = ioctl(self->fd, RTC_WKALM_SET, &alarm); > if (rc == -1) { > + /* > + * Report error if rtc alarm was enabled. Fallback to check ioctl > + * error number if rtc alarm state is unknown. > + */ > + ASSERT_EQ(RTC_ALARM_UNKNOWN, alarm_state); > ASSERT_EQ(EINVAL, errno); > TH_LOG("skip alarms are not supported."); > return; > @@ -307,11 +351,16 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) { > fd_set readfds; > time_t secs, new; > int rc; > + enum rtc_alarm_state alarm_state = RTC_ALARM_UNKNOWN; > > if (self->fd == -1 && errno == ENOENT) > SKIP(return, "Skipping test since %s does not exist", rtc_file); > ASSERT_NE(-1, self->fd); > > + alarm_state = get_rtc_alarm_state(self->fd); > + if (alarm_state == RTC_ALARM_DISABLED) > + SKIP(return, "Skipping test since alarms are not supported."); > + > rc = ioctl(self->fd, RTC_RD_TIME, &tm); > ASSERT_NE(-1, rc); > > @@ -320,6 +369,11 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) { > > rc = ioctl(self->fd, RTC_ALM_SET, &tm); > if (rc == -1) { > + /* > + * Report error if rtc alarm was enabled. Fallback to check ioctl > + * error number if rtc alarm state is unknown. > + */ > + ASSERT_EQ(RTC_ALARM_UNKNOWN, alarm_state); > ASSERT_EQ(EINVAL, errno); > TH_LOG("skip alarms are not supported."); > return; > @@ -365,11 +419,16 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { > fd_set readfds; > time_t secs, new; > int rc; > + enum rtc_alarm_state alarm_state = RTC_ALARM_UNKNOWN; > > if (self->fd == -1 && errno == ENOENT) > SKIP(return, "Skipping test since %s does not exist", rtc_file); > ASSERT_NE(-1, self->fd); > > + alarm_state = get_rtc_alarm_state(self->fd); > + if (alarm_state == RTC_ALARM_DISABLED) > + SKIP(return, "Skipping test since alarms are not supported."); > + > rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time); > ASSERT_NE(-1, rc); > > @@ -380,6 +439,11 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { > > rc = ioctl(self->fd, RTC_WKALM_SET, &alarm); > if (rc == -1) { > + /* > + * Report error if rtc alarm was enabled. Fallback to check ioctl > + * error number if rtc alarm state is unknown. > + */ > + ASSERT_EQ(RTC_ALARM_UNKNOWN, alarm_state); > ASSERT_EQ(EINVAL, errno); > TH_LOG("skip alarms are not supported."); > return; > -- > 2.34.1 >
On 10/22/24 10:01, Alexandre Belloni wrote: > On 20/10/2024 20:22:13-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 error number of >> (RTC_ALM_SET/RTC_WKALM_SET) ioctl call for alarm feature detection. >> >> Requires commit 101ca8d05913b ("rtc: efi: Enable SET/GET WAKEUP services >> as optional") >> >> Reviewed-by: Koba Ko <kobak@nvidia.com> >> Reviewed-by: Matthew R. Ochs <mochs@nvidia.com> >> Signed-off-by: Joseph Jang <jjang@nvidia.com> > > Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > >> --- >> Changes in v2: >> - Changed to use $(top_srcdir) instead of hardcoding the path. >> Thanks. Applied to linux-kselftest next for Linux 6.13-rc1 thanks, -- Shuah
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$(top_srcdir)/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..2b12497eb30d 100644 --- a/tools/testing/selftests/rtc/rtctest.c +++ b/tools/testing/selftests/rtc/rtctest.c @@ -25,6 +25,12 @@ static char *rtc_file = "/dev/rtc0"; +enum rtc_alarm_state { + RTC_ALARM_UNKNOWN, + RTC_ALARM_ENABLED, + RTC_ALARM_DISABLED, +}; + FIXTURE(rtc) { int fd; }; @@ -82,6 +88,24 @@ static void nanosleep_with_retries(long ns) } } +static enum rtc_alarm_state get_rtc_alarm_state(int fd) +{ + struct rtc_param param = { 0 }; + int rc; + + /* 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) + return RTC_ALARM_UNKNOWN; + + if ((param.uvalue & _BITUL(RTC_FEATURE_ALARM)) == 0) + return RTC_ALARM_DISABLED; + + return RTC_ALARM_ENABLED; +} + TEST_F_TIMEOUT(rtc, date_read_loop, READ_LOOP_DURATION_SEC + 2) { int rc; long iter_count = 0; @@ -197,11 +221,16 @@ TEST_F(rtc, alarm_alm_set) { fd_set readfds; time_t secs, new; int rc; + enum rtc_alarm_state alarm_state = RTC_ALARM_UNKNOWN; if (self->fd == -1 && errno == ENOENT) SKIP(return, "Skipping test since %s does not exist", rtc_file); ASSERT_NE(-1, self->fd); + alarm_state = get_rtc_alarm_state(self->fd); + if (alarm_state == RTC_ALARM_DISABLED) + SKIP(return, "Skipping test since alarms are not supported."); + rc = ioctl(self->fd, RTC_RD_TIME, &tm); ASSERT_NE(-1, rc); @@ -210,6 +239,11 @@ TEST_F(rtc, alarm_alm_set) { rc = ioctl(self->fd, RTC_ALM_SET, &tm); if (rc == -1) { + /* + * Report error if rtc alarm was enabled. Fallback to check ioctl + * error number if rtc alarm state is unknown. + */ + ASSERT_EQ(RTC_ALARM_UNKNOWN, alarm_state); ASSERT_EQ(EINVAL, errno); TH_LOG("skip alarms are not supported."); return; @@ -255,11 +289,16 @@ TEST_F(rtc, alarm_wkalm_set) { fd_set readfds; time_t secs, new; int rc; + enum rtc_alarm_state alarm_state = RTC_ALARM_UNKNOWN; if (self->fd == -1 && errno == ENOENT) SKIP(return, "Skipping test since %s does not exist", rtc_file); ASSERT_NE(-1, self->fd); + alarm_state = get_rtc_alarm_state(self->fd); + if (alarm_state == RTC_ALARM_DISABLED) + SKIP(return, "Skipping test since alarms are not supported."); + rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time); ASSERT_NE(-1, rc); @@ -270,6 +309,11 @@ TEST_F(rtc, alarm_wkalm_set) { rc = ioctl(self->fd, RTC_WKALM_SET, &alarm); if (rc == -1) { + /* + * Report error if rtc alarm was enabled. Fallback to check ioctl + * error number if rtc alarm state is unknown. + */ + ASSERT_EQ(RTC_ALARM_UNKNOWN, alarm_state); ASSERT_EQ(EINVAL, errno); TH_LOG("skip alarms are not supported."); return; @@ -307,11 +351,16 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) { fd_set readfds; time_t secs, new; int rc; + enum rtc_alarm_state alarm_state = RTC_ALARM_UNKNOWN; if (self->fd == -1 && errno == ENOENT) SKIP(return, "Skipping test since %s does not exist", rtc_file); ASSERT_NE(-1, self->fd); + alarm_state = get_rtc_alarm_state(self->fd); + if (alarm_state == RTC_ALARM_DISABLED) + SKIP(return, "Skipping test since alarms are not supported."); + rc = ioctl(self->fd, RTC_RD_TIME, &tm); ASSERT_NE(-1, rc); @@ -320,6 +369,11 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) { rc = ioctl(self->fd, RTC_ALM_SET, &tm); if (rc == -1) { + /* + * Report error if rtc alarm was enabled. Fallback to check ioctl + * error number if rtc alarm state is unknown. + */ + ASSERT_EQ(RTC_ALARM_UNKNOWN, alarm_state); ASSERT_EQ(EINVAL, errno); TH_LOG("skip alarms are not supported."); return; @@ -365,11 +419,16 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { fd_set readfds; time_t secs, new; int rc; + enum rtc_alarm_state alarm_state = RTC_ALARM_UNKNOWN; if (self->fd == -1 && errno == ENOENT) SKIP(return, "Skipping test since %s does not exist", rtc_file); ASSERT_NE(-1, self->fd); + alarm_state = get_rtc_alarm_state(self->fd); + if (alarm_state == RTC_ALARM_DISABLED) + SKIP(return, "Skipping test since alarms are not supported."); + rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time); ASSERT_NE(-1, rc); @@ -380,6 +439,11 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) { rc = ioctl(self->fd, RTC_WKALM_SET, &alarm); if (rc == -1) { + /* + * Report error if rtc alarm was enabled. Fallback to check ioctl + * error number if rtc alarm state is unknown. + */ + ASSERT_EQ(RTC_ALARM_UNKNOWN, alarm_state); ASSERT_EQ(EINVAL, errno); TH_LOG("skip alarms are not supported."); return;
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 error number of (RTC_ALM_SET/RTC_WKALM_SET) ioctl call for alarm feature detection. Requires commit 101ca8d05913b ("rtc: efi: Enable SET/GET WAKEUP services as optional") Reviewed-by: Koba Ko <kobak@nvidia.com> Reviewed-by: Matthew R. Ochs <mochs@nvidia.com> Signed-off-by: Joseph Jang <jjang@nvidia.com> --- Changes in v2: - Changed to use $(top_srcdir) instead of hardcoding the path. tools/testing/selftests/rtc/Makefile | 2 +- tools/testing/selftests/rtc/rtctest.c | 64 +++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-)