diff mbox series

lib: sbi: sbi_pmu: Fix multiple start and stop operations of FW counters

Message ID 20251213104146.422972-1-jamestiotio@gmail.com
State Accepted
Headers show
Series lib: sbi: sbi_pmu: Fix multiple start and stop operations of FW counters | expand

Commit Message

James R T Dec. 13, 2025, 10:41 a.m. UTC
Currently, OpenSBI returns SBI_ERR_ALREADY_STARTED when attempting to
start a HW counter that is already started and SBI_ERR_ALREADY_STOPPED
when attempting to stop a HW counter that is already stopped. However,
this is not yet implemented for FW counters.

Add the necessary checks to return the same error codes when attempting
the same actions on FW counters.

Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 lib/sbi/sbi_pmu.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Anup Patel Dec. 27, 2025, 9:09 a.m. UTC | #1
On Sat, Dec 13, 2025 at 4:12 PM James Raphael Tiovalen
<jamestiotio@gmail.com> wrote:
>
> Currently, OpenSBI returns SBI_ERR_ALREADY_STARTED when attempting to
> start a HW counter that is already started and SBI_ERR_ALREADY_STOPPED
> when attempting to stop a HW counter that is already stopped. However,
> this is not yet implemented for FW counters.
>
> Add the necessary checks to return the same error codes when attempting
> the same actions on FW counters.
>
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/sbi/sbi_pmu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> index e24e485d..0218c921 100644
> --- a/lib/sbi/sbi_pmu.c
> +++ b/lib/sbi/sbi_pmu.c
> @@ -468,6 +468,9 @@ static int pmu_ctr_start_fw(struct sbi_pmu_hart_state *phs,
>                                                  cidx - num_hw_ctrs,
>                                                  event_data);
>         } else {
> +               if (phs->fw_counters_started & BIT(cidx - num_hw_ctrs))
> +                       return SBI_EALREADY_STARTED;
> +

This check should also be done for platform firmware counters
hence it should be just before this if-else block.

>                 if (ival_update)
>                         phs->fw_counters_data[cidx - num_hw_ctrs] = ival;
>         }
> @@ -629,6 +632,9 @@ static int pmu_ctr_stop_fw(struct sbi_pmu_hart_state *phs,
>                         return ret;
>         }
>
> +       if (!(phs->fw_counters_started & BIT(cidx - num_hw_ctrs)))
> +                       return SBI_EALREADY_STOPPED;
> +

As comment as above, this check should be before the previous
if block.

>         phs->fw_counters_started &= ~BIT(cidx - num_hw_ctrs);
>
>         return 0;
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

I have taken care of the above comments at the time of
merging this patch.

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

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup
diff mbox series

Patch

diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
index e24e485d..0218c921 100644
--- a/lib/sbi/sbi_pmu.c
+++ b/lib/sbi/sbi_pmu.c
@@ -468,6 +468,9 @@  static int pmu_ctr_start_fw(struct sbi_pmu_hart_state *phs,
 						 cidx - num_hw_ctrs,
 						 event_data);
 	} else {
+		if (phs->fw_counters_started & BIT(cidx - num_hw_ctrs))
+			return SBI_EALREADY_STARTED;
+
 		if (ival_update)
 			phs->fw_counters_data[cidx - num_hw_ctrs] = ival;
 	}
@@ -629,6 +632,9 @@  static int pmu_ctr_stop_fw(struct sbi_pmu_hart_state *phs,
 			return ret;
 	}
 
+	if (!(phs->fw_counters_started & BIT(cidx - num_hw_ctrs)))
+			return SBI_EALREADY_STOPPED;
+
 	phs->fw_counters_started &= ~BIT(cidx - num_hw_ctrs);
 
 	return 0;