Re: [PATCH v10 RESEND 7/9] perf: RISC-V: add support for SSE event

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] perf/riscv: flawed IRQ teardown logic on probe error when using SSE
--

commit 032c8cbfa5a61964759ab79256536a060b79e8f8
Author: Clément Léger <cleger@rivosinc.com>

perf: RISC-V: add support for SSE event

This commit registers a handler for the local PMU overflow SSE event so
that RISC-V perf can receive overflows when normal S-mode interrupts are
masked. It sets up the delivery mechanism once during PMU probe and
handles SSE delivery state transitions during CPU power management.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -1844,6 +1844,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>  	ret = pmu_sbi_setup_irqs(pmu, pdev);
>  	if (ret < 0) {
>  		pr_info("Perf sampling/filtering is not supported as sscof extension is not available\n");
>  		pmu->pmu.capabilities |= PERF_PMU_CAP_NO_INTERRUPT;
>  		pmu->pmu.capabilities |= PERF_PMU_CAP_NO_EXCLUDE;
>  	}
>  	irq_requested = (ret == 0);

[Severity: Low]
When pmu_sbi_setup_irqs() successfully configures SSE delivery, it returns 0
without mapping an IRQ. Does this mistakenly set irq_requested to true,
treating general delivery setup success as an IRQ allocation? Should this
condition check riscv_pmu_use_irq instead?

>  
> +#ifdef CONFIG_RISCV_PMU_SBI_SSE
> +	if (pmu->sse_active) {
> +		pmu->pmu.pmu_enable = pmu_sbi_sse_enable;
> +		pmu->pmu.pmu_disable = pmu_sbi_sse_disable;
> +	}
> +#endif

[ ... ]

> @@ -1885,8 +1895,8 @@ out_unregister:
>  	perf_pmu_unregister(&pmu->pmu);
>  
>  out_destroy:
>  	riscv_pmu_destroy(pmu);
>  	if (irq_requested) {
>  		free_percpu_irq(riscv_pmu_irq, pmu->hw_events);
>  		irq_dispose_mapping(riscv_pmu_irq);
>  		riscv_pmu_irq = 0;
>  	}

[Severity: Low]
If pmu_sbi_device_probe() fails later (such as in perf_pmu_register()) after
SSE delivery is configured, irq_requested will evaluate to true here.

Does this inadvertently cause the cleanup path to attempt to free and dispose
of an unallocated IRQ (with riscv_pmu_irq == 0)?

Although the IRQ subsystem safely mitigates this since irq_dispose_mapping(0)
ignores virq == 0 and free_percpu_irq(0) safely returns early, could this
flawed logic be corrected by verifying riscv_pmu_use_irq instead of the
ret == 0 success flag?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789974241.git.zhangzhanpeng.jasper@bytedance.com?part=7