[PATCH v2 0/4] ACPI/PCC: Correct PCC OperationRegion handling

[PATCH v2 0/4] ACPI/PCC: Correct PCC OperationRegion handling

[Sudeep Holla]

Hi,

This series corrects the handling of PCC OperationRegions and their
shared memory.

ACPI 6.3 states that a PCC OperationRegion describes the shared memory
following the PCC signature. Consequently, COMMAND fields at raw shared
memory offsets 4 and 12 appear at OperationRegion offsets 0 and 8.

The first patch corrects those offsets in ACPICA. This change has already
been submitted upstream:

  https://github.com/acpica/acpica/pull/1205

The ACPICA patch is included here only to document the complete
dependency and aid review and testing. It must enter the kernel through
the ACPICA pull/import process and will not be merged directly through
the Linux ACPI tree.

The remaining Linux patches:

- Preserve the platform-populated PCC signature when copying
  OperationRegion data.
- Release the mailbox channel and region context when ACPICA
  deactivates an OperationRegion.
- Compute and cache the command timeout during channel setup.

There is no strict build dependency in case ACPICA and other 3 changes
need to be merged at different timeline or via different branch.

v1->v2:
 - Updated the commit message to include correct example of PCC Opregion
   as the example added by original commit 77e2a04745ff ("ACPI: PCC:
   Implement OperationRegion handler for the PCC Type 3 subtype")
   incorrectly included the signature as an OperationRegion field.

Regards,
Sudeep

Sudeep Holla (4):
  ACPICA: Fix PCC OperationRegion command offsets
  ACPI: PCC: Preserve shared memory signature in OpRegion handler
  ACPI: PCC: Free channel on OpRegion deactivation
  ACPI: PCC: Cache OpRegion command timeout

 drivers/acpi/acpi_pcc.c       | 55 ++++++++++++++++++++++++++---------
 drivers/acpi/acpica/exfield.c | 11 +++----
 2 files changed, 48 insertions(+), 18 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] ACPICA: Fix PCC OperationRegion command offsets

[Sudeep Holla]

ACPI 6.3, section 5.5.2.4.7.3, states that the PCC Operation
Region is associated with the region of shared memory that follows the
PCC signature.

The generic and extended PCC shared memory layouts include the 4-byte
signature at offset 0, so their raw shared memory COMMAND fields are at
offsets 4 and 12 respectively. Since AML field offsets for the PCC
OperationRegion are relative to the region after that signature, ACPICA
must look for those COMMAND fields at OperationRegion offsets 0 and 8.

Adjust the generic and master subspace command checks to use those
OperationRegion-relative offsets. Otherwise writes to the COMMAND field
can fail to invoke the PCC address space handler at the offset described
by the PCC OperationRegion definition.

Fixes: aa6ec56b574d ("ACPICA: ACPI 6.3: add PCC operation region support for AML interpreter")
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/acpi/acpica/exfield.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/acpica/exfield.c b/drivers/acpi/acpica/exfield.c
index 9a55524ed8f4..a7fac63938b3 100644
--- a/drivers/acpi/acpica/exfield.c
+++ b/drivers/acpi/acpica/exfield.c
@@ -45,12 +45,13 @@ static const u8 acpi_protocol_lengths[] = {
 
 /*
  * The following macros determine a given offset is a COMD field.
- * According to the specification, generic subspaces (types 0-2) contains a
- * 2-byte COMD field at offset 4 and master subspaces (type 3) contains a 4-byte
- * COMD field starting at offset 12.
+ * According to the specification, the PCC OperationRegion begins after
+ * the PCC signature. The raw shared memory COMD offsets of 4 for generic
+ * subspaces (types 0-2) and 12 for master subspaces (type 3) therefore
+ * appear at OperationRegion offsets 0 and 8.
  */
-#define GENERIC_SUBSPACE_COMMAND(a)     (4 == a || a == 5)
-#define MASTER_SUBSPACE_COMMAND(a)      (12 <= a && a <= 15)
+#define GENERIC_SUBSPACE_COMMAND(a)     ((a) < 2)
+#define MASTER_SUBSPACE_COMMAND(a)      (((a) - 8) < 4)
 
 /*******************************************************************************
  *
-- 
2.43.0

Re: [PATCH v2 1/4] ACPICA: Fix PCC OperationRegion command offsets

[Rafael J. Wysocki (Intel)]

On Wed, Jul 22, 2026 at 3:16 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
>
> ACPI 6.3, section 5.5.2.4.7.3, states that the PCC Operation
> Region is associated with the region of shared memory that follows the
> PCC signature.
>
> The generic and extended PCC shared memory layouts include the 4-byte
> signature at offset 0, so their raw shared memory COMMAND fields are at
> offsets 4 and 12 respectively. Since AML field offsets for the PCC
> OperationRegion are relative to the region after that signature, ACPICA
> must look for those COMMAND fields at OperationRegion offsets 0 and 8.
>
> Adjust the generic and master subspace command checks to use those
> OperationRegion-relative offsets. Otherwise writes to the COMMAND field
> can fail to invoke the PCC address space handler at the offset described
> by the PCC OperationRegion definition.
>
> Fixes: aa6ec56b574d ("ACPICA: ACPI 6.3: add PCC operation region support for AML interpreter")
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> ---
>  drivers/acpi/acpica/exfield.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/acpica/exfield.c b/drivers/acpi/acpica/exfield.c
> index 9a55524ed8f4..a7fac63938b3 100644
> --- a/drivers/acpi/acpica/exfield.c
> +++ b/drivers/acpi/acpica/exfield.c
> @@ -45,12 +45,13 @@ static const u8 acpi_protocol_lengths[] = {
>
>  /*
>   * The following macros determine a given offset is a COMD field.
> - * According to the specification, generic subspaces (types 0-2) contains a
> - * 2-byte COMD field at offset 4 and master subspaces (type 3) contains a 4-byte
> - * COMD field starting at offset 12.
> + * According to the specification, the PCC OperationRegion begins after
> + * the PCC signature. The raw shared memory COMD offsets of 4 for generic
> + * subspaces (types 0-2) and 12 for master subspaces (type 3) therefore
> + * appear at OperationRegion offsets 0 and 8.
>   */
> -#define GENERIC_SUBSPACE_COMMAND(a)     (4 == a || a == 5)
> -#define MASTER_SUBSPACE_COMMAND(a)      (12 <= a && a <= 15)
> +#define GENERIC_SUBSPACE_COMMAND(a)     ((a) < 2)
> +#define MASTER_SUBSPACE_COMMAND(a)      (((a) - 8) < 4)
>
>  /*******************************************************************************
>   *
> --

Can you please see the corresponding upstream ACPICA pull request?
There are concerns regarding it.

Thanks!

[PATCH v2 2/4] ACPI: PCC: Preserve shared memory signature in OpRegion handler

[Sudeep Holla]

ACPI 6.3 introduced PCC OperationRegions. Section 5.5.2.4.7.3,
"Declaring message fields within a PCC OperationRegion", states that,
for all PCC subspace types, the PCC Operation Region pertains to the
region of PCC subspace that succeeds the PCC signature. Its length
therefore excludes the 4-byte signature.

The PCC address space handler currently copies the OperationRegion
buffer to and from the start of the shared memory. This overwrites or
exposes the platform-populated signature and omits the final four bytes
of the OperationRegion.

Offset the copies by the size of the signature and reject an
OperationRegion that does not fit in the remaining shared memory.

The example added by commit 77e2a04745ff ("ACPI: PCC: Implement
OperationRegion handler for the PCC Type 3 subtype") incorrectly
included the signature as an OperationRegion field. A Type 3
OperationRegion containing a 100-byte communication area is declared
as follows:

OperationRegion (PFRM, PCC, 2, 0x70)
Field (PFRM, ByteAcc, NoLock, Preserve)
{
    FLGS,   32,
    LEN,    32,
    CMD,    32,
    DATA,   800
}

This contains three 32-bit fields followed by a 100-byte DATA field.
The fields correspond to the Type 3 shared memory layout in ACPI 6.3,
Table 14-372, after excluding its 4-byte signature.

Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
Reviewed-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/acpi/acpi_pcc.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpi_pcc.c b/drivers/acpi/acpi_pcc.c
index 438c67189511..9881c9ee293d 100644
--- a/drivers/acpi/acpi_pcc.c
+++ b/drivers/acpi/acpi_pcc.c
@@ -28,6 +28,7 @@
  * to PCC commands
  */
 #define PCC_CMD_WAIT_RETRIES_NUM	500ULL
+#define PCC_SIGNATURE_SIZE		sizeof(u32)
 
 struct pcc_data {
 	struct pcc_mbox_chan *pcc_chan;
@@ -74,6 +75,14 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
 	}
 
 	pcc_chan = data->pcc_chan;
+	if (pcc_chan->shmem_size < PCC_SIGNATURE_SIZE ||
+	    ctx->length > pcc_chan->shmem_size - PCC_SIGNATURE_SIZE) {
+		pr_err("PCC channel-%d shared memory is too small.\n",
+		       ctx->subspace_id);
+		ret = AE_AML_REGION_LIMIT;
+		goto err_free_channel;
+	}
+
 	if (!pcc_chan->mchan->mbox->txdone_irq) {
 		pr_err("This channel-%d does not support interrupt.\n",
 		       ctx->subspace_id);
@@ -97,14 +106,17 @@ acpi_pcc_address_space_handler(u32 function, acpi_physical_address addr,
 			       u32 bits, acpi_integer *value,
 			       void *handler_context, void *region_context)
 {
-	int ret;
 	struct pcc_data *data = region_context;
+	void __iomem *pcc_opregion;
 	u64 usecs_lat;
+	int ret;
+
+	pcc_opregion = data->pcc_chan->shmem + PCC_SIGNATURE_SIZE;
 
 	reinit_completion(&data->done);
 
-	/* Write to Shared Memory */
-	memcpy_toio(data->pcc_chan->shmem, (void *)value, data->ctx.length);
+	/* Write to the PCC OperationRegion after the shared memory signature. */
+	memcpy_toio(pcc_opregion, (void *)value, data->ctx.length);
 
 	ret = mbox_send_message(data->pcc_chan->mchan, NULL);
 	if (ret < 0)
@@ -125,7 +137,7 @@ acpi_pcc_address_space_handler(u32 function, acpi_physical_address addr,
 
 	mbox_chan_txdone(data->pcc_chan->mchan, ret);
 
-	memcpy_fromio(value, data->pcc_chan->shmem, data->ctx.length);
+	memcpy_fromio(value, pcc_opregion, data->ctx.length);
 
 	return AE_OK;
 }
-- 
2.43.0

[PATCH v2 3/4] ACPI: PCC: Free channel on OpRegion deactivation

[Sudeep Holla]

ACPICA calls the address space setup callback with
ACPI_REGION_DEACTIVATE when a PCC OperationRegion is torn down.

The PCC setup callback currently allocates a fresh pcc_data and requests
the mailbox channel before looking at the function argument. If ACPICA
deactivates a region, this can leave the existing region context and
mailbox channel unreleased, and may also request a channel during
teardown.

Handle ACPI_REGION_DEACTIVATE before allocation. Free the PCC mailbox
channel, release the region context and clear the context pointer.

Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
Reviewed-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/acpi/acpi_pcc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/acpi/acpi_pcc.c b/drivers/acpi/acpi_pcc.c
index 9881c9ee293d..57d13b25c1d6 100644
--- a/drivers/acpi/acpi_pcc.c
+++ b/drivers/acpi/acpi_pcc.c
@@ -55,6 +55,19 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
 	struct pcc_mbox_chan *pcc_chan;
 	acpi_status ret;
 
+	if (function == ACPI_REGION_DEACTIVATE) {
+		data = *region_context;
+		if (data) {
+			pcc_mbox_free_channel(data->pcc_chan);
+			kfree(data);
+			*region_context = NULL;
+		}
+		return AE_OK;
+	}
+
+	if (function != ACPI_REGION_ACTIVATE)
+		return AE_BAD_PARAMETER;
+
 	data = kzalloc_obj(*data);
 	if (!data)
 		return AE_NO_MEMORY;
-- 
2.43.0

[PATCH v2 4/4] ACPI: PCC: Cache OpRegion command timeout

[Sudeep Holla]

The PCC OperationRegion handler computes the same command completion
wait timeout each time it sends a command. The timeout is derived from
static channel properties, so compute it once when the PCC channel is
set up and store the millisecond value in the mailbox client timeout
field.

Use the cached timeout when waiting for the OperationRegion command to
complete. This keeps the timeout calculation in one place and avoids
recomputing it for every command.

Reviewed-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/acpi/acpi_pcc.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/acpi_pcc.c b/drivers/acpi/acpi_pcc.c
index 57d13b25c1d6..345f233d77cd 100644
--- a/drivers/acpi/acpi_pcc.c
+++ b/drivers/acpi/acpi_pcc.c
@@ -50,10 +50,11 @@ static acpi_status
 acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
 			     void *handler_context,  void **region_context)
 {
-	struct pcc_data *data;
 	struct acpi_pcc_info *ctx = handler_context;
 	struct pcc_mbox_chan *pcc_chan;
+	struct pcc_data *data;
 	acpi_status ret;
+	u64 usecs_lat;
 
 	if (function == ACPI_REGION_DEACTIVATE) {
 		data = *region_context;
@@ -103,6 +104,16 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
 		goto err_free_channel;
 	}
 
+	/*
+	 * pcc_chan->latency is just a Nominal value. In reality the remote
+	 * processor could be much slower to reply. So add an arbitrary
+	 * amount of wait on top of Nominal.
+	 */
+	usecs_lat = PCC_CMD_WAIT_RETRIES_NUM * pcc_chan->latency;
+	data->cl.tx_tout = DIV_ROUND_UP_ULL(usecs_lat, 1000);
+	if (!data->cl.tx_tout)
+		data->cl.tx_tout = 1;
+
 	*region_context = data;
 	return AE_OK;
 
@@ -121,7 +132,6 @@ acpi_pcc_address_space_handler(u32 function, acpi_physical_address addr,
 {
 	struct pcc_data *data = region_context;
 	void __iomem *pcc_opregion;
-	u64 usecs_lat;
 	int ret;
 
 	pcc_opregion = data->pcc_chan->shmem + PCC_SIGNATURE_SIZE;
@@ -135,14 +145,8 @@ acpi_pcc_address_space_handler(u32 function, acpi_physical_address addr,
 	if (ret < 0)
 		return AE_ERROR;
 
-	/*
-	 * pcc_chan->latency is just a Nominal value. In reality the remote
-	 * processor could be much slower to reply. So add an arbitrary
-	 * amount of wait on top of Nominal.
-	 */
-	usecs_lat = PCC_CMD_WAIT_RETRIES_NUM * data->pcc_chan->latency;
 	ret = wait_for_completion_timeout(&data->done,
-						usecs_to_jiffies(usecs_lat));
+					  msecs_to_jiffies(data->cl.tx_tout));
 	if (ret == 0) {
 		pr_err("PCC command executed timeout!\n");
 		return AE_TIME;
-- 
2.43.0