Received-SPF: pass (google.com: domain of linux-wireless+bounces-4062-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <7b743820-850a-4871-a0d8-aded36e11aba@quicinc.com>
Date: Tue, 27 Feb 2024 16:39:41 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/3] net: qrtr: support suspend/hibernation
Content-Language: en-US
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
CC: <ath11k@lists.infradead.org>, <linux-wireless@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <mhi@lists.linux.dev>
References: <20240227063613.4478-1-quic_bqiang@quicinc.com>
 <20240227063613.4478-3-quic_bqiang@quicinc.com>
 <20240227071531.GD2587@thinkpad>
From: Baochen Qiang <quic_bqiang@quicinc.com>
In-Reply-To: <20240227071531.GD2587@thinkpad>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2/27/2024 3:15 PM, Manivannan Sadhasivam wrote:
> On Tue, Feb 27, 2024 at 02:36:12PM +0800, Baochen Qiang wrote:
>> MHI devices may not be destroyed during suspend/hibernation, so need
>> to unprepare/prepare MHI channels throughout the transition, this is
>> done by adding suspend/resume callbacks.
>>
>> The suspend callback is called in the late suspend stage, this means
>> MHI channels are still alive at suspend stage, and that makes it
>> possible for an MHI controller driver to communicate with others over
>> those channels at suspend stage. While the resume callback is called
>> in the early resume stage, for a similar reason.
>>
>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
>>
>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Could you please confirm if this patch can be applied separately and won't cause
> regression?
Just searched the kernel and found another two drivers using IPCR 
channels, one is pci_epf_mhi_driver in 
drivers/pci/endpoint/functions/pci-epf-mhi.c and the other is 
mhi_pci_driver in drivers/bus/mhi/host/pci_generic.c.

For pci_epf_mhi_driver, this is not impacted because the devices for 
those channels are attached to mhi_ep_bus_type while QRTR MHI driver 
attached to mhi_bus_type.

For mhi_pci_driver, I am afraid there would be regressions caused by 
this patch. The regression is because when system suspends, 
mhi_pci_suspend() is called and then qcom_mhi_qrtr_pm_suspend_late() 
called, however the latter would fail because MHI is moved to M3 state 
by call mhi_pm_suspend() in mhi_pci_suspend(). To address this, there 
might be two options: the first is to move mhi_pci_suspend() to a late 
suspend stage which would be called after 
qcom_mhi_qrtr_pm_suspend_late(). and the second is to avoid whole QRTR 
suspend operation in such cases. I prefer the second one, because if MHI 
is going to suspend, instead of power down, it is pointless to unprepare 
MHI channels and re-prepare them after resume back. We can achieve this 
purpose by adding a status_cb() to QRTR MHI driver which would be called 
when MHI goes to low power mode. And then QRTR MHI driver could decide 
not to suspend/resume if low power mode is notified.

Your thoughts?

> 
> - Mani
> 
>> ---
>>   net/qrtr/mhi.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
>> index 9ced13c0627a..e96b82a6742c 100644
>> --- a/net/qrtr/mhi.c
>> +++ b/net/qrtr/mhi.c
>> @@ -118,6 +118,32 @@ static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = {
>>   };
>>   MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table);
>>   
>> +static int __maybe_unused qcom_mhi_qrtr_pm_suspend_late(struct device *dev)
>> +{
>> +	struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev);
>> +
>> +	mhi_unprepare_from_transfer(mhi_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused qcom_mhi_qrtr_pm_resume_early(struct device *dev)
>> +{
>> +	struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev);
>> +	int rc;
>> +
>> +	rc = mhi_prepare_for_transfer_autoqueue(mhi_dev);
>> +	if (rc)
>> +		dev_err(dev, "failed to prepare for autoqueue transfer %d\n", rc);
>> +
>> +	return rc;
>> +}
>> +
>> +static const struct dev_pm_ops qcom_mhi_qrtr_pm_ops = {
>> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mhi_qrtr_pm_suspend_late,
>> +				     qcom_mhi_qrtr_pm_resume_early)
>> +};
>> +
>>   static struct mhi_driver qcom_mhi_qrtr_driver = {
>>   	.probe = qcom_mhi_qrtr_probe,
>>   	.remove = qcom_mhi_qrtr_remove,
>> @@ -126,6 +152,7 @@ static struct mhi_driver qcom_mhi_qrtr_driver = {
>>   	.id_table = qcom_mhi_qrtr_id_table,
>>   	.driver = {
>>   		.name = "qcom_mhi_qrtr",
>> +		.pm = &qcom_mhi_qrtr_pm_ops,
>>   	},
>>   };
>>   
>> -- 
>> 2.25.1
>>
>