Re: [PATCH v02] mailbox/pcc.c shmem map/unmap in callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unlocked read-modify-write on potentially shared PCC registers (e.g., doorbell or command update).
--

commit b67c701fcb8eb6ced91271505440c781fa1718bc
Author: Adam Young <admiyo@os.amperecomputing.com>
Subject: mailbox/pcc.c shmem map/unmap in callbacks

This commit moves the shared memory mapping and unmapping out of the
request and free channel functions and into the startup and shutdown
callbacks. This aims to prevent race conditions where a late interrupt
might write to unmapped memory or a channel is requested after its shmem
was torn down.

> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 636879ae1db76..c5873f9f2b919 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
[ ... ]
> @@ -462,9 +441,15 @@ static bool pcc_last_tx_done(struct mbox_chan *chan)
>  static int pcc_startup(struct mbox_chan *chan)
>  {
>  	struct pcc_chan_info *pchan = chan->con_priv;
> +	struct pcc_mbox_chan *pcc_mchan;
>  	unsigned long irqflags;
>  	int rc;
>  
> +	pcc_mchan = &pchan->chan;
> +	pcc_mchan->shmem = acpi_os_ioremap(pcc_mchan->shmem_base_addr,
> +					   pcc_mchan->shmem_size);
> +	if (pcc_mchan->shmem  == NULL)
> +		return -ENOMEM;
>  	/*
>  	 * Clear and acknowledge any pending interrupts on responder channel
>  	 * before enabling the interrupt
>  	 */
>  	pcc_chan_acknowledge(pchan);

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition when
calling pcc_chan_acknowledge() here.

The ACPI PCCT specification allows multiple subspaces to share the same
doorbell or command update register. When pcc_chan_acknowledge() calls
pcc_chan_reg_read_modify_write(), it performs a non-atomic read, modify, and
write sequence on these mapped registers:

drivers/mailbox/pcc.c:pcc_chan_acknowledge() {
    ...
    pcc_chan_reg_read_modify_write(&pchan->cmd_update);
    ...
    if (ioread32(&pcc_hdr->flags) & PCC_CMD_COMPLETION_NOTIFY)
        pcc_chan_reg_read_modify_write(&pchan->db);
}

Since the mailbox framework only serializes operations on a per-channel
basis using the channel lock, could concurrent calls to pcc_send_data() or
pcc_chan_acknowledge() for different channels that share the same register
race with each other?

If this occurs, could it clobber the register bits belonging to other
channels, resulting in lost doorbell rings or lost ACKs?

>  
>  	if (pchan->plat_irq > 0) {
[ ... ]

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