Received-SPF: pass (google.com: domain of linux-kernel+bounces-181787-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 17 May 2024 09:19:51 +0200
From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Joseph Jang <jjang@nvidia.com>
Cc: shuah@kernel.org, avagin@google.com, amir73il@gmail.com,
	brauner@kernel.org, mochs@nvidia.com, jszu@nvidia.com,
	linux-kernel@vger.kernel.org, linux-rtc@vger.kernel.org,
	linux-kselftest@vger.kernel.org, linux-tegra@vger.kernel.org
Subject: Re: [PATCH] selftest: rtc: Add to check rtc alarm status for alarm
 related test
Message-ID: <202405170719515a9c6d2f@mail.local>
References: <20240517022847.4094731-1-jjang@nvidia.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240517022847.4094731-1-jjang@nvidia.com>

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, &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