From: sashiko-bot@kernel.org
To: "Ehab Ababneh" <ehab.ababneh@intel.com>
Cc: linux-trace-kernel@vger.kernel.org
Subject: Re: [PATCH 11/17] platform/x86/intel/ifs: Track and report scan results on a per-CPU basis
Date: Mon, 26 Oct 2026 18:54:11 +0000 [thread overview]
Message-ID: <sashiko-outbox-185646@kernel.org> (raw)
In-Reply-To: <20261026183820.129459-12-ehab.ababneh@intel.com>
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 <jithu.joseph@intel.com>
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/20261026183820.129459-1-ehab.ababneh@intel.com?part=11
next prev parent reply other threads:[~2026-10-26 18:54 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-26 18:38 [PATCH 00/17] platform/x86/intel/ifs: Support group-based In-Field Scan and Diamond Rapids Ehab Ababneh
2026-10-26 18:38 ` [PATCH 01/17] platform/x86/intel/ifs: Revert SBAF feature Ehab Ababneh
2026-10-26 18:38 ` [PATCH 02/17] platform/x86/intel/ifs: Fix IFS sysfs interfaces in the comments Ehab Ababneh
2026-10-26 18:38 ` [PATCH 03/17] platform/x86/intel/ifs: Clarify In-Field Scan MSR bit macro definitions Ehab Ababneh
2026-10-26 18:38 ` [PATCH 04/17] platform/x86/intel/ifs: Use u64 to define the bits of IFS MSR registers Ehab Ababneh
2026-10-26 18:38 ` [PATCH 05/17] platform/x86/intel/ifs: Early return on the IFS image hash/chunk error Ehab Ababneh
2026-10-26 18:38 ` [PATCH 06/17] platform/x86/intel/ifs: Refactor Array BIST function interface Ehab Ababneh
2026-10-26 18:38 ` [PATCH 07/17] stop_machine: Add stop_cluster_cpuslocked() for per-cluster operations Ehab Ababneh
2026-10-26 18:38 ` [PATCH 08/17] platform/x86/intel/ifs: Enumerate the bit to determine if CPUs need to join for IFS tests Ehab Ababneh
2026-10-26 18:38 ` [PATCH 09/17] platform/x86/intel/ifs: Select CPUs from topology_cluster_cpumask() for simultaneous " Ehab Ababneh
2026-10-26 18:47 ` sashiko-bot
2026-10-26 18:38 ` [PATCH 10/17] platform/x86/intel/ifs: Validate TID_BIT_SHIFT matches topology_cluster_cpumask() Ehab Ababneh
2026-10-26 18:49 ` sashiko-bot
2026-10-26 18:38 ` [PATCH 11/17] platform/x86/intel/ifs: Track and report scan results on a per-CPU basis Ehab Ababneh
2026-10-26 18:54 ` sashiko-bot [this message]
2026-10-26 18:38 ` [PATCH 12/17] platform/x86/intel/ifs: Add new sysfs interfaces {status, details, cpus_ran}_list Ehab Ababneh
2026-10-26 18:49 ` sashiko-bot
2026-10-26 18:38 ` [PATCH 13/17] platform/x86/intel/ifs: Add additional details sysfs interface Ehab Ababneh
2026-10-26 18:38 ` [PATCH 14/17] platform/x86/intel/ifs: Add Intel Diamond Rapids server support Ehab Ababneh
2026-10-26 18:38 ` [PATCH 15/17] platform/x86/intel/ifs: Remove limit on number of batch files Ehab Ababneh
2026-10-26 18:38 ` [PATCH 16/17] platform/x86/intel/ifs: Update IFS documentation Ehab Ababneh
2026-10-26 18:58 ` sashiko-bot
2026-10-26 18:38 ` [PATCH 17/17] Documentation/ABI: Update IFS ABI doc Ehab Ababneh
2026-10-26 18:54 ` sashiko-bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=sashiko-outbox-185646@kernel.org \
--to=sashiko-bot@kernel.org \
--cc=ehab.ababneh@intel.com \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=sashiko-reviews@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox