| Message ID | 20240524013807.154338-2-jjang@nvidia.com (mailing list archive) |
|---|---|
| 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.
On 2024/6/24 9:43 AM, Joseph Jang wrote: > > > 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. > > > Hi Alexandre, Thank you for reviewing the kernel patch [PATCH 1/2]. We are still not sure if we could include linux headers files from kernel source directory by the following change ? -CFLAGS += -O3 -Wl,-no-as-needed -Wall +CFLAGS += -O3 -Wl,-no-as-needed -Wall -I../../../../usr/include/ Thank you, Joseph.
On 18/10/2024 12:26:44+0800, Joseph Jang wrote: > > > On 2024/6/24 9:43 AM, Joseph Jang wrote: > > > > > > 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. > > > > > > Hi Alexandre, > > Thank you for reviewing the kernel patch [PATCH 1/2]. > We are still not sure if we could include linux headers files from kernel > source directory by the following change ? > > -CFLAGS += -O3 -Wl,-no-as-needed -Wall > +CFLAGS += -O3 -Wl,-no-as-needed -Wall -I../../../../usr/include/ I guess this is ok, I expected Shuah to take this path too. > > Thank you, > Joseph. > >
On 10/18/24 02:27, Alexandre Belloni wrote: > On 18/10/2024 12:26:44+0800, Joseph Jang wrote: >> >> >> On 2024/6/24 9:43 AM, Joseph Jang wrote: >>> >>> >>> 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? I saw this and figured it is is still in review. >>> >>> 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. >>> >>> > >> Hi Alexandre, >> >> Thank you for reviewing the kernel patch [PATCH 1/2]. >> We are still not sure if we could include linux headers files from kernel >> source directory by the following change ? >> >> -CFLAGS += -O3 -Wl,-no-as-needed -Wall >> +CFLAGS += -O3 -Wl,-no-as-needed -Wall -I../../../../usr/include/ You have to say $(top_srcdir)instead of hardcoding the path > > I guess this is ok, I expected Shuah to take this path too. > >> Not as is. Need v2 for this with the above change. 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../../../../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(-)