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

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

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 | 85 +++++++++++++++++++++++++++++++++----------
 1 file changed, 66 insertions(+), 19 deletions(-)

-- 
2.43.0

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

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

[PATCH 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 update and READ_ONCE() in the interrupt handler when
filtering shared interrupts. The following ordered MMIO accessor used for
the doorbell keeps the earlier store visible before the platform is
notified.

Clear chan_in_use with WRITE_ONCE() when completing the transaction,
matching the lockless flag protocol used by the interrupt and send paths.

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 | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 8dfa80b0a90f..7f99d4ab0129 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 initiator subspaces, such as type 3,
+ *		to filter shared 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 ignore 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;
 
+	/*
+	 * Master 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 just
+	 * before mbox_chan_received_data(), which might call pcc_send_data()
+	 * and set the flag again to start a new transfer. 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 MMIO accessor used to ring the doorbell keeps this store
+	 * visible 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;
+		WRITE_ONCE(pchan->chan_in_use, false);
 
 	return ret;
 }
-- 
2.43.0

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

[lihuisong (C)]

On 7/17/2026 3:56 PM, 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>
> ---
LGTM,
Acked-by: Huisong Li <lihuisong@huawei.com>

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

[Alexey Klimov]

On Fri Jul 17, 2026 at 8:56 AM BST, 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 +++++++++++++++++++++++++++++++-----

[..]

> +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);

IIRC signature comes from table, so I'd even add smth like
"Potentially broken firmware: PCC subspace %d invalid signature %#x expected %#x\n"
or "PCC subspace %d invalid signature %#x expected %#x, broken firmware\n" or
similar to encourage reader/consumer of the message to report bugs to
firmware provider :)
Maybe it will be even fixed.

Anyhow, regardless of above:

Reviewed-by: Alexey Klimov <alexey.klimov@linaro.org>

Best regards,
Alexey

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

[Adam Young]

 
+	/*
+	 * 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 MMIO accessor used to ring the doorbell keeps this store
+	 * visible 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;
+		WRITE_ONCE(pchan->chan_in_use, false);
 
 	return ret;
 }
-- 
2.43.0
iAt the end of the above code, 
 WRITE_ONCE(pchan->chan_in_use, false); 
 should be 
 WRITE_ONCE(pchan->chan_in_use, true);

In order to keep the original semantics.  The flag is cleared when the messages is ACKed, not here.  This version causes a hang.

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

[Sudeep Holla]

On Wed, Jul 22, 2026 at 11:10:51PM -0400, Adam Young wrote:
>  
> +	/*
> +	 * 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 MMIO accessor used to ring the doorbell keeps this store
> +	 * visible 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;
> +		WRITE_ONCE(pchan->chan_in_use, false);
>  
>  	return ret;
>  }
> -- 
> 2.43.0
> iAt the end of the above code, 
>  WRITE_ONCE(pchan->chan_in_use, false); 
>  should be 
>  WRITE_ONCE(pchan->chan_in_use, true);
> 
> In order to keep the original semantics.  The flag is cleared when the
> messages is ACKed, not here.  This version causes a hang.
> 

Did you run and seeing hang or just code inspection. If latter, have you
considered that modified code sets it true before doorbell is rung and
set to false only if there is a failure to ring the doorbell ?

-- 
Regards,
Sudeep

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

[lihuisong (C)]

On 7/23/2026 5:16 PM, Sudeep Holla wrote:
> On Wed, Jul 22, 2026 at 11:10:51PM -0400, Adam Young wrote:
>>   
>> +	/*
>> +	 * 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 MMIO accessor used to ring the doorbell keeps this store
>> +	 * visible 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;
>> +		WRITE_ONCE(pchan->chan_in_use, false);
>>   
>>   	return ret;
>>   }
>> -- 
>> 2.43.0
>> iAt the end of the above code,
>>   WRITE_ONCE(pchan->chan_in_use, false);
>>   should be
>>   WRITE_ONCE(pchan->chan_in_use, true);
>>
>> In order to keep the original semantics.  The flag is cleared when the
>> messages is ACKed, not here.  This version causes a hang.
>>
> Did you run and seeing hang or just code inspection. If latter, have you
> considered that modified code sets it true before doorbell is rung and
> set to false only if there is a failure to ring the doorbell ?
+       /*
+        * 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 MMIO accessor used to ring the doorbell keeps this store
+        * visible 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);


!ret -->ret.
should set to false on failure.  I remember that I fixed this place when 
I tested. But I missed this when I sent it out.
Sorry for my mistake.
>

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

[Sudeep Holla]

On Thu, Jul 23, 2026 at 05:47:35PM +0800, lihuisong (C) wrote:
> 
> On 7/23/2026 5:16 PM, Sudeep Holla wrote:
> > On Wed, Jul 22, 2026 at 11:10:51PM -0400, Adam Young wrote:
> > > +	/*
> > > +	 * 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 MMIO accessor used to ring the doorbell keeps this store
> > > +	 * visible 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;
> > > +		WRITE_ONCE(pchan->chan_in_use, false);
> > >   	return ret;
> > >   }
> > > -- 
> > > 2.43.0
> > > iAt the end of the above code,
> > >   WRITE_ONCE(pchan->chan_in_use, false);
> > >   should be
> > >   WRITE_ONCE(pchan->chan_in_use, true);
> > > 
> > > In order to keep the original semantics.  The flag is cleared when the
> > > messages is ACKed, not here.  This version causes a hang.
> > > 
> > Did you run and seeing hang or just code inspection. If latter, have you
> > considered that modified code sets it true before doorbell is rung and
> > set to false only if there is a failure to ring the doorbell ?
> +       /*
> +        * 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 MMIO accessor used to ring the doorbell keeps this store
> +        * visible 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);
> 
> 
> !ret -->ret.
> should set to false on failure.  I remember that I fixed this place when I
> tested. But I missed this when I sent it out.
> Sorry for my mistake.
> > 

Ah right, my brain was somehow wired to read it w/o !. My mistake as well,
didn't spot it. I will sent v2, so Jassi can pick that up instead.

-- 
Regards,
Sudeep

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

[Adam Young]

On 7/17/26 03:56, 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>

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

[Adam Young]

On 7/23/26 05:16, Sudeep Holla wrote:
> On Wed, Jul 22, 2026 at 11:10:51PM -0400, Adam Young wrote:
>>   
>> +	/*
>> +	 * 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 MMIO accessor used to ring the doorbell keeps this store
>> +	 * visible 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;
>> +		WRITE_ONCE(pchan->chan_in_use, false);
>>   
>>   	return ret;
>>   }
>> -- 
>> 2.43.0
>> iAt the end of the above code,
>>   WRITE_ONCE(pchan->chan_in_use, false);
>>   should be
>>   WRITE_ONCE(pchan->chan_in_use, true);
>>
>> In order to keep the original semantics.  The flag is cleared when the
>> messages is ACKed, not here.  This version causes a hang.
>>
> Did you run and seeing hang or just code inspection. If latter, have you
> considered that modified code sets it true before doorbell is rung and
> set to false only if there is a failure to ring the doorbell ?
>
Yes, I saw the hang, and debugged to see this line semantically 
different.  I changed it to true and things resumed working.  !ret is 0  
so that happens in the success case.

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

[Adam Young]

On 7/17/26 03:56, 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;
>   }
>   
>   /**

My code doesn't poll, so I cannot really claim to have tested it, but it 
does not break the existing IRQ based driver mechanism.

I can claim to have read through the code and confirm that it is 
comparable to what happens on the IRQ case:   It clears the cmd_complete 
field and calls mbox_chan_received_data. However, it does not clear 
pchan->chan_in_use = false; (Which should now be a WRITE_ONCE) and I 
would like to confirm that is not an oversight before providing a 
reviewed-by tag.

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

[Sudeep Holla]

On Thu, Jul 23, 2026 at 12:03:45PM -0400, Adam Young wrote:
> 
> On 7/17/26 03:56, 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;
> >   }
> >   /**
> 
> My code doesn't poll, so I cannot really claim to have tested it, but it
> does not break the existing IRQ based driver mechanism.
> 

Thanks for testing IRQ mode.

> I can claim to have read through the code and confirm that it is comparable
> to what happens on the IRQ case:   It clears the cmd_complete field and
> calls mbox_chan_received_data.

Thanks for taking a look and reviewing it.

> However, it does not clear pchan->chan_in_use
> = false; (Which should now be a WRITE_ONCE) and I would like to confirm that
> is not an oversight before providing a reviewed-by tag.
> 

IIRC I think we set the flag only when irq > 0, no ?

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On Thu, Jul 23, 2026 at 11:57:58AM -0400, Adam Young wrote:
> 
> On 7/23/26 05:16, Sudeep Holla wrote:
> > On Wed, Jul 22, 2026 at 11:10:51PM -0400, Adam Young wrote:
> > > +	/*
> > > +	 * 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 MMIO accessor used to ring the doorbell keeps this store
> > > +	 * visible 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;
> > > +		WRITE_ONCE(pchan->chan_in_use, false);
> > >   	return ret;
> > >   }
> > > -- 
> > > 2.43.0
> > > iAt the end of the above code,
> > >   WRITE_ONCE(pchan->chan_in_use, false);
> > >   should be
> > >   WRITE_ONCE(pchan->chan_in_use, true);
> > > 
> > > In order to keep the original semantics.  The flag is cleared when the
> > > messages is ACKed, not here.  This version causes a hang.
> > > 
> > Did you run and seeing hang or just code inspection. If latter, have you
> > considered that modified code sets it true before doorbell is rung and
> > set to false only if there is a failure to ring the doorbell ?
> > 
> Yes, I saw the hang, and debugged to see this line semantically different. 
> I changed it to true and things resumed working.  !ret is 0  so that happens
> in the success case.
> 

My mistake, I must pay more attention 🙁. I wanted it to be error case
and always read it as if(ret..) instead of if(!ret..), sorry for that.

-- 
Regards,
Sudeep

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

[Adam Young]

On 7/23/26 15:45, Sudeep Holla wrote:
> On Thu, Jul 23, 2026 at 12:03:45PM -0400, Adam Young wrote:
>> On 7/17/26 03:56, 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;
>>>    }
>>>    /**
>> My code doesn't poll, so I cannot really claim to have tested it, but it
>> does not break the existing IRQ based driver mechanism.
>>
> Thanks for testing IRQ mode.
>
>> I can claim to have read through the code and confirm that it is comparable
>> to what happens on the IRQ case:   It clears the cmd_complete field and
>> calls mbox_chan_received_data.
> Thanks for taking a look and reviewing it.
>
>> However, it does not clear pchan->chan_in_use
>> = false; (Which should now be a WRITE_ONCE) and I would like to confirm that
>> is not an oversight before providing a reviewed-by tag.
>>
> IIRC I think we set the flag only when irq > 0, no ?

That is right.  So, this code is good.

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




>

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

[Sudeep Holla]

On Fri, Jul 24, 2026 at 01:11:06PM -0400, Adam Young wrote:
> 
> On 7/23/26 15:45, Sudeep Holla wrote:
> > On Thu, Jul 23, 2026 at 12:03:45PM -0400, Adam Young wrote:
> > > On 7/17/26 03:56, 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;
> > > >    }
> > > >    /**
> > > My code doesn't poll, so I cannot really claim to have tested it, but it
> > > does not break the existing IRQ based driver mechanism.
> > > 
> > Thanks for testing IRQ mode.
> > 
> > > I can claim to have read through the code and confirm that it is comparable
> > > to what happens on the IRQ case:   It clears the cmd_complete field and
> > > calls mbox_chan_received_data.
> > Thanks for taking a look and reviewing it.
> > 
> > > However, it does not clear pchan->chan_in_use
> > > = false; (Which should now be a WRITE_ONCE) and I would like to confirm that
> > > is not an oversight before providing a reviewed-by tag.
> > > 
> > IIRC I think we set the flag only when irq > 0, no ?
> 
> That is right.  So, this code is good.
> 
> Reviewed-by: Adam Young <admiyo@os.amperecomputing.com>

I have posted v2 [1] with the fix in 3/3, so please add any tags there so
that Jassi can pick it up easily.

-- 
Regards,
Sudeep

[1] https://lore.kernel.org/linux-acpi/20260723143928.2625970-1-sudeep.holla@kernel.org