Search Linux Wireless

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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 1/31/2024 2:04 AM, Manivannan Sadhasivam wrote:
On Mon, Nov 27, 2023 at 06:20:15PM +0200, Kalle Valo wrote:
From: Baochen Qiang <quic_bqiang@xxxxxxxxxxx>

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


This is confusing! Maybe this is what confused me initially. mhi_sync_power_up()
never fails, but ath11k timesout waiting for QMI. You also confirmed the same
[1].

mhi_sync_power_up() creates MHI devices which fails to get probed. So from the view of ath11k it fails, while from the sense of code execuation it succeeds. Will rephrase to avoid confusion.


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

diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index 65ceac1837f9..e626b03ffafa 100644
--- 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[DEV_ST_TRANSITION_MAX] = {
  	[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] = {
diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
index 30ac415a3000..3f45c9c447bd 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 a2f2feef1476..8833b0248393 100644
--- a/drivers/bus/mhi/host/pm.c
+++ b/drivers/bus/mhi/host/pm.c
@@ -458,7 +458,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;
@@ -520,8 +521,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); @@ -806,7 +809,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;
@@ -1160,7 +1166,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;
@@ -1196,14 +1203,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);

This is a helper, so should not be exported. You should export the API instead.

int mhi_sync_power_up(struct mhi_controller *mhi_cntrl)
  {
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index d0f9b522f328..ae092bc8b97e 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -648,12 +648,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);

This is a helper, so make it static.

+
  /**
- * 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.

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);
+static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)

No, API cannot be static inline. Make it global.

+{
+	__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

"struct devices for channels"

+ * workaround to make it possible to use mhi_power_up() in a resume

You should mention that the devices are not destroyed and this would be useful
in suspend/hibernation.

+ * handler. When using this variant the caller must also call
+ * mhi_prepare_all_for_transfer_autoqueue() and

mhi_prepare_all_for_transfer*()

+ * 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)

Same as above, make it global.

- Mani





[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux