[PATCH 0/2] selftest: rtc: Add rtc feature detection and rtc file check

[PATCH 0/2] selftest: rtc: Add rtc feature detection and rtc file check

[Joseph Jang]

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(-)

-- 
2.34.1

[PATCH 1/2] selftest: rtc: Add to check rtc alarm status for alarm related test

[Joseph Jang]

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, &param);
+	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

[PATCH 2/2] selftest: rtc: Check if could access /dev/rtc0 before testing

[Joseph Jang]

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;
 }
-- 
2.34.1

Re: [PATCH 1/2] selftest: rtc: Add to check rtc alarm status for alarm related test

[Alexandre Belloni]

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, &param);
> +	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
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/2] selftest: rtc: Check if could access /dev/rtc0 before testing

[Alexandre Belloni]

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
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/2] selftest: rtc: Add to check rtc alarm status for alarm related test

[Joseph Jang]

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, &param);
       |                       ^~~~~~~~~~~~~
       |                       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.

Re: [PATCH 1/2] selftest: rtc: Add to check rtc alarm status for alarm related test

[Joseph Jang]

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, &param);
       |                       ^~~~~~~~~~~~~
       |                       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.

Re: [PATCH 2/2] selftest: rtc: Check if could access /dev/rtc0 before testing

[Joseph Jang]

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
>>
> 

Re: [PATCH 2/2] selftest: rtc: Check if could access /dev/rtc0 before testing

[Shuah Khan]

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

Re: [PATCH 2/2] selftest: rtc: Check if could access /dev/rtc0 before testing

[Alexandre Belloni]

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.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/2] selftest: rtc: Check if could access /dev/rtc0 before testing

[Shuah Khan]

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

Re: [PATCH 2/2] selftest: rtc: Check if could access /dev/rtc0 before testing

[Joseph Jang]

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

Re: [PATCH 1/2] selftest: rtc: Add to check rtc alarm status for alarm related test

[Joseph Jang]

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, &param);
>         |                       ^~~~~~~~~~~~~
>         |                       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.

Re: [PATCH 1/2] selftest: rtc: Add to check rtc alarm status for alarm related test

[Alexandre Belloni]

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, &param);
> >         |                       ^~~~~~~~~~~~~
> >         |                       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.
> 
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/2] selftest: rtc: Check if could access /dev/rtc0 before testing

[Shuah Khan]

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

Re: [PATCH 1/2] selftest: rtc: Add to check rtc alarm status for alarm related test

[Shuah Khan]

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, &param);
>>>          |                       ^~~~~~~~~~~~~
>>>          |                       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