From: Baochen Qiang <quic_bqiang@quicinc.com>
Subject: [PATCH 1/3] bus: mhi: host: add mhi_power_down_no_destroy()
Date: Wed, 21 Feb 2024 11:00:24 +0800
Message-id: <20240221030026.10553-2-quic_bqiang@quicinc.com>
Patch-mainline: Submitted, linux-wireless ML
References: bsc#1207948
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.
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/20231127162022.518834-1-kvalo@kernel.org/
Then Mani changed his mind after a further discussion with kernel PM guys,
see
https://lore.kernel.org/all/21cd2098-97e1-4947-a5bb-a97582902ead@quicinc.com/
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 <quic_kvalo@quicinc.com>
Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
Acked-by: Takashi Iwai <tiwai@suse.de>
---
drivers/bus/mhi/host/init.c | 1 +
drivers/bus/mhi/host/internal.h | 1 +
drivers/bus/mhi/host/pm.c | 36 ++++++++++++++++++++++++++++++------
include/linux/mhi.h | 15 ++++++++++++++-
4 files changed, 46 insertions(+), 7 deletions(-)
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -43,6 +43,7 @@ const char * const dev_state_tran_str[DE
[DEV_ST_TRANSITION_FP] = "FLASH PROGRAMMER",
[DEV_ST_TRANSITION_SYS_ERR] = "SYS ERROR",
[DEV_ST_TRANSITION_DISABLE] = "DISABLE",
+ [DEV_ST_TRANSITION_DISABLE_DESTROY_DEVICE] = "DISABLE (DESTROY DEVICE)",
};
const char * const mhi_ch_state_type_str[MHI_CH_STATE_TYPE_MAX] = {
--- a/drivers/bus/mhi/host/internal.h
+++ b/drivers/bus/mhi/host/internal.h
@@ -69,6 +69,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,
};
--- a/drivers/bus/mhi/host/pm.c
+++ b/drivers/bus/mhi/host/pm.c
@@ -453,7 +453,8 @@ error_mission_mode:
}
/* 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;
@@ -515,8 +516,10 @@ skip_mhi_reset:
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);
+ 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);
@@ -801,7 +804,10 @@ void mhi_pm_st_worker(struct work_struct
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;
@@ -1154,7 +1160,8 @@ error_exit:
}
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;
@@ -1190,15 +1197,32 @@ void mhi_power_down(struct mhi_controlle
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,
+ 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);
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -645,13 +645,26 @@ int mhi_async_power_up(struct mhi_contro
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
+ * 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_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
*/