[PATCH] mailbox: pcc: Complete the transfer when the platform flags an error

[PATCH] mailbox: pcc: Complete the transfer when the platform flags an error

[Breno Leitao]

A single error reported by the platform stops all later traffic on a
PCC channel whose client relies on the controller to complete
transfers, such as the SCMI PCC transport. Every later request on the
channel times out.

When the platform completes a command and sets a bit in the Error
Status register, pcc_mbox_irq() acks the platform interrupt, clears the
error and returns IRQ_NONE without calling mbox_chan_txdone().

No other interrupt arrives for that transfer, so the mailbox core keeps
it as the active request and holds back every later message.

The chan_in_use flag also stays set, so on a shared interrupt line the
next interrupt raised for another subspace passes the checks in
pcc_mbox_irq(), since Command Complete is still set, and runs the
success path on the failed transfer.

Log the error and complete the transfer with -EIO instead, clearing
chan_in_use first as the success path does. Skip
mbox_chan_received_data(), as the shared memory holds no valid response.
Use -EIO rather than the helper's return value, which is a positive
acpi_status when the register read fails.

Clients that wait for the rx callback still time out on the failed
transfer, but the channel works again afterwards.

Fixes: 9c753f7c953c ("mailbox: pcc: Mark Tx as complete in PCC IRQ handler")
Cc: stable@vger.kernel.org
Suggested-by: Michael van der Westhuizen <rmikey@meta.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/mailbox/pcc.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 14dc4a187bfc67..e5df604b367a4a 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -315,6 +315,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 {
 	struct pcc_chan_info *pchan;
 	struct mbox_chan *chan = p;
+	int ret;
 
 	pchan = chan->con_priv;
 
@@ -333,8 +334,20 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	if (!pcc_mbox_cmd_complete_check(pchan))
 		return IRQ_NONE;
 
-	if (pcc_mbox_error_check_and_clear(pchan))
-		return IRQ_NONE;
+	ret = pcc_mbox_error_check_and_clear(pchan);
+	if (ret) {
+		/*
+		 * No other interrupt arrives for this transfer, so complete
+		 * it here or the channel stalls. Skip the rx callback, as the
+		 * shared memory holds no valid response.
+		 */
+		dev_err_ratelimited(chan->mbox->dev,
+				    "PCC subspace %td: transfer error %d\n",
+				    pchan - chan_info, ret);
+		WRITE_ONCE(pchan->chan_in_use, false);
+		mbox_chan_txdone(chan, -EIO);
+		return IRQ_HANDLED;
+	}
 
 	/*
 	 * Clear this flag after updating the interrupt ack register and before

---
base-commit: f1ac44c32cba88ad65dcecd4a279661d14e93651
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
change-id: 20261027-pcc_error_txdone-94f48c610666

Best regards,
--  
Breno Leitao <leitao@debian.org>

Re: [PATCH] mailbox: pcc: Complete the transfer when the platform flags an error

[Sudeep Holla]

On Tue, Oct 27, 2026 at 08:19:14AM -0700, Breno Leitao wrote:
> A single error reported by the platform stops all later traffic on a
> PCC channel whose client relies on the controller to complete
> transfers, such as the SCMI PCC transport. Every later request on the
> channel times out.
> 
> When the platform completes a command and sets a bit in the Error
> Status register, pcc_mbox_irq() acks the platform interrupt, clears the
> error and returns IRQ_NONE without calling mbox_chan_txdone().
> 
> No other interrupt arrives for that transfer, so the mailbox core keeps
> it as the active request and holds back every later message.
> 
> The chan_in_use flag also stays set, so on a shared interrupt line the
> next interrupt raised for another subspace passes the checks in
> pcc_mbox_irq(), since Command Complete is still set, and runs the
> success path on the failed transfer.
> 
> Log the error and complete the transfer with -EIO instead, clearing
> chan_in_use first as the success path does. Skip
> mbox_chan_received_data(), as the shared memory holds no valid response.
> Use -EIO rather than the helper's return value, which is a positive
> acpi_status when the register read fails.
> 
> Clients that wait for the rx callback still time out on the failed
> transfer, but the channel works again afterwards.
> 

Nice, finally good to see this driver and various error paths being tested
on real platforms. Thanks for finding the issues and the fix.

> Fixes: 9c753f7c953c ("mailbox: pcc: Mark Tx as complete in PCC IRQ handler")
> Cc: stable@vger.kernel.org
> Suggested-by: Michael van der Westhuizen <rmikey@meta.com>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/mailbox/pcc.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 14dc4a187bfc67..e5df604b367a4a 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -315,6 +315,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>  {
>  	struct pcc_chan_info *pchan;
>  	struct mbox_chan *chan = p;
> +	int ret;
>  
>  	pchan = chan->con_priv;
>  
> @@ -333,8 +334,20 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>  	if (!pcc_mbox_cmd_complete_check(pchan))
>  		return IRQ_NONE;
>  
> -	if (pcc_mbox_error_check_and_clear(pchan))
> -		return IRQ_NONE;
> +	ret = pcc_mbox_error_check_and_clear(pchan);
> +	if (ret) {
> +		/*
> +		 * No other interrupt arrives for this transfer, so complete
> +		 * it here or the channel stalls. Skip the rx callback, as the
> +		 * shared memory holds no valid response.
> +		 */
> +		dev_err_ratelimited(chan->mbox->dev,
> +				    "PCC subspace %td: transfer error %d\n",
> +				    pchan - chan_info, ret);
> +		WRITE_ONCE(pchan->chan_in_use, false);
> +		mbox_chan_txdone(chan, -EIO);
> +		return IRQ_HANDLED;

Since we are returning IRQ_HANDLED, can we merge this path with the code
below ?

	if (ret)
		dev_err_...

	..
	if(!ret)
		mbox_chan_received_data();

That should work right ? The reason I prefer this way is the comment for
WRITE_ONCE() applies above as well. Instead we can reuse the path. Let me
know what you think.

-- 
Regards,
Sudeep