[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

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

[Breno Leitao]

Hello Sudeep,

On Tue, Oct 27, 2026 at 04:47:39PM +0000, Sudeep Holla wrote:
> On Tue, Oct 27, 2026 at 08:19:14AM -0700, Breno Leitao wrote:
> > 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.

Cool, this will definitely get more eyes now on.

Michael deserves the credit for finding this and proposing this fix,
honestly. He is looking at all of this.

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

Ack. Let me update and respin.

Thanks for the feedback,
--breno

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

[Sudeep Holla]

On Wed, Oct 28, 2026 at 06:11:53AM -0700, Breno Leitao wrote:
> Hello Sudeep,
> 
> On Tue, Oct 27, 2026 at 04:47:39PM +0000, Sudeep Holla wrote:
> > On Tue, Oct 27, 2026 at 08:19:14AM -0700, Breno Leitao wrote:
> > > 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.
> 
> Cool, this will definitely get more eyes now on.
> 
> Michael deserves the credit for finding this and proposing this fix,
> honestly. He is looking at all of this.
> 
> > > @@ -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.
> 
> Ack. Let me update and respin.
> 

Thanks, I just realised, I didn't express what I was thinking correctly.
Something like below to be precise.

Regards,
Sudeep

-->8

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 14dc4a187bfc..d7239efe4aba 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,7 @@ 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);

        /*
         * Clear this flag after updating the interrupt ack register and before
@@ -343,8 +343,20 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
         * the lockless update observed by the send and interrupt paths.
         */
        WRITE_ONCE(pchan->chan_in_use, false);
-       mbox_chan_received_data(chan, NULL);
-       mbox_chan_txdone(chan, 0);
+       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);
+               mbox_chan_txdone(chan, -EIO);
+       } else {
+               mbox_chan_received_data(chan, NULL);
+               mbox_chan_txdone(chan, 0);
+       }

        pcc_chan_acknowledge(pchan);