Date: Fri, 17 May 2024 10:08:43 +0200
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: Joseph Jang <jjang@...dia.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 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@...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, &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

Powered by blists - more mailing lists