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