Re: [PATCH v6 11/21] RISC-V: perf: Modify the counter discovery mechanism

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Negative error codes returned by SBI are improperly cast to an unsigned integer, and systems with >64 counters are incorrectly aborted instead of clamped.
--

commit ea0f7376968d4f5943d9d279886b0b59e6dfd41c
Author: Atish Patra <atishp@rivosinc.com>
Subject: RISC-V: perf: Modify the counter discovery mechanism

This commit modifies the PMU counter discovery mechanism to use a hybrid
scheme. It utilizes counter delegation for hardware PMU counters while falling
back to the SBI PMU for firmware counters, and updates logging appropriately.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 7f21c16003f04..57ab15beab3e1 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c

[ ... ]

> @@ -1159,16 +1180,42 @@ static void rvpmu_ctr_stop(struct perf_event *event, unsigned long flag)
>  	/* TODO: Counter delegation implementation */
>  }
>  
> -static int rvpmu_find_num_ctrs(void)
> +static int rvpmu_find_ctrs(void)
>  {
> -	return rvpmu_sbi_find_num_ctrs();
> -	/* TODO: Counter delegation implementation */
> -}
> +	u32 num_sbi_counters = 0, num_deleg_counters = 0;
> +	u32 num_hw_ctr = 0, num_fw_ctr = 0, num_ctr = 0;
> +	/*
> +	 * We don't know how many firmware counters are available. Just allocate
> +	 * for maximum counters the driver can support. The default is 64 anyways.
> +	 */
> +	pmu_ctr_list = kcalloc(RISCV_MAX_COUNTERS, sizeof(*pmu_ctr_list),
> +			       GFP_KERNEL);
> +	if (!pmu_ctr_list)
> +		return -ENOMEM;
>  
> -static int rvpmu_get_ctrinfo(int nctr, unsigned long *mask)
> -{
> -	return rvpmu_sbi_get_ctrinfo(nctr, mask);
> -	/* TODO: Counter delegation implementation */
> +	if (riscv_pmu_cdeleg_available_boot())
> +		num_deleg_counters = rvpmu_deleg_find_ctrs();
> +
> +	/* This is required for firmware counters even if the above is true */
> +	if (riscv_pmu_sbi_available_boot())
> +		num_sbi_counters = rvpmu_sbi_find_num_ctrs();
> +
> +	if (num_sbi_counters > RISCV_MAX_COUNTERS || num_deleg_counters > RISCV_MAX_COUNTERS)
> +		return -ENOSPC;

[Severity: High]
Does this code inadvertently convert negative error codes from
rvpmu_sbi_find_num_ctrs() into large unsigned values?

Since num_sbi_counters is defined as a u32, if rvpmu_sbi_find_num_ctrs()
returns an error code like -EOPNOTSUPP, it will wrap to a large positive
integer, instantly satisfying the > RISCV_MAX_COUNTERS condition. This seems
to mask the original error code.

If the > RISCV_MAX_COUNTERS condition is met, the function returns -ENOSPC
early. Can this leak the pmu_ctr_list allocated via kcalloc() just above,
since there is no free before the return?

Additionally, could this introduce a regression for platforms legitimately
exposing more than 64 counters? Previously, the driver appeared to safely
clamp the usable count to 64 and continue, whereas this new logic seems to
abort the driver probe entirely.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-counter_delegation-v6-0-285b72ed65a9@meta.com?part=11