diff mbox series

selftests/rtc: continuously read RTC in a loop for 30s

Message ID 20220219072713.5280-1-mat.jonczyk@o2.pl (mailing list archive)
State Accepted
Commit 2aaa36e95ea586ad23edfcc1d474e8b735a4d1c3
Headers show
Series selftests/rtc: continuously read RTC in a loop for 30s | expand

Commit Message

Mateusz Jończyk Feb. 19, 2022, 7:27 a.m. UTC
Some problems with reading the RTC time may happen rarely, for example
while the RTC is updating. So read the RTC many times to catch these
problems. For example, a previous attempt for my
commit ea6fa4961aab ("rtc: mc146818-lib: fix RTC presence check")
was incorrect and would have triggered this selftest.

To avoid the risk of damaging the hardware, wait 11ms before consecutive
reads.

In rtc_time_to_timestamp I copied values manually instead of casting -
just to be on the safe side. The 11ms wait period was chosen so that it is
not a divisor of 1000ms.

Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Shuah Khan <shuah@kernel.org>
---

Also, before
commit cdedc45c579f ("rtc: cmos: avoid UIP when reading alarm time")
reading the RTC alarm time during RTC update produced incorrect results
on many Intel platforms. Preparing a similar selftest for this case
would be more difficult, though, because the RTC alarm time is cached by
the kernel. Direct access would have to be exposed somehow, for example
in debugfs. I may prepare a patch for it in the future.
---
 tools/testing/selftests/rtc/rtctest.c | 66 +++++++++++++++++++++++++++
 tools/testing/selftests/rtc/settings  |  2 +-
 2 files changed, 67 insertions(+), 1 deletion(-)

Comments

Shuah Khan Feb. 25, 2022, 8:56 p.m. UTC | #1
On 2/19/22 12:27 AM, Mateusz Jończyk wrote:
> Some problems with reading the RTC time may happen rarely, for example
> while the RTC is updating. So read the RTC many times to catch these
> problems. For example, a previous attempt for my
> commit ea6fa4961aab ("rtc: mc146818-lib: fix RTC presence check")
> was incorrect and would have triggered this selftest.
> 
> To avoid the risk of damaging the hardware, wait 11ms before consecutive
> reads.
> 
> In rtc_time_to_timestamp I copied values manually instead of casting -
> just to be on the safe side. The 11ms wait period was chosen so that it is
> not a divisor of 1000ms.
> 
> Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Shuah Khan <shuah@kernel.org>
> ---
> 
> Also, before
> commit cdedc45c579f ("rtc: cmos: avoid UIP when reading alarm time")
> reading the RTC alarm time during RTC update produced incorrect results
> on many Intel platforms. Preparing a similar selftest for this case
> would be more difficult, though, because the RTC alarm time is cached by
> the kernel. Direct access would have to be exposed somehow, for example
> in debugfs. I may prepare a patch for it in the future.
> ---

Looks good to me. We end up tweaking the timeout=210 in settings every
now and then. Not sure how we can avoid adjusting it as we find problems.

I will apply this in for Linux 5.18-rc1

thanks,
-- Shuah
Shuah Khan Feb. 26, 2022, 12:02 a.m. UTC | #2
On 2/25/22 1:56 PM, Shuah Khan wrote:
> On 2/19/22 12:27 AM, Mateusz Jończyk wrote:
>> Some problems with reading the RTC time may happen rarely, for example
>> while the RTC is updating. So read the RTC many times to catch these
>> problems. For example, a previous attempt for my
>> commit ea6fa4961aab ("rtc: mc146818-lib: fix RTC presence check")
>> was incorrect and would have triggered this selftest.
>>
>> To avoid the risk of damaging the hardware, wait 11ms before consecutive
>> reads.
>>
>> In rtc_time_to_timestamp I copied values manually instead of casting -
>> just to be on the safe side. The 11ms wait period was chosen so that it is
>> not a divisor of 1000ms.
>>
>> Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
>> Cc: Alessandro Zummo <a.zummo@towertech.it>
>> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> Cc: Shuah Khan <shuah@kernel.org>
>> ---
>>
>> Also, before
>> commit cdedc45c579f ("rtc: cmos: avoid UIP when reading alarm time")
>> reading the RTC alarm time during RTC update produced incorrect results
>> on many Intel platforms. Preparing a similar selftest for this case
>> would be more difficult, though, because the RTC alarm time is cached by
>> the kernel. Direct access would have to be exposed somehow, for example
>> in debugfs. I may prepare a patch for it in the future.
>> ---
> 
> Looks good to me. We end up tweaking the timeout=210 in settings every
> now and then. Not sure how we can avoid adjusting it as we find problems.
> 
> I will apply this in for Linux 5.18-rc1
> 

Applied to linux-kselftest next for Linux 5.18-rc1

thanks,
-- Shuah
diff mbox series

Patch

diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c
index 66af608fb4c6..2b9d929a24ed 100644
--- a/tools/testing/selftests/rtc/rtctest.c
+++ b/tools/testing/selftests/rtc/rtctest.c
@@ -20,6 +20,8 @@ 
 
 #define NUM_UIE 3
 #define ALARM_DELTA 3
+#define READ_LOOP_DURATION_SEC 30
+#define READ_LOOP_SLEEP_MS 11
 
 static char *rtc_file = "/dev/rtc0";
 
@@ -49,6 +51,70 @@  TEST_F(rtc, date_read) {
 	       rtc_tm.tm_hour, rtc_tm.tm_min, rtc_tm.tm_sec);
 }
 
+static time_t rtc_time_to_timestamp(struct rtc_time *rtc_time)
+{
+	struct tm tm_time = {
+	       .tm_sec = rtc_time->tm_sec,
+	       .tm_min = rtc_time->tm_min,
+	       .tm_hour = rtc_time->tm_hour,
+	       .tm_mday = rtc_time->tm_mday,
+	       .tm_mon = rtc_time->tm_mon,
+	       .tm_year = rtc_time->tm_year,
+	};
+
+	return mktime(&tm_time);
+}
+
+static void nanosleep_with_retries(long ns)
+{
+	struct timespec req = {
+		.tv_sec = 0,
+		.tv_nsec = ns,
+	};
+	struct timespec rem;
+
+	while (nanosleep(&req, &rem) != 0) {
+		req.tv_sec = rem.tv_sec;
+		req.tv_nsec = rem.tv_nsec;
+	}
+}
+
+TEST_F_TIMEOUT(rtc, date_read_loop, READ_LOOP_DURATION_SEC + 2) {
+	int rc;
+	long iter_count = 0;
+	struct rtc_time rtc_tm;
+	time_t start_rtc_read, prev_rtc_read;
+
+	TH_LOG("Continuously reading RTC time for %ds (with %dms breaks after every read).",
+	       READ_LOOP_DURATION_SEC, READ_LOOP_SLEEP_MS);
+
+	rc = ioctl(self->fd, RTC_RD_TIME, &rtc_tm);
+	ASSERT_NE(-1, rc);
+	start_rtc_read = rtc_time_to_timestamp(&rtc_tm);
+	prev_rtc_read = start_rtc_read;
+
+	do  {
+		time_t rtc_read;
+
+		rc = ioctl(self->fd, RTC_RD_TIME, &rtc_tm);
+		ASSERT_NE(-1, rc);
+
+		rtc_read = rtc_time_to_timestamp(&rtc_tm);
+		/* Time should not go backwards */
+		ASSERT_LE(prev_rtc_read, rtc_read);
+		/* Time should not increase more then 1s at a time */
+		ASSERT_GE(prev_rtc_read + 1, rtc_read);
+
+		/* Sleep 11ms to avoid killing / overheating the RTC */
+		nanosleep_with_retries(READ_LOOP_SLEEP_MS * 1000000);
+
+		prev_rtc_read = rtc_read;
+		iter_count++;
+	} while (prev_rtc_read <= start_rtc_read + READ_LOOP_DURATION_SEC);
+
+	TH_LOG("Performed %ld RTC time reads.", iter_count);
+}
+
 TEST_F_TIMEOUT(rtc, uie_read, NUM_UIE + 2) {
 	int i, rc, irq = 0;
 	unsigned long data;
diff --git a/tools/testing/selftests/rtc/settings b/tools/testing/selftests/rtc/settings
index a953c96aa16e..0c1a2075d5f3 100644
--- a/tools/testing/selftests/rtc/settings
+++ b/tools/testing/selftests/rtc/settings
@@ -1 +1 @@ 
-timeout=180
+timeout=210