Re: [PATCH v02] mailbox: pcc: report errors for PCC clients

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Clients relying on `rx_callback` for completion will timeout on error because `mbox_chan_received_data` is skipped.
- [High] Slave subspace (Type 4) messages from the platform are silently dropped and acknowledged on error.
- [Medium] Positive ACPI status codes are leaked into the mailbox framework as error codes.
--

commit 02ebdabd8b273de3b908d80f12b3a4d373284736
Author: Adam Young <admiyo@os.amperecomputing.com>

mailbox: pcc: report errors for PCC clients

The commit modifies the PCC mailbox IRQ handler to read the error status
and pass it to the tx_done callback. It also skips the receive callback if
an error occurred.

> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 636879ae1db76..16b9ce087b9ec 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
[ ... ]
> @@ -327,8 +328,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;
> +	rc = pcc_mbox_error_check_and_clear(pchan);

Can pcc_mbox_error_check_and_clear() return a positive acpi_status value
(like AE_ERROR) if pcc_chan_reg_read() fails?

If so, this positive value would be passed to mbox_chan_txdone(), which
might cause issues for clients that check if the returned error is less
than zero.

>  
>  	/*
>  	 * Clear this flag after updating interrupt ack register and just
> @@ -337,8 +337,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>  	 * required to avoid any possible race in updatation of this flag.
>  	 */
>  	pchan->chan_in_use = false;
> -	mbox_chan_received_data(chan, NULL);
> -	mbox_chan_txdone(chan, 0);
> +	if (!rc)
> +		mbox_chan_received_data(chan, NULL);

Since clients like drivers/acpi/cppc_acpi.c and
drivers/soc/hisilicon/kunpeng_hccs.c rely on rx_callback (invoked by
mbox_chan_received_data()) to signal their completions, will skipping it
here cause them to block until their timeout expires instead of processing
the error?

For ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE channels used by the platform to
send notifications to the OS, does skipping mbox_chan_received_data() cause
the OS to silently drop the platform's notification?

> +	mbox_chan_txdone(chan, rc);
>  
>  	pcc_chan_acknowledge(pchan);

If a slave subspace notification is dropped due to an error, does calling
pcc_chan_acknowledge() falsely indicate to the platform that the
notification was successfully handled?

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518193006.27425-1-admiyo@os.amperecomputing.com?part=1