Re: [PATCH v2 4/7] platform/x86/intel/ifs: Implement Array BIST test

[Dave Hansen <dave.hansen@xxxxxxxxx>]

On 2/15/23 08:58, Dave Hansen wrote:
> On 2/14/23 15:44, Jithu Joseph wrote:
> I'd probably do something like the attached patch.  It gets rid of
> 'data' and uses sane types for the bitfield.  It does away with separate
> variables and munging into/out of the msr[] array and just passes a
> single command struct to the work function.  It doesn't have any
> uninitialized structure/bitfield fields.

Real patch attached now.

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 00bc86db1193..fa13350976c9 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -199,14 +199,11 @@ union ifs_status {
 };
 
 /* MSR_ARRAY_BIST bit fields */
-union ifs_array {
-	u64	data;
-	struct {
-		u32	array_bitmask		:32;
-		u32	array_bank		:16;
-		u32	rsvd			:15;
-		u32	ctrl_result		:1;
-	};
+struct ifs_array {
+	u32	array_bitmask;
+	u16	array_bank;
+	u16	rsvd			:15;
+	u16	ctrl_result		:1;
 };
 
 /*
diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
index e74004fab1aa..1586bc6f5529 100644
--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c
@@ -253,8 +253,9 @@ static void wait_for_sibling_cpu(atomic_t *t, long long timeout)
 
 static int do_array_test(void *data)
 {
+	struct ifs_array *command = data;
 	int cpu = smp_processor_id();
-	u64 *msrs = data;
+
 	int first;
 
 	/*
@@ -263,9 +264,9 @@ static int do_array_test(void *data)
 	first = cpumask_first(cpu_smt_mask(cpu));
 
 	if (cpu == first) {
-		wrmsrl(MSR_ARRAY_BIST, msrs[0]);
+		wrmsrl(MSR_ARRAY_BIST, *(u64 *)&command);
 		/* Pass back the result of the test */
-		rdmsrl(MSR_ARRAY_BIST, msrs[1]);
+		rdmsrl(MSR_ARRAY_BIST, (u64 *)&command);
 	}
 
 	/* Tests complete faster if the sibling is spinning here */
@@ -276,43 +277,38 @@ static int do_array_test(void *data)
 
 static void ifs_array_test_core(int cpu, struct device *dev)
 {
-	union ifs_array activate, status = {0};
+	struct ifs_array command = {};
 	bool timed_out = false;
 	struct ifs_data *ifsd;
 	unsigned long timeout;
-	u64 msrvals[2];
 
 	ifsd = ifs_get_data(dev);
 
-	activate.array_bitmask = ~0U;
+	command.array_bitmask = ~0U;
 	timeout = jiffies + HZ / 2;
 
 	do {
+		struct ifs_array before = command;
+
 		if (time_after(jiffies, timeout)) {
 			timed_out = true;
 			break;
 		}
 
-		msrvals[0] = activate.data;
-
 		atomic_set(&array_cpus_out, 0);
-		stop_core_cpuslocked(cpu, do_array_test, msrvals);
-		status.data = msrvals[1];
+		stop_core_cpuslocked(cpu, do_array_test, &command);
 
-		trace_ifs_array(cpu, activate, status);
-		if (status.ctrl_result)
+		trace_ifs_array(cpu, before, command);
+		if (command.ctrl_result)
 			break;
 
-		activate.array_bitmask = status.array_bitmask;
-		activate.array_bank = status.array_bank;
+	} while (command.array_bitmask);
 
-	} while (status.array_bitmask);
-
-	ifsd->scan_details = status.data;
+	ifsd->scan_details = command.data;
 
-	if (status.ctrl_result)
+	if (command.ctrl_result)
 		ifsd->status = SCAN_TEST_FAIL;
-	else if (timed_out || status.array_bitmask)
+	else if (timed_out || command.array_bitmask)
 		ifsd->status = SCAN_NOT_TESTED;
 	else
 		ifsd->status = SCAN_TEST_PASS;