Re: [PATCH v4 1/3] perf: marvell: Add MPAM partid filtering to CN10K TAD PMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `perf_ready` early return in `tad_pmu_offline_cpu()` completely skips updating `pmu->cpu`, potentially pinning the PMU to an offline CPU permanently.
- [Medium] The commit message contradicts the implementation regarding the registration order of the PMU and CPU hotplug instance.
--

commit c5919d39926621f8d8420dda8d78271eeeb122e2
Author: Tanmay Jagdale <tanmay@marvell.com>
Subject: perf: marvell: Add MPAM partid filtering to CN10K TAD PMU

This commit adds MPAM partition ID filtering to the CN10K TAD PMU. It updates
the counter programming to use MPAM-capable filtering for specific events. It
also modifies the probe function's MMIO mapping and attempts to address
hotplug registration races.

> Probe no longer mutates the platform_device MMIO resource (walk a local
> map_start), rejects tad-cnt / page sizes of zero, validates the memory
> window against tad-cnt, and registers the perf PMU before hotplug with
> correct unwind.

[Severity: Medium]
The commit message states that it "registers the perf PMU before hotplug
with correct unwind", but looking at tad_pmu_probe(), the hotplug instance
is still registered before the perf PMU:

	ret = cpuhp_state_add_instance_nocalls(tad_pmu_cpuhp_state,
					       &tad_pmu->node);
	if (ret) {
		dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
		return ret;
	}

	ret = perf_pmu_register(&tad_pmu->pmu, "tad", -1);

Could the commit message be updated, or was the code reordering missed?

> diff --git a/drivers/perf/marvell_cn10k_tad_pmu.c b/drivers/perf/marvell_cn10k_tad_pmu.c
> index 51ccb0befa05d..340be3776fe75 100644
> --- a/drivers/perf/marvell_cn10k_tad_pmu.c
> +++ b/drivers/perf/marvell_cn10k_tad_pmu.c
[ ... ]
> @@ -451,6 +570,9 @@ static int tad_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
>  	struct tad_pmu *pmu = hlist_entry_safe(node, struct tad_pmu, node);
>  	unsigned int target;
>  
> +	if (!READ_ONCE(pmu->perf_ready))
> +		return 0;
> +

[Severity: High]
If the originally assigned CPU goes offline during the race window between
cpuhp_state_add_instance_nocalls() and the setting of perf_ready = true in
tad_pmu_probe(), does this early return skip reassigning pmu->cpu?

Since this driver does not define an online CPU hotplug callback, if
tad_pmu_offline_cpu() returns early here, could pmu->cpu permanently remain
assigned to the offline CPU and break the PMU once registration finishes?

>  	if (cpu != pmu->cpu)
>  		return 0;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618153610.13649-1-gakula@marvell.com?part=1