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

[Sebastian Alba Vives <sebasjosue84@gmail.com>]

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.

Fixes: 13d40f21d588 ("lib: sbi: Add PMU support")
Signed-off-by: Sebastian Alba Vives <sebasjosue84@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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi