Messages in this thread Patch in this message | | | Date | Fri, 17 May 2024 15:53:58 +0800 | Subject | Re: [PATCH] selftest: rtc: Add to check rtc alarm status for alarm related test | From | Joseph Jang <> |
| |
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/20230717175251.54390-1-atulpant.linux@gmail.com/ >
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 <jszu@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 | 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, ¶m); >> + 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.
| |