Re: [Qemu-devel] [PATCH 2/5] tests: more thorough test of ds1338
On 24.03.2018 20:39, Michael Davidsaver wrote: >> On 02/20/2018 09:44 AM, Michael Davidsaver wrote: >>> On 02/18/2018 11:39 PM, Thomas Huth wrote: > ... >>> That magic (together with patch 1/5) is IMHO a little bit ugly. I've hit >>> the same problem with the m48t59 test recently, and I solved it by >>> moving the qtest_start() and qtest_end() calls from the main() function >>> into the single tests instead, so that each test starts with a clean state: >>> >>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9c29830c90d82f27f >>> >>> Could you maybe try whether that approach works for your test cases >>> here, too? Then you could do this without the "0xff" hack here... >> >> Your right, this looks clearer. I'll try this approach. > > I ultimately decided not to go with this approach as test failures > didn't call qtest_quit(), and the process under test is left running > after gtester exits. > > Instead I split the current time test off into a second executable. > This avoids all of the magic. Honestly, I don't like the idea that we put tests into different files just because of this. We're not doing this for any other tests so far, so just using this pattern for the ds1338 / ds1375 tests sounds wrong. If there is an issue with test processes not being killed correctly, I think we should rather fix that issue instead, maybe by adding a special function with atexit() or something similar ... I haven't looked closely into that yet. Just my 0.02 €. Thomas
Re: [Qemu-devel] [PATCH 2/5] tests: more thorough test of ds1338
> On 02/20/2018 09:44 AM, Michael Davidsaver wrote: >> On 02/18/2018 11:39 PM, Thomas Huth wrote: ... >> That magic (together with patch 1/5) is IMHO a little bit ugly. I've hit >> the same problem with the m48t59 test recently, and I solved it by >> moving the qtest_start() and qtest_end() calls from the main() function >> into the single tests instead, so that each test starts with a clean state: >> >> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9c29830c90d82f27f >> >> Could you maybe try whether that approach works for your test cases >> here, too? Then you could do this without the "0xff" hack here... > > Your right, this looks clearer. I'll try this approach. I ultimately decided not to go with this approach as test failures didn't call qtest_quit(), and the process under test is left running after gtester exits. Instead I split the current time test off into a second executable. This avoids all of the magic. FYI. I also looked at using g_test_add(), keeping the QTestState* in a Fixture struct, and using setup and teardown functions to call qtest_start()/qtest_quit(). This works, but seemed to me like too much effort in this case.
Re: [Qemu-devel] [PATCH 2/5] tests: more thorough test of ds1338
On 02/18/2018 11:39 PM, Thomas Huth wrote:
> On 19.02.2018 05:03, Michael Davidsaver wrote:
>> Test current time and set+get round trip.
>>
>> The set+get test is repeated 4 times. These cases are
>> spread across a single day in an attempt to trigger some potential
>> issues regardless of the timezone of the machine running the tests.
>>
>> Signed-off-by: Michael Davidsaver
>> ---
>> tests/Makefile.include | 2 +
>> tests/ds-rtc-i2c-test.c | 193
>>
>> 2 files changed, 195 insertions(+)
>> create mode 100644 tests/ds-rtc-i2c-test.c
> [...]
>> tests/q35-test$(EXESUF): tests/q35-test.o $(libqos-pc-obj-y)
>> diff --git a/tests/ds-rtc-i2c-test.c b/tests/ds-rtc-i2c-test.c
>> new file mode 100644
>> index 00..464eb08558
>> --- /dev/null
>> +++ b/tests/ds-rtc-i2c-test.c
>> @@ -0,0 +1,193 @@
>> +/* Testing of Dallas/Maxim I2C bus RTC devices
>> + *
>> + * Copyright (c) 2017 Michael Davidsaver
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>> + * the LICENSE file in the top-level directory.
>> + */
>> +#include
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/bcd.h"
>> +#include "qemu/cutils.h"
>> +#include "qemu/timer.h"
>> +#include "libqtest.h"
>> +#include "libqos/libqos.h"
>> +#include "libqos/i2c.h"
>> +
>> +#define IMX25_I2C_0_BASE 0x43F8
>> +#define DS1338_ADDR 0x68
>> +
>> +static I2CAdapter *i2c;
>> +static uint8_t addr;
>> +static bool use_century;
>> +
>> +static
>> +time_t rtc_gettime(void)
>> +{
>> +struct tm parts;
>> +uint8_t buf[7];
>> +
>> +buf[0] = 0;
>> +i2c_send(i2c, addr, buf, 1);
>> +i2c_recv(i2c, addr, buf, 7);
>> +
>> +parts.tm_sec = from_bcd(buf[0]);
>> +parts.tm_min = from_bcd(buf[1]);
>> +if (buf[2] & 0x40) {
>> +/* 12 hour */
>> +/* HOUR register is 1-12. */
>> +parts.tm_hour = from_bcd(buf[2] & 0x1f);
>> +g_assert_cmpuint(parts.tm_hour, >=, 1);
>> +g_assert_cmpuint(parts.tm_hour, <=, 12);
>> +parts.tm_hour %= 12u; /* wrap 12 -> 0 */
>> +if (buf[2] & 0x20) {
>> +parts.tm_hour += 12u;
>> +}
>> +} else {
>> +/* 24 hour */
>> +parts.tm_hour = from_bcd(buf[2] & 0x3f);
>> +}
>> +parts.tm_wday = from_bcd(buf[3]);
>> +parts.tm_mday = from_bcd(buf[4]);
>> +parts.tm_mon = from_bcd((buf[5] & 0x1f) - 1u);
>> +parts.tm_year = from_bcd(buf[6]);
>> +if (!use_century || (buf[5] & 0x80)) {
>> +parts.tm_year += 100u;
>> +}
>> +
>> +return mktimegm(&parts);
>> +}
>> +
>> +/* read back and compare with current system time */
>> +static
>> +void test_rtc_current(void)
>> +{
>> +uint8_t buf;
>> +time_t expected, actual;
>> +
>> +/* magic address to zero RTC time offset
>> + * as tests may be run in any order
>> + */
>> +buf = 0xff;
>> +i2c_send(i2c, addr, &buf, 1);
>
> That magic (together with patch 1/5) is IMHO a little bit ugly. I've hit
> the same problem with the m48t59 test recently, and I solved it by
> moving the qtest_start() and qtest_end() calls from the main() function
> into the single tests instead, so that each test starts with a clean state:
>
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9c29830c90d82f27f
>
> Could you maybe try whether that approach works for your test cases
> here, too? Then you could do this without the "0xff" hack here...
Your right, this looks clearer. I'll try this approach.
>> +
>> +actual = time(NULL);
>> +/* new second may start here */
>> +expected = rtc_gettime();
>> +g_assert_cmpuint(expected, <=, actual + 1);
>> +g_assert_cmpuint(expected, >=, actual);
>> +}
>> +
>> +
>> +static uint8_t test_time_24_12am[8] = {
>> +0, /* address */
>> +/* Wed, 22 Nov 2017 00:30:53 + */
>> +0x53,
>> +0x30,
>> +0x00, /* 12 AM in 24 hour mode */
>> +0x03, /* monday is our day 1 */
>> +0x22,
>> +0x11 | 0x80,
>> +0x17,
>> +};
>> +
>> +static uint8_t test_time_24_6am[8] = {
>> +0, /* address */
>> +/* Wed, 22 Nov 2017 06:30:53 + */
>> +0x53,
>> +0x30,
>> +0x06, /* 6 AM in 24 hour mode */
>> +0x03, /* monday is our day 1 */
>> +0x22,
>> +0x11 | 0x80,
>> +0x17,
>> +};
>> +
>> +static uint8_t test_time_24_12pm[8] = {
>> +0, /* address */
>> +/* Wed, 22 Nov 2017 12:30:53 + */
>> +0x53,
>> +0x30,
>> +0x12, /* 12 PM in 24 hour mode */
>> +0x03, /* monday is our day 1 */
>> +0x22,
>> +0x11 | 0x80,
>> +0x17,
>> +};
>> +
>> +static uint8_t test_time_24_6pm[8] = {
>> +0, /* address */
>> +/* Wed, 22 Nov 2017 18:30:53 + */
>> +0x53,
>> +0x30,
>> +0x18, /* 6 PM in 24 hour mode */
>> +0x03, /* monday is our day 1 */
>> +0x22,
>> +0x11 | 0x80,
>> +0x17,
>> +};
>> +
>> +/* write in and read back known time */
>> +static
>> +void test_rtc_set(const void *raw)
>> +{
>> +const uint8_t *testtime = raw;
>> +
Re: [Qemu-devel] [PATCH 2/5] tests: more thorough test of ds1338
On 19.02.2018 05:03, Michael Davidsaver wrote:
> Test current time and set+get round trip.
>
> The set+get test is repeated 4 times. These cases are
> spread across a single day in an attempt to trigger some potential
> issues regardless of the timezone of the machine running the tests.
>
> Signed-off-by: Michael Davidsaver
> ---
> tests/Makefile.include | 2 +
> tests/ds-rtc-i2c-test.c | 193
>
> 2 files changed, 195 insertions(+)
> create mode 100644 tests/ds-rtc-i2c-test.c
[...]
> tests/q35-test$(EXESUF): tests/q35-test.o $(libqos-pc-obj-y)
> diff --git a/tests/ds-rtc-i2c-test.c b/tests/ds-rtc-i2c-test.c
> new file mode 100644
> index 00..464eb08558
> --- /dev/null
> +++ b/tests/ds-rtc-i2c-test.c
> @@ -0,0 +1,193 @@
> +/* Testing of Dallas/Maxim I2C bus RTC devices
> + *
> + * Copyright (c) 2017 Michael Davidsaver
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the LICENSE file in the top-level directory.
> + */
> +#include
> +
> +#include "qemu/osdep.h"
> +#include "qemu/bcd.h"
> +#include "qemu/cutils.h"
> +#include "qemu/timer.h"
> +#include "libqtest.h"
> +#include "libqos/libqos.h"
> +#include "libqos/i2c.h"
> +
> +#define IMX25_I2C_0_BASE 0x43F8
> +#define DS1338_ADDR 0x68
> +
> +static I2CAdapter *i2c;
> +static uint8_t addr;
> +static bool use_century;
> +
> +static
> +time_t rtc_gettime(void)
> +{
> +struct tm parts;
> +uint8_t buf[7];
> +
> +buf[0] = 0;
> +i2c_send(i2c, addr, buf, 1);
> +i2c_recv(i2c, addr, buf, 7);
> +
> +parts.tm_sec = from_bcd(buf[0]);
> +parts.tm_min = from_bcd(buf[1]);
> +if (buf[2] & 0x40) {
> +/* 12 hour */
> +/* HOUR register is 1-12. */
> +parts.tm_hour = from_bcd(buf[2] & 0x1f);
> +g_assert_cmpuint(parts.tm_hour, >=, 1);
> +g_assert_cmpuint(parts.tm_hour, <=, 12);
> +parts.tm_hour %= 12u; /* wrap 12 -> 0 */
> +if (buf[2] & 0x20) {
> +parts.tm_hour += 12u;
> +}
> +} else {
> +/* 24 hour */
> +parts.tm_hour = from_bcd(buf[2] & 0x3f);
> +}
> +parts.tm_wday = from_bcd(buf[3]);
> +parts.tm_mday = from_bcd(buf[4]);
> +parts.tm_mon = from_bcd((buf[5] & 0x1f) - 1u);
> +parts.tm_year = from_bcd(buf[6]);
> +if (!use_century || (buf[5] & 0x80)) {
> +parts.tm_year += 100u;
> +}
> +
> +return mktimegm(&parts);
> +}
> +
> +/* read back and compare with current system time */
> +static
> +void test_rtc_current(void)
> +{
> +uint8_t buf;
> +time_t expected, actual;
> +
> +/* magic address to zero RTC time offset
> + * as tests may be run in any order
> + */
> +buf = 0xff;
> +i2c_send(i2c, addr, &buf, 1);
That magic (together with patch 1/5) is IMHO a little bit ugly. I've hit
the same problem with the m48t59 test recently, and I solved it by
moving the qtest_start() and qtest_end() calls from the main() function
into the single tests instead, so that each test starts with a clean state:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9c29830c90d82f27f
Could you maybe try whether that approach works for your test cases
here, too? Then you could do this without the "0xff" hack here...
> +
> +actual = time(NULL);
> +/* new second may start here */
> +expected = rtc_gettime();
> +g_assert_cmpuint(expected, <=, actual + 1);
> +g_assert_cmpuint(expected, >=, actual);
> +}
> +
> +
> +static uint8_t test_time_24_12am[8] = {
> +0, /* address */
> +/* Wed, 22 Nov 2017 00:30:53 + */
> +0x53,
> +0x30,
> +0x00, /* 12 AM in 24 hour mode */
> +0x03, /* monday is our day 1 */
> +0x22,
> +0x11 | 0x80,
> +0x17,
> +};
> +
> +static uint8_t test_time_24_6am[8] = {
> +0, /* address */
> +/* Wed, 22 Nov 2017 06:30:53 + */
> +0x53,
> +0x30,
> +0x06, /* 6 AM in 24 hour mode */
> +0x03, /* monday is our day 1 */
> +0x22,
> +0x11 | 0x80,
> +0x17,
> +};
> +
> +static uint8_t test_time_24_12pm[8] = {
> +0, /* address */
> +/* Wed, 22 Nov 2017 12:30:53 + */
> +0x53,
> +0x30,
> +0x12, /* 12 PM in 24 hour mode */
> +0x03, /* monday is our day 1 */
> +0x22,
> +0x11 | 0x80,
> +0x17,
> +};
> +
> +static uint8_t test_time_24_6pm[8] = {
> +0, /* address */
> +/* Wed, 22 Nov 2017 18:30:53 + */
> +0x53,
> +0x30,
> +0x18, /* 6 PM in 24 hour mode */
> +0x03, /* monday is our day 1 */
> +0x22,
> +0x11 | 0x80,
> +0x17,
> +};
> +
> +/* write in and read back known time */
> +static
> +void test_rtc_set(const void *raw)
> +{
> +const uint8_t *testtime = raw;
> +uint8_t buf[7];
> +unsigned retry = 2;
> +
> +for (; retry; retry--) {
> +i2c_send(i2c, addr, testtime, 8);
> +/* new second may start here */
> +i2c_send(i2c, addr, testtime, 1);
> +i2c_recv(i2c, addr, buf, 7);
> +
> +if (testti
[Qemu-devel] [PATCH 2/5] tests: more thorough test of ds1338
Test current time and set+get round trip.
The set+get test is repeated 4 times. These cases are
spread across a single day in an attempt to trigger some potential
issues regardless of the timezone of the machine running the tests.
Signed-off-by: Michael Davidsaver
---
tests/Makefile.include | 2 +
tests/ds-rtc-i2c-test.c | 193
2 files changed, 195 insertions(+)
create mode 100644 tests/ds-rtc-i2c-test.c
diff --git a/tests/Makefile.include b/tests/Makefile.include
index a1bcbffe12..f5dcd274e0 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -360,6 +360,7 @@ check-qtest-sparc64-y += tests/boot-serial-test$(EXESUF)
check-qtest-arm-y = tests/tmp105-test$(EXESUF)
check-qtest-arm-y += tests/ds1338-test$(EXESUF)
+check-qtest-arm-y += tests/ds-rtc-i2c-test$(EXESUF)
check-qtest-arm-y += tests/m25p80-test$(EXESUF)
gcov-files-arm-y += hw/misc/tmp105.c
check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
@@ -764,6 +765,7 @@ tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o \
tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
tests/ds1338-test$(EXESUF): tests/ds1338-test.o $(libqos-imx-obj-y)
+tests/ds-rtc-i2c-test$(EXESUF): tests/ds-rtc-i2c-test.o $(libqos-imx-obj-y)
tests/m25p80-test$(EXESUF): tests/m25p80-test.o
tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
tests/q35-test$(EXESUF): tests/q35-test.o $(libqos-pc-obj-y)
diff --git a/tests/ds-rtc-i2c-test.c b/tests/ds-rtc-i2c-test.c
new file mode 100644
index 00..464eb08558
--- /dev/null
+++ b/tests/ds-rtc-i2c-test.c
@@ -0,0 +1,193 @@
+/* Testing of Dallas/Maxim I2C bus RTC devices
+ *
+ * Copyright (c) 2017 Michael Davidsaver
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the LICENSE file in the top-level directory.
+ */
+#include
+
+#include "qemu/osdep.h"
+#include "qemu/bcd.h"
+#include "qemu/cutils.h"
+#include "qemu/timer.h"
+#include "libqtest.h"
+#include "libqos/libqos.h"
+#include "libqos/i2c.h"
+
+#define IMX25_I2C_0_BASE 0x43F8
+#define DS1338_ADDR 0x68
+
+static I2CAdapter *i2c;
+static uint8_t addr;
+static bool use_century;
+
+static
+time_t rtc_gettime(void)
+{
+struct tm parts;
+uint8_t buf[7];
+
+buf[0] = 0;
+i2c_send(i2c, addr, buf, 1);
+i2c_recv(i2c, addr, buf, 7);
+
+parts.tm_sec = from_bcd(buf[0]);
+parts.tm_min = from_bcd(buf[1]);
+if (buf[2] & 0x40) {
+/* 12 hour */
+/* HOUR register is 1-12. */
+parts.tm_hour = from_bcd(buf[2] & 0x1f);
+g_assert_cmpuint(parts.tm_hour, >=, 1);
+g_assert_cmpuint(parts.tm_hour, <=, 12);
+parts.tm_hour %= 12u; /* wrap 12 -> 0 */
+if (buf[2] & 0x20) {
+parts.tm_hour += 12u;
+}
+} else {
+/* 24 hour */
+parts.tm_hour = from_bcd(buf[2] & 0x3f);
+}
+parts.tm_wday = from_bcd(buf[3]);
+parts.tm_mday = from_bcd(buf[4]);
+parts.tm_mon = from_bcd((buf[5] & 0x1f) - 1u);
+parts.tm_year = from_bcd(buf[6]);
+if (!use_century || (buf[5] & 0x80)) {
+parts.tm_year += 100u;
+}
+
+return mktimegm(&parts);
+}
+
+/* read back and compare with current system time */
+static
+void test_rtc_current(void)
+{
+uint8_t buf;
+time_t expected, actual;
+
+/* magic address to zero RTC time offset
+ * as tests may be run in any order
+ */
+buf = 0xff;
+i2c_send(i2c, addr, &buf, 1);
+
+actual = time(NULL);
+/* new second may start here */
+expected = rtc_gettime();
+g_assert_cmpuint(expected, <=, actual + 1);
+g_assert_cmpuint(expected, >=, actual);
+}
+
+
+static uint8_t test_time_24_12am[8] = {
+0, /* address */
+/* Wed, 22 Nov 2017 00:30:53 + */
+0x53,
+0x30,
+0x00, /* 12 AM in 24 hour mode */
+0x03, /* monday is our day 1 */
+0x22,
+0x11 | 0x80,
+0x17,
+};
+
+static uint8_t test_time_24_6am[8] = {
+0, /* address */
+/* Wed, 22 Nov 2017 06:30:53 + */
+0x53,
+0x30,
+0x06, /* 6 AM in 24 hour mode */
+0x03, /* monday is our day 1 */
+0x22,
+0x11 | 0x80,
+0x17,
+};
+
+static uint8_t test_time_24_12pm[8] = {
+0, /* address */
+/* Wed, 22 Nov 2017 12:30:53 + */
+0x53,
+0x30,
+0x12, /* 12 PM in 24 hour mode */
+0x03, /* monday is our day 1 */
+0x22,
+0x11 | 0x80,
+0x17,
+};
+
+static uint8_t test_time_24_6pm[8] = {
+0, /* address */
+/* Wed, 22 Nov 2017 18:30:53 + */
+0x53,
+0x30,
+0x18, /* 6 PM in 24 hour mode */
+0x03, /* monday is our day 1 */
+0x22,
+0x11 | 0x80,
+0x17,
+};
+
+/* write in and read back known time */
+static
+void test_rtc_set(const void *raw)
+{
+const uint8_t *testtime = raw;
+uint8_t buf[7];
+unsigned retry = 2;
+
+for (; retry; retry--) {
+i2c_send(i2c, addr, testtime, 8);
+
