[PATCH v2 0/3] mailbox: pcc: Improve completion handling and validation

[PATCH v2 0/3] mailbox: pcc: Improve completion handling and validation

[Sudeep Holla]

Hi Jassi,

This series addresses three issues in the PCC mailbox controller which
are on the list for a while and discussed/reviewed. Please pick them
for v7.3 or let me know if you prefer pull request.

First, notify clients through their receive callback when a command
completes through polling, matching the interrupt-driven completion
path.

Second, centralize PCC shared memory signature checking in the mailbox
controller. Invalid signatures are reported without rejecting otherwise
working firmware, while mappings too small to contain a signature remain
fatal.

Finally, fix a race where a fast platform can signal command completion
before the channel is marked in use, causing a shared interrupt to be
ignored and the command to time out. Publish the channel state before
ringing the doorbell and use READ_ONCE()/WRITE_ONCE() for the lockless
flag accesses.


Regards,
Sudeep

Changes in v2:
- Reworked to keep chan_in_use set after a successful doorbell write
  and clear it only when the write fails.
- Clarified that ordering is provided by the ordered I/O accessor,
  covering both MMIO and port I/O doorbells.
- Clarified the cross-CPU flag access and interrupt-side clearing
  before the mailbox core can submit another transfer.
- Link to v1: https://lore.kernel.org/all/20260717075649.467172-1-sudeep.holla@kernel.org

Huisong Li (1):
  mailbox: pcc: Fix command timeout due to missed interrupt

Sudeep Holla (2):
  mailbox: pcc: Notify clients on polled completion
  mailbox: pcc: Check shared memory signature on request

 drivers/mailbox/pcc.c | 87 +++++++++++++++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 20 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] mailbox: pcc: Notify clients on polled completion

[Sudeep Holla]

PCC channels without a platform interrupt rely on the mailbox
polling path to detect command completion.

That path currently only reports transmit completion to the mailbox
core, so clients that wait for their receive callback do not get
notified when the command completes.

Call mbox_chan_received_data() when polling observes completion on a
channel without a platform IRQ, matching the interrupt-driven
completion path.

Reported-by: Cristian Marussi <cristian.marussi@arm.com>
Acked-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/mailbox/pcc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 636879ae1db7..d96b8b54e77e 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -449,7 +449,15 @@ static bool pcc_last_tx_done(struct mbox_chan *chan)
 {
 	struct pcc_chan_info *pchan = chan->con_priv;
 
-	return pcc_mbox_cmd_complete_check(pchan);
+	if (!(chan->txdone_method & MBOX_TXDONE_BY_POLL))
+		return false;
+
+	if (!pcc_mbox_cmd_complete_check(pchan))
+		return false;
+
+	mbox_chan_received_data(chan, NULL);
+
+	return true;
 }
 
 /**
-- 
2.43.0

Re: [PATCH v2 1/3] mailbox: pcc: Notify clients on polled completion

[Adam Young]

On 7/23/26 10:39, Sudeep Holla wrote:
> PCC channels without a platform interrupt rely on the mailbox
> polling path to detect command completion.
>
> That path currently only reports transmit completion to the mailbox
> core, so clients that wait for their receive callback do not get
> notified when the command completes.
>
> Call mbox_chan_received_data() when polling observes completion on a
> channel without a platform IRQ, matching the interrupt-driven
> completion path.
>
> Reported-by: Cristian Marussi <cristian.marussi@arm.com>
> Acked-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> ---
>   drivers/mailbox/pcc.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 636879ae1db7..d96b8b54e77e 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -449,7 +449,15 @@ static bool pcc_last_tx_done(struct mbox_chan *chan)
>   {
>   	struct pcc_chan_info *pchan = chan->con_priv;
>   
> -	return pcc_mbox_cmd_complete_check(pchan);
> +	if (!(chan->txdone_method & MBOX_TXDONE_BY_POLL))
> +		return false;
> +
> +	if (!pcc_mbox_cmd_complete_check(pchan))
> +		return false;
> +
> +	mbox_chan_received_data(chan, NULL);
> +
> +	return true;
>   }
>   
>   /**
Reviewed-by: Adam Young <admiyo@os.amperecomputing.com>

[PATCH v2 2/3] mailbox: pcc: Check shared memory signature on request

[Sudeep Holla]

ACPI 6.6 Tables 14.9 and 14.12 define the PCC shared memory
signature as the bitwise OR of 0x50434300 and the PCC subspace ID.
They also clarify that the signature is populated by the platform and
verified by OSPM. The signature is at byte offset 0 in the generic,
extended and reduced PCC shared memory layouts.

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

Treat a signature mismatch as a warning rather than rejecting the
channel request. Making this newly added check fatal could break
existing systems whose firmware did not populate the signature
correctly even though PCC communication works. Continue to reject
shared memory that is too small to contain a signature because it
cannot be inspected safely.

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 | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index d96b8b54e77e..8dfa80b0a90f 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -345,6 +345,26 @@ 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_warn("PCC subspace %d invalid signature %#x expected %#x\n",
+			subspace_id, signature, expected_signature);
+
+	return 0;
+}
+
 /**
  * pcc_mbox_request_channel - PCC clients call this function to
  *		request a pointer to their PCC subspace, from which they
@@ -381,14 +401,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

Re: [PATCH v2 2/3] mailbox: pcc: Check shared memory signature on request

[Adam Young]

On 7/23/26 10:39, Sudeep Holla wrote:
> ACPI 6.6 Tables 14.9 and 14.12 define the PCC shared memory
> signature as the bitwise OR of 0x50434300 and the PCC subspace ID.
> They also clarify that the signature is populated by the platform and
> verified by OSPM. The signature is at byte offset 0 in the generic,
> extended and reduced PCC shared memory layouts.
>
> Check the signature when a client requests a PCC mailbox channel,
> after mapping shared memory and before binding the mailbox client.
> This keeps the check in the PCC mailbox controller instead of
> duplicating it in individual clients.
>
> Treat a signature mismatch as a warning rather than rejecting the
> channel request. Making this newly added check fatal could break
> existing systems whose firmware did not populate the signature
> correctly even though PCC communication works. Continue to reject
> shared memory that is too small to contain a signature because it
> cannot be inspected safely.
>
> 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 | 36 +++++++++++++++++++++++++++++++-----
>   1 file changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index d96b8b54e77e..8dfa80b0a90f 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -345,6 +345,26 @@ 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_warn("PCC subspace %d invalid signature %#x expected %#x\n",
> +			subspace_id, signature, expected_signature);
> +
> +	return 0;
> +}
> +
>   /**
>    * pcc_mbox_request_channel - PCC clients call this function to
>    *		request a pointer to their PCC subspace, from which they
> @@ -381,14 +401,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);
>   
Tested_by: Adam Young <admiyo@os.amperecomputing.com>

[PATCH v2 3/3] mailbox: pcc: Fix command timeout due to missed interrupt

[Sudeep Holla]

From: Huisong Li <lihuisong@huawei.com>

PCC command execution can time out when a fast platform completes a
transaction and signals the platform interrupt before pcc_send_data()
marks the channel as in use. For shared platform interrupts, the type 3
handler uses chan_in_use to decide whether the interrupt belongs to the
channel. If it observes false, it ignores the completion and the caller
waits until timeout.

Publish chan_in_use before ringing the doorbell. Use WRITE_ONCE() for
the lockless flag updates and READ_ONCE() in the interrupt handler. The
following ordered I/O accessor orders the flag store before the platform
is notified.

Clear chan_in_use if ringing the doorbell fails. Otherwise, leave it set
until the interrupt handler completes the transaction, clearing it before
the mailbox core can submit another transfer.

Fixes: 3db174e478cb ("mailbox: pcc: Support shared interrupt for multiple subspaces")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/mailbox/pcc.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 8dfa80b0a90f..9888dab64639 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -91,12 +91,11 @@ struct pcc_chan_reg {
  * @plat_irq: platform interrupt
  * @type: PCC subspace type
  * @plat_irq_flags: platform interrupt flags
- * @chan_in_use: this flag is used just to check if the interrupt needs
- *		handling when it is shared. Since only one transfer can occur
- *		at a time and mailbox takes care of locking, this flag can be
- *		accessed without a lock. Note: the type only support the
- *		communication from OSPM to Platform, like type3, use it, and
- *		other types completely ignore it.
+ * @chan_in_use: lockless flag used by type 3 initiator subspaces to filter
+ *		platform interrupts. Only one transfer can occur at a time, but
+ *		the interrupt handler may sample the flag on another CPU, so all
+ *		accesses must use READ_ONCE() or WRITE_ONCE(). Other subspace
+ *		types do not test it.
  */
 struct pcc_chan_info {
 	struct pcc_mbox_chan chan;
@@ -320,8 +319,13 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
 		return IRQ_NONE;
 
+	/*
+	 * Initiator subspaces use this flag to filter shared interrupts. Use
+	 * READ_ONCE() to sample the lockless flag written by pcc_send_data()
+	 * on another CPU.
+	 */
 	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE &&
-	    !pchan->chan_in_use)
+	    !READ_ONCE(pchan->chan_in_use))
 		return IRQ_NONE;
 
 	if (!pcc_mbox_cmd_complete_check(pchan))
@@ -331,12 +335,12 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 		return IRQ_NONE;
 
 	/*
-	 * Clear this flag after updating interrupt ack register and just
-	 * before mbox_chan_received_data() which might call pcc_send_data()
-	 * where the flag is set again to start new transfer. This is
-	 * required to avoid any possible race in updatation of this flag.
+	 * Clear this flag after updating the interrupt ack register and before
+	 * notifying the client and mailbox core. mbox_chan_txdone() may submit
+	 * the next queued transfer and set the flag again. Use WRITE_ONCE() for
+	 * the lockless update observed by the send and interrupt paths.
 	 */
-	pchan->chan_in_use = false;
+	WRITE_ONCE(pchan->chan_in_use, false);
 	mbox_chan_received_data(chan, NULL);
 	mbox_chan_txdone(chan, 0);
 
@@ -464,9 +468,18 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
 	if (ret)
 		return ret;
 
+	/*
+	 * Set chan_in_use before ringing the doorbell so a fast completion
+	 * interrupt is not mistaken for a shared interrupt from another
+	 * subspace. Use WRITE_ONCE() for the lockless flag update. The
+	 * ordered I/O accessor used to ring the doorbell orders this store
+	 * before the platform is notified.
+	 */
+	if (pchan->plat_irq > 0)
+		WRITE_ONCE(pchan->chan_in_use, true);
 	ret = pcc_chan_reg_read_modify_write(&pchan->db);
-	if (!ret && pchan->plat_irq > 0)
-		pchan->chan_in_use = true;
+	if (ret && pchan->plat_irq > 0)
+		WRITE_ONCE(pchan->chan_in_use, false);
 
 	return ret;
 }
-- 
2.43.0

Re: [PATCH v2 3/3] mailbox: pcc: Fix command timeout due to missed interrupt

[Adam Young]

On 7/23/26 10:39, Sudeep Holla wrote:
> From: Huisong Li <lihuisong@huawei.com>
>
> PCC command execution can time out when a fast platform completes a
> transaction and signals the platform interrupt before pcc_send_data()
> marks the channel as in use. For shared platform interrupts, the type 3
> handler uses chan_in_use to decide whether the interrupt belongs to the
> channel. If it observes false, it ignores the completion and the caller
> waits until timeout.
>
> Publish chan_in_use before ringing the doorbell. Use WRITE_ONCE() for
> the lockless flag updates and READ_ONCE() in the interrupt handler. The
> following ordered I/O accessor orders the flag store before the platform
> is notified.
>
> Clear chan_in_use if ringing the doorbell fails. Otherwise, leave it set
> until the interrupt handler completes the transaction, clearing it before
> the mailbox core can submit another transfer.
>
> Fixes: 3db174e478cb ("mailbox: pcc: Support shared interrupt for multiple subspaces")
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> ---
>   drivers/mailbox/pcc.c | 41 +++++++++++++++++++++++++++--------------
>   1 file changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 8dfa80b0a90f..9888dab64639 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -91,12 +91,11 @@ struct pcc_chan_reg {
>    * @plat_irq: platform interrupt
>    * @type: PCC subspace type
>    * @plat_irq_flags: platform interrupt flags
> - * @chan_in_use: this flag is used just to check if the interrupt needs
> - *		handling when it is shared. Since only one transfer can occur
> - *		at a time and mailbox takes care of locking, this flag can be
> - *		accessed without a lock. Note: the type only support the
> - *		communication from OSPM to Platform, like type3, use it, and
> - *		other types completely ignore it.
> + * @chan_in_use: lockless flag used by type 3 initiator subspaces to filter
> + *		platform interrupts. Only one transfer can occur at a time, but
> + *		the interrupt handler may sample the flag on another CPU, so all
> + *		accesses must use READ_ONCE() or WRITE_ONCE(). Other subspace
> + *		types do not test it.
>    */
>   struct pcc_chan_info {
>   	struct pcc_mbox_chan chan;
> @@ -320,8 +319,13 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
>   		return IRQ_NONE;
>   
> +	/*
> +	 * Initiator subspaces use this flag to filter shared interrupts. Use
> +	 * READ_ONCE() to sample the lockless flag written by pcc_send_data()
> +	 * on another CPU.
> +	 */
>   	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE &&
> -	    !pchan->chan_in_use)
> +	    !READ_ONCE(pchan->chan_in_use))
>   		return IRQ_NONE;
>   
>   	if (!pcc_mbox_cmd_complete_check(pchan))
> @@ -331,12 +335,12 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   		return IRQ_NONE;
>   
>   	/*
> -	 * Clear this flag after updating interrupt ack register and just
> -	 * before mbox_chan_received_data() which might call pcc_send_data()
> -	 * where the flag is set again to start new transfer. This is
> -	 * required to avoid any possible race in updatation of this flag.
> +	 * Clear this flag after updating the interrupt ack register and before
> +	 * notifying the client and mailbox core. mbox_chan_txdone() may submit
> +	 * the next queued transfer and set the flag again. Use WRITE_ONCE() for
> +	 * the lockless update observed by the send and interrupt paths.
>   	 */
> -	pchan->chan_in_use = false;
> +	WRITE_ONCE(pchan->chan_in_use, false);
>   	mbox_chan_received_data(chan, NULL);
>   	mbox_chan_txdone(chan, 0);
>   
> @@ -464,9 +468,18 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
>   	if (ret)
>   		return ret;
>   
> +	/*
> +	 * Set chan_in_use before ringing the doorbell so a fast completion
> +	 * interrupt is not mistaken for a shared interrupt from another
> +	 * subspace. Use WRITE_ONCE() for the lockless flag update. The
> +	 * ordered I/O accessor used to ring the doorbell orders this store
> +	 * before the platform is notified.
> +	 */
> +	if (pchan->plat_irq > 0)
> +		WRITE_ONCE(pchan->chan_in_use, true);
>   	ret = pcc_chan_reg_read_modify_write(&pchan->db);
> -	if (!ret && pchan->plat_irq > 0)
> -		pchan->chan_in_use = true;
> +	if (ret && pchan->plat_irq > 0)
> +		WRITE_ONCE(pchan->chan_in_use, false);
>   
>   	return ret;
>   }

Tested-by: Adam Young <admiyo@os.amperecomputing.com>