[PATCH] lib: sbi_pmu: Fix multiple FW counter start operations with custom PMU device

[PATCH] lib: sbi_pmu: Fix multiple FW counter start operations with custom PMU device

[James Raphael Tiovalen]

Currently, we immediately return the result of `fw_counter_start` if the
event code is 0xFFFF. However, this skips setting the bit in the
`fw_counters_started` bitmap even if the platform-specific call
succeeds. Restore the original behavior of returning early only on an
error so that we still set the bit in the bitmap. This prevents multiple
starts of the same FW counter. This also aligns the expectations of
`pmu_ctr_start_fw` with `pmu_ctr_stop_fw` since we cannot assume that
the platform-specific functions to start and stop FW counters will
modify the bitmap state.

Fixes: 57d3aa3b0dbd ("lib: sbi_pmu: Introduce fw_counter_write_value API")
Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 lib/sbi/sbi_pmu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
index e084005d..75038f91 100644
--- a/lib/sbi/sbi_pmu.c
+++ b/lib/sbi/sbi_pmu.c
@@ -448,6 +448,8 @@ static int pmu_ctr_start_fw(struct sbi_pmu_hart_state *phs,
 			    uint64_t event_data, uint64_t ival,
 			    bool ival_update)
 {
+	int ret;
+
 	if ((event_code >= SBI_PMU_FW_MAX &&
 	    event_code <= SBI_PMU_FW_RESERVED_MAX) ||
 	    event_code > SBI_PMU_FW_PLATFORM)
@@ -468,9 +470,11 @@ static int pmu_ctr_start_fw(struct sbi_pmu_hart_state *phs,
 							cidx - num_hw_ctrs,
 							ival);
 
-		return pmu_dev->fw_counter_start(phs->hartid,
+		ret = pmu_dev->fw_counter_start(phs->hartid,
 						 cidx - num_hw_ctrs,
 						 event_data);
+		if (ret)
+			return ret;
 	} else {
 		if (ival_update)
 			phs->fw_counters_data[cidx - num_hw_ctrs] = ival;
-- 
2.43.0


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH] lib: sbi_pmu: Fix multiple FW counter start operations with custom PMU device

[Anup Patel]

On Fri, Jan 16, 2026 at 10:23 PM James Raphael Tiovalen
<jamestiotio@gmail.com> wrote:
>
> Currently, we immediately return the result of `fw_counter_start` if the
> event code is 0xFFFF. However, this skips setting the bit in the
> `fw_counters_started` bitmap even if the platform-specific call
> succeeds. Restore the original behavior of returning early only on an
> error so that we still set the bit in the bitmap. This prevents multiple
> starts of the same FW counter. This also aligns the expectations of
> `pmu_ctr_start_fw` with `pmu_ctr_stop_fw` since we cannot assume that
> the platform-specific functions to start and stop FW counters will
> modify the bitmap state.
>
> Fixes: 57d3aa3b0dbd ("lib: sbi_pmu: Introduce fw_counter_write_value API")
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>

LGTM.

Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
>  lib/sbi/sbi_pmu.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> index e084005d..75038f91 100644
> --- a/lib/sbi/sbi_pmu.c
> +++ b/lib/sbi/sbi_pmu.c
> @@ -448,6 +448,8 @@ static int pmu_ctr_start_fw(struct sbi_pmu_hart_state *phs,
>                             uint64_t event_data, uint64_t ival,
>                             bool ival_update)
>  {
> +       int ret;
> +
>         if ((event_code >= SBI_PMU_FW_MAX &&
>             event_code <= SBI_PMU_FW_RESERVED_MAX) ||
>             event_code > SBI_PMU_FW_PLATFORM)
> @@ -468,9 +470,11 @@ static int pmu_ctr_start_fw(struct sbi_pmu_hart_state *phs,
>                                                         cidx - num_hw_ctrs,
>                                                         ival);
>
> -               return pmu_dev->fw_counter_start(phs->hartid,
> +               ret = pmu_dev->fw_counter_start(phs->hartid,
>                                                  cidx - num_hw_ctrs,
>                                                  event_data);
> +               if (ret)
> +                       return ret;
>         } else {
>                 if (ival_update)
>                         phs->fw_counters_data[cidx - num_hw_ctrs] = ival;
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi