[PATCH 0/2] mailbox: qcom-cpucp: fix two PREEMPT_RT bugs in IRQ handler

[PATCH 0/2] mailbox: qcom-cpucp: fix two PREEMPT_RT bugs in IRQ handler

[Jia Yang]

This series fixes two bugs in the qcom-cpucp mailbox driver that cause
kernel instability under PREEMPT_RT.

The first patch fixes a self-deadlock: the IRQ handler calls
mbox_chan_received_data() while holding chan->lock. Under PREEMPT_RT,
spin_lock_irqsave() is converted to an rt_spinlock (rtmutex-based). The
callback chain triggered by mbox_chan_received_data() re-acquires the
same lock, causing a permanent deadlock. The fix moves the callback
invocation to after the lock is released.

The second patch fixes a NULL pointer dereference that was masked by
the first bug: mailbox_clear_channel() calls mbox_send_message() with
NULL data, and qcom_cpucp_mbox_send_data() dereferences it without a
NULL check.

Both bugs were reproduced on Kaanapali (SM8850) with CONFIG_PREEMPT_RT=y.

Signed-off-by: Jia Yang <jia.yang@oss.qualcomm.com>
---
Jia Yang (2):
      mailbox: qcom-cpucp: fix PREEMPT_RT self-deadlock in IRQ handler
      mailbox: qcom-cpucp: handle NULL data in send_data callback

 drivers/mailbox/qcom-cpucp-mbox.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)
---
base-commit: 3df25501ceec0d590d3ad416b571563beb364fb7
change-id: 20260806-mailbox-qcom-cpucp-preempt-rt-fix-36472b46a378

Best regards,
-- 
Jia Yang <jia.yang@oss.qualcomm.com>

[PATCH 1/2] mailbox: qcom-cpucp: fix PREEMPT_RT self-deadlock in IRQ handler

[Jia Yang]

qcom_cpucp_mbox_irq_fn() calls mbox_chan_received_data() while holding
chan->lock. Under PREEMPT_RT, spin_lock_irqsave() is converted to an
rt_spinlock (rtmutex-based), which tracks ownership and can sleep.

The callback chain triggered by mbox_chan_received_data() eventually
reaches mailbox_clear_channel() -> mbox_send_message() -> add_to_rbuf(),
which attempts to re-acquire the same chan->lock. Since rtmutex detects
the re-entrant lock attempt by the same owner, the thread blocks waiting
for a lock it already holds, causing a permanent deadlock.

This deadlock manifests as 'irq/N-apss_cpucp_mbox' stuck in D state
with the following call trace:
  rt_spin_lock -> mbox_send_message -> mailbox_clear_channel ->
  scmi_rx_callback -> mbox_chan_received_data [<- held chan->lock here]

Fix by saving chan->cl locally and clearing the HW interrupt register
inside the lock, then invoking mbox_chan_received_data() after releasing
the lock. This preserves the mutual exclusion for chan->cl access while
avoiding the lock re-entrancy that causes the PREEMPT_RT deadlock.

Fixes: 0e2a9a03106c ("mailbox: Add support for QTI CPUCP mailbox controller")
Signed-off-by: Jia Yang <jia.yang@oss.qualcomm.com>
---
 drivers/mailbox/qcom-cpucp-mbox.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
index 862e45e8fbd5..0f7fe189e8b1 100644
--- a/drivers/mailbox/qcom-cpucp-mbox.c
+++ b/drivers/mailbox/qcom-cpucp-mbox.c
@@ -63,14 +63,25 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
 	for_each_set_bit(i, (unsigned long *)&status, cpucp->mbox.num_chans) {
 		u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
 		struct mbox_chan *chan = &cpucp->chans[i];
+		struct mbox_client *cl;
 		unsigned long flags;
 
-		/* Provide mutual exclusion with changes to chan->cl */
+		/*
+		 * Provide mutual exclusion with changes to chan->cl.
+		 * Save cl locally and clear the HW interrupt inside the lock,
+		 * then invoke mbox_chan_received_data() outside the lock to
+		 * avoid a PREEMPT_RT self-deadlock: mbox_chan_received_data()
+		 * can call back into mbox_send_message() via scmi_rx_callback()
+		 * -> mailbox_clear_channel(), which re-acquires chan->lock
+		 * (converted to an rt_spinlock under PREEMPT_RT).
+		 */
 		spin_lock_irqsave(&chan->lock, flags);
-		if (chan->cl)
-			mbox_chan_received_data(chan, &val);
+		cl = chan->cl;
 		writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
 		spin_unlock_irqrestore(&chan->lock, flags);
+
+		if (cl)
+			mbox_chan_received_data(chan, &val);
 	}
 
 	return IRQ_HANDLED;

-- 
2.43.0

[PATCH 2/2] mailbox: qcom-cpucp: handle NULL data in send_data callback

[Jia Yang]

mailbox_clear_channel() calls mbox_send_message() with NULL data to
notify the remote side that the RX channel has been cleared.
qcom_cpucp_mbox_send_data() blindly dereferenced the data pointer,
causing a NULL pointer dereference kernel panic when invoked from
this path under PREEMPT_RT.

Add an explicit NULL check and return early without writing to the
TX register, which is the correct behaviour for a channel-clear
notification.

Fixes: 0e2a9a03106c ("mailbox: Add support for QTI CPUCP mailbox controller")
Signed-off-by: Jia Yang <jia.yang@oss.qualcomm.com>
---
 drivers/mailbox/qcom-cpucp-mbox.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
index 0f7fe189e8b1..298b357c0f9a 100644
--- a/drivers/mailbox/qcom-cpucp-mbox.c
+++ b/drivers/mailbox/qcom-cpucp-mbox.c
@@ -117,6 +117,14 @@ static int qcom_cpucp_mbox_send_data(struct mbox_chan *chan, void *data)
 	unsigned long chan_id = channel_number(chan);
 	u32 *val = data;
 
+	/*
+	 * mailbox_clear_channel() calls mbox_send_message() with NULL data to
+	 * signal the remote side that the channel has been cleared.  Nothing
+	 * needs to be written to the TX register in that case, so just return.
+	 */
+	if (!val)
+		return 0;
+
 	writel(*val, cpucp->tx_base + APSS_CPUCP_TX_MBOX_CMD(chan_id) + APSS_CPUCP_MBOX_CMD_OFF);
 
 	return 0;

-- 
2.43.0

Re: [PATCH 1/2] mailbox: qcom-cpucp: fix PREEMPT_RT self-deadlock in IRQ handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dropping `chan->lock` before `mbox_chan_received_data()` introduces a TOCTOU race with `mbox_free_channel()`, leading to a NULL pointer dereference.
- [Medium] Acknowledging the hardware RX interrupt before invoking the RX callback violates the mailbox framework API and can cause client re-entrancy.
--

--- Patch [1]: [PATCH 1/2] mailbox: qcom-cpucp: fix PREEMPT_RT self-deadlock in IRQ handler ---
commit 995d8aa11aed719aa1d45f140bab12a284779e97
Author: Jia Yang <jia.yang@oss.qualcomm.com>

mailbox: qcom-cpucp: fix PREEMPT_RT self-deadlock in IRQ handler

This patch addresses a PREEMPT_RT self-deadlock in the qcom-cpucp mailbox IRQ
handler. It prevents lock re-entrancy by caching the channel client, clearing
the hardware interrupt inside the lock, and invoking the receive callback
outside the lock.

> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
> index 862e45e8fbd58..0f7fe189e8b1f 100644
> --- a/drivers/mailbox/qcom-cpucp-mbox.c
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -63,14 +63,25 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
[ ... ]
>  		/*
>  		 * Provide mutual exclusion with changes to chan->cl.
>  		 * Save cl locally and clear the HW interrupt inside the lock,
>  		 * then invoke mbox_chan_received_data() outside the lock to
>  		 * avoid a PREEMPT_RT self-deadlock: mbox_chan_received_data()
>  		 * can call back into mbox_send_message() via scmi_rx_callback()
>  		 * -> mailbox_clear_channel(), which re-acquires chan->lock
>  		 * (converted to an rt_spinlock under PREEMPT_RT).
>  		 */
>  		spin_lock_irqsave(&chan->lock, flags);
> -		if (chan->cl)
> -			mbox_chan_received_data(chan, &val);
> +		cl = chan->cl;
>  		writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);

[Severity: Medium]
Does clearing the hardware interrupt before invoking the RX callback violate
the mailbox framework API expectations?

If the remote processor immediately sends another message upon seeing this
ACK, could a new interrupt fire and be processed on another CPU while the
lock is dropped, leading to concurrent entries into the client's rx_callback
for the same channel?

>  		spin_unlock_irqrestore(&chan->lock, flags);
> +
> +		if (cl)
> +			mbox_chan_received_data(chan, &val);

[Severity: High]
If mbox_free_channel() runs concurrently and sets chan->cl = NULL immediately
after spin_unlock_irqrestore(), won't mbox_chan_received_data() cause a NULL
pointer dereference?

Even though we check the local cached "cl" variable here, the framework API
unconditionally dereferences chan->cl without locking:

drivers/mailbox/mailbox.c:mbox_chan_received_data() {
	/* No buffering the received data */
	if (chan->cl->rx_callback)
		chan->cl->rx_callback(chan->cl, mssg);
}

Could this race be reliably triggered during channel teardown or client
driver unbind?

>  	}
>  
>  	return IRQ_HANDLED;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-mailbox-qcom-cpucp-preempt-rt-fix-v1-0-d302a63dfb5e@oss.qualcomm.com?part=1