Received-SPF: pass (google.com: domain of linux-kernel+bounces-178583-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <f1c8eca4-b5cf-662d-8ccc-ff51b4b44eb9@quicinc.com>
Date: Tue, 14 May 2024 16:24:33 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH V4 2/5] mailbox: Add support for QTI CPUCP mailbox
 controller
To: Cristian Marussi <cristian.marussi@arm.com>
CC: <sudeep.holla@arm.com>, <andersson@kernel.org>, <konrad.dybcio@linaro.org>,
        <jassisinghbrar@gmail.com>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <dmitry.baryshkov@linaro.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <quic_rgottimu@quicinc.com>,
        <quic_kshivnan@quicinc.com>, <conor+dt@kernel.org>,
        <quic_gkohli@quicinc.com>, <quic_nkela@quicinc.com>,
        <quic_psodagud@quicinc.com>, <abel.vesa@linaro.org>
References: <20240422164035.1045501-1-quic_sibis@quicinc.com>
 <20240422164035.1045501-3-quic_sibis@quicinc.com> <ZjTdNwzA4F-GxYY5@pluto>
Content-Language: en-US
From: Sibi Sankar <quic_sibis@quicinc.com>
In-Reply-To: <ZjTdNwzA4F-GxYY5@pluto>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 5/3/24 18:18, Cristian Marussi wrote:
> 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@linaro.org>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>

Hey Christian,
Thanks for taking time to review the series :)

> 
> [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...

In addition to shutdown, Bjorn was worried about handling the setup
scenario as well. Since there are multiple channels, irq handler could
end up dereferencing a half baked cl of the second channel while it's
still being setup. Of course this could happen only if the status bits
aren't cleared by the bootloader though. If it's just the shutdown path
your rec should work fine :)

-Sibi

> 
> Thanks,
> Cristian