by Manivannan Sadhasivam

[permalink] [raw]

Subject: Re: [PATCH 2/3] net: qrtr: support suspend/hibernation

On Wed, Feb 21, 2024 at 11:00:25AM +0800, Baochen Qiang wrote:
> MHI devices may not be destroyed during suspend/hibernation, so need
> to unprepare/prepare MHI channels throughout the transition.
>

Again, below content should go to the comment section, not in the commit
message.

Please add only relevant content in the commit message.

- Mani

> The RFC version adds new API to MHI stack with which an MHI controller
> driver can do unprepare/prepare directly by itself, see
>
> https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
>
> Although it works well Mani pointed out that the design is not good
> because MHI channels are managed by MHI client driver thus should not
> be touched by others. See the discussion
>
> https://lore.kernel.org/mhi/[email protected]/
>
> This version changes to add suspend/resume callbacks to achieve the
> same purpose. 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 <[email protected]>
> ---
> net/qrtr/mhi.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
> index 9ced13c0627a..b54a6c2113e9 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 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 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 __maybe_unused 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,9 @@ static struct mhi_driver qcom_mhi_qrtr_driver = {
> .id_table = qcom_mhi_qrtr_id_table,
> .driver = {
> .name = "qcom_mhi_qrtr",
> +#ifdef CONFIG_PM
> + .pm = &qcom_mhi_qrtr_pm_ops,
> +#endif
> },
> };
>
> --
> 2.25.1
>
>

--
மணிவண்ணன் சதாசிவம்

2024-02-27 06:29:23

by Baochen Qiang

[permalink] [raw]

Subject: Re: [PATCH 1/3] bus: mhi: host: add mhi_power_down_no_destroy()

On 2/26/2024 8:15 PM, Manivannan Sadhasivam wrote:
> On Wed, Feb 21, 2024 at 11:00:24AM +0800, Baochen Qiang wrote:
>> ath11k fails to resume:
>>
>> ath11k_pci 0000:06:00.0: timeout while waiting for restart complete
>>
>> This happens because when calling mhi_sync_power_up() the MHI subsystem
>> eventually calls device_add() from mhi_create_devices() but the device
>> creation is deferred:
>>
>> mhi mhi0_IPCR: Driver qcom_mhi_qrtr force probe deferral
>>
>> The reason for deferring device creation is explained in dpm_prepare():
>>
>> /*
>> * It is unsafe if probing of devices will happen during suspend or
>> * hibernation and system behavior will be unpredictable in this case.
>> * So, let's prohibit device's probing here and defer their probes
>> * instead. The normal behavior will be restored in dpm_complete().
>> */
>>
>> Because the device probe is deferred, the qcom_mhi_qrtr_probe() is not
>> called and thus MHI channels are not prepared:
>>
>> So what this means that QRTR is not delivering messages and the QMI connection
>> is not working between ath11k and the firmware, resulting a failure in firmware
>> initialization.
>>
>> To fix this add new function mhi_power_down_no_destroy() which doesn't destroy
>> the devices for channels during power down. This way we avoid probe defer issue
>> and finally can get ath11k hibernation working with the following patches.
>>
>
> Upto this line is the actual commit message and below should be moved to the
> comments section of the patch.
I would like to remove below info since they are included in the cover
letter already. And even keeping them in the comment section won't make
them visible after patch got merged.

>
>> Actually there is an RFC version of this change and it gets positive results
>> from multiple users. Firstly Mani doesn't like this idea and insists that an
>> MHI device should be destroyed when going to suspend/hibernation, see
>>
>> https://lore.kernel.org/mhi/[email protected]/
>>
>> Then Mani changed his mind after a further discussion with kernel PM guys,
>> see
>>
>> https://lore.kernel.org/all/[email protected]/
>>
>> So we come up with the regular version and it is almost identical with that RFC
>> version.
>>
>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
>>
>> Signed-off-by: Kalle Valo <[email protected]>
>> Signed-off-by: Baochen Qiang <[email protected]>
>> ---
>> drivers/bus/mhi/host/internal.h | 4 +++-
>> drivers/bus/mhi/host/pm.c | 36 +++++++++++++++++++++++++++------
>> include/linux/mhi.h | 15 +++++++++++++-
>> 3 files changed, 47 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
>> index 091244cf17c6..8ce4aec56425 100644
>> --- a/drivers/bus/mhi/host/internal.h
>> +++ b/drivers/bus/mhi/host/internal.h
>> @@ -86,6 +86,7 @@ enum dev_st_transition {
>> DEV_ST_TRANSITION_FP,
>> DEV_ST_TRANSITION_SYS_ERR,
>> DEV_ST_TRANSITION_DISABLE,
>> + DEV_ST_TRANSITION_DISABLE_DESTROY_DEVICE,
>> DEV_ST_TRANSITION_MAX,
>> };
>>
>> @@ -96,7 +97,8 @@ enum dev_st_transition {
>> dev_st_trans(MISSION_MODE, "MISSION MODE") \
>> dev_st_trans(FP, "FLASH PROGRAMMER") \
>> dev_st_trans(SYS_ERR, "SYS ERROR") \
>> - dev_st_trans_end(DISABLE, "DISABLE")
>> + dev_st_trans(DISABLE, "DISABLE") \
>> + dev_st_trans_end(DISABLE_DESTROY_DEVICE, "DISABLE (DESTROY DEVICE)")
>>
>> extern const char * const dev_state_tran_str[DEV_ST_TRANSITION_MAX];
>> #define TO_DEV_STATE_TRANS_STR(state) (((state) >= DEV_ST_TRANSITION_MAX) ? \
>> diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
>> index 8b40d3f01acc..5686d32f7458 100644
>> --- a/drivers/bus/mhi/host/pm.c
>> +++ b/drivers/bus/mhi/host/pm.c
>> @@ -468,7 +468,8 @@ static int mhi_pm_mission_mode_transition(struct mhi_controller *mhi_cntrl)
>> }
>>
>> /* Handle shutdown transitions */
>> -static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl)
>> +static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
>> + bool destroy_device)
>> {
>> enum mhi_pm_state cur_state;
>> struct mhi_event *mhi_event;
>> @@ -530,8 +531,10 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl)
>> dev_dbg(dev, "Waiting for all pending threads to complete\n");
>> wake_up_all(&mhi_cntrl->state_event);
>>
>> - dev_dbg(dev, "Reset all active channels and remove MHI devices\n");
>> - device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL, mhi_destroy_device);
>
> I'd be nice to add a comment here on why destroying the device is optional.
>
>> + if (destroy_device) {
>> + dev_dbg(dev, "Reset all active channels and remove MHI devices\n");
>> + device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL, mhi_destroy_device);
>> + }
>>
>> mutex_lock(&mhi_cntrl->pm_mutex);
>>
>> @@ -821,7 +824,10 @@ void mhi_pm_st_worker(struct work_struct *work)
>> mhi_pm_sys_error_transition(mhi_cntrl);
>> break;
>> case DEV_ST_TRANSITION_DISABLE:
>> - mhi_pm_disable_transition(mhi_cntrl);
>> + mhi_pm_disable_transition(mhi_cntrl, false);
>> + break;
>> + case DEV_ST_TRANSITION_DISABLE_DESTROY_DEVICE:
>> + mhi_pm_disable_transition(mhi_cntrl, true);
>> break;
>> default:
>> break;
>> @@ -1175,7 +1181,8 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
>> }
>> EXPORT_SYMBOL_GPL(mhi_async_power_up);
>>
>> -void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>> +static void __mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful,
>> + bool destroy_device)
>> {
>> enum mhi_pm_state cur_state, transition_state;
>> struct device *dev = &mhi_cntrl->mhi_dev->dev;
>> @@ -1211,15 +1218,32 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>> write_unlock_irq(&mhi_cntrl->pm_lock);
>> mutex_unlock(&mhi_cntrl->pm_mutex);
>>
>> - mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_DISABLE);
>> + if (destroy_device)
>> + mhi_queue_state_transition(mhi_cntrl,
>> + DEV_ST_TRANSITION_DISABLE_DESTROY_DEVICE);
>> + else
>> + mhi_queue_state_transition(mhi_cntrl,
>> + DEV_ST_TRANSITION_DISABLE);
>>
>> /* Wait for shutdown to complete */
>> flush_work(&mhi_cntrl->st_worker);
>>
>> disable_irq(mhi_cntrl->irq[0]);
>> }
>> +
>> +void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>> +{
>> + __mhi_power_down(mhi_cntrl, graceful, true);
>> +}
>> EXPORT_SYMBOL_GPL(mhi_power_down);
>>
>> +void mhi_power_down_no_destroy(struct mhi_controller *mhi_cntrl,
>
> How about "mhi_power_down_keep_dev"? Not the best of the API naming suggestion,
> but it reflects what the API does.
>
>> + bool graceful)
>> +{
>> + __mhi_power_down(mhi_cntrl, graceful, false);
>> +}
>> +EXPORT_SYMBOL_GPL(mhi_power_down_no_destroy);
>> +
>> int mhi_sync_power_up(struct mhi_controller *mhi_cntrl)
>> {
>> int ret = mhi_async_power_up(mhi_cntrl);
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index 474d32cb0520..39a6a944a52c 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -647,12 +647,25 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl);
>> int mhi_sync_power_up(struct mhi_controller *mhi_cntrl);
>>
>> /**
>> - * mhi_power_down - Start MHI power down sequence
>> + * mhi_power_down - Start MHI power down sequence. See also
>
> How about?
>
> /**
> * mhi_power_down - Power down the MHI device and also destroy the
> * 'struct device' for the channels associated with it.
>
> ...
>
> * See also mhi_power_down_keep_dev() which is a variant of
> * this API that keeps the 'struct device' for channels (useful during
> * suspend/hibernation).
> */
>
>> + * mhi_power_down_no_destroy() which is a variant of this for
>> + * suspend/hibernation.
>> + *
>> * @mhi_cntrl: MHI controller
>> * @graceful: Link is still accessible, so do a graceful shutdown process
>> */
>> void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful);
>>
>> +/**
>> + * mhi_power_down_no_destroy - Start MHI power down sequence but don't destroy
>> + * struct devices for channels. This is a variant for mhi_power_down() and
>> + * would be useful in suspend/hibernation.
>> + *
>
> /**
> * mhi_power_down_keep_dev - Power down the MHI device but keep the
> * 'struct device' for the channels
> * associated with it.
>
> ...
>
> * This is a variant of 'mhi_power_down' and useful in scenarios such as
> * suspend/hibernation where destroying of the 'struct device' is not
> * needed.
> */
>
> - Mani
>
>> + * @mhi_cntrl: MHI controller
>> + * @graceful: Link is still accessible, so do a graceful shutdown process
>> + */
>> +void mhi_power_down_no_destroy(struct mhi_controller *mhi_cntrl, bool graceful);
>> +
>> /**
>> * mhi_unprepare_after_power_down - Free any allocated memory after power down
>> * @mhi_cntrl: MHI controller
>> --
>> 2.25.1
>>
>>
>