1. 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.
2. The rtctest requires the read permission on /dev/rtc0. The rtctest will be skipped if the /dev/rtc0 is not readable.
Joseph Jang (2): selftest: rtc: Add to check rtc alarm status for alarm related test selftest: rtc: Check if could access /dev/rtc0 before testing
tools/testing/selftests/rtc/Makefile | 2 +- tools/testing/selftests/rtc/rtctest.c | 71 ++++++++++++++++++++++++++- 2 files changed, 71 insertions(+), 2 deletions(-)
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/ 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;
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);@@ -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);@@ -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);@@ -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);-- 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
The rtctest requires the read permission on /dev/rtc0. The rtctest will be skipped if the /dev/rtc0 is not readable.
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/rtctest.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c index 2b12497eb30d..d104f5326cf4 100644 --- a/tools/testing/selftests/rtc/rtctest.c +++ b/tools/testing/selftests/rtc/rtctest.c @@ -483,6 +483,8 @@ __constructor_order_last(void)
int main(int argc, char **argv) { + int ret = -1; + switch (argc) { case 2: rtc_file = argv[1]; @@ -494,5 +496,12 @@ int main(int argc, char **argv) return 1; }
- return test_harness_run(argc, argv); + /* Run the test if rtc_file is accessible */ + if (access(rtc_file, R_OK) == 0) + ret = test_harness_run(argc, argv); + else + ksft_exit_skip("[SKIP]: Cannot access rtc file %s - Exiting\n", + rtc_file); + + return ret; }
On 23/05/2024 18:38:07-0700, Joseph Jang wrote:
The rtctest requires the read permission on /dev/rtc0. The rtctest will be skipped if the /dev/rtc0 is not readable.
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
tools/testing/selftests/rtc/rtctest.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c index 2b12497eb30d..d104f5326cf4 100644 --- a/tools/testing/selftests/rtc/rtctest.c +++ b/tools/testing/selftests/rtc/rtctest.c @@ -483,6 +483,8 @@ __constructor_order_last(void) int main(int argc, char **argv) {
- int ret = -1;
- switch (argc) { case 2: rtc_file = argv[1];
@@ -494,5 +496,12 @@ int main(int argc, char **argv) return 1; }
- return test_harness_run(argc, argv);
- /* Run the test if rtc_file is accessible */
- if (access(rtc_file, R_OK) == 0)
ret = test_harness_run(argc, argv);- else
ksft_exit_skip("[SKIP]: Cannot access rtc file %s - Exiting\n",rtc_file);- return ret;
}
2.34.1
Hi Alexandre,
Thank you for looking at the rtc patch. I saw you Acked the [PATCH 2/2], not sure when could we see the patch in kernel master or next branch ?
Thank you, Joseph.
On 2024/6/21 3:37 AM, Alexandre Belloni wrote:
On 23/05/2024 18:38:07-0700, Joseph Jang wrote:
The rtctest requires the read permission on /dev/rtc0. The rtctest will be skipped if the /dev/rtc0 is not readable.
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
tools/testing/selftests/rtc/rtctest.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c index 2b12497eb30d..d104f5326cf4 100644 --- a/tools/testing/selftests/rtc/rtctest.c +++ b/tools/testing/selftests/rtc/rtctest.c @@ -483,6 +483,8 @@ __constructor_order_last(void) int main(int argc, char **argv) {
- int ret = -1;
- switch (argc) { case 2: rtc_file = argv[1];
@@ -494,5 +496,12 @@ int main(int argc, char **argv) return 1; }
- return test_harness_run(argc, argv);
- /* Run the test if rtc_file is accessible */
- if (access(rtc_file, R_OK) == 0)
ret = test_harness_run(argc, argv);- else
ksft_exit_skip("[SKIP]: Cannot access rtc file %s - Exiting\n",rtc_file);- return ret; }
-- 2.34.1
On 9/23/24 23:37, Joseph Jang wrote:
Hi Alexandre,
Thank you for looking at the rtc patch. I saw you Acked the [PATCH 2/2], not sure when could we see the patch in kernel master or next branch ?
Thank you, Joseph.
Please don't top post. It is hard to follow the thread.
On 2024/6/21 3:37 AM, Alexandre Belloni wrote:
On 23/05/2024 18:38:07-0700, Joseph Jang wrote:
The rtctest requires the read permission on /dev/rtc0. The rtctest will be skipped if the /dev/rtc0 is not readable.
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
Alexandre, I can take this patch through kselftest. Might have slipped through my Inbox or the assumption that this will go through rtc tree.
thanks, -- Shuah
Hello,
On 24/09/2024 10:05:43-0600, Shuah Khan wrote:
On 9/23/24 23:37, Joseph Jang wrote:
Hi Alexandre,
Thank you for looking at the rtc patch. I saw you Acked the [PATCH 2/2], not sure when could we see the patch in kernel master or next branch ?
Thank you, Joseph.
Please don't top post. It is hard to follow the thread.
On 2024/6/21 3:37 AM, Alexandre Belloni wrote:
On 23/05/2024 18:38:07-0700, Joseph Jang wrote:
The rtctest requires the read permission on /dev/rtc0. The rtctest will be skipped if the /dev/rtc0 is not readable.
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
Alexandre, I can take this patch through kselftest. Might have slipped through my Inbox or the assumption that this will go through rtc tree.
I assumed this would go through your tree, this is why I didn't carry it.
On 9/24/24 13:31, Alexandre Belloni wrote:
Hello,
On 24/09/2024 10:05:43-0600, Shuah Khan wrote:
On 9/23/24 23:37, Joseph Jang wrote:
Hi Alexandre,
Thank you for looking at the rtc patch. I saw you Acked the [PATCH 2/2], not sure when could we see the patch in kernel master or next branch ?
Thank you, Joseph.
Please don't top post. It is hard to follow the thread.
On 2024/6/21 3:37 AM, Alexandre Belloni wrote:
On 23/05/2024 18:38:07-0700, Joseph Jang wrote:
The rtctest requires the read permission on /dev/rtc0. The rtctest will be skipped if the /dev/rtc0 is not readable.
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
Alexandre, I can take this patch through kselftest. Might have slipped through my Inbox or the assumption that this will go through rtc tree.
I assumed this would go through your tree, this is why I didn't carry it.
I will take it through my tree then. Sorry for the delay.
thanks, -- Shuah
On 2024/9/25 3:57 AM, Shuah Khan wrote:
On 9/24/24 13:31, Alexandre Belloni wrote:
Hello,
On 24/09/2024 10:05:43-0600, Shuah Khan wrote:
On 9/23/24 23:37, Joseph Jang wrote:
Hi Alexandre,
Thank you for looking at the rtc patch. I saw you Acked the [PATCH 2/2], not sure when could we see the patch in kernel master or next branch ?
Thank you, Joseph.
Please don't top post. It is hard to follow the thread.
On 2024/6/21 3:37 AM, Alexandre Belloni wrote:
On 23/05/2024 18:38:07-0700, Joseph Jang wrote:
The rtctest requires the read permission on /dev/rtc0. The rtctest will be skipped if the /dev/rtc0 is not readable.
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
Alexandre, I can take this patch through kselftest. Might have slipped through my Inbox or the assumption that this will go through rtc tree.
I assumed this would go through your tree, this is why I didn't carry it.
I will take it through my tree then. Sorry for the delay.
Hi Shuah,
Thanks your help. May I know when can we see the patch on master branch ?
Thank you, Joseph.
thanks, -- Shuah
On 10/17/24 22:18, Joseph Jang wrote:
On 2024/9/25 3:57 AM, Shuah Khan wrote:
On 9/24/24 13:31, Alexandre Belloni wrote:
Hello,
On 24/09/2024 10:05:43-0600, Shuah Khan wrote:
On 9/23/24 23:37, Joseph Jang wrote:
Hi Alexandre,
Thank you for looking at the rtc patch. I saw you Acked the [PATCH 2/2], not sure when could we see the patch in kernel master or next branch ?
Thank you, Joseph.
Please don't top post. It is hard to follow the thread.
On 2024/6/21 3:37 AM, Alexandre Belloni wrote:
On 23/05/2024 18:38:07-0700, Joseph Jang wrote: > The rtctest requires the read permission on /dev/rtc0. The rtctest will > be skipped if the /dev/rtc0 is not readable. > > 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
Alexandre, I can take this patch through kselftest. Might have slipped through my Inbox or the assumption that this will go through rtc tree.
I assumed this would go through your tree, this is why I didn't carry it.
I will take it through my tree then. Sorry for the delay.
Hi Shuah,
Thanks your help. May I know when can we see the patch on master branch ?
Did you check the mainline: This is already in Linux 6.12 since rc2
commit 1ad999870a86d58246b6a614a435d055a9edf269 Author: Joseph Jang jjang@nvidia.com Date: Thu May 23 18:38:07 2024 -0700
selftest: rtc: Check if could access /dev/rtc0 before testing
thanks, -- Shuah
linux-kselftest-mirror@lists.linaro.org
-
Alexandre Belloni -
Joseph Jang -
Shuah Khan