Received-SPF: pass (google.com: domain of linux-kernel+bounces-181817-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <ea0bacc0-2765-4d8f-8b62-0e9c45c560fc@nvidia.com>
Date: Fri, 17 May 2024 15:53:58 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] selftest: rtc: Add to check rtc alarm status for alarm
 related test
To: Alexandre Belloni <alexandre.belloni@bootlin.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
References: <20240517022847.4094731-1-jjang@nvidia.com>
 <202405170719515a9c6d2f@mail.local>
Content-Language: en-US
From: Joseph Jang <jjang@nvidia.com>
In-Reply-To: <202405170719515a9c6d2f@mail.local>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?TFJhN29PeUQyRUNaMHpMaGkydjkxdjdQa3lDNnkxRWVBdDd6di9NeW5vNUJG?=
 =?utf-8?B?K25URkZNMnR3L2NTVm1wTzh1WXBFNk9mSlVEdURpRWpzaUR4ajQ2VlFERW9G?=
 =?utf-8?B?eWV2NHlnZytPM1NYMGJyR2pJZzVnSTBXV2RKUVoxbk90VTlOK2RqUlBnTytW?=
 =?utf-8?B?RDBRNFdxQVgvVG9WM1dpUmN1bHcxSjdyZEtsUDBWaTBYRDVrakRzRkJOOXc1?=
 =?utf-8?B?V2kxc24rTzluVi9IV0FzSk8xcnlOdEl3MVNyVEZLQlZ1ZjMvcjA1UEtjVmxJ?=
 =?utf-8?B?TzJzT2NjYXFVR2JBMXduVVJRYkJTdUZ4SnNHYm9TK3JuMENITFMrbm9SK0Vw?=
 =?utf-8?B?QUcxbmtPSWt0cllMZFdvTHZIL0xSV2d2bk4rUklCZHozOTVSRWtHUXQ5d3BF?=
 =?utf-8?B?Mk9pSUMwUkZpaWJqdmsyZldiV2dQT3pOSFFKQlJ5em5NS0d6QlUrWnNLQlBB?=
 =?utf-8?B?RGJiRG93aWZCV2owTzQwTkVXRjVKelB3NGZQL2ZIWGRxdk9RSXdRSjlzYjkw?=
 =?utf-8?B?NHVvYi85cUpyd3NHNitkbXdwRnVwbmtENHlSWmRnWk9iVEQraFV3VXE4ZWpi?=
 =?utf-8?B?c0llbHBVaFduRUxEcHJIZ0dWT25oRVhrWjY0R2d3SmpBcmJYNXJxbDBTQWgv?=
 =?utf-8?B?SlNzdnVkQUozNHp3ZVFIck5Ya1lvU0RPTlpGTEM0THZYR1dKUnpXRkx1ZFZE?=
 =?utf-8?B?cndFY0RqWWcweGQ3bnpaZTl4d0I5dmgvVkFMWno5U3JVU2NxSXcxNElJT28y?=
 =?utf-8?B?ZCt3bDJxcTl4VjFxdnFmL0UzeVF0SjA1Y01xVzc5QklSZ2pmeFRnL3MrVHNL?=
 =?utf-8?B?aWY2aDI3N3BLSU5RYXMwS0NPTis3WE03Mk4yK3ZVRG1yNldUREt4bVVTdmx2?=
 =?utf-8?B?YnlwQmE1cFR5OVp6ZjhFYXpaWWduTW4vVS9TRnlaLzAyd3VoZ29wM0JSallQ?=
 =?utf-8?B?YnZUa3lyeXYydXdhZjhiLzZvNkdzYVRXZi9jeGVWSFB6QzBhTGpIMEN4T3Rs?=
 =?utf-8?B?bnpTQko1L0RxZ2dJQXcrMy9LUE1PbG8xTVlEUXZOcGc2NThJeElKNHd2MFox?=
 =?utf-8?B?czZXdzN4ZHY0dWpHekNyNmF3SnBLM0xIdEo4Sm5ScVI3anp5ZFd1Y0NDM2hN?=
 =?utf-8?B?SWJlY1UyT0tIR3FPTXZYaWpaellIRUdOVFRBaEs3ME9VeXVCd29Ec0tYaVhY?=
 =?utf-8?B?TGlRVVF1cVFnMnU5Zk5LeDNLS2hSSDl2SUY3SWpLbXNJY3NCV1RVQ1hXR1JJ?=
 =?utf-8?B?NFdqVDB6bjcweUZGNUc2R3Zjc1g3KzVIc3JYeWtYRFdCS1h4U05TVlhXbFpD?=
 =?utf-8?B?ZzNSYTV3T0xISlY4allrQWlZOTNiN1NneWJmRm9sUGlrU0NzRG82eDVpdit0?=
 =?utf-8?B?aXpYVENUT1ZobjlmYTJ3S0dlbFpuN05tWXo5SmwwZFF2SWthZThuVVkzUVVS?=
 =?utf-8?B?MWxYZXBUS1UrUG1uWFZKZlpVSUd6L0t3aTlrcnJGWU9waitoRjVpbVdvUTBi?=
 =?utf-8?B?akNqS0liL3B6Smo1KzE3dzVWNGdLQ0JVbWdGY1ZBR2M4cVFCVG5jY3liZUVJ?=
 =?utf-8?B?bUp0R2k4KzZMTjJtTHNUYnd5MmtpQUVhaDduaVExb0I2bkFscWJ2YVZOQlJS?=
 =?utf-8?B?NGIxQkJOYnZpNHk3VUlMUVY3WEk3cmNtVFIva1l3NUVvUXhSeU5LMy94enBy?=
 =?utf-8?B?dHNQVDhRNW9aUTZ1RldYc3l1TGs4S05sZll5VmhSSTZpM1EybVJ4bkhvOEhN?=
 =?utf-8?B?OFBSS3hEbGlHM1dnVlhzelBrVnFvUU0rQ3hIandoZzJxdTMvRWl2SlNzenBJ?=
 =?utf-8?B?cHM3d1RZNmJlQkxCN09ubzROOGZZSDdjODNxTktQUzBmYUovL3puRnIwcE1K?=
 =?utf-8?B?bnh3ZUhMdmN1cW51enhDZHlTcEppOVJpMFZHczBjelhYSkZaTHRhMUt6NDFO?=
 =?utf-8?B?SjdCNDNDVFpZdmE4S2dhT1FHdWNqeUJSTkRlc2laQXBzaUs3YmZ1RnowbERU?=
 =?utf-8?B?NGd0azJYLzlaSElVOVdwelVqMmJLMExnRlZyMjVlajBEUGdIYmI4MnNFMEpm?=
 =?utf-8?B?bmNBamIwVzYzVW9Pbnc0ei82elBoV0F4RmhFYVFQMWpGYURNa3AxNjVQQWFs?=
 =?utf-8?Q?n0diI0vNUhwRHvNgojP2hltAe?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 006e10e1-c2b0-4eaa-0737-08dc76468896
X-MS-Exchange-CrossTenant-AuthSource: IA0PR12MB8374.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 May 2024 07:54:09.4783
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: w/fgsylteoQ+jUzKMndhqr4GxI3k8jv6IGlysjSKPv1VEPtU5634kQ//m5eBe1vjyprM1ddiHsgvjkaFrOtH0g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4049


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

Thank you,
Joseph.