Re: [PATCH V4 2/5] mailbox: Add support for QTI CPUCP mailbox controller

[Cristian Marussi]

On Mon, Apr 22, 2024 at 10:10:32PM +0530, Sibi Sankar wrote:
> Add support for CPUSS Control Processor (CPUCP) mailbox controller,
> this driver enables communication between AP and CPUCP by acting as
> a doorbell between them.
> 

Hi Sibi,

one small reflection about locking on the RX path down below...

> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
> ---
>

[snip]

> +struct qcom_cpucp_mbox {
> +	struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
> +	struct mbox_controller mbox;
> +	void __iomem *tx_base;
> +	void __iomem *rx_base;
> +};
> +
> +static inline int channel_number(struct mbox_chan *chan)
> +{
> +	return chan - chan->mbox->chans;
> +}
> +
> +static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
> +{
> +	struct qcom_cpucp_mbox *cpucp = data;
> +	struct mbox_chan *chan;
> +	unsigned long flags;
> +	u64 status;
> +	u32 val;
> +	int i;
> +
> +	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
> +
> +	for_each_set_bit(i, (unsigned long *)&status, APSS_CPUCP_IPC_CHAN_SUPPORTED) {
> +		val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
> +		chan = &cpucp->chans[i];
> +		/* Provide mutual exclusion with changes to chan->cl */
> +		spin_lock_irqsave(&chan->lock, flags);
> +		if (chan->cl)

So the spinlock here is needed to properly check for races on chan->cl
being NULLified by concurrent calls to mbox_channel_free()...the end
result, though, is that you disable IRQs here on each single data
processed on the RX path, while calling mbox_chan_received_data(), in order
to avoid the remote (but real) possibility that the mbox users could free
the channel while some traffic is still in-flight and processed by this ISR.

Note that, though, that mbox_channel_free() calls straight away at start
your controller provided qcom_cpucp_mbox_shutdown() method, where you disable
the IRQ at the HW level in the chip: this means that the only race which could
then happen between the call to .shutdown and chan->cl = NULL, would happen in
any already executing qcom_cpucp_mbox_irq_fn() ISR...

So, I was thinking, what if you add a

	sincronize_irq(cpucp->irq);

in your shutdown right after having disabled the HW IRQs.

This would mean waiting for the termination of any IRQ handlers pending on your
cpucp->irq (field that does not exist as of now :D), right after having
disabled such irq and so just before NULLifying chan->cl...in this way you
should be able to safely drop this spinlock call from the host RX path,
because once you chan->cl = NULL is executed, the IRQs are disabled and
any ongoing ISR would have been terminated.

syncronize_irq() is blocking of course, potentially, but the shutdown
method in mbox_chan_ops is allowed to be blocking looking at the comments.

..not sure if all of this is worth to avoid this small section of code to be
run with IRQs disabled....note though that the mbox_chan_received_data() calls
straight away into the client provided cl->callback....so the real lenght of this
code path is uncertain ....

..just an idea to reason about...

Thanks,
Cristian