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

[Atish Patra <atish.patra@linux.dev>]

On 6/8/26 11:17 PM, sashiko-bot@kernel.org wrote:
> 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?

Valid. Fixed in v7.

> 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.

That's the current max counter supported by the driver. So it is fine to 
abort

until such a case arises to avoid confusion.

> [ ... ]
>