Re: [PATCH v1 3/4] platform/x86/intel/ifs: Add SBAF test support

[Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>]

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:
>> On Thu, 27 Jun 2024, Kuppuswamy Sathyanarayanan wrote:
>>
>>> From: Jithu Joseph <jithu.joseph@intel.com>
>>>
>>> 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 <ashok.raj@intel.com>
>>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>>> Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
>>> 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(+)
>>>
> ...
>
>>> +
>>> +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...
>>
>>> +		}
>>> +
>>> +		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.

>
>> 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?

>>
> Thanks
> Jithu

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer