[PATCH 0/6] ACPI/PCC: Preserve platform-populated PCC signatures

[PATCH 0/6] ACPI/PCC: Preserve platform-populated PCC signatures

[Sudeep Holla]

ACPI PCC shared memory layouts reserve the first dword for the PCC
signature. ACPI specification defines the signature as 0x50434300 ORed
with the PCC subspace ID, and ACPI 6.6 clarify that the signature is
populated by the platform and verified by OSPM.

This series centralizes PCC shared memory signature validation in the PCC
mailbox controller and stops PCC users from rewriting the signature before
each command. Clients that previously copied complete local PCC headers
now update only the mutable command/status/flags/length/payload fields.

The final patch also fixes the PCC OperationRegion handler. ACPI defines
a PCC OperationRegion as the shared memory fields that follow the PCC
signature, with the OperationRegion length covering only those fields. The
handler is updated to copy to/from the region after the signature and to
reject regions that do not fit there.

All patches can go independently as there is no strict dependency between
them and posted together for the complete context.

Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
Sudeep Holla (6):
      mailbox: pcc: Validate shared memory signature on request
      hwmon: xgene: Stop writing PCC shared memory signature
      i2c: xgene-slimpro: Stop writing PCC shared memory signature
      devfreq: hisi_uncore: Preserve PCC shared memory signature
      soc: hisilicon: kunpeng_hccs: Preserve PCC signatures
      ACPI: PCC: Preserve shared memory signature in OpRegion handler

 drivers/acpi/acpi_pcc.c                | 20 ++++++++++++++----
 drivers/devfreq/hisi_uncore_freq.c     | 15 +++++++-------
 drivers/hwmon/xgene-hwmon.c            |  4 ----
 drivers/i2c/busses/i2c-xgene-slimpro.c |  3 ---
 drivers/mailbox/pcc.c                  | 38 +++++++++++++++++++++++++++++-----
 drivers/soc/hisilicon/kunpeng_hccs.c   | 24 ++++++++-------------
 6 files changed, 65 insertions(+), 39 deletions(-)
---
base-commit: 5a66900afbd6b2a063eebad35294038a654de2b0
change-id: 20260627-acpi_pcc_signature-7b70b0633c8f


-- 
Regards,
Sudeep

[PATCH 1/6] mailbox: pcc: Validate shared memory signature on request

[Sudeep Holla]

ACPI defines the PCC shared memory signature as the bitwise OR of
0x50434300 and the PCC subspace ID. The signature is located at byte
offset 0 for the generic, extended and reduced PCC shared memory region
layouts.

Validate the signature when a client requests a PCC mailbox channel, after
mapping the shared memory and before binding the mailbox client. This
keeps PCC shared memory validation in the PCC mailbox controller instead
of duplicating it in individual clients.

ACPI 6.6 added clarification that the signature is populated by the
platform and verified by OSPM.

Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/mailbox/pcc.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 636879ae1db7..6fbc17d41774 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -345,6 +345,28 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+static int pcc_mbox_validate_signature(struct pcc_mbox_chan *pcc_mchan,
+				       int subspace_id)
+{
+	u32 expected_signature = PCC_SIGNATURE | subspace_id;
+	u32 signature;
+
+	if (pcc_mchan->shmem_size < sizeof(signature)) {
+		pr_err("PCC subspace %d shared memory is too small\n",
+		       subspace_id);
+		return -EINVAL;
+	}
+
+	signature = ioread32(pcc_mchan->shmem);
+	if (signature != expected_signature) {
+		pr_err("PCC subspace %d has invalid signature: %#x expected %#x\n",
+		       subspace_id, signature, expected_signature);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  * pcc_mbox_request_channel - PCC clients call this function to
  *		request a pointer to their PCC subspace, from which they
@@ -381,14 +403,20 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 	if (!pcc_mchan->shmem)
 		return ERR_PTR(-ENXIO);
 
+	rc = pcc_mbox_validate_signature(pcc_mchan, subspace_id);
+	if (rc)
+		goto err_unmap_shmem;
+
 	rc = mbox_bind_client(chan, cl);
-	if (rc) {
-		iounmap(pcc_mchan->shmem);
-		pcc_mchan->shmem = NULL;
-		return ERR_PTR(rc);
-	}
+	if (rc)
+		goto err_unmap_shmem;
 
 	return pcc_mchan;
+
+err_unmap_shmem:
+	iounmap(pcc_mchan->shmem);
+	pcc_mchan->shmem = NULL;
+	return ERR_PTR(rc);
 }
 EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
 

-- 
2.43.0

[PATCH 2/6] hwmon: xgene: Stop writing PCC shared memory signature

[Sudeep Holla]

ACPI specification defines the generic PCC shared memory signature as
the PCC base signature ORed with the subspace ID.

ACPI 6.6 added clarification that the signature is populated by the
platform and verified by OSPM.

The PCC mailbox controller now validates the signature when the channel
is requested. Stop rewriting the signature before each command and
leave the platform-populated value intact.

Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/hwmon/xgene-hwmon.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
index 11c5d80428cd..38b140c23c88 100644
--- a/drivers/hwmon/xgene-hwmon.c
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -133,10 +133,6 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
 	init_completion(&ctx->rd_complete);
 	ctx->resp_pending = true;
 
-	/* Write signature for subspace */
-	WRITE_ONCE(generic_comm_base->signature,
-		   cpu_to_le32(PCC_SIGNATURE | ctx->mbox_idx));
-
 	/* Write to the shared command region */
 	WRITE_ONCE(generic_comm_base->command,
 		   cpu_to_le16(MSG_TYPE(msg[0]) | PCC_CMD_GENERATE_DB_INTR));

-- 
2.43.0

[PATCH 3/6] i2c: xgene-slimpro: Stop writing PCC shared memory signature

[Sudeep Holla]

ACPI specification defines the generic PCC shared memory signature as
the PCC base signature ORed with the subspace ID.

ACPI 6.6 added clarification that the signature is populated by the
platform and verified by OSPM.

The PCC mailbox controller now validates the signature when the channel
is requested. Stop rewriting the signature before each command and
leave the platform-populated value intact.

Cc: Andi Shyti <andi.shyti@kernel.org>
Cc: linux-i2c@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/i2c/busses/i2c-xgene-slimpro.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c
index b29dec66b2c3..f77d5b3a863f 100644
--- a/drivers/i2c/busses/i2c-xgene-slimpro.c
+++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
@@ -174,9 +174,6 @@ static void slimpro_i2c_pcc_tx_prepare(struct slimpro_i2c_dev *ctx, u32 *msg)
 	u16 status;
 	int i;
 
-	WRITE_ONCE(generic_comm_base->signature,
-		   cpu_to_le32(PCC_SIGNATURE | ctx->mbox_idx));
-
 	WRITE_ONCE(generic_comm_base->command,
 		   cpu_to_le16(SLIMPRO_MSG_TYPE(msg[0]) | PCC_CMD_GENERATE_DB_INTR));
 

-- 
2.43.0

[PATCH 4/6] devfreq: hisi_uncore: Preserve PCC shared memory signature

[Sudeep Holla]

ACPI specification defines the generic PCC shared memory signature as
the PCC base signature ORed with the subspace ID.

ACPI 6.6 added clarification that the signature is populated by the
platform and verified by OSPM.

The PCC mailbox controller now validates the signature when the channel
is requested. Stop copying a complete local PCC header into shared
memory and write only the command, status and payload fields, leaving
the platform-populated signature intact.

Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Huisong Li <lihuisong@huawei.com>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/devfreq/hisi_uncore_freq.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/devfreq/hisi_uncore_freq.c b/drivers/devfreq/hisi_uncore_freq.c
index 4d00d813c8ac..7976eb359061 100644
--- a/drivers/devfreq/hisi_uncore_freq.c
+++ b/drivers/devfreq/hisi_uncore_freq.c
@@ -187,7 +187,6 @@ static int hisi_uncore_cmd_send(struct hisi_uncore_freq *uncore,
 				u8 cmd, u32 *data)
 {
 	struct hisi_uncore_pcc_shmem __iomem *addr;
-	struct hisi_uncore_pcc_shmem shmem;
 	struct pcc_mbox_chan *pchan;
 	unsigned int mrtt;
 	s64 time_delta;
@@ -210,13 +209,13 @@ static int hisi_uncore_cmd_send(struct hisi_uncore_freq *uncore,
 	if (mrtt > time_delta)
 		udelay(mrtt - time_delta);
 
-	/* Copy data */
-	shmem.head = (struct acpi_pcct_shared_memory) {
-		.signature = PCC_SIGNATURE | uncore->chan_id,
-		.command = cmd,
-	};
-	shmem.pcc_data.data = *data;
-	memcpy_toio(addr, &shmem, sizeof(shmem));
+	/*
+	 * Copy command and data, leaving the platform-populated signature
+	 * intact.
+	 */
+	iowrite16(cmd, &addr->head.command);
+	iowrite16(0, &addr->head.status);
+	iowrite32(*data, &addr->pcc_data.data);
 
 	/* Ring doorbell */
 	rc = mbox_send_message(pchan->mchan, &cmd);

-- 
2.43.0

[PATCH 5/6] soc: hisilicon: kunpeng_hccs: Preserve PCC signatures

[Sudeep Holla]

ACPI specification defines the generic PCC shared memory signature as
the PCC base signature ORed with the subspace ID.

ACPI 6.6 added clarification that the signature is populated by the
platform and verified by OSPM.

The PCC mailbox controller now validates the signature when the channel
is requested. Stop copying complete local PCC headers into shared
memory and write only the mutable command, status, flags and length
fields, leaving the platform-populated signatures intact.

Cc: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index 0cc2953d59a5..0e5e48e46eba 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -270,14 +270,11 @@ static inline void hccs_fill_pcc_shared_mem_region(struct hccs_dev *hdev,
 						   void __iomem *comm_space,
 						   u16 space_size)
 {
-	struct acpi_pcct_shared_memory tmp = {
-		.signature = PCC_SIGNATURE | hdev->chan_id,
-		.command = cmd,
-		.status = 0,
-	};
+	struct acpi_pcct_shared_memory __iomem *comm_base;
 
-	memcpy_toio(hdev->cl_info.pcc_chan->shmem, (void *)&tmp,
-		    sizeof(struct acpi_pcct_shared_memory));
+	comm_base = hdev->cl_info.pcc_chan->shmem;
+	iowrite16(cmd, &comm_base->command);
+	iowrite16(0, &comm_base->status);
 
 	/* Copy the message to the PCC comm space */
 	memcpy_toio(comm_space, (void *)desc, space_size);
@@ -289,15 +286,12 @@ static inline void hccs_fill_ext_pcc_shared_mem_region(struct hccs_dev *hdev,
 						       void __iomem *comm_space,
 						       u16 space_size)
 {
-	struct acpi_pcct_ext_pcc_shared_memory tmp = {
-		.signature = PCC_SIGNATURE | hdev->chan_id,
-		.flags = PCC_CMD_COMPLETION_NOTIFY,
-		.length = HCCS_PCC_SHARE_MEM_BYTES,
-		.command = cmd,
-	};
+	struct acpi_pcct_ext_pcc_shared_memory __iomem *comm_base;
 
-	memcpy_toio(hdev->cl_info.pcc_chan->shmem, (void *)&tmp,
-		    sizeof(struct acpi_pcct_ext_pcc_shared_memory));
+	comm_base = hdev->cl_info.pcc_chan->shmem;
+	iowrite32(PCC_CMD_COMPLETION_NOTIFY, &comm_base->flags);
+	iowrite32(HCCS_PCC_SHARE_MEM_BYTES, &comm_base->length);
+	iowrite32(cmd, &comm_base->command);
 
 	/* Copy the message to the PCC comm space */
 	memcpy_toio(comm_space, (void *)desc, space_size);

-- 
2.43.0

[PATCH 6/6] ACPI: PCC: Preserve shared memory signature in OpRegion handler

[Sudeep Holla]

ACPI defines a PCC OperationRegion as the shared memory fields that
follow the PCC signature. The OperationRegion length is likewise the
total size of the fields that succeed the signature in shared memory.

ACPI 6.6 adds clarification that the PCC shared memory signature is
populated by the platform and verified by OSPM.

The PCC address space handler currently copies the OperationRegion
buffer to and from the start of the PCC shared memory region. That can
overwrite or expose the signature at byte offset 0, and it also misses
the last 4 bytes of the actual PCC OperationRegion data.

Offset OperationRegion copies by the size of the signature and reject
regions that do not fit in the shared memory after that signature.

Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
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

Re: [PATCH 2/6] hwmon: xgene: Stop writing PCC shared memory signature

[Guenter Roeck]

On Sat, Jun 27, 2026 at 05:37:30PM +0100, Sudeep Holla wrote:
> ACPI specification defines the generic PCC shared memory signature as
> the PCC base signature ORed with the subspace ID.
> 
> ACPI 6.6 added clarification that the signature is populated by the
> platform and verified by OSPM.
> 
> The PCC mailbox controller now validates the signature when the channel
> is requested. Stop rewriting the signature before each command and
> leave the platform-populated value intact.
> 
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>

Applied.

Thanks,
Guenter

Re: [PATCH 1/6] mailbox: pcc: Validate shared memory signature on request

[lihuisong (C)]

On 6/28/2026 12:37 AM, Sudeep Holla wrote:
> ACPI defines the PCC shared memory signature as the bitwise OR of
> 0x50434300 and the PCC subspace ID. The signature is located at byte
> offset 0 for the generic, extended and reduced PCC shared memory region
> layouts.
>
> Validate the signature when a client requests a PCC mailbox channel, after
> mapping the shared memory and before binding the mailbox client. This
> keeps PCC shared memory validation in the PCC mailbox controller instead
> of duplicating it in individual clients.
>
> ACPI 6.6 added clarification that the signature is populated by the
> platform and verified by OSPM.
>
> Cc: Jassi Brar <jassisinghbrar@gmail.com>
> Cc: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> ---
>   drivers/mailbox/pcc.c | 38 +++++++++++++++++++++++++++++++++-----
>   1 file changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 636879ae1db7..6fbc17d41774 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -345,6 +345,28 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	return IRQ_HANDLED;
>   }
>   
> +static int pcc_mbox_validate_signature(struct pcc_mbox_chan *pcc_mchan,
> +				       int subspace_id)
> +{
> +	u32 expected_signature = PCC_SIGNATURE | subspace_id;
> +	u32 signature;
> +
> +	if (pcc_mchan->shmem_size < sizeof(signature)) {
> +		pr_err("PCC subspace %d shared memory is too small\n",
> +		       subspace_id);
> +		return -EINVAL;
> +	}
> +
> +	signature = ioread32(pcc_mchan->shmem);
> +	if (signature != expected_signature) {
> +		pr_err("PCC subspace %d has invalid signature: %#x expected %#x\n",
> +		       subspace_id, signature, expected_signature);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
If the signature is incorrect, there must be an issue with the PCCT 
table reporting.
So how about place this verification in pcc_mbox_probe?
We can mark the channel as unavailable so that other normal channels can 
continue to be used.
And the impact is minimal.
>   /**
>    * pcc_mbox_request_channel - PCC clients call this function to
>    *		request a pointer to their PCC subspace, from which they
> @@ -381,14 +403,20 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>   	if (!pcc_mchan->shmem)
>   		return ERR_PTR(-ENXIO);
>   
> +	rc = pcc_mbox_validate_signature(pcc_mchan, subspace_id);
> +	if (rc)
> +		goto err_unmap_shmem;
> +
>   	rc = mbox_bind_client(chan, cl);
> -	if (rc) {
> -		iounmap(pcc_mchan->shmem);
> -		pcc_mchan->shmem = NULL;
> -		return ERR_PTR(rc);
> -	}
> +	if (rc)
> +		goto err_unmap_shmem;
>   
>   	return pcc_mchan;
> +
> +err_unmap_shmem:
> +	iounmap(pcc_mchan->shmem);
> +	pcc_mchan->shmem = NULL;
> +	return ERR_PTR(rc);
>   }
>   EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
>   
>

Re: [PATCH 1/6] mailbox: pcc: Validate shared memory signature on request

[lihuisong (C)]

On 6/30/2026 5:02 PM, lihuisong (C) wrote:
>
> On 6/28/2026 12:37 AM, Sudeep Holla wrote:
>> ACPI defines the PCC shared memory signature as the bitwise OR of
>> 0x50434300 and the PCC subspace ID. The signature is located at byte
>> offset 0 for the generic, extended and reduced PCC shared memory region
>> layouts.
>>
>> Validate the signature when a client requests a PCC mailbox channel, 
>> after
>> mapping the shared memory and before binding the mailbox client. This
>> keeps PCC shared memory validation in the PCC mailbox controller instead
>> of duplicating it in individual clients.
>>
>> ACPI 6.6 added clarification that the signature is populated by the
>> platform and verified by OSPM.
>>
>> Cc: Jassi Brar <jassisinghbrar@gmail.com>
>> Cc: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
>> ---
>>   drivers/mailbox/pcc.c | 38 +++++++++++++++++++++++++++++++++-----
>>   1 file changed, 33 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 636879ae1db7..6fbc17d41774 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -345,6 +345,28 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>       return IRQ_HANDLED;
>>   }
>>   +static int pcc_mbox_validate_signature(struct pcc_mbox_chan 
>> *pcc_mchan,
>> +                       int subspace_id)
>> +{
>> +    u32 expected_signature = PCC_SIGNATURE | subspace_id;
>> +    u32 signature;
>> +
>> +    if (pcc_mchan->shmem_size < sizeof(signature)) {
>> +        pr_err("PCC subspace %d shared memory is too small\n",
>> +               subspace_id);
>> +        return -EINVAL;
>> +    }
>> +
>> +    signature = ioread32(pcc_mchan->shmem);
>> +    if (signature != expected_signature) {
>> +        pr_err("PCC subspace %d has invalid signature: %#x expected 
>> %#x\n",
>> +               subspace_id, signature, expected_signature);
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
> If the signature is incorrect, there must be an issue with the PCCT 
> table reporting.
> So how about place this verification in pcc_mbox_probe?
> We can mark the channel as unavailable so that other normal channels 
> can continue to be used.
> And the impact is minimal.
It can only be seen after being mapped here.
Sorry, please ignore this.
>>   /**
>>    * pcc_mbox_request_channel - PCC clients call this function to
>>    *        request a pointer to their PCC subspace, from which they
>> @@ -381,14 +403,20 @@ pcc_mbox_request_channel(struct mbox_client 
>> *cl, int subspace_id)
>>       if (!pcc_mchan->shmem)
>>           return ERR_PTR(-ENXIO);
>>   +    rc = pcc_mbox_validate_signature(pcc_mchan, subspace_id);
>> +    if (rc)
>> +        goto err_unmap_shmem;
>> +
>>       rc = mbox_bind_client(chan, cl);
>> -    if (rc) {
>> -        iounmap(pcc_mchan->shmem);
>> -        pcc_mchan->shmem = NULL;
>> -        return ERR_PTR(rc);
>> -    }
>> +    if (rc)
>> +        goto err_unmap_shmem;
>>         return pcc_mchan;
>> +
>> +err_unmap_shmem:
>> +    iounmap(pcc_mchan->shmem);
>> +    pcc_mchan->shmem = NULL;
>> +    return ERR_PTR(rc);
>>   }
>>   EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
>>

Re: [PATCH 0/6] ACPI/PCC: Preserve platform-populated PCC signatures

[lihuisong (C)]

Hi Sudeep,

On 6/28/2026 12:37 AM, Sudeep Holla wrote:
> ACPI PCC shared memory layouts reserve the first dword for the PCC
> signature. ACPI specification defines the signature as 0x50434300 ORed
> with the PCC subspace ID, and ACPI 6.6 clarify that the signature is
> populated by the platform and verified by OSPM.
>
> This series centralizes PCC shared memory signature validation in the PCC
> mailbox controller and stops PCC users from rewriting the signature before
> each command. Clients that previously copied complete local PCC headers
> now update only the mutable command/status/flags/length/payload fields.
I am concerned that this may affect the functionality of drivers on some 
existed platforms.
This largely depends on the implementation of the platform firmware.

I think it's good for the signature to be filled in by the command 
initiator and then verified
by the recipient, as this is how this field can serve its purpose.
Otherwise, I really don't see what use it has.

/Huisong
>
> The final patch also fixes the PCC OperationRegion handler. ACPI defines
> a PCC OperationRegion as the shared memory fields that follow the PCC
> signature, with the OperationRegion length covering only those fields. The
> handler is updated to copy to/from the region after the signature and to
> reject regions that do not fit there.
>
> All patches can go independently as there is no strict dependency between
> them and posted together for the complete context.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> ---
> Sudeep Holla (6):
>        mailbox: pcc: Validate shared memory signature on request
>        hwmon: xgene: Stop writing PCC shared memory signature
>        i2c: xgene-slimpro: Stop writing PCC shared memory signature
>        devfreq: hisi_uncore: Preserve PCC shared memory signature
>        soc: hisilicon: kunpeng_hccs: Preserve PCC signatures
>        ACPI: PCC: Preserve shared memory signature in OpRegion handler
>
>   drivers/acpi/acpi_pcc.c                | 20 ++++++++++++++----
>   drivers/devfreq/hisi_uncore_freq.c     | 15 +++++++-------
>   drivers/hwmon/xgene-hwmon.c            |  4 ----
>   drivers/i2c/busses/i2c-xgene-slimpro.c |  3 ---
>   drivers/mailbox/pcc.c                  | 38 +++++++++++++++++++++++++++++-----
>   drivers/soc/hisilicon/kunpeng_hccs.c   | 24 ++++++++-------------
>   6 files changed, 65 insertions(+), 39 deletions(-)
> ---
> base-commit: 5a66900afbd6b2a063eebad35294038a654de2b0
> change-id: 20260627-acpi_pcc_signature-7b70b0633c8f
>
>

Re: [PATCH 1/6] mailbox: pcc: Validate shared memory signature on request

[Sudeep Holla]

On Tue, Jun 30, 2026 at 05:18:39PM +0800, lihuisong (C) wrote:
> 
> On 6/30/2026 5:02 PM, lihuisong (C) wrote:
> > 
> > On 6/28/2026 12:37 AM, Sudeep Holla wrote:
> > > ACPI defines the PCC shared memory signature as the bitwise OR of
> > > 0x50434300 and the PCC subspace ID. The signature is located at byte
> > > offset 0 for the generic, extended and reduced PCC shared memory region
> > > layouts.
> > > 
> > > Validate the signature when a client requests a PCC mailbox channel,
> > > after
> > > mapping the shared memory and before binding the mailbox client. This
> > > keeps PCC shared memory validation in the PCC mailbox controller instead
> > > of duplicating it in individual clients.
> > > 
> > > ACPI 6.6 added clarification that the signature is populated by the
> > > platform and verified by OSPM.
> > > 
> > > Cc: Jassi Brar <jassisinghbrar@gmail.com>
> > > Cc: Huisong Li <lihuisong@huawei.com>
> > > Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> > > ---
> > >   drivers/mailbox/pcc.c | 38 +++++++++++++++++++++++++++++++++-----
> > >   1 file changed, 33 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > > index 636879ae1db7..6fbc17d41774 100644
> > > --- a/drivers/mailbox/pcc.c
> > > +++ b/drivers/mailbox/pcc.c
> > > @@ -345,6 +345,28 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
> > >       return IRQ_HANDLED;
> > >   }
> > >   +static int pcc_mbox_validate_signature(struct pcc_mbox_chan
> > > *pcc_mchan,
> > > +                       int subspace_id)
> > > +{
> > > +    u32 expected_signature = PCC_SIGNATURE | subspace_id;
> > > +    u32 signature;
> > > +
> > > +    if (pcc_mchan->shmem_size < sizeof(signature)) {
> > > +        pr_err("PCC subspace %d shared memory is too small\n",
> > > +               subspace_id);
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    signature = ioread32(pcc_mchan->shmem);
> > > +    if (signature != expected_signature) {
> > > +        pr_err("PCC subspace %d has invalid signature: %#x expected
> > > %#x\n",
> > > +               subspace_id, signature, expected_signature);
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > If the signature is incorrect, there must be an issue with the PCCT
> > table reporting.
> > So how about place this verification in pcc_mbox_probe?
> > We can mark the channel as unavailable so that other normal channels can
> > continue to be used.
> > And the impact is minimal.
> It can only be seen after being mapped here.

Indeed, we still fail to allocate the channel in case of invalid signature.

> Sorry, please ignore this.

No worries.

-- 
Regards,
Sudeep

Re: [PATCH 0/6] ACPI/PCC: Preserve platform-populated PCC signatures

[Sudeep Holla]

On Tue, Jun 30, 2026 at 05:25:20PM +0800, lihuisong (C) wrote:
> Hi Sudeep,
> 
> On 6/28/2026 12:37 AM, Sudeep Holla wrote:
> > ACPI PCC shared memory layouts reserve the first dword for the PCC
> > signature. ACPI specification defines the signature as 0x50434300 ORed
> > with the PCC subspace ID, and ACPI 6.6 clarify that the signature is
> > populated by the platform and verified by OSPM.
> > 
> > This series centralizes PCC shared memory signature validation in the PCC
> > mailbox controller and stops PCC users from rewriting the signature before
> > each command. Clients that previously copied complete local PCC headers
> > now update only the mutable command/status/flags/length/payload fields.
> I am concerned that this may affect the functionality of drivers on some
> existed platforms.

One other option I was thinking is to not issue error but just log the error
message and let the client driver add that additional check if required.

> This largely depends on the implementation of the platform firmware.
> 

Sure. However I always expected it to be taken care by the platform, v6.6
just adds that clarification explicitly. It was never clear that the sender
or the OSPM needs to write that signature, so I disagree that it is platform
firmware dependent entirely. The clarification wouldn't have got added if
there was any disagreement on that fact.

> I think it's good for the signature to be filled in by the command initiator
> and then verified by the recipient, as this is how this field can serve its
> purpose.
> Otherwise, I really don't see what use it has.

Please get the spec updated accordingly, we can just change in the kernel
based on what we think is the best way to use it. It needs to be clearly
specified, otherwise it may cause issue for non-Linux OSVs.

-- 
Regards,
Sudeep

Re: [PATCH 6/6] ACPI: PCC: Preserve shared memory signature in OpRegion handler

[Rafael J. Wysocki (Intel)]

On Sat, Jun 27, 2026 at 6:39 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
>
> ACPI defines a PCC OperationRegion as the shared memory fields that
> follow the PCC signature. The OperationRegion length is likewise the
> total size of the fields that succeed the signature in shared memory.
>
> ACPI 6.6 adds clarification that the PCC shared memory signature is
> populated by the platform and verified by OSPM.
>
> The PCC address space handler currently copies the OperationRegion
> buffer to and from the start of the PCC shared memory region. That can
> overwrite or expose the signature at byte offset 0, and it also misses
> the last 4 bytes of the actual PCC OperationRegion data.
>
> Offset OperationRegion copies by the size of the signature and reject
> regions that do not fit in the shared memory after that signature.
>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>

Does this depend on anything else in the series or is it independent?

> ---
>  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
>

Re: [PATCH 6/6] ACPI: PCC: Preserve shared memory signature in OpRegion handler

[Sudeep Holla]

On Tue, Jun 30, 2026 at 02:33:08PM +0200, Rafael J. Wysocki (Intel) wrote:
> On Sat, Jun 27, 2026 at 6:39 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
> >
> > ACPI defines a PCC OperationRegion as the shared memory fields that
> > follow the PCC signature. The OperationRegion length is likewise the
> > total size of the fields that succeed the signature in shared memory.
> >
> > ACPI 6.6 adds clarification that the PCC shared memory signature is
> > populated by the platform and verified by OSPM.
> >
> > The PCC address space handler currently copies the OperationRegion
> > buffer to and from the start of the PCC shared memory region. That can
> > overwrite or expose the signature at byte offset 0, and it also misses
> > the last 4 bytes of the actual PCC OperationRegion data.
> >
> > Offset OperationRegion copies by the size of the signature and reject
> > regions that do not fit in the shared memory after that signature.
> >
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
> > Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> 
> Does this depend on anything else in the series or is it independent?
> 

It is independent. This just fixes the issue of including the signature in
the opregion data. It can go independently as all other patches as well.

-- 
Regards,
Sudeep

Re: [PATCH 0/6] ACPI/PCC: Preserve platform-populated PCC signatures

[lihuisong (C)]

On 6/30/2026 6:49 PM, Sudeep Holla wrote:
> On Tue, Jun 30, 2026 at 05:25:20PM +0800, lihuisong (C) wrote:
>> Hi Sudeep,
>>
>> On 6/28/2026 12:37 AM, Sudeep Holla wrote:
>>> ACPI PCC shared memory layouts reserve the first dword for the PCC
>>> signature. ACPI specification defines the signature as 0x50434300 ORed
>>> with the PCC subspace ID, and ACPI 6.6 clarify that the signature is
>>> populated by the platform and verified by OSPM.
>>>
>>> This series centralizes PCC shared memory signature validation in the PCC
>>> mailbox controller and stops PCC users from rewriting the signature before
>>> each command. Clients that previously copied complete local PCC headers
>>> now update only the mutable command/status/flags/length/payload fields.
>> I am concerned that this may affect the functionality of drivers on some
>> existed platforms.
> One other option I was thinking is to not issue error but just log the error
> message and let the client driver add that additional check if required.
The patch 1/6 is not good to our platform.
Yeah, suggest to use gentle approach to do this if we have to clarify 
this signature.
After all, all client drivers work well before.
>> This largely depends on the implementation of the platform firmware.
>>
> Sure. However I always expected it to be taken care by the platform, v6.6
> just adds that clarification explicitly. It was never clear that the sender
> or the OSPM needs to write that signature, so I disagree that it is platform
> firmware dependent entirely. The clarification wouldn't have got added if
> there was any disagreement on that fact.
The reasons I say this largely depends on the firmware are as follows:
1> The previous ACPI spec did not clearly require that the platform 
firmware needs to fill in this signature first.
      The client driver working on these platform doesn't work anymore 
if use the way in patch 1/6.
2> We are not sure if some firmware verify this signature or clear this 
signature field in shared memory.
>
>> I think it's good for the signature to be filled in by the command initiator
>> and then verified by the recipient, as this is how this field can serve its
>> purpose.
>> Otherwise, I really don't see what use it has.
> Please get the spec updated accordingly, we can just change in the kernel
> based on what we think is the best way to use it. It needs to be clearly
> specified, otherwise it may cause issue for non-Linux OSVs.
Agree.

Yeah, I found spec v6.5 just said how to compute it and didn't specify 
how to use it for platform and OSPM.
I saw the update of v6.6 for the signature field in "Generic 
Communications Channel Shared Memory Region" and "Extended PCC Subspace 
Shared Memory Region".
like: "The signature is populated by the platform and is verified by OSPM."
But please note v6.6 didn't say above words in "Reduced PCC Subspace 
Shared Memory Region".

I don't know why v6.6 specify like that.
After all. all PCC application parties (client drivers) in Linux have 
already filled this field.

According to my understanding of normal signature in communication,
it is generally filled by the initiator (sender) and verified by the 
responder (receiver).
In this way, the data is valid, and the signature is meaningful.

If it is only initialized by firmware, and OSPM verifies it, its 
lifecycle ends there.
I don't think this signature has much significance.

The signature field is just in shared memory and may be cleared due to 
some other exception.
Some similar case also need to be considered.
How to make it work better and more resilient may also be something we 
need to consider.
>

Re: [PATCH 6/6] ACPI: PCC: Preserve shared memory signature in OpRegion handler

[Rafael J. Wysocki (Intel)]

On Tue, Jun 30, 2026 at 7:00 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
>
> On Tue, Jun 30, 2026 at 02:33:08PM +0200, Rafael J. Wysocki (Intel) wrote:
> > On Sat, Jun 27, 2026 at 6:39 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
> > >
> > > ACPI defines a PCC OperationRegion as the shared memory fields that
> > > follow the PCC signature. The OperationRegion length is likewise the
> > > total size of the fields that succeed the signature in shared memory.
> > >
> > > ACPI 6.6 adds clarification that the PCC shared memory signature is
> > > populated by the platform and verified by OSPM.
> > >
> > > The PCC address space handler currently copies the OperationRegion
> > > buffer to and from the start of the PCC shared memory region. That can
> > > overwrite or expose the signature at byte offset 0, and it also misses
> > > the last 4 bytes of the actual PCC OperationRegion data.
> > >
> > > Offset OperationRegion copies by the size of the signature and reject
> > > regions that do not fit in the shared memory after that signature.
> > >
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
> > > Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> >
> > Does this depend on anything else in the series or is it independent?
> >
>
> It is independent. This just fixes the issue of including the signature in
> the opregion data. It can go independently as all other patches as well.

So I've looked at Sashiko's feedback on this change:

https://sashiko.dev/#/patchset/20260627-acpi_pcc_signature-v1-0-c1b7268d4fdc%40kernel.org

and I'm a bit worried.

Of course, the preexisting issues found by it will need to be
addressed separately, but what about firmware released before the
clarification in ACPI 6.6 and the possible misalignment issue
mentioned by it?

Re: [PATCH 6/6] ACPI: PCC: Preserve shared memory signature in OpRegion handler

[Sudeep Holla]

On Wed, Jul 01, 2026 at 12:44:30PM +0200, Rafael J. Wysocki (Intel) wrote:
> On Tue, Jun 30, 2026 at 7:00 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
> >
> > On Tue, Jun 30, 2026 at 02:33:08PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > On Sat, Jun 27, 2026 at 6:39 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
> > > >
> > > > ACPI defines a PCC OperationRegion as the shared memory fields that
> > > > follow the PCC signature. The OperationRegion length is likewise the
> > > > total size of the fields that succeed the signature in shared memory.
> > > >
> > > > ACPI 6.6 adds clarification that the PCC shared memory signature is
> > > > populated by the platform and verified by OSPM.
> > > >
> > > > The PCC address space handler currently copies the OperationRegion
> > > > buffer to and from the start of the PCC shared memory region. That can
> > > > overwrite or expose the signature at byte offset 0, and it also misses
> > > > the last 4 bytes of the actual PCC OperationRegion data.
> > > >
> > > > Offset OperationRegion copies by the size of the signature and reject
> > > > regions that do not fit in the shared memory after that signature.
> > > >
> > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> > >
> > > Does this depend on anything else in the series or is it independent?
> > >
> >
> > It is independent. This just fixes the issue of including the signature in
> > the opregion data. It can go independently as all other patches as well.
> 
> So I've looked at Sashiko's feedback on this change:
> 
> https://sashiko.dev/#/patchset/20260627-acpi_pcc_signature-v1-0-c1b7268d4fdc%40kernel.org
> 
> and I'm a bit worried.
> 
> Of course, the preexisting issues found by it will need to be
> addressed separately, but what about firmware released before the
> clarification in ACPI 6.6 and the possible misalignment issue
> mentioned by it?

I haven't analysed all the report from Sashiko yet. But unlike other patches
in the series, this one is not strictly to validate the signature.

Even before v6.6, the PCC Opregion didn't include the signature which is
at offset 0. The code always started copying to/from offset 0 missing the
last 32-bit field(word). Since v6.3 when PCC Opregion was added we have
this in the specification:

  | <section> Declaring message fields within a PCC OperationRegion
  |
  | For all PCC subspace types, the PCC Operation Region pertains to the
  | region of PCC subspace that succeeds the PCC signature.

So, this patch addresses that issue of wrong starting offset used by this
driver.

Other patches I agree, it is more likely we just need to warn and not
reject absence of wrong PCC signatures.

I will take a look at the other issues reported by Sashiko soon.

-- 
Regards,
Sudeep

Re: [PATCH 6/6] ACPI: PCC: Preserve shared memory signature in OpRegion handler

[Sudeep Holla]

On Wed, Jul 01, 2026 at 02:34:10PM +0100, Sudeep Holla wrote:
> On Wed, Jul 01, 2026 at 12:44:30PM +0200, Rafael J. Wysocki (Intel) wrote:
> > On Tue, Jun 30, 2026 at 7:00 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
> > >
> > > On Tue, Jun 30, 2026 at 02:33:08PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > On Sat, Jun 27, 2026 at 6:39 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
> > > > >
> > > > > ACPI defines a PCC OperationRegion as the shared memory fields that
> > > > > follow the PCC signature. The OperationRegion length is likewise the
> > > > > total size of the fields that succeed the signature in shared memory.
> > > > >
> > > > > ACPI 6.6 adds clarification that the PCC shared memory signature is
> > > > > populated by the platform and verified by OSPM.
> > > > >
> > > > > The PCC address space handler currently copies the OperationRegion
> > > > > buffer to and from the start of the PCC shared memory region. That can
> > > > > overwrite or expose the signature at byte offset 0, and it also misses
> > > > > the last 4 bytes of the actual PCC OperationRegion data.
> > > > >
> > > > > Offset OperationRegion copies by the size of the signature and reject
> > > > > regions that do not fit in the shared memory after that signature.
> > > > >
> > > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > > Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
> > > > > Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> > > >
> > > > Does this depend on anything else in the series or is it independent?
> > > >
> > >
> > > It is independent. This just fixes the issue of including the signature in
> > > the opregion data. It can go independently as all other patches as well.
> > 
> > So I've looked at Sashiko's feedback on this change:
> > 
> > https://sashiko.dev/#/patchset/20260627-acpi_pcc_signature-v1-0-c1b7268d4fdc%40kernel.org
> > 
> > and I'm a bit worried.
> > 
> > Of course, the preexisting issues found by it will need to be
> > addressed separately, but what about firmware released before the
> > clarification in ACPI 6.6 and the possible misalignment issue
> > mentioned by it?
> 
> I haven't analysed all the report from Sashiko yet. But unlike other patches
> in the series, this one is not strictly to validate the signature.
> 
> Even before v6.6, the PCC Opregion didn't include the signature which is
> at offset 0. The code always started copying to/from offset 0 missing the
> last 32-bit field(word). Since v6.3 when PCC Opregion was added we have
> this in the specification:
> 
>   | <section> Declaring message fields within a PCC OperationRegion
>   |
>   | For all PCC subspace types, the PCC Operation Region pertains to the
>   | region of PCC subspace that succeeds the PCC signature.
> 
> So, this patch addresses that issue of wrong starting offset used by this
> driver.
> 
> Other patches I agree, it is more likely we just need to warn and not
> reject absence of wrong PCC signatures.
> 
> I will take a look at the other issues reported by Sashiko soon.
> 

Forgot to add that let us wait at-least until I go through the report
understand and conclude it is not fatal/critical before we can merge this.
Indeed a quick read concerned as much that I would like to spend time
analysing it carefully.

-- 
Regards,
Sudeep

Re: [PATCH 6/6] ACPI: PCC: Preserve shared memory signature in OpRegion handler

[Rafael J. Wysocki (Intel)]

On Wed, Jul 1, 2026 at 4:13 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
>
> On Wed, Jul 01, 2026 at 02:34:10PM +0100, Sudeep Holla wrote:
> > On Wed, Jul 01, 2026 at 12:44:30PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > On Tue, Jun 30, 2026 at 7:00 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
> > > >
> > > > On Tue, Jun 30, 2026 at 02:33:08PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > > On Sat, Jun 27, 2026 at 6:39 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
> > > > > >
> > > > > > ACPI defines a PCC OperationRegion as the shared memory fields that
> > > > > > follow the PCC signature. The OperationRegion length is likewise the
> > > > > > total size of the fields that succeed the signature in shared memory.
> > > > > >
> > > > > > ACPI 6.6 adds clarification that the PCC shared memory signature is
> > > > > > populated by the platform and verified by OSPM.
> > > > > >
> > > > > > The PCC address space handler currently copies the OperationRegion
> > > > > > buffer to and from the start of the PCC shared memory region. That can
> > > > > > overwrite or expose the signature at byte offset 0, and it also misses
> > > > > > the last 4 bytes of the actual PCC OperationRegion data.
> > > > > >
> > > > > > Offset OperationRegion copies by the size of the signature and reject
> > > > > > regions that do not fit in the shared memory after that signature.
> > > > > >
> > > > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > > > Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
> > > > > > Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> > > > >
> > > > > Does this depend on anything else in the series or is it independent?
> > > > >
> > > >
> > > > It is independent. This just fixes the issue of including the signature in
> > > > the opregion data. It can go independently as all other patches as well.
> > >
> > > So I've looked at Sashiko's feedback on this change:
> > >
> > > https://sashiko.dev/#/patchset/20260627-acpi_pcc_signature-v1-0-c1b7268d4fdc%40kernel.org
> > >
> > > and I'm a bit worried.
> > >
> > > Of course, the preexisting issues found by it will need to be
> > > addressed separately, but what about firmware released before the
> > > clarification in ACPI 6.6 and the possible misalignment issue
> > > mentioned by it?
> >
> > I haven't analysed all the report from Sashiko yet. But unlike other patches
> > in the series, this one is not strictly to validate the signature.
> >
> > Even before v6.6, the PCC Opregion didn't include the signature which is
> > at offset 0. The code always started copying to/from offset 0 missing the
> > last 32-bit field(word). Since v6.3 when PCC Opregion was added we have
> > this in the specification:
> >
> >   | <section> Declaring message fields within a PCC OperationRegion
> >   |
> >   | For all PCC subspace types, the PCC Operation Region pertains to the
> >   | region of PCC subspace that succeeds the PCC signature.
> >
> > So, this patch addresses that issue of wrong starting offset used by this
> > driver.
> >
> > Other patches I agree, it is more likely we just need to warn and not
> > reject absence of wrong PCC signatures.
> >
> > I will take a look at the other issues reported by Sashiko soon.
> >
>
> Forgot to add that let us wait at-least until I go through the report
> understand and conclude it is not fatal/critical before we can merge this.
> Indeed a quick read concerned as much that I would like to spend time
> analysing it carefully.

OK

I think that it would be good to update the changelog of the $subject
patch to clarify that the bug fixed by it is with respect to the
original definition in ACPI 6.3 and not with respect to the ACPI 6.6
clarification, so there is no legacy firmware that might be affected
by it.

Re: [PATCH 0/6] ACPI/PCC: Preserve platform-populated PCC signatures

[Sudeep Holla]

On Wed, Jul 01, 2026 at 03:38:21PM +0800, lihuisong (C) wrote:
> 
> On 6/30/2026 6:49 PM, Sudeep Holla wrote:
> > On Tue, Jun 30, 2026 at 05:25:20PM +0800, lihuisong (C) wrote:
> > > Hi Sudeep,
> > > 
> > > On 6/28/2026 12:37 AM, Sudeep Holla wrote:
> > > > ACPI PCC shared memory layouts reserve the first dword for the PCC
> > > > signature. ACPI specification defines the signature as 0x50434300 ORed
> > > > with the PCC subspace ID, and ACPI 6.6 clarify that the signature is
> > > > populated by the platform and verified by OSPM.
> > > > 
> > > > This series centralizes PCC shared memory signature validation in the PCC
> > > > mailbox controller and stops PCC users from rewriting the signature before
> > > > each command. Clients that previously copied complete local PCC headers
> > > > now update only the mutable command/status/flags/length/payload fields.
> > > I am concerned that this may affect the functionality of drivers on some
> > > existed platforms.
> > One other option I was thinking is to not issue error but just log the error
> > message and let the client driver add that additional check if required.
> The patch 1/6 is not good to our platform.
> Yeah, suggest to use gentle approach to do this if we have to clarify this
> signature.

Fair enough, we can just log the warning for now.

> After all, all client drivers work well before.

Sure, but not doing anything will just make the bug carry on for ever in
the firmware. SO logging warning is minimum we should do IMO.

> > > This largely depends on the implementation of the platform firmware.
> > > 
> > Sure. However I always expected it to be taken care by the platform, v6.6
> > just adds that clarification explicitly. It was never clear that the sender
> > or the OSPM needs to write that signature, so I disagree that it is platform
> > firmware dependent entirely. The clarification wouldn't have got added if
> > there was any disagreement on that fact.
> The reasons I say this largely depends on the firmware are as follows:
> 1> The previous ACPI spec did not clearly require that the platform firmware
> needs to fill in this signature first.
>      The client driver working on these platform doesn't work anymore if use
> the way in patch 1/6.

Fair enough.

> 2> We are not sure if some firmware verify this signature or clear this
> signature field in shared memory.

Anyways, better to check that for current and future platforms if not the
legacy ones.

> > 
> > > I think it's good for the signature to be filled in by the command initiator
> > > and then verified by the recipient, as this is how this field can serve its
> > > purpose.
> > > Otherwise, I really don't see what use it has.
> > Please get the spec updated accordingly, we can just change in the kernel
> > based on what we think is the best way to use it. It needs to be clearly
> > specified, otherwise it may cause issue for non-Linux OSVs.
> Agree.
> 
> Yeah, I found spec v6.5 just said how to compute it and didn't specify how
> to use it for platform and OSPM.
> I saw the update of v6.6 for the signature field in "Generic Communications
> Channel Shared Memory Region" and "Extended PCC Subspace Shared Memory
> Region".
> like: "The signature is populated by the platform and is verified by OSPM."
> But please note v6.6 didn't say above words in "Reduced PCC Subspace Shared
> Memory Region".
> 

Ah, that's just inconsistency I believe. I will raise a defect.

> I don't know why v6.6 specify like that.
> After all. all PCC application parties (client drivers) in Linux have
> already filled this field.
> 

I don't think what Linux drivers should be the reference as there are other
OSVs.

> According to my understanding of normal signature in communication,
> it is generally filled by the initiator (sender) and verified by the
> responder (receiver).
> In this way, the data is valid, and the signature is meaningful.
> 
> If it is only initialized by firmware, and OSPM verifies it, its lifecycle
> ends there.
> I don't think this signature has much significance.
> 
> The signature field is just in shared memory and may be cleared due to some
> other exception.
> Some similar case also need to be considered.
> How to make it work better and more resilient may also be something we need
> to consider.

Not sure if I follow that. The way I expect is platform won't populate the
signature if everything is not initialised and running at it's end which
implies it is not ready to accept the request.

-- 
Regards,
Sudeep

Re: [PATCH 4/6] devfreq: hisi_uncore: Preserve PCC shared memory signature

[Sudeep Holla]

On Sat, Jun 27, 2026 at 05:37:32PM +0100, Sudeep Holla wrote:
> ACPI specification defines the generic PCC shared memory signature as
> the PCC base signature ORed with the subspace ID.
> 
> ACPI 6.6 added clarification that the signature is populated by the
> platform and verified by OSPM.
> 
> The PCC mailbox controller now validates the signature when the channel
> is requested. Stop copying a complete local PCC header into shared
> memory and write only the command, status and payload fields, leaving
> the platform-populated signature intact.
> 

Based on the discussions in 1/6, I assume 4/6 and 5/6 doesn't work well
for Hisilicon platforms. So I will drop them. I will just post 1/6 reducing
it to a warning.

-- 
Regards,
Sudeep

Re: [PATCH 6/6] ACPI: PCC: Preserve shared memory signature in OpRegion handler

[Sudeep Holla]

On Wed, Jul 01, 2026 at 03:13:42PM +0100, Sudeep Holla wrote:
> On Wed, Jul 01, 2026 at 02:34:10PM +0100, Sudeep Holla wrote:
> > On Wed, Jul 01, 2026 at 12:44:30PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > On Tue, Jun 30, 2026 at 7:00 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
> > > >
> > > > On Tue, Jun 30, 2026 at 02:33:08PM +0200, Rafael J. Wysocki (Intel) wrote:
> > > > > On Sat, Jun 27, 2026 at 6:39 PM Sudeep Holla <sudeep.holla@kernel.org> wrote:
> > > > > >
> > > > > > ACPI defines a PCC OperationRegion as the shared memory fields that
> > > > > > follow the PCC signature. The OperationRegion length is likewise the
> > > > > > total size of the fields that succeed the signature in shared memory.
> > > > > >
> > > > > > ACPI 6.6 adds clarification that the PCC shared memory signature is
> > > > > > populated by the platform and verified by OSPM.
> > > > > >
> > > > > > The PCC address space handler currently copies the OperationRegion
> > > > > > buffer to and from the start of the PCC shared memory region. That can
> > > > > > overwrite or expose the signature at byte offset 0, and it also misses
> > > > > > the last 4 bytes of the actual PCC OperationRegion data.
> > > > > >
> > > > > > Offset OperationRegion copies by the size of the signature and reject
> > > > > > regions that do not fit in the shared memory after that signature.
> > > > > >
> > > > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > > > Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
> > > > > > Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> > > > >
> > > > > Does this depend on anything else in the series or is it independent?
> > > > >
> > > >
> > > > It is independent. This just fixes the issue of including the signature in
> > > > the opregion data. It can go independently as all other patches as well.
> > > 
> > > So I've looked at Sashiko's feedback on this change:
> > > 
> > > https://sashiko.dev/#/patchset/20260627-acpi_pcc_signature-v1-0-c1b7268d4fdc%40kernel.org
> > > 
> > > and I'm a bit worried.
> > > 
> > > Of course, the preexisting issues found by it will need to be
> > > addressed separately, but what about firmware released before the
> > > clarification in ACPI 6.6 and the possible misalignment issue
> > > mentioned by it?
> > 
> > I haven't analysed all the report from Sashiko yet. But unlike other patches
> > in the series, this one is not strictly to validate the signature.
> > 
> > Even before v6.6, the PCC Opregion didn't include the signature which is
> > at offset 0. The code always started copying to/from offset 0 missing the
> > last 32-bit field(word). Since v6.3 when PCC Opregion was added we have
> > this in the specification:
> > 
> >   | <section> Declaring message fields within a PCC OperationRegion
> >   |
> >   | For all PCC subspace types, the PCC Operation Region pertains to the
> >   | region of PCC subspace that succeeds the PCC signature.
> > 
> > So, this patch addresses that issue of wrong starting offset used by this
> > driver.
> > 
> > Other patches I agree, it is more likely we just need to warn and not
> > reject absence of wrong PCC signatures.
> > 
> > I will take a look at the other issues reported by Sashiko soon.
> > 
> 
> Forgot to add that let us wait at-least until I go through the report
> understand and conclude it is not fatal/critical before we can merge this.
> Indeed a quick read concerned as much that I would like to spend time
> analysing it carefully.
> 

Looking at the comments and digging ACPICA in more detail, I found the issue
exist there as well and that is the reason why we hadn't observed it in the
kernel. I will first post the fix for that and follow up here.

Thanks.

-- 
Regards,
Sudeep

Re: [PATCH 0/6] ACPI/PCC: Preserve platform-populated PCC signatures

[lihuisong (C)]

On 7/2/2026 6:21 PM, Sudeep Holla wrote:
> On Wed, Jul 01, 2026 at 03:38:21PM +0800, lihuisong (C) wrote:
>> On 6/30/2026 6:49 PM, Sudeep Holla wrote:
>>> On Tue, Jun 30, 2026 at 05:25:20PM +0800, lihuisong (C) wrote:
>>>> Hi Sudeep,
>>>>
>>>> On 6/28/2026 12:37 AM, Sudeep Holla wrote:
>>>>> ACPI PCC shared memory layouts reserve the first dword for the PCC
>>>>> signature. ACPI specification defines the signature as 0x50434300 ORed
>>>>> with the PCC subspace ID, and ACPI 6.6 clarify that the signature is
>>>>> populated by the platform and verified by OSPM.
>>>>>
>>>>> This series centralizes PCC shared memory signature validation in the PCC
>>>>> mailbox controller and stops PCC users from rewriting the signature before
>>>>> each command. Clients that previously copied complete local PCC headers
>>>>> now update only the mutable command/status/flags/length/payload fields.
>>>> I am concerned that this may affect the functionality of drivers on some
>>>> existed platforms.
>>> One other option I was thinking is to not issue error but just log the error
>>> message and let the client driver add that additional check if required.
>> The patch 1/6 is not good to our platform.
>> Yeah, suggest to use gentle approach to do this if we have to clarify this
>> signature.
> Fair enough, we can just log the warning for now.
Ack.
>
>> After all, all client drivers work well before.
> Sure, but not doing anything will just make the bug carry on for ever in
> the firmware. SO logging warning is minimum we should do IMO.
Agree. We need to clearify this based on spec.
>
>>>> This largely depends on the implementation of the platform firmware.
>>>>
>>> Sure. However I always expected it to be taken care by the platform, v6.6
>>> just adds that clarification explicitly. It was never clear that the sender
>>> or the OSPM needs to write that signature, so I disagree that it is platform
>>> firmware dependent entirely. The clarification wouldn't have got added if
>>> there was any disagreement on that fact.
>> The reasons I say this largely depends on the firmware are as follows:
>> 1> The previous ACPI spec did not clearly require that the platform firmware
>> needs to fill in this signature first.
>>       The client driver working on these platform doesn't work anymore if use
>> the way in patch 1/6.
> Fair enough.
>
>> 2> We are not sure if some firmware verify this signature or clear this
>> signature field in shared memory.
> Anyways, better to check that for current and future platforms if not the
> legacy ones.
>
>>>> I think it's good for the signature to be filled in by the command initiator
>>>> and then verified by the recipient, as this is how this field can serve its
>>>> purpose.
>>>> Otherwise, I really don't see what use it has.
>>> Please get the spec updated accordingly, we can just change in the kernel
>>> based on what we think is the best way to use it. It needs to be clearly
>>> specified, otherwise it may cause issue for non-Linux OSVs.
>> Agree.
>>
>> Yeah, I found spec v6.5 just said how to compute it and didn't specify how
>> to use it for platform and OSPM.
>> I saw the update of v6.6 for the signature field in "Generic Communications
>> Channel Shared Memory Region" and "Extended PCC Subspace Shared Memory
>> Region".
>> like: "The signature is populated by the platform and is verified by OSPM."
>> But please note v6.6 didn't say above words in "Reduced PCC Subspace Shared
>> Memory Region".
>>
> Ah, that's just inconsistency I believe. I will raise a defect.
Nice.
>
>> I don't know why v6.6 specify like that.
>> After all. all PCC application parties (client drivers) in Linux have
>> already filled this field.
>>
> I don't think what Linux drivers should be the reference as there are other
> OSVs.
Agree.
>> According to my understanding of normal signature in communication,
>> it is generally filled by the initiator (sender) and verified by the
>> responder (receiver).
>> In this way, the data is valid, and the signature is meaningful.
>>
>> If it is only initialized by firmware, and OSPM verifies it, its lifecycle
>> ends there.
>> I don't think this signature has much significance.
>>
>> The signature field is just in shared memory and may be cleared due to some
>> other exception.
>> Some similar case also need to be considered.
>> How to make it work better and more resilient may also be something we need
>> to consider.
> Not sure if I follow that. The way I expect is platform won't populate the
> signature if everything is not initialised and running at it's end which
> implies it is not ready to accept the request.
It can also be defined this way.
But platform firmware is generally ready to accept request when OS is 
running.
If platform isn't ready to accept the request from OSPM, the command 
will execute failed.
So it seems that the signature is a bit redundant if it is only used to 
indicate that the platform is ready to receive data.

Anyway, I follow spec, just does not understand the signature usage spec 
6.6 added.

Re: [PATCH 4/6] devfreq: hisi_uncore: Preserve PCC shared memory signature

[lihuisong (C)]

On 7/2/2026 6:25 PM, Sudeep Holla wrote:
> On Sat, Jun 27, 2026 at 05:37:32PM +0100, Sudeep Holla wrote:
>> ACPI specification defines the generic PCC shared memory signature as
>> the PCC base signature ORed with the subspace ID.
>>
>> ACPI 6.6 added clarification that the signature is populated by the
>> platform and verified by OSPM.
>>
>> The PCC mailbox controller now validates the signature when the channel
>> is requested. Stop copying a complete local PCC header into shared
>> memory and write only the command, status and payload fields, leaving
>> the platform-populated signature intact.
>>
> Based on the discussions in 1/6, I assume 4/6 and 5/6 doesn't work well
> for Hisilicon platforms. So I will drop them. I will just post 1/6 reducing
> it to a warning.
Thanks for reducing error level.
We will test them to make sure if these driver work well after removing 
the signature filling.
Then will update these code in driver if it's ok.
>