[LTP] [PATCH v2] sys_hwmon01: always report a result

[LTP] [PATCH v2] sys_hwmon01: always report a result

[Li Wang (Hieco)]

On machines where the only hwmon devices expose no checkable
attributes (e.g. a VM or laptop where hwmon0 is just 'ACAD' with
nothing but a name attribute), every *_SILENT assertion skips
silently and the test finishes without reporting a single result,
which the library turns into:

    tst_test.c:1730: TBROK: Test haven't reported results!

Fix this by counting the attributes that actually exist and get
validated, then report a TPASS summary at the end, or TCONF when
hwmon devices are present but expose nothing we can check.

While at it, fix a preexisting off-by-one in the inN channel loop:
probe in0..in31 so that all channel types probe exactly MAX_CHANNELS
indices.

Signed-off-by: Li Wang (Hieco) <li.wang@linux.dev>
Reviewed-by: Xiaochen Shen <shenxiaochen@open-hieco.net>
---

Notes:
    v2:
    	- rename attr_exists() to sysfs_attr_exists() (Xiaochen)
    	- flatten the final if/else result reporting with an early return (Xiaochen)
    	- fix preexisting off-by-one in the inN channel loop, probe in0..in31
    	  so that all channel types probe exactly MAX_CHANNELS indices (Xiaochen)

 .../kernel/sysfs/class/hwmon/sys_hwmon01.c    | 111 ++++++++++++------
 1 file changed, 77 insertions(+), 34 deletions(-)

diff --git a/testcases/kernel/sysfs/class/hwmon/sys_hwmon01.c b/testcases/kernel/sysfs/class/hwmon/sys_hwmon01.c
index 1bb574c23..991852da7 100644
--- a/testcases/kernel/sysfs/class/hwmon/sys_hwmon01.c
+++ b/testcases/kernel/sysfs/class/hwmon/sys_hwmon01.c
@@ -49,6 +49,8 @@
 
 #include <string.h>
 #include <dirent.h>
+#include <stdarg.h>
+#include <limits.h>
 
 #include "tst_test.h"
 #include "tst_sysfs_assert.h"
@@ -56,39 +58,67 @@
 
 #define MAX_CHANNELS 32
 
+static unsigned int checked;
+
+static int sysfs_attr_exists(const char *fmt, ...)
+{
+	char path[PATH_MAX];
+	va_list va;
+
+	va_start(va, fmt);
+	vsnprintf(path, sizeof(path), fmt, va);
+	va_end(va);
+
+	return tst_sysfs_exists(path);
+}
+
+#define CHECK_RANGELL(lo, hi, ...) do { \
+	if (sysfs_attr_exists(__VA_ARGS__)) { \
+		checked++; \
+		TST_SYSFS_ASSERT_RANGELL_SILENT(lo, hi, __VA_ARGS__); \
+	} \
+} while (0)
+
+#define CHECK_BOOL(...) do { \
+	if (sysfs_attr_exists(__VA_ARGS__)) { \
+		checked++; \
+		TST_SYSFS_ASSERT_BOOL_SILENT(__VA_ARGS__); \
+	} \
+} while (0)
+
 static void check_temp_channel(const char *hwmon, int idx)
 {
-	TST_SYSFS_ASSERT_RANGELL_SILENT(-55000, 200000,
-					PATH_CLASS_HWMON "/%s/temp%d_input", hwmon, idx);
-
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/temp%d_alarm", hwmon, idx);
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/temp%d_crit_alarm", hwmon, idx);
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/temp%d_min_alarm", hwmon, idx);
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/temp%d_max_alarm", hwmon, idx);
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/temp%d_fault", hwmon, idx);
+	CHECK_RANGELL(-55000, 200000,
+		      PATH_CLASS_HWMON "/%s/temp%d_input", hwmon, idx);
+
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/temp%d_alarm", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/temp%d_crit_alarm", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/temp%d_min_alarm", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/temp%d_max_alarm", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/temp%d_fault", hwmon, idx);
 }
 
 static void check_fan_channel(const char *hwmon, int idx)
 {
-	TST_SYSFS_ASSERT_RANGELL_SILENT(0, 100000,
-					PATH_CLASS_HWMON "/%s/fan%d_input", hwmon, idx);
+	CHECK_RANGELL(0, 100000,
+		      PATH_CLASS_HWMON "/%s/fan%d_input", hwmon, idx);
 
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/fan%d_alarm", hwmon, idx);
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/fan%d_min_alarm", hwmon, idx);
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/fan%d_max_alarm", hwmon, idx);
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/fan%d_fault", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/fan%d_alarm", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/fan%d_min_alarm", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/fan%d_max_alarm", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/fan%d_fault", hwmon, idx);
 }
 
 static void check_in_channel(const char *hwmon, int idx)
 {
-	TST_SYSFS_ASSERT_RANGELL_SILENT(-100000, 100000,
-					PATH_CLASS_HWMON "/%s/in%d_input", hwmon, idx);
-
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/in%d_alarm", hwmon, idx);
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/in%d_min_alarm", hwmon, idx);
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/in%d_max_alarm", hwmon, idx);
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/in%d_crit_alarm", hwmon, idx);
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/in%d_lcrit_alarm", hwmon, idx);
+	CHECK_RANGELL(-100000, 100000,
+		      PATH_CLASS_HWMON "/%s/in%d_input", hwmon, idx);
+
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/in%d_alarm", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/in%d_min_alarm", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/in%d_max_alarm", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/in%d_crit_alarm", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/in%d_lcrit_alarm", hwmon, idx);
 }
 
 static void check_curr_channel(const char *hwmon, int idx)
@@ -97,10 +127,10 @@ static void check_curr_channel(const char *hwmon, int idx)
 	 * Unlike fan/power, current can be negative on bidirectional sensors,
 	 * e.g. a battery charge/discharge current monitor.
 	 */
-	TST_SYSFS_ASSERT_RANGELL_SILENT(-500000, 500000,
-					PATH_CLASS_HWMON "/%s/curr%d_input", hwmon, idx);
+	CHECK_RANGELL(-500000, 500000,
+		      PATH_CLASS_HWMON "/%s/curr%d_input", hwmon, idx);
 
-	TST_SYSFS_ASSERT_BOOL_SILENT(PATH_CLASS_HWMON "/%s/curr%d_alarm", hwmon, idx);
+	CHECK_BOOL(PATH_CLASS_HWMON "/%s/curr%d_alarm", hwmon, idx);
 }
 
 static void check_power_channel(const char *hwmon, int idx)
@@ -110,13 +140,13 @@ static void check_power_channel(const char *hwmon, int idx)
 	 * sensor (CPU, GPU, PSU rail, ...) while still fitting within
 	 * LONG_MAX on 32bit architectures.
 	 */
-	TST_SYSFS_ASSERT_RANGELL_SILENT(0, 2000000000,
-					PATH_CLASS_HWMON "/%s/power%d_input", hwmon, idx);
+	CHECK_RANGELL(0, 2000000000,
+		      PATH_CLASS_HWMON "/%s/power%d_input", hwmon, idx);
 }
 
 static void check_pwm_channel(const char *hwmon, int idx)
 {
-	TST_SYSFS_ASSERT_RANGELL_SILENT(0, 255, PATH_CLASS_HWMON "/%s/pwm%d", hwmon, idx);
+	CHECK_RANGELL(0, 255, PATH_CLASS_HWMON "/%s/pwm%d", hwmon, idx);
 }
 
 static void check_hwmon_device(const char *hwmon)
@@ -127,11 +157,11 @@ static void check_hwmon_device(const char *hwmon)
 	if (TST_SYSFS_READ_STR(name, sizeof(name), PATH_CLASS_HWMON "/%s/name", hwmon))
 		tst_res(TINFO, "%s: name = '%s'", hwmon, name);
 
-	TST_SYSFS_ASSERT_RANGELL_SILENT(1, 3600000,
-					PATH_CLASS_HWMON "/%s/update_interval", hwmon);
+	CHECK_RANGELL(1, 3600000,
+		      PATH_CLASS_HWMON "/%s/update_interval", hwmon);
 
 	/* inN channels are numbered from 0, per the hwmon ABI */
-	for (i = 0; i <= MAX_CHANNELS; i++)
+	for (i = 0; i < MAX_CHANNELS; i++)
 		check_in_channel(hwmon, i);
 
 	for (i = 1; i <= MAX_CHANNELS; i++) {
@@ -147,7 +177,9 @@ static void do_test(void)
 {
 	DIR *d;
 	struct dirent *ent;
-	int found = 0;
+	int devices = 0;
+
+	checked = 0;
 
 	if (!tst_sysfs_exists(PATH_CLASS_HWMON))
 		tst_brk(TCONF, PATH_CLASS_HWMON ": not present");
@@ -158,14 +190,25 @@ static void do_test(void)
 		if (strncmp(ent->d_name, "hwmon", 5))
 			continue;
 
-		found = 1;
+		devices++;
 		check_hwmon_device(ent->d_name);
 	}
 
 	SAFE_CLOSEDIR(d);
 
-	if (!found)
+	if (!devices) {
 		tst_res(TCONF, "No hwmon device found");
+		return;
+	}
+
+	if (!checked) {
+		tst_res(TCONF, "%d hwmon devices present, but no checkable attributes",
+				devices);
+		return;
+	}
+
+	tst_res(TPASS, "Validated %u attributes on %d hwmon devices",
+			checked, devices);
 }
 
 static struct tst_test test = {
-- 
2.56.0


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

Re: [LTP] [PATCH v2] sys_hwmon01: always report a result

[Cyril Hrubis]

Hi!
>  #include "tst_test.h"
>  #include "tst_sysfs_assert.h"
> @@ -56,39 +58,67 @@
>  
>  #define MAX_CHANNELS 32
>  
> +static unsigned int checked;
> +
> +static int sysfs_attr_exists(const char *fmt, ...)
> +{
> +	char path[PATH_MAX];
> +	va_list va;
> +
> +	va_start(va, fmt);
> +	vsnprintf(path, sizeof(path), fmt, va);
> +	va_end(va);
> +
> +	return tst_sysfs_exists(path);
> +}

Uff, the tst_sysfs_attr_exists() does work with printf-like parameters.

What do we need the this wrapper for?

> +#define CHECK_RANGELL(lo, hi, ...) do { \
> +	if (sysfs_attr_exists(__VA_ARGS__)) { \
> +		checked++; \
> +		TST_SYSFS_ASSERT_RANGELL_SILENT(lo, hi, __VA_ARGS__); \
> +	} \
> +} while (0)
> +
> +#define CHECK_BOOL(...) do { \
> +	if (sysfs_attr_exists(__VA_ARGS__)) { \
> +		checked++; \
> +		TST_SYSFS_ASSERT_BOOL_SILENT(__VA_ARGS__); \
> +	} \
> +} while (0)

What about we add the counter into the sysfs library instead?

If we add:

void tst_sysfs_cnt_reset(void);

unsigned int tst_sysfs_cnt_get(void);

We can call the tst_sysfs_cnt_rest() at the start of the test function,
and get the counter at the end.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2] sys_hwmon01: always report a result

[Li Wang]

> Hi!
> >  #include "tst_test.h"
> >  #include "tst_sysfs_assert.h"
> > @@ -56,39 +58,67 @@
> >  
> >  #define MAX_CHANNELS 32
> >  
> > +static unsigned int checked;
> > +
> > +static int sysfs_attr_exists(const char *fmt, ...)
> > +{
> > +	char path[PATH_MAX];
> > +	va_list va;
> > +
> > +	va_start(va, fmt);
> > +	vsnprintf(path, sizeof(path), fmt, va);
> > +	va_end(va);
> > +
> > +	return tst_sysfs_exists(path);
> > +}
> 
> Uff, the tst_sysfs_attr_exists() does work with printf-like parameters.
> 
> What do we need the this wrapper for?

Indeed, I missed that it already takes printf-like parameters.
 
> > +#define CHECK_RANGELL(lo, hi, ...) do { \
> > +	if (sysfs_attr_exists(__VA_ARGS__)) { \
> > +		checked++; \
> > +		TST_SYSFS_ASSERT_RANGELL_SILENT(lo, hi, __VA_ARGS__); \
> > +	} \
> > +} while (0)
> > +
> > +#define CHECK_BOOL(...) do { \
> > +	if (sysfs_attr_exists(__VA_ARGS__)) { \
> > +		checked++; \
> > +		TST_SYSFS_ASSERT_BOOL_SILENT(__VA_ARGS__); \
> > +	} \
> > +} while (0)
> 
> What about we add the counter into the sysfs library instead?
> 
> If we add:
> 
> void tst_sysfs_cnt_reset(void);
> 
> unsigned int tst_sysfs_cnt_get(void);
> 
> We can call the tst_sysfs_cnt_rest() at the start of the test function,
> and get the counter at the end.

Good idea, that's cleaner on all fronts: the CHECK_* wrappers and the
extra existence probe go away entirely (the current version stats each
attribute twice), and any other SILENT-only sysfs test gets the same
fix for free.

I'll split v3 into two patches: one adding
tst_sysfs_cnt_reset()/tst_sysfs_cnt_get() with the counter bumped in
the SILENT assertions when the attribute exists, and the hwmon test on
top of it.

One question: should the counter count attributes that exist and get
checked regardless of pass/fail, or only passing ones? I'd go with the
former, since failures already produce TFAIL results on their own.

-- 
Regards,
Li Wang

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