Re: [PATCH 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 5 potential issue(s) to consider:

New issues:
- [Medium] The MPAM partition ID filter incorrectly conflates `partid == 0` with 'filter disabled', making it impossible to specifically monitor traffic for the default partition (Partition 0).
- [Medium] The driver fails to validate `config1` during event initialization, silently ignoring unsupported filter requests instead of rejecting them.

Pre-existing issues:
- [High] The probe function permanently mutates the platform device's memory resource, corrupting it and causing subsequent PMU probes to fail.
- [High] If the memory resource size is smaller than expected, an early loop termination leaves region pointers unmapped but sets `region_cnt` incorrectly, causing a kernel panic.
- [High] Registering the CPU hotplug instance before the PMU creates a race window where a concurrent CPU offline event triggers context migration on an uninitialized PMU context.
--

commit f8a0b0ecde1dfeec7798c8aca48869ad592602c8
Author: Tanmay Jagdale <tanmay@marvell.com>

perf: marvell: Add MPAM partid filtering to CN10K TAD PMU

The TAD PMU exposes counters that can be filtered by MPAM partition id
for a subset of allocation and hit events.

Add a 16-bit partid format attribute (config1) and route counter
programming through variant-specific ops so CN10K keeps MPAM-capable
programming while Odyssey keeps the reduced event set without
advertising partid in sysfs.

> diff --git a/drivers/perf/marvell_cn10k_tad_pmu.c b/drivers/perf/marvell_cn10k_tad_pmu.c
> index 51ccb0befa05d..7a3b659c999a2 100644
> --- a/drivers/perf/marvell_cn10k_tad_pmu.c
> +++ b/drivers/perf/marvell_cn10k_tad_pmu.c
[ ... ]
> @@ -27,30 +30,92 @@ struct tad_region {
>  	void __iomem	*base;
>  };
>  
[ ... ]
> +static void tad_pmu_start_counter(struct tad_pmu *pmu,
> +				  struct perf_event *event)
> +{
> +	const struct tad_pmu_data *pdata = pmu->pdata;
> +	struct hw_perf_event *hwc = &event->hw;
> +	u32 event_idx = event->attr.config;
> +	u32 counter_idx = hwc->idx;
> +	u64 partid_filter = 0;
> +	u64 reg_val;
> +	u32 partid;
> +	int i;
> +
> +	partid = (u32)(event->attr.config1 & GENMASK(15, 0));
> +
> +	for (i = 0; i < pmu->region_cnt; i++)
> +		writeq_relaxed(0, pmu->regions[i].base +
> +			       TAD_PFC(pdata->tad_pfc_offset, counter_idx));
> +
> +	if (partid && event_idx > 0x19 && event_idx < 0x21) {

[Severity: Medium]
Since an unconfigured perf_event_attr defaults to 0, using 0 as a sentinel
for "no filter" means explicit requests for Partition 0 will bypass the filter.

Can this be changed so that users can specifically monitor traffic for
Partition 0?

> +		partid_filter = TAD_PRF_MATCH_PARTID | TAD_PRF_PARTID_NS |
> +				((u64)partid << 11);
> +	}
[ ... ]
> @@ -242,9 +288,11 @@ static const struct attribute_group ody_tad_pmu_events_attr_group = {
>  };
>  
>  PMU_FORMAT_ATTR(event, "config:0-7");
> +PMU_FORMAT_ATTR(partid, "config1:0-15");

[Severity: Medium]
Is there missing validation for config1 in tad_pmu_event_init()?

Without it, the driver silently ignores unsupported filter requests instead of
rejecting them with -EINVAL, returning globally aggregated counts rather than
the requested isolated counts.

>  
>  static struct attribute *tad_pmu_format_attrs[] = {
>  	&format_attr_event.attr,
> +	&format_attr_partid.attr,
>  	NULL
>  };
[ ... ]
> @@ -344,7 +411,6 @@ static int tad_pmu_probe(struct platform_device *pdev)
>  	if (!regions)
>  		return -ENOMEM;
>  
> -	/* ioremap the distributed TAD pmu regions */
>  	for (i = 0; i < tad_cnt && res->start < res->end; i++) {
>  		regions[i].base = devm_ioremap(&pdev->dev,
>  					       res->start,

[Severity: High]
This is a pre-existing issue, but the probe function mutates the platform
device's memory resource (res->start += tad_page_size inside the loop).

If the driver is unbound and rebound, does this corrupted res->start cause
the loop condition to fail, leaving regions[i].base unpopulated and causing
a NULL pointer dereference later?

[Severity: High]
This is also a pre-existing issue, but if this loop terminates early because
the memory resource size is smaller than expected, tad_pmu->region_cnt is
later set to tad_cnt instead of the number of mapped regions (i).

Could this result in out-of-bounds NULL pointer dereferences during PMU
operations?

> @@ -374,14 +440,16 @@ static int tad_pmu_probe(struct platform_device *pdev)
>  		.read		= tad_pmu_event_counter_read,
>  	};
>  
> -	if (version == TAD_PMU_V1)
> +	if (version == TAD_PMU_V1) {
>  		tad_pmu->pmu.attr_groups = tad_pmu_attr_groups;
> -	else
> +		tad_pmu->ops		 = &tad_pmu_ops;
> +	} else {
>  		tad_pmu->pmu.attr_groups = ody_tad_pmu_attr_groups;
> +		tad_pmu->ops		 = &tad_pmu_v2_ops;
> +	}
>  
>  	tad_pmu->cpu = raw_smp_processor_id();
>  
> -	/* Register pmu instance for cpu hotplug */
>  	ret = cpuhp_state_add_instance_nocalls(tad_pmu_cpuhp_state,
>  					       &tad_pmu->node);

[Severity: High]
This is a pre-existing issue, but does registering the CPU hotplug instance
before the PMU is fully initialized create a race window?

If a CPU offline event happens before perf_pmu_register() completes,
could perf_pmu_migrate_context() dereference an unallocated
pmu->cpu_pmu_context, leading to a kernel panic?

>  	if (ret) {

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