[PATCH 1/2] lib: sbi_pmu: do not ignore pmu_ctr_start_hw() error in cfg_match

[Anup Patel]

On Sun, Oct 25, 2026 at 11:59 AM Sebastian Alba Vives
<sebasjosue84 at gmail.com> wrote:
>
> sbi_pmu_ctr_cfg_match() calls pmu_ctr_start_hw() for the AUTO_START
> flag and drops the result. pmu_ctr_start_hw() carries its own index
> guard
>
>         if (cidx >= num_hw_ctrs || cidx == 1)
>                 return SBI_EINVAL;
>
> so a rejected index is reported to the caller as a successful
> configure.
>
> This is the only one of the three start paths in the file that
> discards the error. sbi_pmu_ctr_start() propagates it:
>
>         ret = pmu_ctr_start_hw(cidx, ival, bUpdate);
>         if (ret)
>                 return ret;
>
> and the firmware branch directly below this call site propagates the
> error from pmu_dev->fw_counter_start() as well.
>
> Propagate the error, except SBI_EALREADY_STARTED. That value only
> says the mcountinhibit bit was already clear, which is a benign
> condition for a configure call, and failing the whole call because
> the counter was already running would be a behaviour change for
> existing supervisors.

On the contrary, the configure should fail if the counter was already
running because the counter might have been configured for a different
event previously.

Otherwise, this looks good.

Regards,
Anup

>
> Fixes: 13d40f21d588 ("lib: sbi: Add PMU support")
> Signed-off-by: Sebastian Alba Vives <sebasjosue84 at gmail.com>
> ---
>
> Two things I kept out of the commit message because they are design
> questions rather than part of the fix.
>
> I filtered SBI_EALREADY_STARTED on the assumption that a configure
> call should not fail just because the counter was already running. If
> you would rather have every value propagated, the change is to drop
> the second half of the condition.
>
> The other is partial success. By the time this call site runs,
> phs->active_events[ctr_idx] has already been written, so returning an
> error leaves the counter configured while reporting a failed configure
> to the supervisor. I did not add a rollback because that goes beyond
> what you asked for, but I can send that instead if you would rather
> the call be all-or-nothing.
>
>  lib/sbi/sbi_pmu.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> index 653b8f69..62d5e206 100644
> --- a/lib/sbi/sbi_pmu.c
> +++ b/lib/sbi/sbi_pmu.c
> @@ -982,8 +982,11 @@ skip_match:
>             event_type == SBI_PMU_EVENT_TYPE_HW_RAW_V2) {
>                 if (flags & SBI_PMU_CFG_FLAG_CLEAR_VALUE)
>                         pmu_ctr_write_hw(ctr_idx, 0);
> -               if (flags & SBI_PMU_CFG_FLAG_AUTO_START)
> -                       pmu_ctr_start_hw(ctr_idx, 0, false);
> +               if (flags & SBI_PMU_CFG_FLAG_AUTO_START) {
> +                       ret = pmu_ctr_start_hw(ctr_idx, 0, false);
> +                       if (ret && ret != SBI_EALREADY_STARTED)
> +                               return ret;
> +               }
>         } else if (event_type == SBI_PMU_EVENT_TYPE_FW) {
>                 if (flags & SBI_PMU_CFG_FLAG_CLEAR_VALUE)
>                         phs->fw_counters_data[ctr_idx - num_hw_ctrs] = 0;
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi