| Message ID | 20240524013807.154338-2-jjang@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | selftest: rtc: Add rtc feature detection and rtc file check | expand |
On 23/05/2024 18:38:06-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> > --- > 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../../../../usr/include/ Is this change actually needed? > 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 2024/6/21 3:36 AM, Alexandre Belloni wrote: > On 23/05/2024 18:38:06-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> >> --- >> 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../../../../usr/include/ > > Is this change actually needed? If we didn't include "-I../../../../usr/include/" in rtctest Makefile, we may encounter build errors like the following because rtctest default look at the header file from /usr/include/linux/rtc.h which miss the definition of struct rtc_param, RTC_PARAM_FEATURES and RTC_PARAM_GET. rtctest.c: In function ‘get_rtc_alarm_state’: rtctest.c:94:15: error: variable ‘param’ has initializer but incomplete type 94 | struct rtc_param param = { 0 }; | ^~~~~~~~~ rtctest.c:94:35: warning: excess elements in struct initializer 94 | struct rtc_param param = { 0 }; | ^ rtctest.c:94:35: note: (near initialization for ‘param’) rtctest.c:94:25: error: storage size of ‘param’ isn’t known 94 | struct rtc_param param = { 0 }; | ^~~~~ rtctest.c:98:22: error: ‘RTC_PARAM_FEATURES’ undeclared (first use in this function) 98 | param.param = RTC_PARAM_FEATURES; | ^~~~~~~~~~~~~~~~~~ rtctest.c:98:22: note: each undeclared identifier is reported only once for each function it appears in rtctest.c:100:23: error: ‘RTC_PARAM_GET’ undeclared (first use in this function); did you mean ‘RTC_ALM_SET’? 100 | rc = ioctl(fd, RTC_PARAM_GET, ¶m); | ^~~~~~~~~~~~~ | RTC_ALM_SET After adding "-I../../../../usr/include/" in rtctest Makefile, the rtctest will look at linux kernel source header files from <Linux root directory>/usr/include/linux/rtc.h to find the definition of struct rtc_param, RTC_PARAM_FEATURES and RTC_PARAM_GET and then fix the rtctest build errors. Thank you, Joseph.
On 2024/6/21 3:36 AM, Alexandre Belloni wrote: > On 23/05/2024 18:38:06-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> >> --- >> 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../../../../usr/include/ > > Is this change actually needed? If we didn't include "-I../../../../usr/include/" in rtctest Makefile, we may encounter build errors like the following because rtctest default look at the header file from /usr/include/linux/rtc.h which miss the definition of struct rtc_param, RTC_PARAM_FEATURES and RTC_PARAM_GET. rtctest.c: In function ‘get_rtc_alarm_state’: rtctest.c:94:15: error: variable ‘param’ has initializer but incomplete type 94 | struct rtc_param param = { 0 }; | ^~~~~~~~~ rtctest.c:94:35: warning: excess elements in struct initializer 94 | struct rtc_param param = { 0 }; | ^ rtctest.c:94:35: note: (near initialization for ‘param’) rtctest.c:94:25: error: storage size of ‘param’ isn’t known 94 | struct rtc_param param = { 0 }; | ^~~~~ rtctest.c:98:22: error: ‘RTC_PARAM_FEATURES’ undeclared (first use in this function) 98 | param.param = RTC_PARAM_FEATURES; | ^~~~~~~~~~~~~~~~~~ rtctest.c:98:22: note: each undeclared identifier is reported only once for each function it appears in rtctest.c:100:23: error: ‘RTC_PARAM_GET’ undeclared (first use in this function); did you mean ‘RTC_ALM_SET’? 100 | rc = ioctl(fd, RTC_PARAM_GET, ¶m); | ^~~~~~~~~~~~~ | RTC_ALM_SET After adding "-I../../../../usr/include/", the rtctest will look at linux kernel source header files from <Linux root directory>/usr/include/linux/rtc.h to find the definition of struct rtc_param, RTC_PARAM_FEATURES and RTC_PARAM_GET and fix the rtctest build errors. Thank you, Joseph.
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..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> --- tools/testing/selftests/rtc/Makefile | 2 +- tools/testing/selftests/rtc/rtctest.c | 64 +++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-)