* [LTP] [PATCH v2 1/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to read and set RTC time
2020-04-23 15:06 [LTP] [PATCH v2 0/3] Add tests for a group of real time clock ioctls Filip Bozuta
@ 2020-04-23 15:06 ` Filip Bozuta
2020-04-30 15:13 ` Cyril Hrubis
2020-04-23 15:06 ` [LTP] [PATCH v2 2/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to read and set RTC alarm time Filip Bozuta
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Filip Bozuta @ 2020-04-23 15:06 UTC (permalink / raw)
To: ltp
From: Filip Bozuta <Filip.Bozuta@rt-rk.com>
This patch tests functionalities of following ioctls:
RTC_RD_TIME - Getting RTC time
Returns this RTC's time in the following structure:
struct rtc_time {
int tm_sec;
int tm_min;
int tm_hour;
int tm_mday;
int tm_mon;
int tm_year;
int tm_wday; /* unused */
int tm_yday; /* unused */
int tm_isdst; /* unused */
};
The fields in this structure have the same meaning and
ranges as the tm structure described in gmtime man page.
A pointer to this structure should be passed as the third
ioctl argument.
RTC_SET_TIME - Setting RTC time
Sets this RTC's time to the time specified by the rtc_time
structure pointed to by the third ioctl argument. To set the
RTC's time the process must be privileged (i.e., have the
CAP_SYS_TIME capability).
Signed-off-by: Filip Bozuta <Filip.Bozuta@rt-rk.com>
---
runtest/syscalls | 2 +
testcases/kernel/syscalls/ioctl/.gitignore | 1 +
testcases/kernel/syscalls/ioctl/ioctl_rtc01.c | 121 ++++++++++++++++++
3 files changed, 124 insertions(+)
create mode 100644 testcases/kernel/syscalls/ioctl/ioctl_rtc01.c
diff --git a/runtest/syscalls b/runtest/syscalls
index 44254d7da..c6b8a85ad 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -539,6 +539,8 @@ ioctl_ns07 ioctl_ns07
ioctl_sg01 ioctl_sg01
+ioctl_rtc01 ioctl_rtc01
+
inotify_init1_01 inotify_init1_01
inotify_init1_02 inotify_init1_02
diff --git a/testcases/kernel/syscalls/ioctl/.gitignore b/testcases/kernel/syscalls/ioctl/.gitignore
index 97fbb9681..b297407bd 100644
--- a/testcases/kernel/syscalls/ioctl/.gitignore
+++ b/testcases/kernel/syscalls/ioctl/.gitignore
@@ -14,3 +14,4 @@
/ioctl_ns06
/ioctl_ns07
/ioctl_sg01
+/ioctl_rtc01
diff --git a/testcases/kernel/syscalls/ioctl/ioctl_rtc01.c b/testcases/kernel/syscalls/ioctl/ioctl_rtc01.c
new file mode 100644
index 000000000..e097f8f65
--- /dev/null
+++ b/testcases/kernel/syscalls/ioctl/ioctl_rtc01.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Filip Bozuta Filip.Bozuta@rt-rk.com
+ */
+
+/*
+ * Test RTC ioctls with RTC_RD_TIME and RTC_SET_TIME requests
+ *
+ * Reads the current time from RTC device using RTC_RD_TIME
+ * request and displays the time information as follows:
+ * hour:minute, month day.month.year
+ *
+ * Sets a new time in RTC device using RTC_SET_TIME request
+ * and displays the new time information as follws:
+ * hour:minute, month day.month.year
+ *
+ * Reads the new time from RTC device using RTC_RD_TIME
+ * request and checks whether the read time information
+ * is same as the one set by RTC_SET_TIME
+
+ * Runs RTC_SET_TIME to set back the current time read by
+ * RTC_RD_TIME at the beginning of the test
+ */
+
+#include <stdint.h>
+#include <errno.h>
+#include <linux/rtc.h>
+#include "tst_test.h"
+
+static void setup(void)
+{
+ int exists = access("/dev/rtc", O_RDONLY);
+
+ if (exists < 0)
+ tst_brk(TCONF, "RTC device driver file not found");
+}
+
+char *read_time_request = "RTC_RD_TIME";
+char *set_time_request = "RTC_SET_TIME";
+
+static void run(void)
+{
+ int fd;
+
+ struct rtc_time rtc_read_time;
+ struct rtc_time rtc_cur_time;
+ struct rtc_time rtc_set_time = {
+ .tm_sec = 0, .tm_min = 15, .tm_hour = 13,
+ .tm_mday = 26, .tm_mon = 8, .tm_year = 119};
+
+ int time_read_supported, time_set_supported = 0;
+
+ fd = SAFE_OPEN("/dev/rtc", O_RDONLY);
+
+ if (fd == -1)
+ tst_brk(TCONF, "RTC device driver file could not be opened");
+
+ if (ioctl(fd, RTC_RD_TIME, &rtc_cur_time) == -1) {
+ if (errno == ENOTTY)
+ tst_res(TCONF, "ioctl %s not supported on RTC device",
+ read_time_request);
+ else
+ tst_res(TFAIL | TERRNO, "unexpected ioctl error");
+ } else {
+ tst_res(TPASS, "time successfully read from RTC device");
+ tst_res(TINFO, "current RTC time: %d:%d, %d.%d.%d",
+ rtc_cur_time.tm_hour, rtc_cur_time.tm_min,
+ rtc_cur_time.tm_mday, rtc_cur_time.tm_mon,
+ rtc_cur_time.tm_year);
+ time_read_supported = 1;
+ }
+
+ if (ioctl(fd, RTC_SET_TIME, &rtc_set_time) == -1) {
+ if (errno == ENOTTY)
+ tst_res(TCONF, "ioctl %s not supported on RTC device",
+ set_time_request);
+ else
+ tst_res(TFAIL | TERRNO, "unexpected ioctl error");
+ } else {
+ tst_res(TPASS, "time successfully set to RTC device");
+ tst_res(TINFO, "new RTC time: %d:%d, %d.%d.%d",
+ rtc_set_time.tm_hour, rtc_set_time.tm_min,
+ rtc_set_time.tm_mday, rtc_set_time.tm_mon,
+ rtc_set_time.tm_year);
+ time_set_supported = 1;
+ }
+
+ if (time_read_supported && time_set_supported) {
+ ioctl(fd, RTC_RD_TIME, &rtc_read_time);
+
+ char time_data[][10] = {"minute", "hour", "month",
+ "month day", "year"};
+ int read_time_data[] = {
+ rtc_read_time.tm_min, rtc_read_time.tm_hour,
+ rtc_read_time.tm_mday, rtc_read_time.tm_mon,
+ rtc_read_time.tm_year};
+ int set_time_data[] = {
+ rtc_set_time.tm_min, rtc_set_time.tm_hour,
+ rtc_set_time.tm_mday, rtc_set_time.tm_mon,
+ rtc_set_time.tm_year};
+ for (int i = 0; i < 5; i++)
+ if (read_time_data[i] == set_time_data[i])
+ tst_res(TPASS, "%s reads new %s as expected",
+ read_time_request, time_data[i]);
+ else
+ tst_res(TFAIL, "%s reads different %s than set",
+ read_time_request, time_data[i]);
+ }
+
+ if (time_set_supported)
+ ioctl(fd, RTC_SET_TIME, &rtc_cur_time);
+
+ SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+ .test_all = run,
+ .needs_root = 1,
+ .needs_device = 1,
+ .setup = setup,
+};
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [LTP] [PATCH v2 1/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to read and set RTC time
2020-04-23 15:06 ` [LTP] [PATCH v2 1/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to read and set RTC time Filip Bozuta
@ 2020-04-30 15:13 ` Cyril Hrubis
2020-04-30 20:28 ` Filip Bozuta
0 siblings, 1 reply; 10+ messages in thread
From: Cyril Hrubis @ 2020-04-30 15:13 UTC (permalink / raw)
To: ltp
Hi!
> This patch tests functionalities of following ioctls:
>
> RTC_RD_TIME - Getting RTC time
>
> Returns this RTC's time in the following structure:
>
> struct rtc_time {
> int tm_sec;
> int tm_min;
> int tm_hour;
> int tm_mday;
> int tm_mon;
> int tm_year;
> int tm_wday; /* unused */
> int tm_yday; /* unused */
> int tm_isdst; /* unused */
> };
>
> The fields in this structure have the same meaning and
> ranges as the tm structure described in gmtime man page.
> A pointer to this structure should be passed as the third
> ioctl argument.
>
> RTC_SET_TIME - Setting RTC time
>
> Sets this RTC's time to the time specified by the rtc_time
> structure pointed to by the third ioctl argument. To set the
> RTC's time the process must be privileged (i.e., have the
> CAP_SYS_TIME capability).
>
> Signed-off-by: Filip Bozuta <Filip.Bozuta@rt-rk.com>
> ---
> runtest/syscalls | 2 +
> testcases/kernel/syscalls/ioctl/.gitignore | 1 +
> testcases/kernel/syscalls/ioctl/ioctl_rtc01.c | 121 ++++++++++++++++++
> 3 files changed, 124 insertions(+)
> create mode 100644 testcases/kernel/syscalls/ioctl/ioctl_rtc01.c
>
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 44254d7da..c6b8a85ad 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -539,6 +539,8 @@ ioctl_ns07 ioctl_ns07
>
> ioctl_sg01 ioctl_sg01
>
> +ioctl_rtc01 ioctl_rtc01
> +
> inotify_init1_01 inotify_init1_01
> inotify_init1_02 inotify_init1_02
>
> diff --git a/testcases/kernel/syscalls/ioctl/.gitignore b/testcases/kernel/syscalls/ioctl/.gitignore
> index 97fbb9681..b297407bd 100644
> --- a/testcases/kernel/syscalls/ioctl/.gitignore
> +++ b/testcases/kernel/syscalls/ioctl/.gitignore
> @@ -14,3 +14,4 @@
> /ioctl_ns06
> /ioctl_ns07
> /ioctl_sg01
> +/ioctl_rtc01
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_rtc01.c b/testcases/kernel/syscalls/ioctl/ioctl_rtc01.c
> new file mode 100644
> index 000000000..e097f8f65
> --- /dev/null
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_rtc01.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2020 Filip Bozuta Filip.Bozuta@rt-rk.com
> + */
> +
> +/*
> + * Test RTC ioctls with RTC_RD_TIME and RTC_SET_TIME requests
> + *
> + * Reads the current time from RTC device using RTC_RD_TIME
> + * request and displays the time information as follows:
> + * hour:minute, month day.month.year
> + *
> + * Sets a new time in RTC device using RTC_SET_TIME request
> + * and displays the new time information as follws:
> + * hour:minute, month day.month.year
> + *
> + * Reads the new time from RTC device using RTC_RD_TIME
> + * request and checks whether the read time information
> + * is same as the one set by RTC_SET_TIME
> +
> + * Runs RTC_SET_TIME to set back the current time read by
> + * RTC_RD_TIME at the beginning of the test
> + */
> +
> +#include <stdint.h>
> +#include <errno.h>
> +#include <linux/rtc.h>
> +#include "tst_test.h"
> +
> +static void setup(void)
> +{
> + int exists = access("/dev/rtc", O_RDONLY);
> +
> + if (exists < 0)
> + tst_brk(TCONF, "RTC device driver file not found");
The old LTP test allowed an RTC device to be passed to the test, I
wonder what should we do here. Should we loop over all rtcX devices under
/dev/ by default?
Also the same interface seems to be exported via proc and sysfs, it may make
sense to check that /sys/class/rtc/rtcX/time and date matches the
results from ioctl commands. We do have SAFE_FILE_SCANF() so reading
these should be simple enough.
> +}
> +
> +char *read_time_request = "RTC_RD_TIME";
> +char *set_time_request = "RTC_SET_TIME";
> +
> +static void run(void)
> +{
> + int fd;
> +
> + struct rtc_time rtc_read_time;
> + struct rtc_time rtc_cur_time;
> + struct rtc_time rtc_set_time = {
> + .tm_sec = 0, .tm_min = 15, .tm_hour = 13,
> + .tm_mday = 26, .tm_mon = 8, .tm_year = 119};
> +
> + int time_read_supported, time_set_supported = 0;
> +
> + fd = SAFE_OPEN("/dev/rtc", O_RDONLY);
> +
> + if (fd == -1)
> + tst_brk(TCONF, "RTC device driver file could not be opened");
You are using SAFE_OPEN() the return value will never be -1, SAFE_OPEN()
either succeeds or exits the test.
You have to use raw open() call if you want to handle the errors
yourself. Also you should really check for errno here, I guess that we
should TCONF only or ENOENT?
And lastly but not least we should open the device once in the test
setup and close it once in test cleanup, there is no point in opening it
for each test iteration (the test -i parameter).
> + if (ioctl(fd, RTC_RD_TIME, &rtc_cur_time) == -1) {
Just for the sake of being pedantic we should test that the retuned
value here is exactly the one that describes success, so we should do:
ioctl(fd, RTC_RD_TIME, &foo) != 0
There have been cases where wrongly applied patch caused random values
to be returned from a syscall which would not be caught here.
> + if (errno == ENOTTY)
> + tst_res(TCONF, "ioctl %s not supported on RTC device",
> + read_time_request);
> + else
> + tst_res(TFAIL | TERRNO, "unexpected ioctl error");
> + } else {
> + tst_res(TPASS, "time successfully read from RTC device");
> + tst_res(TINFO, "current RTC time: %d:%d, %d.%d.%d",
> + rtc_cur_time.tm_hour, rtc_cur_time.tm_min,
> + rtc_cur_time.tm_mday, rtc_cur_time.tm_mon,
> + rtc_cur_time.tm_year);
> + time_read_supported = 1;
> + }
> +
> + if (ioctl(fd, RTC_SET_TIME, &rtc_set_time) == -1) {
> + if (errno == ENOTTY)
> + tst_res(TCONF, "ioctl %s not supported on RTC device",
> + set_time_request);
Huh, why do we pass static string as %s? Why can't we just simply put it
verbatim into the string?
> + else
> + tst_res(TFAIL | TERRNO, "unexpected ioctl error");
> + } else {
> + tst_res(TPASS, "time successfully set to RTC device");
> + tst_res(TINFO, "new RTC time: %d:%d, %d.%d.%d",
> + rtc_set_time.tm_hour, rtc_set_time.tm_min,
> + rtc_set_time.tm_mday, rtc_set_time.tm_mon,
> + rtc_set_time.tm_year);
> + time_set_supported = 1;
> + }
> +
> + if (time_read_supported && time_set_supported) {
> + ioctl(fd, RTC_RD_TIME, &rtc_read_time);
> +
> + char time_data[][10] = {"minute", "hour", "month",
> + "month day", "year"};
This can be just const char *time_data[] = {...};
> + int read_time_data[] = {
> + rtc_read_time.tm_min, rtc_read_time.tm_hour,
> + rtc_read_time.tm_mday, rtc_read_time.tm_mon,
> + rtc_read_time.tm_year};
> + int set_time_data[] = {
> + rtc_set_time.tm_min, rtc_set_time.tm_hour,
> + rtc_set_time.tm_mday, rtc_set_time.tm_mon,
> + rtc_set_time.tm_year};
> + for (int i = 0; i < 5; i++)
> + if (read_time_data[i] == set_time_data[i])
> + tst_res(TPASS, "%s reads new %s as expected",
> + read_time_request, time_data[i]);
> + else
> + tst_res(TFAIL, "%s reads different %s than set",
> + read_time_request, time_data[i]);
Here as well, why do we pass static string as a parameter?
> + }
> +
> + if (time_set_supported)
> + ioctl(fd, RTC_SET_TIME, &rtc_cur_time);
If we are allowed to set the time but not read it we will not restore
the value at the end. Also does having write-only RTC even make sense?
Shouldn't we just drop the time_read_supported flag and exit the test if
we can't read it?
> + SAFE_CLOSE(fd);
> +}
> +
> +static struct tst_test test = {
> + .test_all = run,
> + .needs_root = 1,
> + .needs_device = 1,
> + .setup = setup,
> +};
Lastly there are some minor coding style violations, LTP uses LKML
coding style, and there is a script packed along with the Linux kernel
that can check your patches before sending them, see
linux/scripts/checkpatch.pl.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 10+ messages in thread* [LTP] [PATCH v2 1/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to read and set RTC time
2020-04-30 15:13 ` Cyril Hrubis
@ 2020-04-30 20:28 ` Filip Bozuta
0 siblings, 0 replies; 10+ messages in thread
From: Filip Bozuta @ 2020-04-30 20:28 UTC (permalink / raw)
To: ltp
On Thu, Apr 30, 2020 at 5:13 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > This patch tests functionalities of following ioctls:
> >
> > RTC_RD_TIME - Getting RTC time
> >
> > Returns this RTC's time in the following structure:
> >
> > struct rtc_time {
> > int tm_sec;
> > int tm_min;
> > int tm_hour;
> > int tm_mday;
> > int tm_mon;
> > int tm_year;
> > int tm_wday; /* unused */
> > int tm_yday; /* unused */
> > int tm_isdst; /* unused */
> > };
> >
> > The fields in this structure have the same meaning and
> > ranges as the tm structure described in gmtime man page.
> > A pointer to this structure should be passed as the third
> > ioctl argument.
> >
> > RTC_SET_TIME - Setting RTC time
> >
> > Sets this RTC's time to the time specified by the rtc_time
> > structure pointed to by the third ioctl argument. To set the
> > RTC's time the process must be privileged (i.e., have the
> > CAP_SYS_TIME capability).
> >
> > Signed-off-by: Filip Bozuta <Filip.Bozuta@rt-rk.com>
> > ---
> > runtest/syscalls | 2 +
> > testcases/kernel/syscalls/ioctl/.gitignore | 1 +
> > testcases/kernel/syscalls/ioctl/ioctl_rtc01.c | 121 ++++++++++++++++++
> > 3 files changed, 124 insertions(+)
> > create mode 100644 testcases/kernel/syscalls/ioctl/ioctl_rtc01.c
> >
> > diff --git a/runtest/syscalls b/runtest/syscalls
> > index 44254d7da..c6b8a85ad 100644
> > --- a/runtest/syscalls
> > +++ b/runtest/syscalls
> > @@ -539,6 +539,8 @@ ioctl_ns07 ioctl_ns07
> >
> > ioctl_sg01 ioctl_sg01
> >
> > +ioctl_rtc01 ioctl_rtc01
> > +
> > inotify_init1_01 inotify_init1_01
> > inotify_init1_02 inotify_init1_02
> >
> > diff --git a/testcases/kernel/syscalls/ioctl/.gitignore b/testcases/kernel/syscalls/ioctl/.gitignore
> > index 97fbb9681..b297407bd 100644
> > --- a/testcases/kernel/syscalls/ioctl/.gitignore
> > +++ b/testcases/kernel/syscalls/ioctl/.gitignore
> > @@ -14,3 +14,4 @@
> > /ioctl_ns06
> > /ioctl_ns07
> > /ioctl_sg01
> > +/ioctl_rtc01
> > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_rtc01.c b/testcases/kernel/syscalls/ioctl/ioctl_rtc01.c
> > new file mode 100644
> > index 000000000..e097f8f65
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/ioctl/ioctl_rtc01.c
> > @@ -0,0 +1,121 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2020 Filip Bozuta Filip.Bozuta@rt-rk.com
> > + */
> > +
> > +/*
> > + * Test RTC ioctls with RTC_RD_TIME and RTC_SET_TIME requests
> > + *
> > + * Reads the current time from RTC device using RTC_RD_TIME
> > + * request and displays the time information as follows:
> > + * hour:minute, month day.month.year
> > + *
> > + * Sets a new time in RTC device using RTC_SET_TIME request
> > + * and displays the new time information as follws:
> > + * hour:minute, month day.month.year
> > + *
> > + * Reads the new time from RTC device using RTC_RD_TIME
> > + * request and checks whether the read time information
> > + * is same as the one set by RTC_SET_TIME
> > +
> > + * Runs RTC_SET_TIME to set back the current time read by
> > + * RTC_RD_TIME at the beginning of the test
> > + */
> > +
> > +#include <stdint.h>
> > +#include <errno.h>
> > +#include <linux/rtc.h>
> > +#include "tst_test.h"
> > +
> > +static void setup(void)
> > +{
> > + int exists = access("/dev/rtc", O_RDONLY);
> > +
> > + if (exists < 0)
> > + tst_brk(TCONF, "RTC device driver file not found");
>
> The old LTP test allowed an RTC device to be passed to the test, I
> wonder what should we do here. Should we loop over all rtcX devices under
> /dev/ by default?
>
Thank you for your review!
I will see to it that I correct all of the mistakes you have pointed
out to me in the v3 series of these patches.
I agree with this. I shall create a mechanism to run the test for all
/dev/rtcX devices that are available on the system.
> Also the same interface seems to be exported via proc and sysfs, it may make
> sense to check that /sys/class/rtc/rtcX/time and date matches the
> results from ioctl commands. We do have SAFE_FILE_SCANF() so reading
> these should be simple enough.
>
Yes you are right, this would be a good addition to the test. Should I
read the time using both the ioctl command and SAFE_FILE_SCANF() or
should I use just one of these two?
> > +}
> > +
> > +char *read_time_request = "RTC_RD_TIME";
> > +char *set_time_request = "RTC_SET_TIME";
> > +
> > +static void run(void)
> > +{
> > + int fd;
> > +
> > + struct rtc_time rtc_read_time;
> > + struct rtc_time rtc_cur_time;
> > + struct rtc_time rtc_set_time = {
> > + .tm_sec = 0, .tm_min = 15, .tm_hour = 13,
> > + .tm_mday = 26, .tm_mon = 8, .tm_year = 119};
> > +
> > + int time_read_supported, time_set_supported = 0;
> > +
> > + fd = SAFE_OPEN("/dev/rtc", O_RDONLY);
> > +
> > + if (fd == -1)
> > + tst_brk(TCONF, "RTC device driver file could not be opened");
>
> You are using SAFE_OPEN() the return value will never be -1, SAFE_OPEN()
> either succeeds or exits the test.
>
> You have to use raw open() call if you want to handle the errors
> yourself. Also you should really check for errno here, I guess that we
> should TCONF only or ENOENT?
>
> And lastly but not least we should open the device once in the test
> setup and close it once in test cleanup, there is no point in opening it
> for each test iteration (the test -i parameter).
>
Thank you for pointing this out to me. I will consider both options:
using SAFE_OPEN() and remove the error check or use raw open() with
appropriate error handling. In any case, I will move this opening of
the device file in the setup() function
and create a cleanup() function where the file will be closed.
> > + if (ioctl(fd, RTC_RD_TIME, &rtc_cur_time) == -1) {
>
> Just for the sake of being pedantic we should test that the retuned
> value here is exactly the one that describes success, so we should do:
>
> ioctl(fd, RTC_RD_TIME, &foo) != 0
>
> There have been cases where wrongly applied patch caused random values
> to be returned from a syscall which would not be caught here.
>
I will change this in every part of the test where the ioctl() call is checked.
> > + if (errno == ENOTTY)
> > + tst_res(TCONF, "ioctl %s not supported on RTC device",
> > + read_time_request);
> > + else
> > + tst_res(TFAIL | TERRNO, "unexpected ioctl error");
> > + } else {
> > + tst_res(TPASS, "time successfully read from RTC device");
> > + tst_res(TINFO, "current RTC time: %d:%d, %d.%d.%d",
> > + rtc_cur_time.tm_hour, rtc_cur_time.tm_min,
> > + rtc_cur_time.tm_mday, rtc_cur_time.tm_mon,
> > + rtc_cur_time.tm_year);
> > + time_read_supported = 1;
> > + }
> > +
> > + if (ioctl(fd, RTC_SET_TIME, &rtc_set_time) == -1) {
> > + if (errno == ENOTTY)
> > + tst_res(TCONF, "ioctl %s not supported on RTC device",
> > + set_time_request);
>
> Huh, why do we pass static string as %s? Why can't we just simply put it
> verbatim into the string?
>
> > + else
> > + tst_res(TFAIL | TERRNO, "unexpected ioctl error");
> > + } else {
> > + tst_res(TPASS, "time successfully set to RTC device");
> > + tst_res(TINFO, "new RTC time: %d:%d, %d.%d.%d",
> > + rtc_set_time.tm_hour, rtc_set_time.tm_min,
> > + rtc_set_time.tm_mday, rtc_set_time.tm_mon,
> > + rtc_set_time.tm_year);
> > + time_set_supported = 1;
> > + }
> > +
> > + if (time_read_supported && time_set_supported) {
> > + ioctl(fd, RTC_RD_TIME, &rtc_read_time);
> > +
> > + char time_data[][10] = {"minute", "hour", "month",
> > + "month day", "year"};
>
> This can be just const char *time_data[] = {...};
>
> > + int read_time_data[] = {
> > + rtc_read_time.tm_min, rtc_read_time.tm_hour,
> > + rtc_read_time.tm_mday, rtc_read_time.tm_mon,
> > + rtc_read_time.tm_year};
> > + int set_time_data[] = {
> > + rtc_set_time.tm_min, rtc_set_time.tm_hour,
> > + rtc_set_time.tm_mday, rtc_set_time.tm_mon,
> > + rtc_set_time.tm_year};
> > + for (int i = 0; i < 5; i++)
> > + if (read_time_data[i] == set_time_data[i])
> > + tst_res(TPASS, "%s reads new %s as expected",
> > + read_time_request, time_data[i]);
> > + else
> > + tst_res(TFAIL, "%s reads different %s than set",
> > + read_time_request, time_data[i]);
>
> Here as well, why do we pass static string as a parameter?
>
I am sorry. I got confused when writting this part of the test (last
20 lines). I will
change this whole time comparing section in v3 and remove all static
strings.
> > + }
> > +
> > + if (time_set_supported)
> > + ioctl(fd, RTC_SET_TIME, &rtc_cur_time);
>
> If we are allowed to set the time but not read it we will not restore
> the value at the end. Also does having write-only RTC even make sense?
> Shouldn't we just drop the time_read_supported flag and exit the test if
> we can't read it?
>
I will drop both time_read_supported and time_set_supported flags as
they are unnecessary. Instead, I shall just use tst_brk() to exit the
test in the beginning part if RTC_RD_TIME or RTC_SET_TIME request
fails (as there is no need to move forward with the test if any of
these requests fail at the beginning).
> > + SAFE_CLOSE(fd);
> > +}
> > +
> > +static struct tst_test test = {
> > + .test_all = run,
> > + .needs_root = 1,
> > + .needs_device = 1,
> > + .setup = setup,
> > +};
>
> Lastly there are some minor coding style violations, LTP uses LKML
> coding style, and there is a script packed along with the Linux kernel
> that can check your patches before sending them, see
> linux/scripts/checkpatch.pl.
>
I will check all future patches with checkpatch.pl script from the Linux kernel.
I would just like to inform you that I am going on a short holiday in
the next three days after which I will have some other tasks that I
have to attend to. For that reason, I won't be able to complete the v3
series with the corrected flaws of this patch in the near future.
However, I am looking forward to getting back at work on these tests
and correcting all of the mistakes that you have pointed out to me as
soon as possible. I am also looking forward to your reviews of my
future patches.
Thanks again for all of your comments!
Best regards,
Filip
> --
> Cyril Hrubis
> chrubis@suse.cz
^ permalink raw reply [flat|nested] 10+ messages in thread
* [LTP] [PATCH v2 2/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to read and set RTC alarm time
2020-04-23 15:06 [LTP] [PATCH v2 0/3] Add tests for a group of real time clock ioctls Filip Bozuta
2020-04-23 15:06 ` [LTP] [PATCH v2 1/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to read and set RTC time Filip Bozuta
@ 2020-04-23 15:06 ` Filip Bozuta
2020-04-27 17:36 ` Petr Vorel
2020-04-23 15:06 ` [LTP] [PATCH v2 3/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to turn on/off RTC interrupts Filip Bozuta
2020-04-24 14:58 ` [LTP] [PATCH v2 0/3] Add tests for a group of real time clock ioctls Cyril Hrubis
3 siblings, 1 reply; 10+ messages in thread
From: Filip Bozuta @ 2020-04-23 15:06 UTC (permalink / raw)
To: ltp
From: Filip Bozuta <Filip.Bozuta@rt-rk.com>
This patch tests functionalities of following ioctls:
RTC_ALM_READ, RTC_ALM_SET - Getting/Setting alarm time
Read and set the alarm time, for RTCs that support alarms.
The alarm interrupt must be separately enabled or disabled
using the RTC_AIE_ON, RTC_AIE_OFF requests. The third ioctl's
argument is a pointer to a rtc_time structure. Only the tm_sec,
tm_min, and tm_hour fields of this structure are used.
Signed-off-by: Filip Bozuta <Filip.Bozuta@rt-rk.com>
---
runtest/syscalls | 1 +
testcases/kernel/syscalls/ioctl/.gitignore | 1 +
testcases/kernel/syscalls/ioctl/ioctl_rtc02.c | 115 ++++++++++++++++++
3 files changed, 117 insertions(+)
create mode 100644 testcases/kernel/syscalls/ioctl/ioctl_rtc02.c
diff --git a/runtest/syscalls b/runtest/syscalls
index c6b8a85ad..0e358337f 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -540,6 +540,7 @@ ioctl_ns07 ioctl_ns07
ioctl_sg01 ioctl_sg01
ioctl_rtc01 ioctl_rtc01
+ioctl_rtc02 ioctl_rtc02
inotify_init1_01 inotify_init1_01
inotify_init1_02 inotify_init1_02
diff --git a/testcases/kernel/syscalls/ioctl/.gitignore b/testcases/kernel/syscalls/ioctl/.gitignore
index b297407bd..b9ed19724 100644
--- a/testcases/kernel/syscalls/ioctl/.gitignore
+++ b/testcases/kernel/syscalls/ioctl/.gitignore
@@ -15,3 +15,4 @@
/ioctl_ns07
/ioctl_sg01
/ioctl_rtc01
+/ioctl_rtc02
diff --git a/testcases/kernel/syscalls/ioctl/ioctl_rtc02.c b/testcases/kernel/syscalls/ioctl/ioctl_rtc02.c
new file mode 100644
index 000000000..6a750a02a
--- /dev/null
+++ b/testcases/kernel/syscalls/ioctl/ioctl_rtc02.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Filip Bozuta Filip.Bozuta@rt-rk.com
+ */
+
+/*
+ * Test RTC ioctls with RTC_ALM_READ and RTC_ALM_SET requests
+ *
+ * Reads the current alarm time from RTC device using
+ * RTC_ALM_READ request and displays the alarm time
+ * information as follows: hour:minute:second
+ *
+ * Sets a new alarm time in RTC device using RTC_ALM_SET
+ * request and displays the new alarm time information
+ * as follows: hour:minute:second
+ *
+ * Reads the new alarm time from RTC device using RTC_ALM_READ
+ * request and checks whether the read alarm time information
+ * is same as the one set by RTC_ALM_SET
+ *
+ * Runs RTC_ALM_SET to set back the current alarm time read by
+ * RTC_ALM_READ at the beginning of the test
+ */
+
+#include <stdint.h>
+#include <errno.h>
+#include <linux/rtc.h>
+#include "tst_test.h"
+
+static void setup(void)
+{
+ int exists = ("/dev/rtc", O_RDONLY);
+
+ if (exists < 0)
+ tst_brk(TCONF, "RTC device driver file not available");
+}
+
+char *read_alarm_request = "RTC_ALM_READ";
+char *set_alarm_request = "RTC_ALM_SET";
+
+static void run(void)
+{
+ int fd;
+
+ struct rtc_time rtc_read_alarm;
+ struct rtc_time rtc_cur_alarm;
+ struct rtc_time rtc_set_alarm = {
+ .tm_sec = 13, .tm_min = 35, .tm_hour = 12};
+
+ int alarm_read_supported, alarm_set_supported = 0;
+
+ fd = SAFE_OPEN("/dev/rtc", O_RDONLY);
+
+ if (fd == -1)
+ tst_brk(TCONF, "RTC device driver file could not be opened");
+
+ if (ioctl(fd, RTC_ALM_READ, &rtc_cur_alarm) == -1) {
+ if (errno == ENOTTY)
+ tst_res(TCONF, "ioctl %s not supported on RTC device",
+ read_alarm_request);
+ else
+ tst_res(TFAIL | TERRNO, "unexpected ioctl error");
+ } else {
+ tst_res(TPASS, "alarm time successfully read from RTC device");
+ tst_res(TINFO, "current RTC alarm time: %d:%d:%d",
+ rtc_cur_alarm.tm_hour, rtc_cur_alarm.tm_min,
+ rtc_cur_alarm.tm_sec);
+ alarm_read_supported = 1;
+ }
+
+ if (ioctl(fd, RTC_ALM_SET, &rtc_set_alarm) == -1) {
+ if (errno == ENOTTY)
+ tst_res(TCONF, "ioctl %s not supported on RTC device",
+ set_alarm_request);
+ else
+ tst_res(TFAIL | TERRNO, "unexpected ioctl error");
+ } else {
+ tst_res(TPASS, "alarm time successfully set to RTC device");
+ tst_res(TINFO, "new RTC alarm time: %d:%d:%d",
+ rtc_set_alarm.tm_hour, rtc_set_alarm.tm_min,
+ rtc_set_alarm.tm_sec);
+ alarm_set_supported = 1;
+ }
+
+ if (alarm_read_supported && alarm_set_supported) {
+ ioctl(fd, RTC_ALM_READ, &rtc_read_alarm);
+
+ char alarm_data[][10] = {"second", "minute", "hour"};
+ int read_alarm_data[] = {
+ rtc_read_alarm.tm_sec, rtc_read_alarm.tm_min,
+ rtc_read_alarm.tm_hour};
+ int set_alarm_data[] = {
+ rtc_set_alarm.tm_sec, rtc_set_alarm.tm_min,
+ rtc_set_alarm.tm_hour};
+ for (int i = 0; i < 3; i++)
+ if (read_alarm_data[i] == set_alarm_data[i])
+ tst_res(TPASS, "%s reads new %s as expected",
+ read_alarm_request, alarm_data[i]);
+ else
+ tst_res(TPASS, "%s reads different %s than set",
+ read_alarm_request, alarm_data[i]);
+ }
+
+ if (alarm_set_supported)
+ ioctl(fd, RTC_ALM_SET, &rtc_cur_alarm);
+
+ SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+ .test_all = run,
+ .needs_root = 1,
+ .needs_device = 1,
+ .setup = setup,
+};
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [LTP] [PATCH v2 3/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to turn on/off RTC interrupts
2020-04-23 15:06 [LTP] [PATCH v2 0/3] Add tests for a group of real time clock ioctls Filip Bozuta
2020-04-23 15:06 ` [LTP] [PATCH v2 1/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to read and set RTC time Filip Bozuta
2020-04-23 15:06 ` [LTP] [PATCH v2 2/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to read and set RTC alarm time Filip Bozuta
@ 2020-04-23 15:06 ` Filip Bozuta
2020-04-30 15:18 ` Cyril Hrubis
2020-04-24 14:58 ` [LTP] [PATCH v2 0/3] Add tests for a group of real time clock ioctls Cyril Hrubis
3 siblings, 1 reply; 10+ messages in thread
From: Filip Bozuta @ 2020-04-23 15:06 UTC (permalink / raw)
To: ltp
From: Filip Bozuta <Filip.Bozuta@rt-rk.com>
This patch tests functionalities of following ioctls:
RTC_AIE_ON, RTC_AIE_OFF - Alarm interrupt enabling on/off
Enable or disable the alarm interrupt, for RTCs that support
alarms. The third ioctl's argument is ignored.
RTC_UIE_ON, RTC_UIE_OFF - Update interrupt enabling on/off
Enable or disable the interrupt on every clock update, for
RTCs that support this once-per-second interrupt. The third
ioctl's argument is ignored.
RTC_PIE_ON, RTC_PIE_OFF - Periodic interrupt enabling on/off
Enable or disable the periodic interrupt, for RTCs that sup?
port these periodic interrupts. The third ioctl's argument
is ignored. Only a privileged process (i.e., one having the
CAP_SYS_RESOURCE capability) can enable the periodic interrupt
if the frequency is currently set above the value specified in
/proc/sys/dev/rtc/max-user-freq.
RTC_WIE_ON, RTC_WIE_OFF - Watchdog interrupt enabling on/off
Enable or disable the Watchdog interrupt, for RTCs that sup-
port this Watchdog interrupt. The third ioctl's argument is
ignored.
Signed-off-by: Filip Bozuta <Filip.Bozuta@rt-rk.com>
---
runtest/syscalls | 1 +
testcases/kernel/syscalls/ioctl/.gitignore | 1 +
testcases/kernel/syscalls/ioctl/ioctl_rtc03.c | 86 +++++++++++++++++++
3 files changed, 88 insertions(+)
create mode 100644 testcases/kernel/syscalls/ioctl/ioctl_rtc03.c
diff --git a/runtest/syscalls b/runtest/syscalls
index 0e358337f..d5b9789d3 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -541,6 +541,7 @@ ioctl_sg01 ioctl_sg01
ioctl_rtc01 ioctl_rtc01
ioctl_rtc02 ioctl_rtc02
+ioctl_rtc03 ioctl_rtc03
inotify_init1_01 inotify_init1_01
inotify_init1_02 inotify_init1_02
diff --git a/testcases/kernel/syscalls/ioctl/.gitignore b/testcases/kernel/syscalls/ioctl/.gitignore
index b9ed19724..f16c11f58 100644
--- a/testcases/kernel/syscalls/ioctl/.gitignore
+++ b/testcases/kernel/syscalls/ioctl/.gitignore
@@ -16,3 +16,4 @@
/ioctl_sg01
/ioctl_rtc01
/ioctl_rtc02
+/ioctl_rtc03
diff --git a/testcases/kernel/syscalls/ioctl/ioctl_rtc03.c b/testcases/kernel/syscalls/ioctl/ioctl_rtc03.c
new file mode 100644
index 000000000..d5c946629
--- /dev/null
+++ b/testcases/kernel/syscalls/ioctl/ioctl_rtc03.c
@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Filip Bozuta Filip.Bozuta@rt-rk.com
+ */
+
+/*
+ * Test RTC ioctls with requests RTC_AIE_ON, RTC_AIE_OFF,
+ * RTC_PIE_ON, RTC_PIE_OFF, RTC_UIE_ON, RTC_UIE_OFF,
+ * RTC_WIE_ON, RTC_WIE_OFF
+ *
+ * Runs ioctls with the above mentioned requests one by one
+ * and sequentially turns on and off RTC alarm, periodic,
+ * update and watchdog interrupt.
+ */
+
+#include <stdint.h>
+#include <errno.h>
+#include <string.h>
+#include <linux/rtc.h>
+#include "tst_test.h"
+
+static void setup(void)
+{
+ int exists = ("/dev/rtc", O_RDONLY);
+
+ if (exists < 0)
+ tst_brk(TCONF, "RTC device driver file not available");
+}
+
+static char interrupts[][10] = {"alarm", "periodic", "update", "watchdog"};
+static int interrupt_requests[] = {
+ RTC_AIE_ON, RTC_PIE_ON, RTC_UIE_ON, RTC_WIE_ON,
+ RTC_AIE_OFF, RTC_PIE_OFF, RTC_UIE_OFF, RTC_WIE_OFF};
+static char requests_text[][15] = {
+ "RTC_AIE_ON", "RTC_PIE_ON", "RTC_UIE_ON", "RTC_WIE_ON",
+ "RTC_AIE_OFF", "RTC_PIE_OFF", "RTC_UIE_OFF", "RTC_WIE_OFF"};
+
+static void test_request(unsigned int n)
+{
+ int fd;
+
+ int on_request = interrupt_requests[n];
+ int off_request = interrupt_requests[n + 4];
+
+ char on_request_text[15], off_request_text[15];
+
+ strcpy(on_request_text, requests_text[n]);
+ strcpy(off_request_text, requests_text[n + 4]);
+
+ fd = SAFE_OPEN("/dev/rtc", O_RDWR);
+
+ if (fd == -1)
+ tst_brk(TCONF, "RTC device driver file could not be opened");
+
+ if (ioctl(fd, on_request) == -1) {
+ if (errno == ENOTTY) {
+ tst_res(TCONF, "ioctl %s not supported on RTC device",
+ on_request_text);
+ } else {
+ tst_res(TFAIL, "unexpected ioctl error");
+ }
+ } else {
+ tst_res(TPASS, "%s interrupt enabled", interrupts[n]);
+ }
+
+ if (ioctl(fd, off_request) == -1) {
+ if (errno == ENOTTY) {
+ tst_res(TCONF, "ioctl %s not supported on RTC device",
+ off_request_text);
+ } else {
+ tst_res(TFAIL, "unexpected ioctl error");
+ }
+ } else {
+ tst_res(TPASS, "%s interrupt disabled", interrupts[n]);
+ }
+
+ SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+ .tcnt = ARRAY_SIZE(interrupts),
+ .test = test_request,
+ .needs_root = 1,
+ .needs_device = 1,
+ .setup = setup,
+};
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [LTP] [PATCH v2 0/3] Add tests for a group of real time clock ioctls
2020-04-23 15:06 [LTP] [PATCH v2 0/3] Add tests for a group of real time clock ioctls Filip Bozuta
` (2 preceding siblings ...)
2020-04-23 15:06 ` [LTP] [PATCH v2 3/3] testcases/kernel/syscalls/ioctl: Add test for RTC ioctls used to turn on/off RTC interrupts Filip Bozuta
@ 2020-04-24 14:58 ` Cyril Hrubis
2020-04-24 19:54 ` Filip Bozuta
3 siblings, 1 reply; 10+ messages in thread
From: Cyril Hrubis @ 2020-04-24 14:58 UTC (permalink / raw)
To: ltp
Hi!
Looking at these I guess that we should also delete the
testcases/kernel/device-drivers/rtc/ as a last patch in this series
since these tests seems to cover a bit more than the original test.
I will have a closer look at the patchset next week.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 10+ messages in thread* [LTP] [PATCH v2 0/3] Add tests for a group of real time clock ioctls
2020-04-24 14:58 ` [LTP] [PATCH v2 0/3] Add tests for a group of real time clock ioctls Cyril Hrubis
@ 2020-04-24 19:54 ` Filip Bozuta
0 siblings, 0 replies; 10+ messages in thread
From: Filip Bozuta @ 2020-04-24 19:54 UTC (permalink / raw)
To: ltp
Hello Cyrill,
Thank you for your response!
I didn't take a look at the file
testcases/kernel/device-drivers/rtc/rtc01.c when writing rtc ioctl tests.
I see that it runs tests for certain ioctls that are covered in the patches
I sent (i.e. RTC_RD_TIME, RTC_SET_TIME, RTC_AIE_ON,...).
As I see, this file runs tests for certain ioctls more thoroughly than my
test since it is meant to test the functionality of the rtc driver. My
tests are only meant to test the functionalities of rtc ioctls without
testing whether the driver works correctly. For instance, when running
tests for ioctls used for interrupts (RTC_AIE_ON, RTC_AIE_OFF,
RTC_PIE_ON,...), I only check whether these ioctls work to turn on/off rtc
interrupts (alarm, periodic, update, watchdog). I don't check whether the
interrupts work on the driver correctly.
Anyway, I am new to LTP so I am not that familiar with the scope of
functionalities ioctl tests should cover. Any feedback you can give me on
this would be greatly appreciated.
I am looking forward to your review of my patches.
Best regards,
Filip
On Fri, Apr 24, 2020 at 4:58 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> Hi!
> Looking at these I guess that we should also delete the
> testcases/kernel/device-drivers/rtc/ as a last patch in this series
> since these tests seems to cover a bit more than the original test.
>
> I will have a closer look at the patchset next week.
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200424/2a05c03b/attachment.htm>
^ permalink raw reply [flat|nested] 10+ messages in thread