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

[Adam Young <adam@younglogic.com>]

On 7/23/26 10:39, Sudeep Holla wrote:
> 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 updates and READ_ONCE() in the interrupt handler. The
> following ordered I/O accessor orders the flag store before the platform
> is notified.
>
> Clear chan_in_use if ringing the doorbell fails. Otherwise, leave it set
> until the interrupt handler completes the transaction, clearing it before
> the mailbox core can submit another transfer.
>
> 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 | 41 +++++++++++++++++++++++++++--------------
>   1 file changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 8dfa80b0a90f..9888dab64639 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 type 3 initiator subspaces to filter
> + *		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 do not test 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;
>   
> +	/*
> +	 * Initiator 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 before
> +	 * notifying the client and mailbox core. mbox_chan_txdone() may submit
> +	 * the next queued transfer and set the flag again. 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 I/O accessor used to ring the doorbell orders this store
> +	 * 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);
>   
>   	return ret;
>   }

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