On 7/6/24 10:20 AM, Joseph, Jithu wrote:

Thanks very much for the review Ilpo,

On 7/6/2024 6:19 PM, Ilpo Järvinen wrote:

quoted

On Thu, 27 Jun 2024, Kuppuswamy Sathyanarayanan wrote:

quoted

From: Jithu Joseph <redacted>

In a core, the SBAF test engine is shared between sibling CPUs.

An SBAF test image contains multiple bundles. Each bundle is further
composed of subunits called programs. When a SBAF test (for a particular
core) is triggered by the user, each SBAF bundle from the loaded test
image is executed sequentially on all the threads on the core using
the stop_core_cpuslocked mechanism. Each bundle execution is initiated by
writing to MSR_ACTIVATE_SBAF.

SBAF test bundle execution may be aborted when an interrupt occurs or
if the CPU does not have enough power budget for the test. In these
cases the kernel restarts the test from the aborted bundle. SBAF
execution is not retried if the test fails or if the test makes no
forward progress after 5 retries.

Reviewed-by: Ashok Raj <redacted>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Jithu Joseph <redacted>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/platform/x86/intel/ifs/ifs.h     |  30 +++
 drivers/platform/x86/intel/ifs/runtest.c | 234 +++++++++++++++++++++++
 2 files changed, 264 insertions(+)

...

quoted

quoted

+
+static void ifs_sbaf_test_core(int cpu, struct device *dev)
+{
+	struct sbaf_run_params run_params;
+	union ifs_sbaf_status status;
+	union ifs_sbaf activate;
+	unsigned long timeout;
+	struct ifs_data *ifsd;
+	int stop_bundle;
+	int retries;
+
+	ifsd = ifs_get_data(dev);
+
+	activate.data = 0;
+	activate.delay = IFS_THREAD_WAIT;
+
+	timeout = jiffies + (2 * HZ);
+	retries = MAX_IFS_RETRIES;
+	activate.bundle_idx = 0;
+	stop_bundle = ifsd->max_bundle;
+
+	while (activate.bundle_idx <= stop_bundle) {
+		if (time_after(jiffies, timeout)) {
+			status.error_code = IFS_SW_TIMEOUT;
+			break;

If we take this branch on the first iteration...

quoted

+		}
+
+		atomic_set(&sbaf_cpus_in, 0);
+
+		run_params.ifsd = ifsd;
+		run_params.activate = &activate;
+		run_params.retry_cnt = &retries;
+		stop_core_cpuslocked(cpu, dosbaf, &run_params);
+
+		status = run_params.status;
+
+		if (sbaf_bundle_completed(status)) {
+			activate.bundle_idx = status.bundle_idx + 1;
+			activate.pgm_idx = 0;
+			retries = MAX_IFS_RETRIES;
+			continue;
+		}
+
+		/* Some cases can be retried, give up for others */
+		if (!sbaf_can_restart(status))
+			break;
+
+		if (status.pgm_idx == activate.pgm_idx) {
+			/* If no progress retry */
+			if (--retries == 0) {
+				if (status.error_code == IFS_NO_ERROR)
+					status.error_code = IFS_SW_PARTIAL_COMPLETION;
+				break;
+			}
+		} else {
+			/* if some progress, more pgms remaining in bundle, reset retries */
+			retries = MAX_IFS_RETRIES;
+			activate.bundle_idx = status.bundle_idx;
+			activate.pgm_idx = status.pgm_idx;
+		}
+	}
+
+	/* Update status for this core */
+	ifsd->scan_details = status.data;
+
+	if (status.sbaf_status == SBAF_STATUS_SIGN_FAIL ||
+	    status.sbaf_status == SBAF_STATUS_TEST_FAIL) {

...is status.sbar_status uninitialized? (And perhaps other fields too 
that are used after the loop?).

Will initialize status.data to 0 (at function entry, prior to the loop) to ensure that in the unlikely scenario of taking
the aforementioned branch (on the first iteration),  all the status fields will be zeroed (apart from status.error_code, 
which would be set to IFS_SW_TIMEOUT at the branch)

Yes, it is better to initialize the status variable to 0. While moving the
timeout check to the end of the loop could also address this issue,
explicitly fixing it at the initialization is a cleaner solution.

--- a/drivers/platform/x86/intel/ifs/runtest.c
+++ b/drivers/platform/x86/intel/ifs/runtest.c

@@ -542,7 +542,7 @@ static int dosbaf(void *data)

 static void ifs_sbaf_test_core(int cpu, struct device *dev)
 {
        struct sbaf_run_params run_params;
-       union ifs_sbaf_status status;
+       union ifs_sbaf_status status = {};

I think there is a similar usage in ifs_test_core() as well. Will fix that in
a separate patch.

quoted

It could also be that the timeout check shouldn't be before the first real 
iteration inside the loop to ensure that kind of shortcut cannot occur?

quoted

Thanks
Jithu

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer