Re: [PATCH 1/2] selftest: rtc: Add to check rtc alarm status for alarm related test

[Shuah Khan <skhan@linuxfoundation.org>]

On 10/18/24 02:27, Alexandre Belloni wrote:
> On 18/10/2024 12:26:44+0800, Joseph Jang wrote:
>>
>>
>> On 2024/6/24 9:43 AM, Joseph Jang wrote:
>>>
>>>
>>> On 2024/6/21 3:36 AM, Alexandre Belloni wrote:
>>>> On 23/05/2024 18:38:06-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 error number of
>>>>> (RTC_ALM_SET/RTC_WKALM_SET) ioctl call for alarm feature detection.
>>>>>
>>>>> Requires commit 101ca8d05913b ("rtc: efi: Enable SET/GET WAKEUP services
>>>>> as optional")
>>>>>
>>>>> Reviewed-by: Koba Ko <kobak@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 | 64 +++++++++++++++++++++++++++
>>>>>     2 files changed, 65 insertions(+), 1 deletion(-)
>>>>>
>>>>> 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/
>>>>
>>>> Is this change actually needed?

I saw this and figured it is is still in review.

>>>
>>> If we didn't include "-I../../../../usr/include/" in rtctest Makefile,
>>> we may encounter build errors like the following because rtctest default
>>> look at the header file from /usr/include/linux/rtc.h which miss the
>>> definition of struct rtc_param, RTC_PARAM_FEATURES and RTC_PARAM_GET.
>>>
>>> rtctest.c: In function ‘get_rtc_alarm_state’:
>>> rtctest.c:94:15: error: variable ‘param’ has initializer but incomplete
>>> type
>>>       94 |        struct rtc_param param = { 0 };
>>>          |               ^~~~~~~~~
>>> rtctest.c:94:35: warning: excess elements in struct initializer
>>>       94 |        struct rtc_param param = { 0 };
>>>          |                                   ^
>>> rtctest.c:94:35: note: (near initialization for ‘param’)
>>> rtctest.c:94:25: error: storage size of ‘param’ isn’t known
>>>       94 |        struct rtc_param param = { 0 };
>>>          |                         ^~~~~
>>> rtctest.c:98:22: error: ‘RTC_PARAM_FEATURES’ undeclared (first use in
>>> this function)
>>>       98 |        param.param = RTC_PARAM_FEATURES;
>>>          |                      ^~~~~~~~~~~~~~~~~~
>>> rtctest.c:98:22: note: each undeclared identifier is reported only once
>>> for each function it appears in
>>> rtctest.c:100:23: error: ‘RTC_PARAM_GET’ undeclared (first use in this
>>> function); did you mean ‘RTC_ALM_SET’?
>>>      100 |        rc = ioctl(fd, RTC_PARAM_GET, &param);
>>>          |                       ^~~~~~~~~~~~~
>>>          |                       RTC_ALM_SET
>>>
>>> After adding "-I../../../../usr/include/", the rtctest will look at
>>> linux kernel source header files from
>>> <Linux root directory>/usr/include/linux/rtc.h to find the definition of
>>> struct rtc_param, RTC_PARAM_FEATURES and RTC_PARAM_GET and fix the
>>> rtctest build errors.
>>>
>>>
>>> Thank you,
>>> Joseph.
>>>
>>>   >
>> Hi Alexandre,
>>
>> Thank you for reviewing the kernel patch [PATCH 1/2].
>> We are still not sure if we could include linux headers files from kernel
>> source directory by the following change ?
>>
>> -CFLAGS += -O3 -Wl,-no-as-needed -Wall
>> +CFLAGS += -O3 -Wl,-no-as-needed -Wall -I../../../../usr/include/

You have to say $(top_srcdir)instead of hardcoding the path
> 
> I guess this is ok, I expected Shuah to take this path too.
> 
>>

Not as is. Need v2 for this with the above change.

thanks,
-- Shuah