[PATCH RFC 0/8] wifi: ath11k: hibernation support

[PATCH RFC 0/8] wifi: ath11k: hibernation support

[Kalle Valo]

From: Kalle Valo <quic_kvalo@quicinc.com>

Currently in ath11k we keep the firmware running on the WLAN device when the
network interface (wlan0) is down. The problem is that this will break
hibernation, obviously the firmware can't be running after the whole system is
powered off. To power down the ath11k firmware for suspend/hibernation some
changes both in MHI subsystem and ath11k is needed.

This patchset fixes a longstanding bug report about broken hibernation support:

https://bugzilla.kernel.org/show_bug.cgi?id=214649

This patchset is marked as RFC as it requires changes in MHI subsystem. Also
this has been tested only on WCN6855, need to test also on more AP based
chipsets like IPQ8074 and QCN9074.

The patches are also available at:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/log/?h=ath11k-hibernation-support

Earlier versions of this patchset have been tested by multiple users with
positive results. Takashi also tested this latest version:

  Tested-by: Takashi Iwai <tiwai@suse.de>

Baochen Qiang (7):
  bus: mhi: host: add mhi_power_down_no_destroy()
  bus: mhi: host: add new interfaces to handle MHI channels directly
  wifi: ath11k: handle irq enable/disable in several code path
  wifi: ath11k: remove MHI LOOPBACK channels
  wifi: ath11k: do not dump SRNG statistics during resume
  wifi: ath11k: fix warning on DMA ring capabilities event
  wifi: ath11k: support hibernation

Kalle Valo (1):
  wifi: ath11k: thermal: don't try to register multiple times

 drivers/bus/mhi/host/internal.h           |  1 +
 drivers/bus/mhi/host/main.c               | 91 +++++++++++++++++++++++
 drivers/bus/mhi/host/pm.c                 | 26 +++++--
 drivers/net/wireless/ath/ath11k/ahb.c     |  8 +-
 drivers/net/wireless/ath/ath11k/core.c    | 44 ++++++-----
 drivers/net/wireless/ath/ath11k/core.h    |  2 +
 drivers/net/wireless/ath/ath11k/hif.h     | 12 +--
 drivers/net/wireless/ath/ath11k/mhi.c     | 49 +++++-------
 drivers/net/wireless/ath/ath11k/mhi.h     |  4 +-
 drivers/net/wireless/ath/ath11k/pci.c     | 55 ++++++++++++--
 drivers/net/wireless/ath/ath11k/qmi.c     |  7 +-
 drivers/net/wireless/ath/ath11k/thermal.c |  3 +
 drivers/net/wireless/ath/ath11k/wmi.c     |  1 +
 include/linux/mhi.h                       | 47 +++++++++++-
 14 files changed, 268 insertions(+), 82 deletions(-)


base-commit: f24dee89bb8a7ef33c28e31632b1f3dd4e196413
-- 
2.39.2

[PATCH RFC 1/8] bus: mhi: host: add mhi_power_down_no_destroy()

[Kalle Valo]

From: Baochen Qiang <quic_bqiang@quicinc.com>

If ath11k tries to call mhi_power_up() during resume it fails:

ath11k_pci 0000:06:00.0: timeout while waiting for restart complete

This happens because when calling mhi_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
qcom_mhi_qrtr_dl_callback() fails silently as qdev is zero:

static void qcom_mhi_qrtr_dl_callback(struct mhi_device *mhi_dev,
				      struct mhi_result *mhi_res)
{
	struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
	int rc;

	if (!qdev || mhi_res->transaction_status)
		return;

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

To fix this add new function mhi_power_down_no_destroy() which does not destroy
the devices during power down. This way mhi_power_up() can be called during
resume and we can get ath11k hibernation working with the following patches.

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>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/bus/mhi/host/internal.h |  1 +
 drivers/bus/mhi/host/pm.c       | 26 +++++++++++++++++++-------
 include/linux/mhi.h             | 29 +++++++++++++++++++++++++++--
 3 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
index 2e139e76de4c..d1033af285e2 100644
--- 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,
 };
 
diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
index 8a4362d75fc4..e0e429adbda6 100644
--- a/drivers/bus/mhi/host/pm.c
+++ b/drivers/bus/mhi/host/pm.c
@@ -453,7 +453,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;
@@ -515,8 +516,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);
+	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 *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;
@@ -1154,7 +1160,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)
+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,14 +1197,19 @@ 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]);
 }
-EXPORT_SYMBOL_GPL(mhi_power_down);
+EXPORT_SYMBOL_GPL(__mhi_power_down);
 
 int mhi_sync_power_up(struct mhi_controller *mhi_cntrl)
 {
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 039943ec4d4e..85edc9c5df88 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -644,12 +644,37 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl);
  */
 int mhi_sync_power_up(struct mhi_controller *mhi_cntrl);
 
+void __mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful,
+		    bool destroy_device);
+
 /**
- * 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.
+ *
  * @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);
+static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
+{
+	__mhi_power_down(mhi_cntrl, graceful, true);
+}
+
+/**
+ * mhi_power_down_no_destroy - Start MHI power down sequence but don't
+ * destroy struct devices. This is a variant for mhi_power_down() and is a
+ * workaround to make it possible to use mhi_power_up() in a resume
+ * handler. When using this variant the caller must also call
+ * mhi_prepare_all_for_transfer_autoqueue() and
+ * mhi_unprepare_all_from_transfer().
+ *
+ * @mhi_cntrl: MHI controller
+ * @graceful: Link is still accessible, so do a graceful shutdown process
+ */
+static inline void mhi_power_down_no_destroy(struct mhi_controller *mhi_cntrl,
+					     bool graceful)
+{
+	__mhi_power_down(mhi_cntrl, graceful, false);
+}
 
 /**
  * mhi_unprepare_after_power_down - Free any allocated memory after power down
-- 
2.39.2

[PATCH RFC 2/8] bus: mhi: host: add new interfaces to handle MHI channels directly

[Kalle Valo]

From: Baochen Qiang <quic_bqiang@quicinc.com>

When using mhi_power_down_no_destroy() MHI hosts need to unprepare MHI channels
by themselves.  Similarly, MHI stack will also not create new MHI device since
old devices were not destroyed, so MHI hosts need to prepare channels as well.
Hence add these two interfaces to make that possible.

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>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/bus/mhi/host/main.c | 91 +++++++++++++++++++++++++++++++++++++
 include/linux/mhi.h         | 18 ++++++++
 2 files changed, 109 insertions(+)

diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index dcf627b36e82..9bcf8a49c000 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -1667,6 +1667,49 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
 }
 EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
 
+static int __mhi_prepare_for_transfer_autoqueue(struct device *dev, void *data)
+{
+	struct mhi_device *mhi_dev;
+	struct mhi_chan *ul_chan, *dl_chan;
+	enum mhi_ee_type ee = MHI_EE_MAX;
+
+	if (dev->bus != &mhi_bus_type)
+		return 0;
+
+	mhi_dev = to_mhi_device(dev);
+
+	/* Only prepare virtual devices thats attached to bus */
+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	ul_chan = mhi_dev->ul_chan;
+	dl_chan = mhi_dev->dl_chan;
+
+	/*
+	 * If execution environment is specified, remove only those devices that
+	 * started in them based on ee_mask for the channels as we move on to a
+	 * different execution environment
+	 */
+	if (data)
+		ee = *(enum mhi_ee_type *)data;
+
+	if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
+		return 0;
+
+
+	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
+		return 0;
+
+	return mhi_prepare_for_transfer_autoqueue(mhi_dev);
+}
+
+int mhi_prepare_all_for_transfer_autoqueue(struct mhi_controller *mhi_cntrl)
+{
+	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
+				     __mhi_prepare_for_transfer_autoqueue);
+}
+EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer_autoqueue);
+
 void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
 {
 	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
@@ -1682,3 +1725,51 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
 	}
 }
 EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
+
+static int __mhi_unprepare_from_transfer(struct device *dev, void *data)
+{
+	struct mhi_device *mhi_dev;
+	struct mhi_chan *ul_chan, *dl_chan;
+	enum mhi_ee_type ee = MHI_EE_MAX;
+
+	if (dev->bus != &mhi_bus_type)
+		return 0;
+
+	mhi_dev = to_mhi_device(dev);
+
+	/* Only unprepare virtual devices thats attached to bus */
+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	ul_chan = mhi_dev->ul_chan;
+	dl_chan = mhi_dev->dl_chan;
+
+	/*
+	 * If execution environment is specified, remove only those devices that
+	 * started in them based on ee_mask for the channels as we move on to a
+	 * different execution environment
+	 */
+	if (data)
+		ee = *(enum mhi_ee_type *)data;
+
+	if (ul_chan) {
+		if (ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
+			return 0;
+	}
+
+	if (dl_chan) {
+		if (ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
+			return 0;
+	}
+
+	mhi_unprepare_from_transfer(mhi_dev);
+
+	return 0;
+}
+
+int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl)
+{
+	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
+				     __mhi_unprepare_from_transfer);
+}
+EXPORT_SYMBOL_GPL(mhi_unprepare_all_from_transfer);
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 85edc9c5df88..c15f4c44029d 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -838,4 +838,22 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
  */
 bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir);
 
+/**
+ * mhi_prepare_all_for_transfer_autoqueue - if you are using
+ * mhi_power_down_no_destroy() variant this needs to be called after
+ * calling mhi_power_up().
+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_prepare_all_for_transfer_autoqueue(struct mhi_controller *mhi_cntrl);
+
+/**
+ * mhi_unprepare_all_from_transfer - if you are using
+ * mhi_power_down_no_destroy() variant this function needs to be called
+ * before calling mhi_power_down_no_destroy().
+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
+
 #endif /* _MHI_H_ */
-- 
2.39.2

[PATCH RFC 3/8] wifi: ath11k: handle irq enable/disable in several code path

[Kalle Valo]

From: Baochen Qiang <quic_bqiang@quicinc.com>

For non WoW suspend/resume, ath11k host powers down whole hardware
when suspend and power up it when resume, the code path it goes
through is very like the ath11k reset logic.

In order to reuse that logic, do some IRQ management work to make
it work.

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>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/core.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c
index 0c6ecbb9a066..fbd6b6a0e12c 100644
--- a/drivers/net/wireless/ath/ath11k/core.c
+++ b/drivers/net/wireless/ath/ath11k/core.c
@@ -852,9 +852,6 @@ int ath11k_core_resume(struct ath11k_base *ab)
 		return ret;
 	}
 
-	ath11k_hif_ce_irq_enable(ab);
-	ath11k_hif_irq_enable(ab);
-
 	ret = ath11k_dp_rx_pktlog_start(ab);
 	if (ret) {
 		ath11k_warn(ab, "failed to start rx pktlog during resume: %d\n",
@@ -1775,10 +1772,9 @@ static int ath11k_core_reconfigure_on_crash(struct ath11k_base *ab)
 
 	mutex_lock(&ab->core_lock);
 	ath11k_thermal_unregister(ab);
-	ath11k_hif_irq_disable(ab);
 	ath11k_dp_pdev_free(ab);
 	ath11k_spectral_deinit(ab);
-	ath11k_hif_stop(ab);
+	ath11k_ce_cleanup_pipes(ab);
 	ath11k_wmi_detach(ab);
 	ath11k_dp_pdev_reo_cleanup(ab);
 	mutex_unlock(&ab->core_lock);
@@ -2033,8 +2029,8 @@ static void ath11k_core_reset(struct work_struct *work)
 	time_left = wait_for_completion_timeout(&ab->recovery_start,
 						ATH11K_RECOVER_START_TIMEOUT_HZ);
 
-	ath11k_hif_power_down(ab);
-	ath11k_hif_power_up(ab);
+	ath11k_hif_irq_disable(ab);
+	ath11k_hif_ce_irq_disable(ab);
 
 	ath11k_dbg(ab, ATH11K_DBG_BOOT, "reset started\n");
 }
-- 
2.39.2

[PATCH RFC 4/8] wifi: ath11k: remove MHI LOOPBACK channels

[Kalle Valo]

From: Baochen Qiang <quic_bqiang@quicinc.com>

There is no driver to match these two channels, so
remove them. This fixes warnings from MHI subsystem during suspend:

mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting

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>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/mhi.c | 28 ---------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
index afeabd6ecc67..d4fe1d1c86e4 100644
--- a/drivers/net/wireless/ath/ath11k/mhi.c
+++ b/drivers/net/wireless/ath/ath11k/mhi.c
@@ -21,34 +21,6 @@
 #define RDDM_DUMP_SIZE	0x420000
 
 static struct mhi_channel_config ath11k_mhi_channels_qca6390[] = {
-	{
-		.num = 0,
-		.name = "LOOPBACK",
-		.num_elements = 32,
-		.event_ring = 0,
-		.dir = DMA_TO_DEVICE,
-		.ee_mask = 0x4,
-		.pollcfg = 0,
-		.doorbell = MHI_DB_BRST_DISABLE,
-		.lpm_notify = false,
-		.offload_channel = false,
-		.doorbell_mode_switch = false,
-		.auto_queue = false,
-	},
-	{
-		.num = 1,
-		.name = "LOOPBACK",
-		.num_elements = 32,
-		.event_ring = 0,
-		.dir = DMA_FROM_DEVICE,
-		.ee_mask = 0x4,
-		.pollcfg = 0,
-		.doorbell = MHI_DB_BRST_DISABLE,
-		.lpm_notify = false,
-		.offload_channel = false,
-		.doorbell_mode_switch = false,
-		.auto_queue = false,
-	},
 	{
 		.num = 20,
 		.name = "IPCR",
-- 
2.39.2

[PATCH RFC 5/8] wifi: ath11k: do not dump SRNG statistics during resume

[Kalle Valo]

From: Baochen Qiang <quic_bqiang@quicinc.com>

Both the firmware reset feature and the power management
suspend/resume feature share common power-down and power-up
functionality. One aspect of the power-up functionality is
the handling of the ATH11K_QMI_EVENT_FW_INIT_DONE event.
When this event is received, a call is made to
ath11k_hal_dump_srng_stats(), with the purpose to collect
information that may be useful in debugging the cause of a
firmware reset.

Unfortunately, since this functionality is shared between
both the firmware reset path and the power management
resume path, the kernel log is flooded with messages during
resume. Since these messages are not useful during resume,
and in fact can be confusing and can increase the time it
takes to resume, update the logic to only call
ath11k_hal_dump_srng_stats() during firmware reset.

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>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/qmi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
index c270dc46d506..97a74563d4a6 100644
--- a/drivers/net/wireless/ath/ath11k/qmi.c
+++ b/drivers/net/wireless/ath/ath11k/qmi.c
@@ -3249,7 +3249,8 @@ static void ath11k_qmi_driver_event_work(struct work_struct *work)
 		case ATH11K_QMI_EVENT_FW_INIT_DONE:
 			clear_bit(ATH11K_FLAG_QMI_FAIL, &ab->dev_flags);
 			if (test_bit(ATH11K_FLAG_REGISTERED, &ab->dev_flags)) {
-				ath11k_hal_dump_srng_stats(ab);
+				if (ab->is_reset)
+					ath11k_hal_dump_srng_stats(ab);
 				queue_work(ab->workqueue, &ab->restart_work);
 				break;
 			}
-- 
2.39.2

[PATCH RFC 6/8] wifi: ath11k: fix warning on DMA ring capabilities event

[Kalle Valo]

From: Baochen Qiang <quic_bqiang@quicinc.com>

We are seeing below warning in both reset and suspend/resume scenarios:

[69663.691847] ath11k_pci 0000:02:00.0: Already processed, so ignoring dma ring caps

This is because ab->num_db_cap is not cleared in
ath11k_wmi_free_dbring_caps(), so clear it to avoid such
warnings.

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>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/wmi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c
index 2845b4313d3a..b73d4286f7d3 100644
--- a/drivers/net/wireless/ath/ath11k/wmi.c
+++ b/drivers/net/wireless/ath/ath11k/wmi.c
@@ -4786,6 +4786,7 @@ static void ath11k_wmi_free_dbring_caps(struct ath11k_base *ab)
 {
 	kfree(ab->db_caps);
 	ab->db_caps = NULL;
+	ab->num_db_cap = 0;
 }
 
 static int ath11k_wmi_tlv_dma_ring_caps(struct ath11k_base *ab,
-- 
2.39.2

[PATCH RFC 7/8] wifi: ath11k: thermal: don't try to register multiple times

[Kalle Valo]

From: Kalle Valo <quic_kvalo@quicinc.com>

Every time the firmware boots we call ath11k_core_qmi_firmware_ready() which
ends up calling ath11k_thermal_register(). So we try to register thermal
devices multiple times. And when we power off the firmware during
suspend/hibernation (implemented in the next patch) we get a warning in resume:

hwmon hwmon4: PM: parent phy0 should not be sleeping

Workaround this similarly like ath11k_mac_register() does by testing
ATH11K_FLAG_REGISTERED.

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>
---
 drivers/net/wireless/ath/ath11k/thermal.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/ath/ath11k/thermal.c b/drivers/net/wireless/ath/ath11k/thermal.c
index c9b012f97ba5..80abf472fb87 100644
--- a/drivers/net/wireless/ath/ath11k/thermal.c
+++ b/drivers/net/wireless/ath/ath11k/thermal.c
@@ -162,6 +162,9 @@ int ath11k_thermal_register(struct ath11k_base *ab)
 	struct ath11k_pdev *pdev;
 	int i, ret;
 
+	if (test_bit(ATH11K_FLAG_REGISTERED, &ab->dev_flags))
+		return 0;
+
 	for (i = 0; i < ab->num_radios; i++) {
 		pdev = &ab->pdevs[i];
 		ar = pdev->ar;
-- 
2.39.2

[PATCH RFC 8/8] wifi: ath11k: support hibernation

[Kalle Valo]

From: Baochen Qiang <quic_bqiang@quicinc.com>

Now that all infrastructure is in place and ath11k is fixed to handle all the
corner cases, power down the ath11k firmware during suspend and power it back
up during resume. This fixes the problem when using hibernation with ath11k PCI
devices.

Change to use ath11k_hif_power_down() instead of ath11k_hif_suspend()
in suspend callback and to use ath11k_hif_power_up() instead of
ath11k_hif_resume() in resume callback.

In ath11k_hif_power_down(), we reset MHI channels to keep from unexpected
activities, and last we go PCI power down path to completely reset whole
hardware. Most importantly in power down path, we tell mhi_power_down() to not
to destroy MHI devices, making us get rid of the probe-defer issue when resume.

In ath11k_hif_power_up(), we go normal PCI power up path to download firmware
etc. Since MHI channels are not activated automatically, we do it manually as
the last part.

Also change related code due to interface changes.

Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30

Tested-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/ahb.c  |  8 ++--
 drivers/net/wireless/ath/ath11k/core.c | 34 ++++++++--------
 drivers/net/wireless/ath/ath11k/core.h |  2 +
 drivers/net/wireless/ath/ath11k/hif.h  | 12 +++---
 drivers/net/wireless/ath/ath11k/mhi.c  | 21 +++++++++-
 drivers/net/wireless/ath/ath11k/mhi.h  |  4 +-
 drivers/net/wireless/ath/ath11k/pci.c  | 55 +++++++++++++++++++++++---
 drivers/net/wireless/ath/ath11k/qmi.c  |  4 +-
 8 files changed, 103 insertions(+), 37 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c
index 235336ef2a7a..155ab9897bff 100644
--- a/drivers/net/wireless/ath/ath11k/ahb.c
+++ b/drivers/net/wireless/ath/ath11k/ahb.c
@@ -401,7 +401,7 @@ static void ath11k_ahb_stop(struct ath11k_base *ab)
 	ath11k_ce_cleanup_pipes(ab);
 }
 
-static int ath11k_ahb_power_up(struct ath11k_base *ab)
+static int ath11k_ahb_power_up(struct ath11k_base *ab, bool is_resume)
 {
 	struct ath11k_ahb *ab_ahb = ath11k_ahb_priv(ab);
 	int ret;
@@ -413,11 +413,11 @@ static int ath11k_ahb_power_up(struct ath11k_base *ab)
 	return ret;
 }
 
-static void ath11k_ahb_power_down(struct ath11k_base *ab)
+static int ath11k_ahb_power_down(struct ath11k_base *ab, bool is_suspend)
 {
 	struct ath11k_ahb *ab_ahb = ath11k_ahb_priv(ab);
 
-	rproc_shutdown(ab_ahb->tgt_rproc);
+	return rproc_shutdown(ab_ahb->tgt_rproc);
 }
 
 static void ath11k_ahb_init_qmi_ce_config(struct ath11k_base *ab)
@@ -1256,7 +1256,7 @@ static int ath11k_ahb_remove(struct platform_device *pdev)
 	struct ath11k_base *ab = platform_get_drvdata(pdev);
 
 	if (test_bit(ATH11K_FLAG_QMI_FAIL, &ab->dev_flags)) {
-		ath11k_ahb_power_down(ab);
+		ath11k_ahb_power_down(ab, false);
 		ath11k_debugfs_soc_destroy(ab);
 		ath11k_qmi_deinit_service(ab);
 		goto qmi_fail;
diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c
index fbd6b6a0e12c..0e0b02692282 100644
--- a/drivers/net/wireless/ath/ath11k/core.c
+++ b/drivers/net/wireless/ath/ath11k/core.c
@@ -800,12 +800,6 @@ int ath11k_core_suspend(struct ath11k_base *ab)
 		return ret;
 	}
 
-	ret = ath11k_wow_enable(ab);
-	if (ret) {
-		ath11k_warn(ab, "failed to enable wow during suspend: %d\n", ret);
-		return ret;
-	}
-
 	ret = ath11k_dp_rx_pktlog_stop(ab, false);
 	if (ret) {
 		ath11k_warn(ab, "failed to stop dp rx pktlog during suspend: %d\n",
@@ -819,7 +813,7 @@ int ath11k_core_suspend(struct ath11k_base *ab)
 	ath11k_hif_irq_disable(ab);
 	ath11k_hif_ce_irq_disable(ab);
 
-	ret = ath11k_hif_suspend(ab);
+	ret = ath11k_hif_power_down(ab, true);
 	if (ret) {
 		ath11k_warn(ab, "failed to suspend hif: %d\n", ret);
 		return ret;
@@ -834,6 +828,7 @@ int ath11k_core_resume(struct ath11k_base *ab)
 	int ret;
 	struct ath11k_pdev *pdev;
 	struct ath11k *ar;
+	long time_left;
 
 	if (!ab->hw_params.supports_suspend)
 		return -EOPNOTSUPP;
@@ -846,11 +841,18 @@ int ath11k_core_resume(struct ath11k_base *ab)
 	if (!ar || ar->state != ATH11K_STATE_OFF)
 		return 0;
 
-	ret = ath11k_hif_resume(ab);
+	reinit_completion(&ab->restart_completed);
+	ret = ath11k_hif_power_up(ab, true);
 	if (ret) {
 		ath11k_warn(ab, "failed to resume hif during resume: %d\n", ret);
 		return ret;
 	}
+	time_left = wait_for_completion_timeout(&ab->restart_completed,
+						ATH11K_RESET_TIMEOUT_HZ);
+	if (time_left == 0) {
+		ath11k_warn(ab, "timeout while waiting for restart complete");
+		return -ETIMEDOUT;
+	}
 
 	ret = ath11k_dp_rx_pktlog_start(ab);
 	if (ret) {
@@ -859,12 +861,6 @@ int ath11k_core_resume(struct ath11k_base *ab)
 		return ret;
 	}
 
-	ret = ath11k_wow_wakeup(ab);
-	if (ret) {
-		ath11k_warn(ab, "failed to wakeup wow during resume: %d\n", ret);
-		return ret;
-	}
-
 	return 0;
 }
 EXPORT_SYMBOL(ath11k_core_resume);
@@ -1488,7 +1484,7 @@ static int ath11k_core_soc_create(struct ath11k_base *ab)
 		goto err_qmi_deinit;
 	}
 
-	ret = ath11k_hif_power_up(ab);
+	ret = ath11k_hif_power_up(ab, false);
 	if (ret) {
 		ath11k_err(ab, "failed to power up :%d\n", ret);
 		goto err_debugfs_reg;
@@ -1963,6 +1959,8 @@ static void ath11k_core_restart(struct work_struct *work)
 
 	if (!ab->is_reset)
 		ath11k_core_post_reconfigure_recovery(ab);
+
+	complete(&ab->restart_completed);
 }
 
 static void ath11k_core_reset(struct work_struct *work)
@@ -2032,6 +2030,9 @@ static void ath11k_core_reset(struct work_struct *work)
 	ath11k_hif_irq_disable(ab);
 	ath11k_hif_ce_irq_disable(ab);
 
+	ath11k_hif_power_down(ab, false);
+	ath11k_hif_power_up(ab, false);
+
 	ath11k_dbg(ab, ATH11K_DBG_BOOT, "reset started\n");
 }
 
@@ -2102,7 +2103,7 @@ void ath11k_core_deinit(struct ath11k_base *ab)
 
 	mutex_unlock(&ab->core_lock);
 
-	ath11k_hif_power_down(ab);
+	ath11k_hif_power_down(ab, false);
 	ath11k_mac_destroy(ab);
 	ath11k_core_soc_destroy(ab);
 	ath11k_fw_destroy(ab);
@@ -2155,6 +2156,7 @@ struct ath11k_base *ath11k_core_alloc(struct device *dev, size_t priv_size,
 	timer_setup(&ab->rx_replenish_retry, ath11k_ce_rx_replenish_retry, 0);
 	init_completion(&ab->htc_suspend);
 	init_completion(&ab->wow.wakeup_completed);
+	init_completion(&ab->restart_completed);
 
 	ab->dev = dev;
 	ab->hif.bus = bus;
diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
index f12b606e2d2e..444f5d95acea 100644
--- a/drivers/net/wireless/ath/ath11k/core.h
+++ b/drivers/net/wireless/ath/ath11k/core.h
@@ -996,6 +996,8 @@ struct ath11k_base {
 		DECLARE_BITMAP(fw_features, ATH11K_FW_FEATURE_COUNT);
 	} fw;
 
+	struct completion restart_completed;
+
 #ifdef CONFIG_NL80211_TESTMODE
 	struct {
 		u32 data_pos;
diff --git a/drivers/net/wireless/ath/ath11k/hif.h b/drivers/net/wireless/ath/ath11k/hif.h
index d68ed4214dec..7f08591ed5c8 100644
--- a/drivers/net/wireless/ath/ath11k/hif.h
+++ b/drivers/net/wireless/ath/ath11k/hif.h
@@ -16,8 +16,8 @@ struct ath11k_hif_ops {
 	void (*irq_disable)(struct ath11k_base *ab);
 	int (*start)(struct ath11k_base *ab);
 	void (*stop)(struct ath11k_base *ab);
-	int (*power_up)(struct ath11k_base *ab);
-	void (*power_down)(struct ath11k_base *ab);
+	int (*power_up)(struct ath11k_base *ab, bool is_resume);
+	int (*power_down)(struct ath11k_base *ab, bool is_suspend);
 	int (*suspend)(struct ath11k_base *ab);
 	int (*resume)(struct ath11k_base *ab);
 	int (*map_service_to_pipe)(struct ath11k_base *ab, u16 service_id,
@@ -64,14 +64,14 @@ static inline void ath11k_hif_irq_disable(struct ath11k_base *ab)
 	ab->hif.ops->irq_disable(ab);
 }
 
-static inline int ath11k_hif_power_up(struct ath11k_base *ab)
+static inline int ath11k_hif_power_up(struct ath11k_base *ab, bool is_resume)
 {
-	return ab->hif.ops->power_up(ab);
+	return ab->hif.ops->power_up(ab, is_resume);
 }
 
-static inline void ath11k_hif_power_down(struct ath11k_base *ab)
+static inline int ath11k_hif_power_down(struct ath11k_base *ab, bool is_resume)
 {
-	ab->hif.ops->power_down(ab);
+	return ab->hif.ops->power_down(ab, is_resume);
 }
 
 static inline int ath11k_hif_suspend(struct ath11k_base *ab)
diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
index d4fe1d1c86e4..76c67bdf48a1 100644
--- a/drivers/net/wireless/ath/ath11k/mhi.c
+++ b/drivers/net/wireless/ath/ath11k/mhi.c
@@ -469,9 +469,16 @@ int ath11k_mhi_start(struct ath11k_pci *ab_pci)
 	return 0;
 }
 
-void ath11k_mhi_stop(struct ath11k_pci *ab_pci)
+void ath11k_mhi_stop(struct ath11k_pci *ab_pci, bool is_suspend)
 {
-	mhi_power_down(ab_pci->mhi_ctrl, true);
+	/* During suspend we need to use mhi_power_down_no_destroy()
+	 * workaround, otherwise mhi_power_up() will fail during resume.
+	 */
+	if (is_suspend)
+		mhi_power_down_no_destroy(ab_pci->mhi_ctrl, true);
+	else
+		mhi_power_down(ab_pci->mhi_ctrl, true);
+
 	mhi_unprepare_after_power_down(ab_pci->mhi_ctrl);
 }
 
@@ -506,3 +513,13 @@ int ath11k_mhi_resume(struct ath11k_pci *ab_pci)
 
 	return 0;
 }
+
+int ath11k_mhi_prepare_for_transfer(struct ath11k_pci *ab_pci)
+{
+	return mhi_prepare_all_for_transfer_autoqueue(ab_pci->mhi_ctrl);
+}
+
+int ath11k_mhi_unprepare_from_transfer(struct ath11k_pci *ab_pci)
+{
+	return mhi_unprepare_all_from_transfer(ab_pci->mhi_ctrl);
+}
diff --git a/drivers/net/wireless/ath/ath11k/mhi.h b/drivers/net/wireless/ath/ath11k/mhi.h
index 8d9f852da695..80902fda5842 100644
--- a/drivers/net/wireless/ath/ath11k/mhi.h
+++ b/drivers/net/wireless/ath/ath11k/mhi.h
@@ -17,7 +17,7 @@
 #define MHICTRL_RESET_MASK			0x2
 
 int ath11k_mhi_start(struct ath11k_pci *ar_pci);
-void ath11k_mhi_stop(struct ath11k_pci *ar_pci);
+void ath11k_mhi_stop(struct ath11k_pci *ar_pci, bool is_suspend);
 int ath11k_mhi_register(struct ath11k_pci *ar_pci);
 void ath11k_mhi_unregister(struct ath11k_pci *ar_pci);
 void ath11k_mhi_set_mhictrl_reset(struct ath11k_base *ab);
@@ -26,4 +26,6 @@ void ath11k_mhi_clear_vector(struct ath11k_base *ab);
 int ath11k_mhi_suspend(struct ath11k_pci *ar_pci);
 int ath11k_mhi_resume(struct ath11k_pci *ar_pci);
 
+int ath11k_mhi_prepare_for_transfer(struct ath11k_pci *ar_pci);
+int ath11k_mhi_unprepare_from_transfer(struct ath11k_pci *ar_pci);
 #endif
diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
index 09e65c5e55c4..3d6195bc6f55 100644
--- a/drivers/net/wireless/ath/ath11k/pci.c
+++ b/drivers/net/wireless/ath/ath11k/pci.c
@@ -625,7 +625,7 @@ static int ath11k_pci_power_up(struct ath11k_base *ab)
 	return 0;
 }
 
-static void ath11k_pci_power_down(struct ath11k_base *ab)
+static void ath11k_pci_power_down(struct ath11k_base *ab, bool is_suspend)
 {
 	struct ath11k_pci *ab_pci = ath11k_pci_priv(ab);
 
@@ -636,11 +636,54 @@ static void ath11k_pci_power_down(struct ath11k_base *ab)
 
 	ath11k_pci_msi_disable(ab_pci);
 
-	ath11k_mhi_stop(ab_pci);
+	ath11k_mhi_stop(ab_pci, is_suspend);
 	clear_bit(ATH11K_FLAG_DEVICE_INIT_DONE, &ab->dev_flags);
 	ath11k_pci_sw_reset(ab_pci->ab, false);
 }
 
+static int ath11k_pci_hif_power_down(struct ath11k_base *ab, bool is_suspend)
+{
+	struct ath11k_pci *ab_pci = ath11k_pci_priv(ab);
+	int ret;
+
+	if (is_suspend) {
+		ret = ath11k_mhi_unprepare_from_transfer(ab_pci);
+		if (ret) {
+			ath11k_err(ab_pci->ab, "failed to unprepare from transfer %d\n",
+				   ret);
+			return ret;
+		}
+	}
+
+	ath11k_pci_power_down(ab, is_suspend);
+	return 0;
+}
+
+static int ath11k_pci_hif_power_up(struct ath11k_base *ab, bool is_resume)
+{
+	struct ath11k_pci *ab_pci = ath11k_pci_priv(ab);
+	int ret;
+
+	ret =  ath11k_pci_power_up(ab);
+	if (ret) {
+		ath11k_err(ab_pci->ab, "failed to power up %d\n", ret);
+		return ret;
+	}
+
+	if (is_resume) {
+		/* sleep for 500ms to let mhi_pm_mission_mode_transition()
+		 * finishes, or we may be wake up immediately after mission
+		 * mode event received and call
+		 * ath11k_mhi_prepare_for_transfer(), while bottom half of
+		 * mhi_pm_mission_mode_transition() does not finish.
+		 */
+		msleep(500);
+		ret = ath11k_mhi_prepare_for_transfer(ab_pci);
+	}
+
+	return ret;
+}
+
 static int ath11k_pci_hif_suspend(struct ath11k_base *ab)
 {
 	struct ath11k_pci *ar_pci = ath11k_pci_priv(ab);
@@ -688,8 +731,8 @@ static const struct ath11k_hif_ops ath11k_pci_hif_ops = {
 	.read32 = ath11k_pcic_read32,
 	.write32 = ath11k_pcic_write32,
 	.read = ath11k_pcic_read,
-	.power_down = ath11k_pci_power_down,
-	.power_up = ath11k_pci_power_up,
+	.power_down = ath11k_pci_hif_power_down,
+	.power_up = ath11k_pci_hif_power_up,
 	.suspend = ath11k_pci_hif_suspend,
 	.resume = ath11k_pci_hif_resume,
 	.irq_enable = ath11k_pcic_ext_irq_enable,
@@ -938,7 +981,7 @@ static void ath11k_pci_remove(struct pci_dev *pdev)
 	ath11k_pci_set_irq_affinity_hint(ab_pci, NULL);
 
 	if (test_bit(ATH11K_FLAG_QMI_FAIL, &ab->dev_flags)) {
-		ath11k_pci_power_down(ab);
+		ath11k_pci_power_down(ab, false);
 		ath11k_debugfs_soc_destroy(ab);
 		ath11k_qmi_deinit_service(ab);
 		goto qmi_fail;
@@ -966,7 +1009,7 @@ static void ath11k_pci_shutdown(struct pci_dev *pdev)
 	struct ath11k_pci *ab_pci = ath11k_pci_priv(ab);
 
 	ath11k_pci_set_irq_affinity_hint(ab_pci, NULL);
-	ath11k_pci_power_down(ab);
+	ath11k_pci_power_down(ab, false);
 }
 
 static __maybe_unused int ath11k_pci_pm_suspend(struct device *dev)
diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
index 97a74563d4a6..7d856d8b7f89 100644
--- a/drivers/net/wireless/ath/ath11k/qmi.c
+++ b/drivers/net/wireless/ath/ath11k/qmi.c
@@ -2877,8 +2877,8 @@ int ath11k_qmi_fwreset_from_cold_boot(struct ath11k_base *ab)
 	}
 
 	/* reset the firmware */
-	ath11k_hif_power_down(ab);
-	ath11k_hif_power_up(ab);
+	ath11k_hif_power_down(ab, false);
+	ath11k_hif_power_up(ab, false);
 	ath11k_dbg(ab, ATH11K_DBG_QMI, "exit wait for cold boot done\n");
 	return 0;
 }
-- 
2.39.2

Re: [PATCH RFC 4/8] wifi: ath11k: remove MHI LOOPBACK channels

[Jeffrey Hugo]

On 11/10/2023 3:21 AM, Kalle Valo wrote:
> From: Baochen Qiang <quic_bqiang@quicinc.com>
> 
> There is no driver to match these two channels, so
> remove them. This fixes warnings from MHI subsystem during suspend:
> 
> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting

This feels like just masking a real issue.

If LOOPBACK is not being consumed, then the channel should never go into 
the start state.  Why would we be trying to transition to the reset 
state then?

-Jeff

Re: [PATCH RFC 2/8] bus: mhi: host: add new interfaces to handle MHI channels directly

[Jeffrey Hugo]

On 11/10/2023 3:21 AM, Kalle Valo wrote:
> From: Baochen Qiang <quic_bqiang@quicinc.com>
> 
> When using mhi_power_down_no_destroy() MHI hosts need to unprepare MHI channels
> by themselves.  Similarly, MHI stack will also not create new MHI device since
> old devices were not destroyed, so MHI hosts need to prepare channels as well.
> Hence add these two interfaces to make that possible.
> 
> 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>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
>   drivers/bus/mhi/host/main.c | 91 +++++++++++++++++++++++++++++++++++++
>   include/linux/mhi.h         | 18 ++++++++
>   2 files changed, 109 insertions(+)
> 
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index dcf627b36e82..9bcf8a49c000 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -1667,6 +1667,49 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
>   }
>   EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
>   
> +static int __mhi_prepare_for_transfer_autoqueue(struct device *dev, void *data)
> +{
> +	struct mhi_device *mhi_dev;
> +	struct mhi_chan *ul_chan, *dl_chan;
> +	enum mhi_ee_type ee = MHI_EE_MAX;
> +
> +	if (dev->bus != &mhi_bus_type)
> +		return 0;
> +
> +	mhi_dev = to_mhi_device(dev);
> +
> +	/* Only prepare virtual devices thats attached to bus */

"that are"?

> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> +		return 0;
> +
> +	ul_chan = mhi_dev->ul_chan;
> +	dl_chan = mhi_dev->dl_chan;
> +
> +	/*
> +	 * If execution environment is specified, remove only those devices that
> +	 * started in them based on ee_mask for the channels as we move on to a
> +	 * different execution environment
> +	 */
> +	if (data)
> +		ee = *(enum mhi_ee_type *)data;
> +
> +	if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
> +		return 0;
> +
> +
> +	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
> +		return 0;
> +
> +	return mhi_prepare_for_transfer_autoqueue(mhi_dev);
> +}
> +
> +int mhi_prepare_all_for_transfer_autoqueue(struct mhi_controller *mhi_cntrl)
> +{
> +	return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
> +				     __mhi_prepare_for_transfer_autoqueue);
> +}
> +EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer_autoqueue);

This seems broken.  It appears to configure all channels as autoqueue, 
regardless of how the controller initially configured them.  This would 
only be safe to use if all channels were configured for autoqueue, but 
would silently cause issues otherwise.

Re: [PATCH RFC 2/8] bus: mhi: host: add new interfaces to handle MHI channels directly

[Baochen Qiang]

On 11/11/2023 1:14 AM, Jeffrey Hugo wrote:
> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>
>> When using mhi_power_down_no_destroy() MHI hosts need to unprepare 
>> MHI channels
>> by themselves.  Similarly, MHI stack will also not create new MHI 
>> device since
>> old devices were not destroyed, so MHI hosts need to prepare channels 
>> as well.
>> Hence add these two interfaces to make that possible.
>>
>> 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>
>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>> ---
>>   drivers/bus/mhi/host/main.c | 91 +++++++++++++++++++++++++++++++++++++
>>   include/linux/mhi.h         | 18 ++++++++
>>   2 files changed, 109 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
>> index dcf627b36e82..9bcf8a49c000 100644
>> --- a/drivers/bus/mhi/host/main.c
>> +++ b/drivers/bus/mhi/host/main.c
>> @@ -1667,6 +1667,49 @@ int mhi_prepare_for_transfer_autoqueue(struct 
>> mhi_device *mhi_dev)
>>   }
>>   EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
>>   +static int __mhi_prepare_for_transfer_autoqueue(struct device 
>> *dev, void *data)
>> +{
>> +    struct mhi_device *mhi_dev;
>> +    struct mhi_chan *ul_chan, *dl_chan;
>> +    enum mhi_ee_type ee = MHI_EE_MAX;
>> +
>> +    if (dev->bus != &mhi_bus_type)
>> +        return 0;
>> +
>> +    mhi_dev = to_mhi_device(dev);
>> +
>> +    /* Only prepare virtual devices thats attached to bus */
>
> "that are"?
>
It means MHI devices with a type of MHI_DEVICE_XFER. See also 
mhi_destroy_device();


>> +    if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>> +        return 0;
>> +
>> +    ul_chan = mhi_dev->ul_chan;
>> +    dl_chan = mhi_dev->dl_chan;
>> +
>> +    /*
>> +     * If execution environment is specified, remove only those 
>> devices that
>> +     * started in them based on ee_mask for the channels as we move 
>> on to a
>> +     * different execution environment
>> +     */
>> +    if (data)
>> +        ee = *(enum mhi_ee_type *)data;
>> +
>> +    if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
>> +        return 0;
>> +
>> +
>> +    if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
>> +        return 0;
>> +
>> +    return mhi_prepare_for_transfer_autoqueue(mhi_dev);
>> +}
>> +
>> +int mhi_prepare_all_for_transfer_autoqueue(struct mhi_controller 
>> *mhi_cntrl)
>> +{
>> +    return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>> +                     __mhi_prepare_for_transfer_autoqueue);
>> +}
>> +EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer_autoqueue);
>
> This seems broken.  It appears to configure all channels as autoqueue, 
> regardless of how the controller initially configured them.  This 
> would only be safe to use if all channels were configured for 
> autoqueue, but would silently cause issues otherwise.

Thanks for pointing that. Yes, it is not correct to treat all channels 
as autoqueue regardless of its initial configuration. So how about 
change as below:

/* The difference between mhi_prepare_for_transfer_autoqueue() and 
mhi_prepare_for_transfer() comes from how to treat downlink channel */

mhi_prepare_for_transfer_dev(struct device *dev, ...)

{

...

dl_chan = mhi_dev->dl_chan;

...

if (dl_chan->pre_alloc)

        mhi_prepare_for_transfer_autoqueue(dev);

else

        mhi_prepare_for_transfer(dev);

}

/* And then iterate all devices and call mhi_prepare_for_transfer_dev() 
for each. */

int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl)
{
     return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
                      mhi_prepare_for_transfer_dev);
}
EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);

Re: [PATCH RFC 4/8] wifi: ath11k: remove MHI LOOPBACK channels

[Baochen Qiang]

On 11/11/2023 12:54 AM, Jeffrey Hugo wrote:
> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>
>> There is no driver to match these two channels, so
>> remove them. This fixes warnings from MHI subsystem during suspend:
>>
>> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
>> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting
>
> This feels like just masking a real issue.
>
> If LOOPBACK is not being consumed, then the channel should never go 
> into the start state.  Why would we be trying to transition to the 
> reset state then?
>
> -Jeff
That is because, with patch 'bus: mhi: host: add new interfaces to 
handle MHI channels directly' in this patch set, ath11k is able to call 
mhi_unprepare_all_from_transfer(), which will reset all channels.

Re: [PATCH RFC 4/8] wifi: ath11k: remove MHI LOOPBACK channels

[Jeffrey Hugo]

On 11/11/2023 9:24 PM, Baochen Qiang wrote:
> 
> On 11/11/2023 12:54 AM, Jeffrey Hugo wrote:
>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>
>>> There is no driver to match these two channels, so
>>> remove them. This fixes warnings from MHI subsystem during suspend:
>>>
>>> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
>>> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting
>>
>> This feels like just masking a real issue.
>>
>> If LOOPBACK is not being consumed, then the channel should never go 
>> into the start state.  Why would we be trying to transition to the 
>> reset state then?
>>
>> -Jeff
> That is because, with patch 'bus: mhi: host: add new interfaces to 
> handle MHI channels directly' in this patch set, ath11k is able to call 
> mhi_unprepare_all_from_transfer(), which will reset all channels.

that implementation is flawed if it is causing this.  Looks like you 
never check to see if the channel was prepared in the first place.

If you go fix that, then it looks like this change is not needed.

Re: [PATCH RFC 2/8] bus: mhi: host: add new interfaces to handle MHI channels directly

[Jeffrey Hugo]

On 11/11/2023 8:59 PM, Baochen Qiang wrote:
> 
> On 11/11/2023 1:14 AM, Jeffrey Hugo wrote:
>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>
>>> When using mhi_power_down_no_destroy() MHI hosts need to unprepare 
>>> MHI channels
>>> by themselves.  Similarly, MHI stack will also not create new MHI 
>>> device since
>>> old devices were not destroyed, so MHI hosts need to prepare channels 
>>> as well.
>>> Hence add these two interfaces to make that possible.
>>>
>>> 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>
>>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>>> ---
>>>   drivers/bus/mhi/host/main.c | 91 +++++++++++++++++++++++++++++++++++++
>>>   include/linux/mhi.h         | 18 ++++++++
>>>   2 files changed, 109 insertions(+)
>>>
>>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
>>> index dcf627b36e82..9bcf8a49c000 100644
>>> --- a/drivers/bus/mhi/host/main.c
>>> +++ b/drivers/bus/mhi/host/main.c
>>> @@ -1667,6 +1667,49 @@ int mhi_prepare_for_transfer_autoqueue(struct 
>>> mhi_device *mhi_dev)
>>>   }
>>>   EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
>>>   +static int __mhi_prepare_for_transfer_autoqueue(struct device 
>>> *dev, void *data)
>>> +{
>>> +    struct mhi_device *mhi_dev;
>>> +    struct mhi_chan *ul_chan, *dl_chan;
>>> +    enum mhi_ee_type ee = MHI_EE_MAX;
>>> +
>>> +    if (dev->bus != &mhi_bus_type)
>>> +        return 0;
>>> +
>>> +    mhi_dev = to_mhi_device(dev);
>>> +
>>> +    /* Only prepare virtual devices thats attached to bus */
>>
>> "that are"?
>>
> It means MHI devices with a type of MHI_DEVICE_XFER. See also 
> mhi_destroy_device();

I think you are confused about my comment.  "thats" is not correct 
English.  I was suggesting you replace it with "that are", but there are 
many ways to reword the comment.

> 
> 
>>> +    if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>>> +        return 0;
>>> +
>>> +    ul_chan = mhi_dev->ul_chan;
>>> +    dl_chan = mhi_dev->dl_chan;
>>> +
>>> +    /*
>>> +     * If execution environment is specified, remove only those 
>>> devices that
>>> +     * started in them based on ee_mask for the channels as we move 
>>> on to a
>>> +     * different execution environment
>>> +     */
>>> +    if (data)
>>> +        ee = *(enum mhi_ee_type *)data;
>>> +
>>> +    if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
>>> +        return 0;
>>> +
>>> +
>>> +    if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
>>> +        return 0;
>>> +
>>> +    return mhi_prepare_for_transfer_autoqueue(mhi_dev);
>>> +}
>>> +
>>> +int mhi_prepare_all_for_transfer_autoqueue(struct mhi_controller 
>>> *mhi_cntrl)
>>> +{
>>> +    return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>>> +                     __mhi_prepare_for_transfer_autoqueue);
>>> +}
>>> +EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer_autoqueue);
>>
>> This seems broken.  It appears to configure all channels as autoqueue, 
>> regardless of how the controller initially configured them.  This 
>> would only be safe to use if all channels were configured for 
>> autoqueue, but would silently cause issues otherwise.
> 
> Thanks for pointing that. Yes, it is not correct to treat all channels 
> as autoqueue regardless of its initial configuration. So how about 
> change as below:

Seems ok.

> 
> /* The difference between mhi_prepare_for_transfer_autoqueue() and 
> mhi_prepare_for_transfer() comes from how to treat downlink channel */
> 
> mhi_prepare_for_transfer_dev(struct device *dev, ...)
> 
> {
> 
> ...
> 
> dl_chan = mhi_dev->dl_chan;
> 
> ...
> 
> if (dl_chan->pre_alloc)
> 
>         mhi_prepare_for_transfer_autoqueue(dev);
> 
> else
> 
>         mhi_prepare_for_transfer(dev);
> 
> }
> 
> /* And then iterate all devices and call mhi_prepare_for_transfer_dev() 
> for each. */
> 
> int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl)
> {
>      return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>                       mhi_prepare_for_transfer_dev);
> }
> EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
> 

Re: [PATCH RFC 4/8] wifi: ath11k: remove MHI LOOPBACK channels

[Baochen Qiang]

On 11/13/2023 12:15 AM, Jeffrey Hugo wrote:
> On 11/11/2023 9:24 PM, Baochen Qiang wrote:
>>
>> On 11/11/2023 12:54 AM, Jeffrey Hugo wrote:
>>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>>
>>>> There is no driver to match these two channels, so
>>>> remove them. This fixes warnings from MHI subsystem during suspend:
>>>>
>>>> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
>>>> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting
>>>
>>> This feels like just masking a real issue.
>>>
>>> If LOOPBACK is not being consumed, then the channel should never go 
>>> into the start state.  Why would we be trying to transition to the 
>>> reset state then?
>>>
>>> -Jeff
>> That is because, with patch 'bus: mhi: host: add new interfaces to 
>> handle MHI channels directly' in this patch set, ath11k is able to 
>> call mhi_unprepare_all_from_transfer(), which will reset all channels.
>
> that implementation is flawed if it is causing this.  Looks like you 
> never check to see if the channel was prepared in the first place.
>
> If you go fix that, then it looks like this change is not needed.

Sure, will check whether channel is in MHI_CH_STATE_ENABLED state in 
next version.

Re: [PATCH RFC 2/8] bus: mhi: host: add new interfaces to handle MHI channels directly

[Baochen Qiang]

On 11/13/2023 12:18 AM, Jeffrey Hugo wrote:
> On 11/11/2023 8:59 PM, Baochen Qiang wrote:
>>
>> On 11/11/2023 1:14 AM, Jeffrey Hugo wrote:
>>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>>
>>>> When using mhi_power_down_no_destroy() MHI hosts need to unprepare 
>>>> MHI channels
>>>> by themselves.  Similarly, MHI stack will also not create new MHI 
>>>> device since
>>>> old devices were not destroyed, so MHI hosts need to prepare 
>>>> channels as well.
>>>> Hence add these two interfaces to make that possible.
>>>>
>>>> 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>
>>>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>>>> ---
>>>>   drivers/bus/mhi/host/main.c | 91 
>>>> +++++++++++++++++++++++++++++++++++++
>>>>   include/linux/mhi.h         | 18 ++++++++
>>>>   2 files changed, 109 insertions(+)
>>>>
>>>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
>>>> index dcf627b36e82..9bcf8a49c000 100644
>>>> --- a/drivers/bus/mhi/host/main.c
>>>> +++ b/drivers/bus/mhi/host/main.c
>>>> @@ -1667,6 +1667,49 @@ int 
>>>> mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
>>>>   +static int __mhi_prepare_for_transfer_autoqueue(struct device 
>>>> *dev, void *data)
>>>> +{
>>>> +    struct mhi_device *mhi_dev;
>>>> +    struct mhi_chan *ul_chan, *dl_chan;
>>>> +    enum mhi_ee_type ee = MHI_EE_MAX;
>>>> +
>>>> +    if (dev->bus != &mhi_bus_type)
>>>> +        return 0;
>>>> +
>>>> +    mhi_dev = to_mhi_device(dev);
>>>> +
>>>> +    /* Only prepare virtual devices thats attached to bus */
>>>
>>> "that are"?
>>>
>> It means MHI devices with a type of MHI_DEVICE_XFER. See also 
>> mhi_destroy_device();
>
> I think you are confused about my comment.  "thats" is not correct 
> English.  I was suggesting you replace it with "that are", but there 
> are many ways to reword the comment.

Sorry for misunderstood your comment. Will refine it in next version.


>
>>
>>
>>>> +    if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>>>> +        return 0;
>>>> +
>>>> +    ul_chan = mhi_dev->ul_chan;
>>>> +    dl_chan = mhi_dev->dl_chan;
>>>> +
>>>> +    /*
>>>> +     * If execution environment is specified, remove only those 
>>>> devices that
>>>> +     * started in them based on ee_mask for the channels as we 
>>>> move on to a
>>>> +     * different execution environment
>>>> +     */
>>>> +    if (data)
>>>> +        ee = *(enum mhi_ee_type *)data;
>>>> +
>>>> +    if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
>>>> +        return 0;
>>>> +
>>>> +
>>>> +    if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
>>>> +        return 0;
>>>> +
>>>> +    return mhi_prepare_for_transfer_autoqueue(mhi_dev);
>>>> +}
>>>> +
>>>> +int mhi_prepare_all_for_transfer_autoqueue(struct mhi_controller 
>>>> *mhi_cntrl)
>>>> +{
>>>> +    return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>>>> +                     __mhi_prepare_for_transfer_autoqueue);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer_autoqueue);
>>>
>>> This seems broken.  It appears to configure all channels as 
>>> autoqueue, regardless of how the controller initially configured 
>>> them.  This would only be safe to use if all channels were 
>>> configured for autoqueue, but would silently cause issues otherwise.
>>
>> Thanks for pointing that. Yes, it is not correct to treat all 
>> channels as autoqueue regardless of its initial configuration. So how 
>> about change as below:
>
> Seems ok.
>
>>
>> /* The difference between mhi_prepare_for_transfer_autoqueue() and 
>> mhi_prepare_for_transfer() comes from how to treat downlink channel */
>>
>> mhi_prepare_for_transfer_dev(struct device *dev, ...)
>>
>> {
>>
>> ...
>>
>> dl_chan = mhi_dev->dl_chan;
>>
>> ...
>>
>> if (dl_chan->pre_alloc)
>>
>>         mhi_prepare_for_transfer_autoqueue(dev);
>>
>> else
>>
>>         mhi_prepare_for_transfer(dev);
>>
>> }
>>
>> /* And then iterate all devices and call 
>> mhi_prepare_for_transfer_dev() for each. */
>>
>> int mhi_prepare_all_for_transfer(struct mhi_controller *mhi_cntrl)
>> {
>>      return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
>>                       mhi_prepare_for_transfer_dev);
>> }
>> EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
>>
>

Re: [PATCH RFC 4/8] wifi: ath11k: remove MHI LOOPBACK channels

[Kalle Valo]

Jeffrey Hugo <quic_jhugo@quicinc.com> writes:

> On 11/11/2023 9:24 PM, Baochen Qiang wrote:
>
>> On 11/11/2023 12:54 AM, Jeffrey Hugo wrote:
>>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>>
>>>> There is no driver to match these two channels, so
>>>> remove them. This fixes warnings from MHI subsystem during suspend:
>>>>
>>>> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
>>>> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting
>>>
>>> This feels like just masking a real issue.
>>>
>>> If LOOPBACK is not being consumed, then the channel should never go
>>> into the start state.  Why would we be trying to transition to the
>>> reset state then?
>>>
>>> -Jeff
>> That is because, with patch 'bus: mhi: host: add new interfaces to
>> handle MHI channels directly' in this patch set, ath11k is able to
>> call mhi_unprepare_all_from_transfer(), which will reset all
>> channels.
>
> that implementation is flawed if it is causing this.  Looks like you
> never check to see if the channel was prepared in the first place.
>
> If you go fix that, then it looks like this change is not needed.

BTW what do these loopback channels do? I didn't notice any difference
in the functionality so I'm wondering the reason for these.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH RFC 4/8] wifi: ath11k: remove MHI LOOPBACK channels

[Jeffrey Hugo]

On 11/13/2023 7:15 AM, Kalle Valo wrote:
> Jeffrey Hugo <quic_jhugo@quicinc.com> writes:
> 
>> On 11/11/2023 9:24 PM, Baochen Qiang wrote:
>>
>>> On 11/11/2023 12:54 AM, Jeffrey Hugo wrote:
>>>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>>>
>>>>> There is no driver to match these two channels, so
>>>>> remove them. This fixes warnings from MHI subsystem during suspend:
>>>>>
>>>>> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
>>>>> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting
>>>>
>>>> This feels like just masking a real issue.
>>>>
>>>> If LOOPBACK is not being consumed, then the channel should never go
>>>> into the start state.  Why would we be trying to transition to the
>>>> reset state then?
>>>>
>>>> -Jeff
>>> That is because, with patch 'bus: mhi: host: add new interfaces to
>>> handle MHI channels directly' in this patch set, ath11k is able to
>>> call mhi_unprepare_all_from_transfer(), which will reset all
>>> channels.
>>
>> that implementation is flawed if it is causing this.  Looks like you
>> never check to see if the channel was prepared in the first place.
>>
>> If you go fix that, then it looks like this change is not needed.
> 
> BTW what do these loopback channels do? I didn't notice any difference
> in the functionality so I'm wondering the reason for these.
> 

The loopback channel is defined as a service where any data the host 
sends to the device is immediately sent back to the host, unmodified. 
The typical usecase is smoke test and performance profiling.

I do not object to the removal of the channel from the atheros devices, 
assuming suitable justification.  Not having a use for the channel seems 
like good justification.  Working around a bug seem more like a hack 
than proper justification.

Re: [PATCH RFC 4/8] wifi: ath11k: remove MHI LOOPBACK channels

[Kalle Valo]

Jeffrey Hugo <quic_jhugo@quicinc.com> writes:

> On 11/13/2023 7:15 AM, Kalle Valo wrote:
>
>> Jeffrey Hugo <quic_jhugo@quicinc.com> writes:
>> 
>>> On 11/11/2023 9:24 PM, Baochen Qiang wrote:
>>>
>>>> On 11/11/2023 12:54 AM, Jeffrey Hugo wrote:
>>>>> On 11/10/2023 3:21 AM, Kalle Valo wrote:
>>>>>> From: Baochen Qiang <quic_bqiang@quicinc.com>
>>>>>>
>>>>>> There is no driver to match these two channels, so
>>>>>> remove them. This fixes warnings from MHI subsystem during suspend:
>>>>>>
>>>>>> mhi mhi0_LOOPBACK: 1: Failed to reset channel, still resetting
>>>>>> mhi mhi0_LOOPBACK: 0: Failed to reset channel, still resetting
>>>>>
>>>>> This feels like just masking a real issue.
>>>>>
>>>>> If LOOPBACK is not being consumed, then the channel should never go
>>>>> into the start state.  Why would we be trying to transition to the
>>>>> reset state then?
>>>>>
>>>>> -Jeff
>>>> That is because, with patch 'bus: mhi: host: add new interfaces to
>>>> handle MHI channels directly' in this patch set, ath11k is able to
>>>> call mhi_unprepare_all_from_transfer(), which will reset all
>>>> channels.
>>>
>>> that implementation is flawed if it is causing this.  Looks like you
>>> never check to see if the channel was prepared in the first place.
>>>
>>> If you go fix that, then it looks like this change is not needed.
>> BTW what do these loopback channels do? I didn't notice any
>> difference
>> in the functionality so I'm wondering the reason for these.
>> 
>
> The loopback channel is defined as a service where any data the host
> sends to the device is immediately sent back to the host, unmodified.
> The typical usecase is smoke test and performance profiling.

Thanks, good to know. We don't have any such functionality in ath11k
right now and if we ever do we can always add the channels back.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH RFC 1/8] bus: mhi: host: add mhi_power_down_no_destroy()

[Mayank Rana]

On 11/10/2023 2:21 AM, Kalle Valo wrote:
> From: Baochen Qiang <quic_bqiang@quicinc.com>
> 
> If ath11k tries to call mhi_power_up() during resume it fails:
> 
> ath11k_pci 0000:06:00.0: timeout while waiting for restart complete
> 
> This happens because when calling mhi_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().
> 	 */
have you consider registering for pm_notifier callback specifically for 
PM_POST_HIBERNATION (Documentation/power/notifiers.txt), and then making 
decision of calling MHI power up sequence which would make sure that 
created device based driver probe() is completed.
> 
> Because the device probe is deferred, the qcom_mhi_qrtr_probe() is not called and
> qcom_mhi_qrtr_dl_callback() fails silently as qdev is zero:
> 
> static void qcom_mhi_qrtr_dl_callback(struct mhi_device *mhi_dev,
> 				      struct mhi_result *mhi_res)
> {
> 	struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> 	int rc;
> 
> 	if (!qdev || mhi_res->transaction_status)
> 		return;
> 
> 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
> initialisation.
> 
> To fix this add new function mhi_power_down_no_destroy() which does not destroy
> the devices during power down. This way mhi_power_up() can be called during
> resume and we can get ath11k hibernation working with the following patches.
> 
> 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>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
>   drivers/bus/mhi/host/internal.h |  1 +
>   drivers/bus/mhi/host/pm.c       | 26 +++++++++++++++++++-------
>   include/linux/mhi.h             | 29 +++++++++++++++++++++++++++--
>   3 files changed, 47 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
> index 2e139e76de4c..d1033af285e2 100644
> --- 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,
>   };
>   
> diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
> index 8a4362d75fc4..e0e429adbda6 100644
> --- a/drivers/bus/mhi/host/pm.c
> +++ b/drivers/bus/mhi/host/pm.c
> @@ -453,7 +453,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;
> @@ -515,8 +516,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);
> +	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 *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;
> @@ -1154,7 +1160,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)
> +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,14 +1197,19 @@ 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]);
>   }
> -EXPORT_SYMBOL_GPL(mhi_power_down);
> +EXPORT_SYMBOL_GPL(__mhi_power_down);
>   
>   int mhi_sync_power_up(struct mhi_controller *mhi_cntrl)
>   {
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 039943ec4d4e..85edc9c5df88 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -644,12 +644,37 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl);
>    */
>   int mhi_sync_power_up(struct mhi_controller *mhi_cntrl);
>   
> +void __mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful,
> +		    bool destroy_device);
> +
>   /**
> - * 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.
> + *
>    * @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);
> +static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
> +{
> +	__mhi_power_down(mhi_cntrl, graceful, true);
> +}
> +
> +/**
> + * mhi_power_down_no_destroy - Start MHI power down sequence but don't
> + * destroy struct devices. This is a variant for mhi_power_down() and is a
> + * workaround to make it possible to use mhi_power_up() in a resume
> + * handler. When using this variant the caller must also call
> + * mhi_prepare_all_for_transfer_autoqueue() and
> + * mhi_unprepare_all_from_transfer().
> + *
> + * @mhi_cntrl: MHI controller
> + * @graceful: Link is still accessible, so do a graceful shutdown process
> + */
> +static inline void mhi_power_down_no_destroy(struct mhi_controller *mhi_cntrl,
> +					     bool graceful)
> +{
> +	__mhi_power_down(mhi_cntrl, graceful, false);
> +}
>   
>   /**
>    * mhi_unprepare_after_power_down - Free any allocated memory after power down