2024-05-17 02:29:21

by Joseph Jang

[permalink] [raw]
Subject: [PATCH] selftest: rtc: Add to check rtc alarm status for alarm related test

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 presence of "alarm" in
/proc/driver/rtc.

The rtctest requires the read permission on /dev/rtc0. The rtctest will
be skipped if the /dev/rtc0 is not readable.

Requires commit 101ca8d05913b ("rtc: efi: Enable SET/GET WAKEUP services
as optional")

Reviewed-by: Jeremy Szu <[email protected]>
Reviewed-by: Matthew R. Ochs <[email protected]>
Signed-off-by: Joseph Jang <[email protected]>
---
tools/testing/selftests/rtc/Makefile | 2 +-
tools/testing/selftests/rtc/rtctest.c | 72 +++++++++++++++++++--------
2 files changed, 53 insertions(+), 21 deletions(-)

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..aa47b17fbd1a 100644
--- a/tools/testing/selftests/rtc/rtctest.c
+++ b/tools/testing/selftests/rtc/rtctest.c
@@ -8,6 +8,7 @@
#include <errno.h>
#include <fcntl.h>
#include <linux/rtc.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/ioctl.h>
@@ -24,12 +25,17 @@
#define READ_LOOP_SLEEP_MS 11

static char *rtc_file = "/dev/rtc0";
+static char *rtc_procfs = "/proc/driver/rtc";

FIXTURE(rtc) {
int fd;
};

FIXTURE_SETUP(rtc) {
+ if (access(rtc_file, R_OK) != 0)
+ SKIP(return, "Skipping test since cannot access %s, perhaps miss sudo",
+ rtc_file);
+
self->fd = open(rtc_file, O_RDONLY);
}

@@ -82,6 +88,36 @@ static void nanosleep_with_retries(long ns)
}
}

+static bool is_rtc_alarm_supported(int fd)
+{
+ struct rtc_param param = { 0 };
+ int rc;
+ char buf[1024] = { 0 };
+
+ /* 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) {
+ /* Fallback to read rtc procfs */
+ fd = open(rtc_procfs, O_RDONLY);
+ if (fd != -1) {
+ rc = read(fd, buf, sizeof(buf));
+ close(fd);
+
+ /* Check for the presence of "alarm" in the buf */
+ if (strstr(buf, "alarm") == NULL)
+ return false;
+ } else
+ return false;
+ } else {
+ if ((param.uvalue & _BITUL(RTC_FEATURE_ALARM)) == 0)
+ return false;
+ }
+
+ return true;
+}
+
TEST_F_TIMEOUT(rtc, date_read_loop, READ_LOOP_DURATION_SEC + 2) {
int rc;
long iter_count = 0;
@@ -202,6 +238,9 @@ TEST_F(rtc, alarm_alm_set) {
SKIP(return, "Skipping test since %s does not exist", rtc_file);
ASSERT_NE(-1, self->fd);

+ if (!is_rtc_alarm_supported(self->fd))
+ SKIP(return, "Skipping test since alarms are not supported.");
+
rc = ioctl(self->fd, RTC_RD_TIME, &tm);
ASSERT_NE(-1, rc);

@@ -209,11 +248,7 @@ TEST_F(rtc, alarm_alm_set) {
gmtime_r(&secs, (struct tm *)&tm);

rc = ioctl(self->fd, RTC_ALM_SET, &tm);
- if (rc == -1) {
- ASSERT_EQ(EINVAL, errno);
- TH_LOG("skip alarms are not supported.");
- return;
- }
+ ASSERT_NE(-1, rc);

rc = ioctl(self->fd, RTC_ALM_READ, &tm);
ASSERT_NE(-1, rc);
@@ -260,6 +295,9 @@ TEST_F(rtc, alarm_wkalm_set) {
SKIP(return, "Skipping test since %s does not exist", rtc_file);
ASSERT_NE(-1, self->fd);

+ if (!is_rtc_alarm_supported(self->fd))
+ SKIP(return, "Skipping test since alarms are not supported.");
+
rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time);
ASSERT_NE(-1, rc);

@@ -269,11 +307,7 @@ TEST_F(rtc, alarm_wkalm_set) {
alarm.enabled = 1;

rc = ioctl(self->fd, RTC_WKALM_SET, &alarm);
- if (rc == -1) {
- ASSERT_EQ(EINVAL, errno);
- TH_LOG("skip alarms are not supported.");
- return;
- }
+ ASSERT_NE(-1, rc);

rc = ioctl(self->fd, RTC_WKALM_RD, &alarm);
ASSERT_NE(-1, rc);
@@ -312,6 +346,9 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) {
SKIP(return, "Skipping test since %s does not exist", rtc_file);
ASSERT_NE(-1, self->fd);

+ if (!is_rtc_alarm_supported(self->fd))
+ SKIP(return, "Skipping test since alarms are not supported.");
+
rc = ioctl(self->fd, RTC_RD_TIME, &tm);
ASSERT_NE(-1, rc);

@@ -319,11 +356,7 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) {
gmtime_r(&secs, (struct tm *)&tm);

rc = ioctl(self->fd, RTC_ALM_SET, &tm);
- if (rc == -1) {
- ASSERT_EQ(EINVAL, errno);
- TH_LOG("skip alarms are not supported.");
- return;
- }
+ ASSERT_NE(-1, rc);

rc = ioctl(self->fd, RTC_ALM_READ, &tm);
ASSERT_NE(-1, rc);
@@ -370,6 +403,9 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) {
SKIP(return, "Skipping test since %s does not exist", rtc_file);
ASSERT_NE(-1, self->fd);

+ if (!is_rtc_alarm_supported(self->fd))
+ SKIP(return, "Skipping test since alarms are not supported.");
+
rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time);
ASSERT_NE(-1, rc);

@@ -379,11 +415,7 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) {
alarm.enabled = 1;

rc = ioctl(self->fd, RTC_WKALM_SET, &alarm);
- if (rc == -1) {
- ASSERT_EQ(EINVAL, errno);
- TH_LOG("skip alarms are not supported.");
- return;
- }
+ ASSERT_NE(-1, rc);

rc = ioctl(self->fd, RTC_WKALM_RD, &alarm);
ASSERT_NE(-1, rc);
--
2.34.1



2024-05-17 07:20:19

by Alexandre Belloni

[permalink] [raw]
Subject: Re: [PATCH] selftest: rtc: Add to check rtc alarm status for alarm related test

On 16/05/2024 19:28:47-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 presence of "alarm" in
> /proc/driver/rtc.
>
> The rtctest requires the read permission on /dev/rtc0. The rtctest will
> be skipped if the /dev/rtc0 is not readable.
>

This change as to be separated. Also, I'm not sure what happened with
https://lore.kernel.org/all/[email protected]/

> Requires commit 101ca8d05913b ("rtc: efi: Enable SET/GET WAKEUP services
> as optional")
>
> Reviewed-by: Jeremy Szu <[email protected]>
> Reviewed-by: Matthew R. Ochs <[email protected]>
> Signed-off-by: Joseph Jang <[email protected]>
> ---
> tools/testing/selftests/rtc/Makefile | 2 +-
> tools/testing/selftests/rtc/rtctest.c | 72 +++++++++++++++++++--------
> 2 files changed, 53 insertions(+), 21 deletions(-)
>
> 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..aa47b17fbd1a 100644
> --- a/tools/testing/selftests/rtc/rtctest.c
> +++ b/tools/testing/selftests/rtc/rtctest.c
> @@ -8,6 +8,7 @@
> #include <errno.h>
> #include <fcntl.h>
> #include <linux/rtc.h>
> +#include <stdbool.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/ioctl.h>
> @@ -24,12 +25,17 @@
> #define READ_LOOP_SLEEP_MS 11
>
> static char *rtc_file = "/dev/rtc0";
> +static char *rtc_procfs = "/proc/driver/rtc";
>
> FIXTURE(rtc) {
> int fd;
> };
>
> FIXTURE_SETUP(rtc) {
> + if (access(rtc_file, R_OK) != 0)
> + SKIP(return, "Skipping test since cannot access %s, perhaps miss sudo",
> + rtc_file);

> +
> self->fd = open(rtc_file, O_RDONLY);
> }
>
> @@ -82,6 +88,36 @@ static void nanosleep_with_retries(long ns)
> }
> }
>
> +static bool is_rtc_alarm_supported(int fd)
> +{
> + struct rtc_param param = { 0 };
> + int rc;
> + char buf[1024] = { 0 };
> +
> + /* 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) {
> + /* Fallback to read rtc procfs */
> + fd = open(rtc_procfs, O_RDONLY);

I think I was clear on the previous thread, no new users of the procfs
interface. You can carry this n your own tree but that can't be
upstream.

> + if (fd != -1) {
> + rc = read(fd, buf, sizeof(buf));
> + close(fd);
> +
> + /* Check for the presence of "alarm" in the buf */
> + if (strstr(buf, "alarm") == NULL)
> + return false;
> + } else
> + return false;
> + } else {
> + if ((param.uvalue & _BITUL(RTC_FEATURE_ALARM)) == 0)
> + return false;
> + }
> +
> + return true;
> +}
> +
> TEST_F_TIMEOUT(rtc, date_read_loop, READ_LOOP_DURATION_SEC + 2) {
> int rc;
> long iter_count = 0;
> @@ -202,6 +238,9 @@ TEST_F(rtc, alarm_alm_set) {
> SKIP(return, "Skipping test since %s does not exist", rtc_file);
> ASSERT_NE(-1, self->fd);
>
> + if (!is_rtc_alarm_supported(self->fd))
> + SKIP(return, "Skipping test since alarms are not supported.");
> +
> rc = ioctl(self->fd, RTC_RD_TIME, &tm);
> ASSERT_NE(-1, rc);
>
> @@ -209,11 +248,7 @@ TEST_F(rtc, alarm_alm_set) {
> gmtime_r(&secs, (struct tm *)&tm);
>
> rc = ioctl(self->fd, RTC_ALM_SET, &tm);
> - if (rc == -1) {
> - ASSERT_EQ(EINVAL, errno);
> - TH_LOG("skip alarms are not supported.");
> - return;
> - }
> + ASSERT_NE(-1, rc);
>
> rc = ioctl(self->fd, RTC_ALM_READ, &tm);
> ASSERT_NE(-1, rc);
> @@ -260,6 +295,9 @@ TEST_F(rtc, alarm_wkalm_set) {
> SKIP(return, "Skipping test since %s does not exist", rtc_file);
> ASSERT_NE(-1, self->fd);
>
> + if (!is_rtc_alarm_supported(self->fd))
> + SKIP(return, "Skipping test since alarms are not supported.");
> +
> rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time);
> ASSERT_NE(-1, rc);
>
> @@ -269,11 +307,7 @@ TEST_F(rtc, alarm_wkalm_set) {
> alarm.enabled = 1;
>
> rc = ioctl(self->fd, RTC_WKALM_SET, &alarm);
> - if (rc == -1) {
> - ASSERT_EQ(EINVAL, errno);
> - TH_LOG("skip alarms are not supported.");
> - return;
> - }
> + ASSERT_NE(-1, rc);
>
> rc = ioctl(self->fd, RTC_WKALM_RD, &alarm);
> ASSERT_NE(-1, rc);
> @@ -312,6 +346,9 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) {
> SKIP(return, "Skipping test since %s does not exist", rtc_file);
> ASSERT_NE(-1, self->fd);
>
> + if (!is_rtc_alarm_supported(self->fd))
> + SKIP(return, "Skipping test since alarms are not supported.");
> +
> rc = ioctl(self->fd, RTC_RD_TIME, &tm);
> ASSERT_NE(-1, rc);
>
> @@ -319,11 +356,7 @@ TEST_F_TIMEOUT(rtc, alarm_alm_set_minute, 65) {
> gmtime_r(&secs, (struct tm *)&tm);
>
> rc = ioctl(self->fd, RTC_ALM_SET, &tm);
> - if (rc == -1) {
> - ASSERT_EQ(EINVAL, errno);
> - TH_LOG("skip alarms are not supported.");
> - return;
> - }
> + ASSERT_NE(-1, rc);
>
> rc = ioctl(self->fd, RTC_ALM_READ, &tm);
> ASSERT_NE(-1, rc);
> @@ -370,6 +403,9 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) {
> SKIP(return, "Skipping test since %s does not exist", rtc_file);
> ASSERT_NE(-1, self->fd);
>
> + if (!is_rtc_alarm_supported(self->fd))
> + SKIP(return, "Skipping test since alarms are not supported.");
> +
> rc = ioctl(self->fd, RTC_RD_TIME, &alarm.time);
> ASSERT_NE(-1, rc);
>
> @@ -379,11 +415,7 @@ TEST_F_TIMEOUT(rtc, alarm_wkalm_set_minute, 65) {
> alarm.enabled = 1;
>
> rc = ioctl(self->fd, RTC_WKALM_SET, &alarm);
> - if (rc == -1) {
> - ASSERT_EQ(EINVAL, errno);
> - TH_LOG("skip alarms are not supported.");
> - return;
> - }
> + ASSERT_NE(-1, rc);
>
> rc = ioctl(self->fd, RTC_WKALM_RD, &alarm);
> ASSERT_NE(-1, rc);
> --
> 2.34.1
>

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

2024-05-17 07:54:29

by Joseph Jang

[permalink] [raw]
Subject: Re: [PATCH] selftest: rtc: Add to check rtc alarm status for alarm related test



On 2024/5/17 3:19 PM, Alexandre Belloni wrote:
> On 16/05/2024 19:28:47-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 presence of "alarm" in
>> /proc/driver/rtc.
>>
>> The rtctest requires the read permission on /dev/rtc0. The rtctest will
>> be skipped if the /dev/rtc0 is not readable.
>>
>
> This change as to be separated. Also, I'm not sure what happened with
> https://lore.kernel.org/all/[email protected]/
>

I apply above patch and seems like still cannot detect the read
permission on /dev/rtc0. I guess the 'F_OK' just check the `/dev/rtc0`
was there.

I share the error logs by following for your reference.

TAP version 13
1..1
# timeout set to 210
# selftests: rtc: rtctest
# TAP version 13
# 1..8
# # Starting 8 tests from 1 test cases.
# # RUN rtc.date_read ...
# # rtctest.c:53:date_read:Expected -1 (-1) != self->fd (-1)
# # date_read: Test terminated by assertion
# # FAIL rtc.date_read

Not sure if we could skip the testing by following change ?

FIXTURE_SETUP(rtc) {
+ if (access(rtc_file, R_OK) != 0)
+ SKIP(return, "Skipping test since cannot access %s,
perhaps miss sudo",
+ rtc_file)
+
self->fd = open(rtc_file, O_RDONLY);
}

And I make sure we need root permission to access `/dev/rtc0`.



>> Requires commit 101ca8d05913b ("rtc: efi: Enable SET/GET WAKEUP services
>> as optional")
>>
>> Reviewed-by: Jeremy Szu <[email protected]>
>> Reviewed-by: Matthew R. Ochs <[email protected]>
>> Signed-off-by: Joseph Jang <[email protected]>
>> ---
>> tools/testing/selftests/rtc/Makefile | 2 +-
>> tools/testing/selftests/rtc/rtctest.c | 72 +++++++++++++++++++--------
>> 2 files changed, 53 insertions(+), 21 deletions(-)
>>
>> 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..aa47b17fbd1a 100644
>> --- a/tools/testing/selftests/rtc/rtctest.c
>> +++ b/tools/testing/selftests/rtc/rtctest.c
>> @@ -8,6 +8,7 @@
>> #include <errno.h>
>> #include <fcntl.h>
>> #include <linux/rtc.h>
>> +#include <stdbool.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <sys/ioctl.h>
>> @@ -24,12 +25,17 @@
>> #define READ_LOOP_SLEEP_MS 11
>>
>> static char *rtc_file = "/dev/rtc0";
>> +static char *rtc_procfs = "/proc/driver/rtc";
>>
>> FIXTURE(rtc) {
>> int fd;
>> };
>>
>> FIXTURE_SETUP(rtc) {
>> + if (access(rtc_file, R_OK) != 0)
>> + SKIP(return, "Skipping test since cannot access %s, perhaps miss sudo",
>> + rtc_file);
>
>> +
>> self->fd = open(rtc_file, O_RDONLY);
>> }
>>
>> @@ -82,6 +88,36 @@ static void nanosleep_with_retries(long ns)
>> }
>> }
>>
>> +static bool is_rtc_alarm_supported(int fd)
>> +{
>> + struct rtc_param param = { 0 };
>> + int rc;
>> + char buf[1024] = { 0 };
>> +
>> + /* 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) {
>> + /* Fallback to read rtc procfs */
>> + fd = open(rtc_procfs, O_RDONLY);
>
> I think I was clear on the previous thread, no new users of the procfs
> interface. You can carry this n your own tree but that can't be
> upstream.
>

Okay ~ If we use RTC_PARAM_GET ioctl to detect rtc feature only, not
sure if that is okay for upstream ?

Thank you,
Joseph.


2024-05-17 08:09:01

by Alexandre Belloni

[permalink] [raw]
Subject: Re: [PATCH] selftest: rtc: Add to check rtc alarm status for alarm related test

On 17/05/2024 15:53:58+0800, Joseph Jang wrote:
>
>
> On 2024/5/17 3:19 PM, Alexandre Belloni wrote:
> > On 16/05/2024 19:28:47-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 presence of "alarm" in
> > > /proc/driver/rtc.
> > >
> > > The rtctest requires the read permission on /dev/rtc0. The rtctest will
> > > be skipped if the /dev/rtc0 is not readable.
> > >
> >
> > This change as to be separated. Also, I'm not sure what happened with
> > https://lore.kernel.org/all/[email protected]/
> >
>
> I apply above patch and seems like still cannot detect the read
> permission on /dev/rtc0. I guess the 'F_OK' just check the `/dev/rtc0`
> was there.
>
> I share the error logs by following for your reference.
>
> TAP version 13
> 1..1
> # timeout set to 210
> # selftests: rtc: rtctest
> # TAP version 13
> # 1..8
> # # Starting 8 tests from 1 test cases.
> # # RUN rtc.date_read ...
> # # rtctest.c:53:date_read:Expected -1 (-1) != self->fd (-1)
> # # date_read: Test terminated by assertion
> # # FAIL rtc.date_read
>
> Not sure if we could skip the testing by following change ?
>
> FIXTURE_SETUP(rtc) {
> + if (access(rtc_file, R_OK) != 0)
> + SKIP(return, "Skipping test since cannot access %s, perhaps
> miss sudo",
> + rtc_file)
> +
> self->fd = open(rtc_file, O_RDONLY);
> }
>
> And I make sure we need root permission to access `/dev/rtc0`.
>

There is no need to test for every tests of the suite, the check could
be done once. To be clear, you don't need to be root to access the RTC,
you need CAP_SYS_TIME and CAP_SYS_RESOURCE.

>
>
> > > Requires commit 101ca8d05913b ("rtc: efi: Enable SET/GET WAKEUP services
> > > as optional")
> > >
> > > Reviewed-by: Jeremy Szu <[email protected]>
> > > Reviewed-by: Matthew R. Ochs <[email protected]>
> > > Signed-off-by: Joseph Jang <[email protected]>
> > > ---
> > > tools/testing/selftests/rtc/Makefile | 2 +-
> > > tools/testing/selftests/rtc/rtctest.c | 72 +++++++++++++++++++--------
> > > 2 files changed, 53 insertions(+), 21 deletions(-)
> > >
> > > 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..aa47b17fbd1a 100644
> > > --- a/tools/testing/selftests/rtc/rtctest.c
> > > +++ b/tools/testing/selftests/rtc/rtctest.c
> > > @@ -8,6 +8,7 @@
> > > #include <errno.h>
> > > #include <fcntl.h>
> > > #include <linux/rtc.h>
> > > +#include <stdbool.h>
> > > #include <stdio.h>
> > > #include <stdlib.h>
> > > #include <sys/ioctl.h>
> > > @@ -24,12 +25,17 @@
> > > #define READ_LOOP_SLEEP_MS 11
> > > static char *rtc_file = "/dev/rtc0";
> > > +static char *rtc_procfs = "/proc/driver/rtc";
> > > FIXTURE(rtc) {
> > > int fd;
> > > };
> > > FIXTURE_SETUP(rtc) {
> > > + if (access(rtc_file, R_OK) != 0)
> > > + SKIP(return, "Skipping test since cannot access %s, perhaps miss sudo",
> > > + rtc_file);
> >
> > > +
> > > self->fd = open(rtc_file, O_RDONLY);
> > > }
> > > @@ -82,6 +88,36 @@ static void nanosleep_with_retries(long ns)
> > > }
> > > }
> > > +static bool is_rtc_alarm_supported(int fd)
> > > +{
> > > + struct rtc_param param = { 0 };
> > > + int rc;
> > > + char buf[1024] = { 0 };
> > > +
> > > + /* 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) {
> > > + /* Fallback to read rtc procfs */
> > > + fd = open(rtc_procfs, O_RDONLY);
> >
> > I think I was clear on the previous thread, no new users of the procfs
> > interface. You can carry this n your own tree but that can't be
> > upstream.
> >
>
> Okay ~ If we use RTC_PARAM_GET ioctl to detect rtc feature only, not
> sure if that is okay for upstream ?

Yes, using RTC_PARAM_GET is ok but I'm pretty sure this is not solving
you are seeing following the efi patch you are pointing to.

The patch clears RTC_FEATURE_ALARM when the alarm is not present which
will ensure the ioctl fails and so the test will already be skipped. My
guess is that your ssue ias actually when the alarm is present and the
test will run and wait forever for an interrupt that will never come.


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

2024-05-17 08:48:19

by Joseph Jang

[permalink] [raw]
Subject: Re: [PATCH] selftest: rtc: Add to check rtc alarm status for alarm related test



On 2024/5/17 4:08 PM, Alexandre Belloni wrote:
> On 17/05/2024 15:53:58+0800, Joseph Jang wrote:
>>
>>
>> On 2024/5/17 3:19 PM, Alexandre Belloni wrote:
>>> On 16/05/2024 19:28:47-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 presence of "alarm" in
>>>> /proc/driver/rtc.
>>>>
>>>> The rtctest requires the read permission on /dev/rtc0. The rtctest will
>>>> be skipped if the /dev/rtc0 is not readable.
>>>>
>>>
>>> This change as to be separated. Also, I'm not sure what happened with
>>> https://lore.kernel.org/all/[email protected]/
>>>
>>
>> I apply above patch and seems like still cannot detect the read
>> permission on /dev/rtc0. I guess the 'F_OK' just check the `/dev/rtc0`
>> was there.
>>
>> I share the error logs by following for your reference.
>>
>> TAP version 13
>> 1..1
>> # timeout set to 210
>> # selftests: rtc: rtctest
>> # TAP version 13
>> # 1..8
>> # # Starting 8 tests from 1 test cases.
>> # # RUN rtc.date_read ...
>> # # rtctest.c:53:date_read:Expected -1 (-1) != self->fd (-1)
>> # # date_read: Test terminated by assertion
>> # # FAIL rtc.date_read
>>
>> Not sure if we could skip the testing by following change ?
>>
>> FIXTURE_SETUP(rtc) {
>> + if (access(rtc_file, R_OK) != 0)
>> + SKIP(return, "Skipping test since cannot access %s, perhaps
>> miss sudo",
>> + rtc_file)
>> +
>> self->fd = open(rtc_file, O_RDONLY);
>> }
>>
>> And I make sure we need root permission to access `/dev/rtc0`.
>>
>
> There is no need to test for every tests of the suite, the check could
> be done once. To be clear, you don't need to be root to access the RTC,
> you need CAP_SYS_TIME and CAP_SYS_RESOURCE.
>

Thank you so much ~ not sure if we could have the following change ?

Use `access(rtc_file, R_OK)` to check read permission on
`/dev/rtc0` in main(). And user could try to figure out why cannot
access rtc file by themselves.

@@ -430,5 +433,11 @@ int main(int argc, char **argv)
return 1;
}

+ // Run the test if rtc_file is valid
+ if (access(rtc_file, R_OK) == 0)
+ ret = test_harness_run(argc, argv);
+ else
+ ksft_exit_fail_msg("[ERROR]: Cannot access rtc file %s - Exiting\n",
rtc_file);
+
+ return ret;

>>
>>
>>>> Requires commit 101ca8d05913b ("rtc: efi: Enable SET/GET WAKEUP services
>>>> as optional")
>>>>
>>>> Reviewed-by: Jeremy Szu <[email protected]>
>>>> Reviewed-by: Matthew R. Ochs <[email protected]>
>>>> Signed-off-by: Joseph Jang <[email protected]>
>>>> ---
>>>> tools/testing/selftests/rtc/Makefile | 2 +-
>>>> tools/testing/selftests/rtc/rtctest.c | 72 +++++++++++++++++++--------
>>>> 2 files changed, 53 insertions(+), 21 deletions(-)
>>>>
>>>> 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..aa47b17fbd1a 100644
>>>> --- a/tools/testing/selftests/rtc/rtctest.c
>>>> +++ b/tools/testing/selftests/rtc/rtctest.c
>>>> @@ -8,6 +8,7 @@
>>>> #include <errno.h>
>>>> #include <fcntl.h>
>>>> #include <linux/rtc.h>
>>>> +#include <stdbool.h>
>>>> #include <stdio.h>
>>>> #include <stdlib.h>
>>>> #include <sys/ioctl.h>
>>>> @@ -24,12 +25,17 @@
>>>> #define READ_LOOP_SLEEP_MS 11
>>>> static char *rtc_file = "/dev/rtc0";
>>>> +static char *rtc_procfs = "/proc/driver/rtc";
>>>> FIXTURE(rtc) {
>>>> int fd;
>>>> };
>>>> FIXTURE_SETUP(rtc) {
>>>> + if (access(rtc_file, R_OK) != 0)
>>>> + SKIP(return, "Skipping test since cannot access %s, perhaps miss sudo",
>>>> + rtc_file);
>>>
>>>> +
>>>> self->fd = open(rtc_file, O_RDONLY);
>>>> }
>>>> @@ -82,6 +88,36 @@ static void nanosleep_with_retries(long ns)
>>>> }
>>>> }
>>>> +static bool is_rtc_alarm_supported(int fd)
>>>> +{
>>>> + struct rtc_param param = { 0 };
>>>> + int rc;
>>>> + char buf[1024] = { 0 };
>>>> +
>>>> + /* 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) {
>>>> + /* Fallback to read rtc procfs */
>>>> + fd = open(rtc_procfs, O_RDONLY);
>>>
>>> I think I was clear on the previous thread, no new users of the procfs
>>> interface. You can carry this n your own tree but that can't be
>>> upstream.
>>>
>>
>> Okay ~ If we use RTC_PARAM_GET ioctl to detect rtc feature only, not
>> sure if that is okay for upstream ?
>
> Yes, using RTC_PARAM_GET is ok but I'm pretty sure this is not solving
> you are seeing following the efi patch you are pointing to.
>
> The patch clears RTC_FEATURE_ALARM when the alarm is not present which
> will ensure the ioctl fails and so the test will already be skipped. My
> guess is that your ssue ias actually when the alarm is present and the
> test will run and wait forever for an interrupt that will never come.
>
>

Thanks for helping to review our test again. Our original intention was
to check whether miss the efi kernel patch on target testing kernel.

I verified it and make sure we still could use RTC_PARAM_GET for our
testing on new kernel (> 5.16). If the target testing kernel miss the
efi patch, rtctest could detect the RTC_FEATURE_ALARM was enabled and
the RTC_WKALM_SET will report error due to efi.set_wakeup_time() return
EFI_UNSUPPORTED (3).

rc = ioctl(self->fd, RTC_WKALM_SET, &alarm);
ASSERT_NE(-1, rc);

And if the efi patch has been applied and it clears RTC_FEATURE_ALARM,
we could use the RTC_PARAM_GET to know rtc feature was disabled. So the
rtctest will skip the testing without reporting errors. That behavior is
what we expect.

Not sure if that work for you ?

Thank you,
Joseph.