Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] platform/x86/intel/ifs: broken group status handling in ifs_test_core() - [Medium] platform/x86/intel/ifs: stale test_details bits in update_group_status() - [Low] platform/x86/intel/ifs: ifs_scan_n zeroed after misc_register() in ifs_init() --
commit 21af90f07c96c13c521c905b29cd18ddd39f88e1 Author: Jithu Joseph <[email protected]> platform/x86/intel/ifs: Track and report scan results on a per-CPU basis This commit records In-Field Scan (IFS) results in a per-CPU structure (ifs_scan_n) for all CPUs in a test group when tests are run simultaneously, and updates error reporting and retry checks to operate on the CPU group. > diff --git a/drivers/platform/x86/intel/ifs/core.c > b/drivers/platform/x86/intel/ifs/core.c > index c830a02fe7b33..08a3e519749be 100644 > --- a/drivers/platform/x86/intel/ifs/core.c > +++ b/drivers/platform/x86/intel/ifs/core.c [ ... ] > @@ -175,6 +180,9 @@ static int __init ifs_init(void) > if (ret) > goto err_exit; > } > + > + for_each_possible_cpu(cpu) > + memset(per_cpu_ptr(&ifs_scan_n, cpu), 0, sizeof(struct > ifs_test_output)); > return 0; [Severity: Low] Can this memset() in drivers/platform/x86/intel/ifs/core.c:ifs_init() race with an early test run? Inside the loop above, misc_register(&ifs_devices[i].misc) registers intel_ifs_0 and exposes its sysfs attributes (plat_ifs_groups) to userspace before continuing the loop for subsequent test devices. If userspace loads a batch and writes to run_test as soon as intel_ifs_0 appears, run_test_store() -> do_core_test() -> ifs_test_core() can invoke doscan() and update_group_result() under ifs_sem while ifs_init() is zeroing ifs_scan_n without holding ifs_sem. Since DEFINE_PER_CPU(struct ifs_test_output, ifs_scan_n) is already statically zero-initialized, is this loop needed at all, or should any initialization happen before misc_register()? > diff --git a/drivers/platform/x86/intel/ifs/runtest.c > b/drivers/platform/x86/intel/ifs/runtest.c > index 09e7a81e92204..6c3d025817e77 100644 > --- a/drivers/platform/x86/intel/ifs/runtest.c > +++ b/drivers/platform/x86/intel/ifs/runtest.c [ ... ] > @@ -211,12 +205,76 @@ static int doscan(void *data) > trace_ifs_status(ifsd->cur_batch, start, stop, status.data); > > /* Pass back the result of the scan */ > - if (cpu == first) > + if (cpu == ifsd->cpu) > params->status = status; > > + pcpu_scan->test_details = status.data; > + > return 0; > } > > +#define get_scan_status(stat) ((union ifs_status *)(&((stat)->test_details))) > + > +static void update_group_status(struct device *dev, int reason) > +{ > + struct ifs_data *ifsd = ifs_get_data(dev); > + struct ifs_test_output *pcpu_scan; > + union ifs_status *scan_status; > + int lcpu; > + > + for_each_cpu(lcpu, &ifsd->grp_cpumask) { > + pcpu_scan = per_cpu_ptr(ifsd->result_ptr, lcpu); > + scan_status = get_scan_status(pcpu_scan); > + scan_status->error_code = reason; > + } > +} [Severity: Medium] Does update_group_status() in drivers/platform/x86/intel/ifs/runtest.c preserve stale bitfields in pcpu_scan->test_details from previous runs and clobber hardware error codes from the current run? In run_test_store() -> do_core_test() -> ifs_test_core(), the local status variable is zero-initialized (union ifs_status status = {};), which commit 3114f77e9453 ("platform/x86/intel/ifs: Initialize status in ifs_test_core()") added so that an early timeout before doscan() would not report stale status bits. However, the per-CPU pcpu_scan->test_details entries in ifsd->result_ptr are not cleared at the start of ifs_test_core(). If time_after(jiffies, timeout) triggers before doscan() runs on the first iteration, update_group_status(dev, IFS_SW_TIMEOUT) only overwrites scan_status->error_code, leaving signature_error, control_error, and chunk_num in pcpu_scan->test_details set to values from a previous test on those CPUs. When update_group_result() then evaluates the results: if (scan_status->signature_error) { pcpu_scan->test_result = SCAN_TEST_FAIL; message_fail(dev, lcpu, *scan_status); } else if (scan_status->control_error || scan_status->error_code || reached_chunk <= stop_chunk) { pcpu_scan->test_result = SCAN_NOT_TESTED; message_not_tested(dev, lcpu, *scan_status); } signature_error (and control_error inside message_not_tested()) are checked before error_code, so a stale signature_error or control_error from an earlier run will cause a software timeout to be misreported as SCAN_TEST_FAIL ("test signature incorrect") or a scan controller error. In addition, if a timeout or IFS_SW_PARTIAL_COMPLETION occurs after one or more loop iterations, update_group_status() unconditionally overwrites scan_status->error_code across all CPUs in ifsd->grp_cpumask without checking whether individual CPUs already recorded a hardware error_code or completed all chunks. Should pcpu_scan->test_details be cleared for all CPUs in ifsd->grp_cpumask at the start of ifs_test_core(), and should update_group_status() avoid overwriting non-zero hardware error codes? > + > +static bool can_restart_test_group(struct device *dev) > +{ > + struct ifs_data *ifsd = ifs_get_data(dev); > + struct ifs_test_output *pcpu_scan; > + union ifs_status *scan_status; > + int lcpu; > + > + for_each_cpu(lcpu, &ifsd->grp_cpumask) { > + pcpu_scan = per_cpu_ptr(ifsd->result_ptr, lcpu); > + scan_status = get_scan_status(pcpu_scan); > + if (!can_restart(*scan_status)) > + return false; > + } > + return true; > +} > + > +static void update_group_result(struct device *dev, int stop_chunk) > +{ > + struct ifs_data *ifsd = ifs_get_data(dev); > + struct ifs_test_output *pcpu_scan; > + union ifs_status *scan_status; > + u32 reached_chunk; > + int lcpu; > + > + for_each_cpu(lcpu, &ifsd->grp_cpumask) { > + pcpu_scan = per_cpu_ptr(ifsd->result_ptr, lcpu); > + scan_status = get_scan_status(pcpu_scan); > + reached_chunk = ifsd->generation ? scan_status->gen2.chunk_num : > + scan_status->gen0.chunk_num; > + > + if (scan_status->signature_error) { > + pcpu_scan->test_result = SCAN_TEST_FAIL; > + message_fail(dev, lcpu, *scan_status); > + } else if (scan_status->control_error || > scan_status->error_code || > + reached_chunk <= stop_chunk) { > + pcpu_scan->test_result = SCAN_NOT_TESTED; > + message_not_tested(dev, lcpu, *scan_status); > + } else { > + pcpu_scan->test_result = SCAN_TEST_PASS; > + } > + > + if (lcpu == ifsd->cpu) > + ifsd->status = pcpu_scan->test_result; > + } > +} [ ... ] > @@ -272,15 +332,17 @@ static void ifs_test_core(int cpu, struct device *dev) > status = params.status; > > /* Some cases can be retried, give up for others */ > - if (!can_restart(status)) > + if (!can_restart_test_group(dev)) > break; > > status_chunk = ifsd->generation ? status.gen2.chunk_num : > status.gen0.chunk_num; > if (status_chunk == to_start) { > /* Check for forward progress */ > if (--retries == 0) { > - if (status.error_code == IFS_NO_ERROR) > + if (status.error_code == IFS_NO_ERROR) { > status.error_code = > IFS_SW_PARTIAL_COMPLETION; > + update_group_status(dev, > IFS_SW_PARTIAL_COMPLETION); > + } > break; > } > } else { > @@ -295,16 +357,7 @@ static void ifs_test_core(int cpu, struct device *dev) > > /* Update status for this core */ > ifsd->scan_details = status.data; > - > - if (status.signature_error) { > - ifsd->status = SCAN_TEST_FAIL; > - message_fail(dev, cpu, status); > - } else if (status.control_error || status.error_code) { > - ifsd->status = SCAN_NOT_TESTED; > - message_not_tested(dev, cpu, status); > - } else { > - ifsd->status = SCAN_TEST_PASS; > - } > + update_group_result(dev, to_stop); > } [Severity: Medium] What happens in drivers/platform/x86/intel/ifs/runtest.c:ifs_test_core() and update_group_result() when CPUs in ifsd->grp_cpumask observe different scan statuses or stop at different chunks? In doscan(), every CPU in the group writes its MSR_SCAN_STATUS to pcpu_scan->test_details, but params->status is only written when cpu == ifsd->cpu. 1. In ifs_test_core(), can_restart_test_group(dev) checks can_restart() across all CPUs in ifsd->grp_cpumask, but status_chunk and to_start are computed solely from params.status (ifsd->cpu): status_chunk = ifsd->generation ? status.gen2.chunk_num : status.gen0.chunk_num; If ifsd->cpu advances to status_chunk (or finishes all chunks with status_chunk == to_stop + 1) while a sibling CPU in ifsd->grp_cpumask had a restartable abort at an earlier chunk C2 < status_chunk, to_start and activate.gen0/gen2.start are updated to ifsd->cpu's status_chunk. Doesn't this either exit the while (to_start <= to_stop) loop without retrying the sibling CPU, or restart the group scan at status_chunk and skip chunks C2 .. status_chunk - 1 on the sibling CPU (which update_group_result() will then mark SCAN_TEST_PASS if the final pass reaches stop_chunk + 1)? 2. When retries reaches 0 in ifs_test_core(), it only checks if (status.error_code == IFS_NO_ERROR) on ifsd->cpu before calling update_group_status(dev, IFS_SW_PARTIAL_COMPLETION), which overwrites error_code on all CPUs in the group even if a sibling CPU had a specific hardware error_code, or skips update_group_status() if ifsd->cpu had a non-zero error_code while a sibling had IFS_NO_ERROR. 3. In update_group_result(): } else if (scan_status->control_error || scan_status->error_code || reached_chunk <= stop_chunk) { pcpu_scan->test_result = SCAN_NOT_TESTED; message_not_tested(dev, lcpu, *scan_status); } The reached_chunk <= stop_chunk condition is evaluated when both scan_status->control_error == 0 and scan_status->error_code == IFS_NO_ERROR (0) (for example, when another CPU in the group aborted the scan via !can_restart_test_group(dev)). Calling message_not_tested() with status.error_code == 0 hits: if (status.error_code < ARRAY_SIZE(scan_test_status)) { dev_info(dev, "CPU %d: SCAN operation did not start. %s\n", cpu, scan_test_status[status.error_code]); and logs "CPU %d: SCAN operation did not start. SCAN no error". 4. Finally, update_group_result() only updates ifsd->status when lcpu == ifsd->cpu (and ifs_test_core() only sets ifsd->scan_details from ifsd->cpu). If a sibling CPU in ifsd->grp_cpumask encounters a signature_error (SCAN_TEST_FAIL) while ifsd->cpu does not, won't ifsd->status report SCAN_NOT_TESTED or SCAN_TEST_PASS instead of SCAN_TEST_FAIL? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
