[LTP] [PATCH 1/2] rtc01: add space to beginning of comments

[LTP] [PATCH 1/2] rtc01: add space to beginning of comments

[Krzysztof Kozlowski]

Improve readability of comments - no functional change.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 testcases/kernel/device-drivers/rtc/rtc01.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/testcases/kernel/device-drivers/rtc/rtc01.c b/testcases/kernel/device-drivers/rtc/rtc01.c
index 8a1f62eadcec..ebd15306d583 100644
--- a/testcases/kernel/device-drivers/rtc/rtc01.c
+++ b/testcases/kernel/device-drivers/rtc/rtc01.c
@@ -67,7 +67,7 @@ void read_alarm_test(void)
 
 	tst_resm(TINFO, "RTC READ TEST:");
 
-	/*Read RTC Time */
+	/* Read RTC Time */
 	ret = ioctl(rtc_fd, RTC_RD_TIME, &rtc_tm);
 	if (ret == -1) {
 		tst_resm(TFAIL | TERRNO, "RTC_RD_TIME ioctl failed");
@@ -82,7 +82,7 @@ void read_alarm_test(void)
 
 	tst_resm(TINFO, "RTC ALARM TEST :");
 
-	/*set Alarm to 5 Seconds */
+	/* Set Alarm to 5 Seconds */
 	rtc_tm.tm_sec += 5;
 	if (rtc_tm.tm_sec >= 60) {
 		rtc_tm.tm_sec %= 60;
@@ -106,7 +106,7 @@ void read_alarm_test(void)
 		return;
 	}
 
-	/*Read current alarm time */
+	/* Read current alarm time */
 	ret = ioctl(rtc_fd, RTC_ALM_READ, &rtc_tm);
 	if (ret == -1) {
 		if (errno == EINVAL) {
@@ -129,13 +129,13 @@ void read_alarm_test(void)
 
 	tst_resm(TINFO, "Waiting 5 seconds for the alarm...");
 
-	tv.tv_sec = 6;		/*set 6 seconds as the time out */
+	tv.tv_sec = 6;		/* set 6 seconds as the time out */
 	tv.tv_usec = 0;
 
 	FD_ZERO(&rfds);
 	FD_SET(rtc_fd, &rfds);
 
-	ret = select(rtc_fd + 1, &rfds, NULL, NULL, &tv);	/*wait for alarm */
+	ret = select(rtc_fd + 1, &rfds, NULL, NULL, &tv);	/* wait for alarm */
 
 	if (ret == -1) {
 		tst_resm(TFAIL | TERRNO, "select failed");
@@ -174,7 +174,7 @@ void update_interrupts_test(void)
 	struct timeval tv;
 
 	tst_resm(TINFO, "RTC UPDATE INTERRUPTS TEST :");
-	/*Turn on update interrupts */
+	/* Turn on update interrupts */
 	ret = ioctl(rtc_fd, RTC_UIE_ON, 0);
 	if (ret == -1) {
 		if (errno == EINVAL)
@@ -187,7 +187,7 @@ void update_interrupts_test(void)
 	tst_resm(TINFO, "Waiting for  5 update interrupts...");
 	for (i = 1; i < 6; i++) {
 
-		tv.tv_sec = 2;	/*2 sec time out for each interrupt */
+		tv.tv_sec = 2;	/* 2 sec time out for each interrupt */
 		tv.tv_usec = 0;
 
 		FD_ZERO(&rfds);
@@ -231,10 +231,10 @@ int main(int argc, char *argv[])
 
 	rtc_fd = SAFE_OPEN(NULL, rtc_dev, O_RDONLY);
 
-	/*Read and alarm tests */
+	/* Read and alarm tests */
 	read_alarm_test();
 
-	/*Update interrupts test */
+	/* Update interrupts test */
 	update_interrupts_test();
 
 	close(rtc_fd);
-- 
2.30.2


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 2/2] rtc01: add workaround for broken CMOS RTC on Microsoft Hyper-V cloud

[Krzysztof Kozlowski]

rtc01 fails on missed alarm on multiple different Azure instances if the
alarm is set for the next minute:

  rtc01 0 TINFO : RTC READ TEST:
  rtc01 1 TPASS : RTC READ TEST Passed
  rtc01 0 TINFO : Current RTC date/time is 11-6-2021, 09:00:58.
  rtc01 0 TINFO : RTC ALARM TEST :
  rtc01 0 TINFO : Alarm time set to 09:01:03.
  rtc01 0 TINFO : Waiting 5 seconds for the alarm...
  rtc01 2 TFAIL : rtc01.c:151: Timed out waiting for the alarm

Reproduced easily with rtcwake:

  $ rtcwake -d rtc0 -m on -s 50 -v

If alarm is set for now+60 seconds, it works fine.  Clearly Microsoft
Hyper-V cloud instances have a broken CMOS RTC which unfortunately
cannot be easily fixed.  Adding simple workaround to extend the time to
60 seconds allows to avoid false positives in expense of longer testing
time.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 testcases/kernel/device-drivers/rtc/rtc01.c | 25 +++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/device-drivers/rtc/rtc01.c b/testcases/kernel/device-drivers/rtc/rtc01.c
index ebd15306d583..51b144f201ad 100644
--- a/testcases/kernel/device-drivers/rtc/rtc01.c
+++ b/testcases/kernel/device-drivers/rtc/rtc01.c
@@ -64,6 +64,23 @@ void read_alarm_test(void)
 	unsigned long data;
 	fd_set rfds;
 	struct timeval tv;
+	unsigned int alarm_sec = 5;
+
+	if (tst_is_virt(VIRT_HYPERV)) {
+		/*
+		 * Microsoft Hyper-V hypervisor has broken CMOS RTC which
+		 * does not generate interrupts if alarm is set for 5-59
+		 * seconds from now and it advances to next the minute
+		 * (e.g. 10:12:57 -> 10:13:02).
+		 * However alarm set to fire in 60 seconds (and more) works
+		 * fine.
+		 *
+		 * This was confirmed on different Linux kernels (from v4.15 up
+		 * to v5.14.2) and instances (Standard_B1ms, Standard_B4ms,
+		 * Standard_D4s_v3, Standard_D8d_v4).
+		 */
+		alarm_sec = 60;
+	}
 
 	tst_resm(TINFO, "RTC READ TEST:");
 
@@ -82,8 +99,8 @@ void read_alarm_test(void)
 
 	tst_resm(TINFO, "RTC ALARM TEST :");
 
-	/* Set Alarm to 5 Seconds */
-	rtc_tm.tm_sec += 5;
+	/* Set Alarm to 5 (or more) seconds */
+	rtc_tm.tm_sec += alarm_sec;
 	if (rtc_tm.tm_sec >= 60) {
 		rtc_tm.tm_sec %= 60;
 		rtc_tm.tm_min++;
@@ -127,9 +144,9 @@ void read_alarm_test(void)
 		return;
 	}
 
-	tst_resm(TINFO, "Waiting 5 seconds for the alarm...");
+	tst_resm(TINFO, "Waiting %u seconds for the alarm...", alarm_sec+1);
 
-	tv.tv_sec = 6;		/* set 6 seconds as the time out */
+	tv.tv_sec = alarm_sec+1;		/* set 5+1 seconds as the time out */
 	tv.tv_usec = 0;
 
 	FD_ZERO(&rfds);
-- 
2.30.2


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/2] rtc01: add workaround for broken CMOS RTC on Microsoft Hyper-V cloud

[Cyril Hrubis]

Hi!
> rtc01 fails on missed alarm on multiple different Azure instances if the
> alarm is set for the next minute:
> 
>   rtc01 0 TINFO : RTC READ TEST:
>   rtc01 1 TPASS : RTC READ TEST Passed
>   rtc01 0 TINFO : Current RTC date/time is 11-6-2021, 09:00:58.
>   rtc01 0 TINFO : RTC ALARM TEST :
>   rtc01 0 TINFO : Alarm time set to 09:01:03.
>   rtc01 0 TINFO : Waiting 5 seconds for the alarm...
>   rtc01 2 TFAIL : rtc01.c:151: Timed out waiting for the alarm
> 
> Reproduced easily with rtcwake:
> 
>   $ rtcwake -d rtc0 -m on -s 50 -v
> 
> If alarm is set for now+60 seconds, it works fine.  Clearly Microsoft
> Hyper-V cloud instances have a broken CMOS RTC which unfortunately
> cannot be easily fixed.  Adding simple workaround to extend the time to
> 60 seconds allows to avoid false positives in expense of longer testing
> time.

I do not think that adding workarounds for a broken platforms into
testcases is a right thing to do.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/2] rtc01: add workaround for broken CMOS RTC on Microsoft Hyper-V cloud

[Krzysztof Kozlowski]

On 10/09/2021 10:34, Cyril Hrubis wrote:
> Hi!
>> rtc01 fails on missed alarm on multiple different Azure instances if the
>> alarm is set for the next minute:
>>
>>   rtc01 0 TINFO : RTC READ TEST:
>>   rtc01 1 TPASS : RTC READ TEST Passed
>>   rtc01 0 TINFO : Current RTC date/time is 11-6-2021, 09:00:58.
>>   rtc01 0 TINFO : RTC ALARM TEST :
>>   rtc01 0 TINFO : Alarm time set to 09:01:03.
>>   rtc01 0 TINFO : Waiting 5 seconds for the alarm...
>>   rtc01 2 TFAIL : rtc01.c:151: Timed out waiting for the alarm
>>
>> Reproduced easily with rtcwake:
>>
>>   $ rtcwake -d rtc0 -m on -s 50 -v
>>
>> If alarm is set for now+60 seconds, it works fine.  Clearly Microsoft
>> Hyper-V cloud instances have a broken CMOS RTC which unfortunately
>> cannot be easily fixed.  Adding simple workaround to extend the time to
>> 60 seconds allows to avoid false positives in expense of longer testing
>> time.
> 
> I do not think that adding workarounds for a broken platforms into
> testcases is a right thing to do.

I am actually also not sure, but the broken platform is one of three
main cloud providers. :)


Best regards,
Krzysztof

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/2] rtc01: add space to beginning of comments

[Cyril Hrubis]

Hi!
> Improve readability of comments - no functional change.

Actually some of the comments are really commenting the obvious so it
would be better just to remove most.

For instance this:

	/* Read and alarm tests */
	read_alarm_test();

How does the comment here add any value?

And I would say that it's mostly the same for the ioclt() calls as well,
the constants do have reasonable names so I guess that in cases like
this:

	/* Read current alarm time */
	ret = ioctl(rtc_fd, RTC_ALM_READ, &rtc_tm);

it's kind of obvious that we do read the alarm even without the comment.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/2] rtc01: add workaround for broken CMOS RTC on Microsoft Hyper-V cloud

[Thadeu Lima de Souza Cascardo]

On Fri, Sep 10, 2021 at 10:35:32AM +0200, Krzysztof Kozlowski wrote:
> On 10/09/2021 10:34, Cyril Hrubis wrote:
> > Hi!
> >> rtc01 fails on missed alarm on multiple different Azure instances if the
> >> alarm is set for the next minute:
> >>
> >>   rtc01 0 TINFO : RTC READ TEST:
> >>   rtc01 1 TPASS : RTC READ TEST Passed
> >>   rtc01 0 TINFO : Current RTC date/time is 11-6-2021, 09:00:58.
> >>   rtc01 0 TINFO : RTC ALARM TEST :
> >>   rtc01 0 TINFO : Alarm time set to 09:01:03.
> >>   rtc01 0 TINFO : Waiting 5 seconds for the alarm...
> >>   rtc01 2 TFAIL : rtc01.c:151: Timed out waiting for the alarm
> >>
> >> Reproduced easily with rtcwake:
> >>
> >>   $ rtcwake -d rtc0 -m on -s 50 -v
> >>
> >> If alarm is set for now+60 seconds, it works fine.  Clearly Microsoft
> >> Hyper-V cloud instances have a broken CMOS RTC which unfortunately
> >> cannot be easily fixed.  Adding simple workaround to extend the time to
> >> 60 seconds allows to avoid false positives in expense of longer testing
> >> time.
> > 
> > I do not think that adding workarounds for a broken platforms into
> > testcases is a right thing to do.
> 
> I am actually also not sure, but the broken platform is one of three
> main cloud providers. :)
> 

The test here is doing the right thing, verifying that the driver behaves as
documented and expected. The kernel should be doing any workarounds/quirks
necessary to make the RTC interface behave as expected.

Once that is done, I think it would be valuable to add to the test the specific
scenario where this is failing. That is, add the specific test where the alarm
seconds is behind the date seconds.

By the way, I have tested it and the reason 60 seconds work is because the
seconds alarm value is most often after the current value. If you use 115, for
example, it will most likely fail, unless you are within that 5 second range
where it doesn't.

The RTC CMOS driver has changed a lot over the years. I wonder if using proper
UIE instead of simulating it with set_alarm would work. But, then,
RTC_ALM_SET+RTC_AIE_ON would still fail.

Also, of note, the VM instances are lacking an HPET, which is used instead of
the old CMOS RTC when present.

Cascardo.

> 
> Best regards,
> Krzysztof
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/2] rtc01: add space to beginning of comments

[Krzysztof Kozlowski]

On 10/09/2021 11:14, Cyril Hrubis wrote:
> Hi!
>> Improve readability of comments - no functional change.
> 
> Actually some of the comments are really commenting the obvious so it
> would be better just to remove most.
> 
> For instance this:
> 
> 	/* Read and alarm tests */
> 	read_alarm_test();
> 
> How does the comment here add any value?
> 
> And I would say that it's mostly the same for the ioclt() calls as well,
> the constants do have reasonable names so I guess that in cases like
> this:
> 
> 	/* Read current alarm time */
> 	ret = ioctl(rtc_fd, RTC_ALM_READ, &rtc_tm);
> 
> it's kind of obvious that we do read the alarm even without the comment.

Good point but also I am not sure if removing these comments is worth
patch on its own (without patch 2/2).


Best regards,
Krzysztof

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp