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

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

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?
> > 
> > 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/

I guess this is ok, I expected Shuah to take this path too.

> 
> Thank you,
> Joseph.
> 
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com