Re: [PATCH 2/3] perf: marvell: Add CN20K LLC-TAD PMU support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] The shifted `partid` value overlaps with the hardcoded Non-Secure (`NS`) flag on CN20K (V3), corrupting the filter configuration.
- [Medium] The driver shares the PMU events attribute array between CN10K (V1) and CN20K (V3) without visibility checks, exposing unsupported V3 events on V1 systems and breaking the pre-existing `tad_tot_cycle` (0xFF) event.
- [Medium] The PMU driver prevents users from profiling MPAM PARTID 0, as it overloads `partid == 0` to mean 'filtering disabled'.

Pre-existing issues:
- [High] Malformed ACPI/DT memory resources cause NULL pointer dereferences if they are smaller than `tad_cnt`, as the driver unconditionally assigns the expected count instead of the mapped count.
- [High] The driver modifies the global platform_device resource bounds during probe, corrupting subsequent ioremap attempts and causing driver rebinds to fail.
- [High] Missing `hwc->prev_count` reset when starting counters leads to bogus perf event counts upon read.
--

commit db735a497cd5d9c282e3c5a1fa6ad9ee8d6ea442
Author: Geetha sowjanya <gakula@marvell.com>
Subject: perf: marvell: Add CN20K LLC-TAD PMU support

Adds support for the CN20K LLC-TAD PMU by defining a TAD_PMU_V3 profile
with per-platform register bases. It extends the CN10K event map for new
counters and matches the block via OF and ACPI.

> diff --git a/drivers/perf/marvell_cn10k_tad_pmu.c b/drivers/perf/marvell_cn10k_tad_pmu.c
> index 7a3b659c999a2..4daf109b89eea 100644
> --- a/drivers/perf/marvell_cn10k_tad_pmu.c
> +++ b/drivers/perf/marvell_cn10k_tad_pmu.c
[ ... ]
> @@ -80,8 +83,14 @@ static void tad_pmu_start_counter(struct tad_pmu *pmu,
>  	if (partid && event_idx > 0x19 && event_idx < 0x21) {

[Severity: High]
This is a pre-existing issue, but does tad_pmu_event_counter_start() fail to
reset the software prev_count when starting the counters?

When the hardware counters are zeroed out earlier in tad_pmu_start_counter(),
local64_set(&hwc->prev_count, 0) is not called to match. When the counter is
later stopped or read, computing new minus prev can underflow because prev
still holds the value from the previous time the event was stopped.

[Severity: Medium]
Does checking if partid is non-zero prevent users from profiling MPAM PARTID 0?

If a user explicitly specifies partid 0, which is a valid MPAM partition, it
evaluates to false. This bypasses the hardware filter configuration entirely,
causing the hardware to count events for all partitions instead of exclusively
restricting to PARTID 0.

>  		partid_filter = TAD_PRF_MATCH_PARTID | TAD_PRF_PARTID_NS |
>  				((u64)partid << 11);
> +
> +		if (pdata->id == TAD_PMU_V3)
> +			partid_filter = TAD_PRF_MATCH_PARTID | BIT_ULL(25) |
> +				((u64)partid << 10);

[Severity: High]
Does shifting a 16-bit partid by 10 cause a bit collision with the hardcoded
Non-Secure flag at bit 25?

Because BIT_ULL(25) occupies the exact same bit position as the most
significant bit (bit 15) of the shifted 16-bit partid, any user-provided
partid with bit 15 set might corrupt the hardware filter value.

>  	}
>  
> +	if (pdata->id == TAD_PMU_V1 && event_idx > 0x24)
> +		return;

[Severity: Medium]
Does this early return prevent tad_tot_cycle (0xFF) from working on V1
hardware?

Because 0xFF is greater than 0x24, attempting to measure the total cycle
event on V1 will be silently ignored, leaving the counter unprogrammed.

>  
>  	for (i = 0; i < pmu->region_cnt; i++) {
>  		reg_val = event_idx & 0xFF;
[ ... ]
> @@ -261,6 +270,18 @@ static struct attribute *tad_pmu_event_attrs[] = {
>  	TAD_PMU_EVENT_ATTR(tad_dat_rd_byp, 0x22),
>  	TAD_PMU_EVENT_ATTR(tad_ifb_occ, 0x23),
>  	TAD_PMU_EVENT_ATTR(tad_req_occ, 0x24),
> +	TAD_PMU_EVENT_ATTR(tad_req_msh_out_dtg_evict, 0x25),
> +	TAD_PMU_EVENT_ATTR(tad_req_msh_out_ltg_evict, 0x26),
> +	TAD_PMU_EVENT_ATTR(tad_rsp_msh_out_mpam, 0x28),
> +	TAD_PMU_EVENT_ATTR(tad_replays, 0x29),
> +	TAD_PMU_EVENT_ATTR(tad_req_byp0, 0x2a),
> +	TAD_PMU_EVENT_ATTR(tad_req_byp1, 0x2b),
> +	TAD_PMU_EVENT_ATTR(tad_txreq_byp, 0x2c),
> +	TAD_PMU_EVENT_ATTR(tad_time_in_dslp, 0x2d),
> +	TAD_PMU_EVENT_ATTR(tad_time_elapsed, 0x2e),
> +	TAD_PMU_EVENT_ATTR(tad_req_msh_out_dss_rd_128mrg, 0x2f),
> +	TAD_PMU_EVENT_ATTR(tad_req_msh_out_dss_wr_128mrg, 0x30),
> +	TAD_PMU_EVENT_ATTR(tad_tot_cycle, 0xff),
>  	NULL
>  };

[Severity: Medium]
Since these V3-specific events are added to the shared attribute array
without an is_visible callback, are they incorrectly advertised to
userspace on V1 hardware?

[ ... ]
> @@ -440,7 +460,7 @@ 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 || version == TAD_PMU_V3) {
>  		tad_pmu->pmu.attr_groups = tad_pmu_attr_groups;
>  		tad_pmu->ops		 = &tad_pmu_ops;
>  	} else {

[Severity: High]
This is a pre-existing issue, but in the region initialization loop earlier
in tad_pmu_probe():

for (i = 0; i < tad_cnt && res->start < res->end; i++) {
    ...
}
tad_pmu->region_cnt = tad_cnt;

If the loop terminates early due to the resource size being smaller than
expected, region_cnt is unconditionally assigned tad_cnt instead of the
successfully mapped count (i). Does this lead to NULL pointer dereferences
later when PMU operations iterate up to region_cnt and access the
uninitialized base pointers?

[Severity: High]
This is another pre-existing issue, but in that same tad_pmu_probe()
initialization loop:

for (i = 0; i < tad_cnt && res->start < res->end; i++) {
    ...
    res->start += tad_page_size;
}

Does modifying res->start directly corrupt the global platform_device
resource bounds?

Since platform_get_resource() returns a direct pointer to the device's
resource descriptor, permanently altering it means subsequent ioremap
attempts will start from the corrupted offset if the driver module is
reloaded or if probe defers.

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