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@xxxxxxxxxxxxxxx
> Suggested-by: Michael van der Westhuizen <rmikey@xxxxxxxx>
> Signed-off-by: Breno Leitao <leitao@xxxxxxxxxx>
> ---
>  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