[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ea0bacc0-2765-4d8f-8b62-0e9c45c560fc@nvidia.com>
Date: Fri, 17 May 2024 15:53:58 +0800
From: Joseph Jang <jjang@...dia.com>
To: Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc: shuah@...nel.org, avagin@...gle.com, amir73il@...il.com,
brauner@...nel.org, mochs@...dia.com, jszu@...dia.com,
linux-kernel@...r.kernel.org, linux-rtc@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-tegra@...r.kernel.org
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/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@...dia.com>
>> Reviewed-by: Matthew R. Ochs <mochs@...dia.com>
>> Signed-off-by: Joseph Jang <jjang@...dia.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.
Powered by blists - more mailing lists