| Message ID | 20240517022847.4094731-1-jjang@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftest: rtc: Add to check rtc alarm status for alarm related test | expand |
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/ > 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. > + 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 >
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.
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/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`. > 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 <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 ? 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.
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/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`. >> > > 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 <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 ? > > 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.
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); + 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);
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 <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(-)