[PATCH v5 03/15] ACPI: CPPC: Propagate performance-control write errors

[Christian Loehle]

cppc_set_perf() can skip malformed controls, discard cpc_write() failures,
and report success without programming the requested performance tuple.

Return every control-write error to the caller. For mixed PCC and non-PCC
performance controls, complete all requested non-PCC writes before taking
PCC ownership or changing its payload. A non-PCC failure therefore cannot
submit only the PCC portion of a request. Once ownership is held, stage
only PCC controls and mark the command pending after successful staging.

Treat zero Minimum and Maximum Performance as values rather than omitted
updates. Lowest Performance may legitimately be zero, and skipping such a
request can leave a stale nonzero minimum programmed. All in-tree callers
provide both bounds when calling cppc_set_perf().

Validate every requested PCC field before the first payload store. The
current PCC accessor can fail only for a malformed type or unsupported
width, and the descriptor type is already known to be writable. Checking
all widths up front prevents a later malformed field from leaving an
earlier value in shared memory for a subsequent doorbell to commit.

Multiple Phase-I callers may set the pending flag to true while holding the
shared side of pcc_lock. Mark this intentional same-value store with
WRITE_ONCE(); transitions back to false remain protected by the exclusive
side.

If PCC staging fails while another CPU has already staged a request, take
the exclusive PCC lock and abort the pending batch before returning. This
advances the write generation and wakes Phase-II waiters which would
otherwise wait indefinitely for a doorbell that no CPU will ring.

Cross-address-space updates cannot be atomic, but this ordering ensures a
known non-PCC failure never commits the PCC portion by itself.

Fixes: 337aadff8e45 ("ACPI: Introduce CPU performance controls using CPPC")
Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
Link: https://sashiko.dev/#/patchset/20260807111303.1062391-1-christian.loehle%40arm.com
Signed-off-by: Christian Loehle <christian.loehle@xxxxxxx>
---
 drivers/acpi/cppc_acpi.c | 143 ++++++++++++++++++++++++++++-----------
 1 file changed, 104 insertions(+), 39 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index d9d5c877b9fd..3fbfb2eea6cf 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -233,6 +233,19 @@ show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time);
 			     (reg)->space_id != ACPI_ADR_SPACE_PLATFORM_COMM) ? \
 			    (8 << ((reg)->access_width - 1)) : (reg)->bit_width)
 
+static bool cpc_pcc_write_supported(const struct cpc_register_resource *reg)
+{
+	switch (GET_BIT_WIDTH(&reg->cpc_entry.reg)) {
+	case 8:
+	case 16:
+	case 32:
+	case 64:
+		return true;
+	default:
+		return false;
+	}
+}
+
 /* Shift and apply the mask for CPC reads/writes */
 #define MASK_VAL_READ(reg, val) (((val) >> (reg)->bit_offset) &				\
 					GENMASK(((reg)->bit_width) - 1, 0))
@@ -373,13 +386,45 @@ static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
 	return ret;
 }
 
+static void cppc_complete_pcc_write(struct cppc_pcc_data *pcc_ss_data,
+				    int ret)
+{
+	int i;
+
+	if (unlikely(ret)) {
+		for_each_possible_cpu(i) {
+			struct cpc_desc *desc = per_cpu(cpc_desc_ptr, i);
+
+			if (!desc)
+				continue;
+
+			if (desc->write_cmd_id == pcc_ss_data->pcc_write_cnt)
+				desc->write_cmd_status = ret;
+		}
+	}
+
+	pcc_ss_data->pcc_write_cnt++;
+	wake_up_all(&pcc_ss_data->pcc_write_wait_q);
+}
+
+/* The caller must hold pcc_lock for write. */
+static void cppc_abort_pending_pcc_write(struct cppc_pcc_data *pcc_ss_data,
+					 int ret)
+{
+	if (!pcc_ss_data->pending_pcc_write_cmd)
+		return;
+
+	pcc_ss_data->pending_pcc_write_cmd = false;
+	cppc_complete_pcc_write(pcc_ss_data, ret);
+}
+
 /*
  * This function transfers the ownership of the PCC to the platform
  * So it must be called while holding write_lock(pcc_lock)
  */
 static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
 {
-	int ret = -EIO, i;
+	int ret = -EIO;
 	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
 					pcc_ss_data->pcc_channel->shmem;
@@ -471,21 +516,8 @@ static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
 		mbox_client_txdone(pcc_ss_data->pcc_channel->mchan, ret);
 
 end:
-	if (cmd == CMD_WRITE) {
-		if (unlikely(ret)) {
-			for_each_possible_cpu(i) {
-				struct cpc_desc *desc = per_cpu(cpc_desc_ptr, i);
-
-				if (!desc)
-					continue;
-
-				if (desc->write_cmd_id == pcc_ss_data->pcc_write_cnt)
-					desc->write_cmd_status = ret;
-			}
-		}
-		pcc_ss_data->pcc_write_cnt++;
-		wake_up_all(&pcc_ss_data->pcc_write_wait_q);
-	}
+	if (cmd == CMD_WRITE)
+		cppc_complete_pcc_write(pcc_ss_data, ret);
 
 	return ret;
 }
@@ -2164,7 +2196,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	struct cpc_register_resource *desired_reg, *min_perf_reg, *max_perf_reg;
 	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
 	struct cppc_pcc_data *pcc_ss_data = NULL;
-	bool regs_in_pcc;
+	bool desired_pcc, min_pcc, max_pcc, regs_in_pcc;
 	int ret = 0;
 
 	if (!cpc_desc) {
@@ -2175,8 +2207,31 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
 	min_perf_reg = &cpc_desc->cpc_regs[MIN_PERF];
 	max_perf_reg = &cpc_desc->cpc_regs[MAX_PERF];
-	regs_in_pcc = CPC_IN_PCC(desired_reg) || CPC_IN_PCC(min_perf_reg) ||
-		      CPC_IN_PCC(max_perf_reg);
+	desired_pcc = cpc_is_writable(desired_reg) && CPC_IN_PCC(desired_reg);
+	min_pcc = cpc_is_writable(min_perf_reg) && CPC_IN_PCC(min_perf_reg);
+	max_pcc = cpc_is_writable(max_perf_reg) && CPC_IN_PCC(max_perf_reg);
+	regs_in_pcc = desired_pcc || min_pcc || max_pcc;
+	if ((desired_pcc && !cpc_pcc_write_supported(desired_reg)) ||
+	    (min_pcc && !cpc_pcc_write_supported(min_perf_reg)) ||
+	    (max_pcc && !cpc_pcc_write_supported(max_perf_reg)))
+		return -EFAULT;
+
+	/* Do not stage PCC data if a fallible non-PCC write has failed. */
+	if (cpc_is_writable(desired_reg) && !desired_pcc) {
+		ret = cpc_write(cpu, desired_reg, perf_ctrls->desired_perf);
+		if (ret)
+			return ret;
+	}
+	if (cpc_is_writable(min_perf_reg) && !min_pcc) {
+		ret = cpc_write(cpu, min_perf_reg, perf_ctrls->min_perf);
+		if (ret)
+			return ret;
+	}
+	if (cpc_is_writable(max_perf_reg) && !max_pcc) {
+		ret = cpc_write(cpu, max_perf_reg, perf_ctrls->max_perf);
+		if (ret)
+			return ret;
+	}
 
 	/*
 	 * This is Phase-I where we want to write to CPC registers
@@ -2199,30 +2254,32 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 				return ret;
 			}
 		}
-		/*
-		 * Update the pending_write to make sure a PCC CMD_READ will not
-		 * arrive and steal the channel during the switch to write lock
-		 */
-		pcc_ss_data->pending_pcc_write_cmd = true;
-		cpc_desc->write_cmd_id = pcc_ss_data->pcc_write_cnt;
-		cpc_desc->write_cmd_status = 0;
 	}
 
-	if (CPC_SUPPORTED(desired_reg))
-		cpc_write(cpu, desired_reg, perf_ctrls->desired_perf);
+	if (desired_pcc) {
+		ret = cpc_write(cpu, desired_reg, perf_ctrls->desired_perf);
+		if (ret)
+			goto out_pcc_read_unlock;
+	}
 
-	/*
-	 * Only write if min_perf and max_perf not zero. Some drivers pass zero
-	 * value to min and max perf, but they don't mean to set the zero value,
-	 * they just don't want to write to those registers.
-	 */
-	if (perf_ctrls->min_perf && CPC_SUPPORTED(min_perf_reg))
-		cpc_write(cpu, min_perf_reg, perf_ctrls->min_perf);
-	if (perf_ctrls->max_perf && CPC_SUPPORTED(max_perf_reg))
-		cpc_write(cpu, max_perf_reg, perf_ctrls->max_perf);
+	if (min_pcc) {
+		ret = cpc_write(cpu, min_perf_reg, perf_ctrls->min_perf);
+		if (ret)
+			goto out_pcc_read_unlock;
+	}
+	if (max_pcc) {
+		ret = cpc_write(cpu, max_perf_reg, perf_ctrls->max_perf);
+		if (ret)
+			goto out_pcc_read_unlock;
+	}
 
-	if (regs_in_pcc)
+	if (regs_in_pcc) {
+		/* Block a PCC read until the staged payload has been submitted. */
+		WRITE_ONCE(pcc_ss_data->pending_pcc_write_cmd, true);
+		cpc_desc->write_cmd_id = pcc_ss_data->pcc_write_cnt;
+		cpc_desc->write_cmd_status = 0;
 		up_read(&pcc_ss_data->pcc_lock);	/* END Phase-I */
+	}
 	/*
 	 * This is Phase-II where we transfer the ownership of PCC to Platform
 	 *
@@ -2281,9 +2338,17 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 				   cpc_desc->write_cmd_id != pcc_ss_data->pcc_write_cnt);
 
 		/* send_pcc_cmd updates the status in case of failure */
-		ret = cpc_desc->write_cmd_status;
+		if (!ret)
+			ret = cpc_desc->write_cmd_status;
 	}
 	return ret;
+
+out_pcc_read_unlock:
+	up_read(&pcc_ss_data->pcc_lock);
+	down_write(&pcc_ss_data->pcc_lock);
+	cppc_abort_pending_pcc_write(pcc_ss_data, ret);
+	up_write(&pcc_ss_data->pcc_lock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_set_perf);
 
-- 
2.34.1