wifi: ath11k: hibernation support

From: Baochen Qiang <[email protected]>

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 <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

---
 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(-)

drivers/bus/mhi/host/init.c +1 -0

index 65ceac1837f9..e626b03ffafa 100644
@@ -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] = {

drivers/bus/mhi/host/internal.h +1 -0

index 30ac415a3000..3f45c9c447bd 100644
@@ -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,
 };
 

drivers/bus/mhi/host/pm.c +19 -7

index a2f2feef1476..8833b0248393 100644
@@ -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);
 
 int mhi_sync_power_up(struct mhi_controller *mhi_cntrl)
 {

include/linux/mhi.h +27 -2

index d0f9b522f328..ae092bc8b97e 100644
@@ -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);
+
 /**
- * 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

Signature · 1 line

2.39.2

Quoted 37 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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.

Quoted 8 lines

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 <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

Any reason for reposting this series without discussing the suggestion from Mayank? As I said in the internal thread, this patch breaks the Linux device driver model by not destroying the "struct device" when the actual device gets removed. We should try to explore alternate options instead of persisting with this solution.

- Mani

Quoted 151 lines

---
  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(-)

drivers/bus/mhi/host/init.c +1 -0

index 65ceac1837f9..e626b03ffafa 100644
@@ -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] = {

drivers/bus/mhi/host/internal.h +1 -0

index 30ac415a3000..3f45c9c447bd 100644
@@ -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,
  };
 

drivers/bus/mhi/host/pm.c +19 -7

index a2f2feef1476..8833b0248393 100644
@@ -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);
 
  int mhi_sync_power_up(struct mhi_controller *mhi_cntrl)
  {

include/linux/mhi.h +27 -2

index d0f9b522f328..ae092bc8b97e 100644
@@ -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);
+
  /**
- * 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

Signature · 1 line

2.39.2

Quoted 41 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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.

Quoted 4 lines

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]>

Signed-off-by Kalle Valo <[email protected]>

Any reason for reposting this series without discussing the suggestion from Mayank?

As I said in the internal thread, this patch breaks the Linux device driver model by not destroying the "struct device" when the actual device gets removed.

We should try to explore alternate options instead of persisting with this solution.

Quoted 43 lines · Manivannan Sadhasivam

Quoted 41 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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.

Quoted 7 lines

Quoted 4 lines

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]>

Signed-off-by Kalle Valo <[email protected]>

Any reason for reposting this series without discussing the suggestion from Mayank?

Baochen quickly sent me fixes for the v1 review comments, as I have been out of office for some time I didn't want to sit on Baochen's fixes for too long. Better to get them out of the door as soon as possible. I will definitely look at Mayank's proposal but that will take longer.

Quoted 3 lines

As I said in the internal thread, this patch breaks the Linux device driver model by not destroying the "struct device" when the actual device gets removed.

This patchset has been tested by several people, I'm even using this patchset on main laptop every day, and we haven't noticed any issues.

Can you elaborate more about this driver model? We are not removing any ath11k devices, we just want to power down the ath11k (and in the future ath12k) devices for suspend and power up during resume.

Quoted 2 lines

We should try to explore alternate options instead of persisting with this solution.

What other options we have here? At least Baochen is not optimistic that using PM_POST_HIBERNATION as a workaround would work. The issue we have here is that mhi_power_up() doesn't work in the resume handler and that's what we should try to fix, not make workarounds.

Quoted 43 lines · Manivannan Sadhasivam

Quoted 41 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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.

Quoted 7 lines

Quoted 4 lines

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]>

Signed-off-by Kalle Valo <[email protected]>

Any reason for reposting this series without discussing the suggestion from Mayank?

Baochen quickly sent me fixes for the v1 review comments, as I have been out of office for some time I didn't want to sit on Baochen's fixes for too long. Better to get them out of the door as soon as possible. I will definitely look at Mayank's proposal but that will take longer.

Quoted 3 lines

As I said in the internal thread, this patch breaks the Linux device driver model by not destroying the "struct device" when the actual device gets removed.

This patchset has been tested by several people, I'm even using this patchset on main laptop every day, and we haven't noticed any issues.

Can you elaborate more about this driver model? We are not removing any ath11k devices, we just want to power down the ath11k (and in the future ath12k) devices for suspend and power up during resume.

Quoted 2 lines

We should try to explore alternate options instead of persisting with this solution.

What other options we have here? At least Baochen is not optimistic that using PM_POST_HIBERNATION as a workaround would work. The issue we have here is that mhi_power_up() doesn't work in the resume handler and that's what we should try to fix, not make workarounds.

Signature · 3 lines

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

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

Quoted 62 lines · Kalle Valo

Quoted 43 lines · Manivannan Sadhasivam

Quoted 41 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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.

Quoted 7 lines

Quoted 4 lines

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]>

Signed-off-by Kalle Valo <[email protected]>

Any reason for reposting this series without discussing the suggestion from Mayank?

Baochen quickly sent me fixes for the v1 review comments, as I have been out of office for some time I didn't want to sit on Baochen's fixes for too long. Better to get them out of the door as soon as possible. I will definitely look at Mayank's proposal but that will take longer.

Quoted 3 lines

As I said in the internal thread, this patch breaks the Linux device driver model by not destroying the "struct device" when the actual device gets removed.

Quoted 7 lines

This patchset has been tested by several people, I'm even using this patchset on main laptop every day, and we haven't noticed any issues.

Can you elaborate more about this driver model? We are not removing any ath11k devices, we just want to power down the ath11k (and in the future ath12k) devices for suspend and power up during resume.

Devices (struct dev) for each channels are created once the device (WLAN) enters runtime mode such as (MISSION, SBL etc...). During hibernation, ath11k stack calls mhi_power_down() which essentially resets the device to POR and also the stack powers down the device properly.

In that case, MHI channels do not exist as the device (WLAN) itself is powered down. As per kernel driver model, each struct device is tied to its reference count. And the reference count should be decremented whenever the actual device is not in use. Once the actual device is removed from the system, then the respective struct device has to be destroyed altogether.

So in this case, even though the channels are not active (present) in the device, the device itself gets powered off, you want MHI stack to keep the struct device active, which is against the model I referenced above.

To fix this issue properly, we need to investigate on how other subsystems are handling this situation (device getting powered down during hibernation), like USB.

- Mani

Quoted 12 lines

Quoted 2 lines

We should try to explore alternate options instead of persisting with this solution.

What other options we have here? At least Baochen is not optimistic that using PM_POST_HIBERNATION as a workaround would work. The issue we have here is that mhi_power_up() doesn't work in the resume handler and that's what we should try to fix, not make workarounds.

Signature · 3 lines

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

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

Signature · 1 line

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

Quoted 82 lines · Manivannan Sadhasivam

Quoted 62 lines · Kalle Valo

Quoted 43 lines · Manivannan Sadhasivam

Quoted 41 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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.

Quoted 7 lines

Quoted 4 lines

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]>

Signed-off-by Kalle Valo <[email protected]>

Any reason for reposting this series without discussing the suggestion from Mayank?

Baochen quickly sent me fixes for the v1 review comments, as I have been out of office for some time I didn't want to sit on Baochen's fixes for too long. Better to get them out of the door as soon as possible. I will definitely look at Mayank's proposal but that will take longer.

Quoted 3 lines

As I said in the internal thread, this patch breaks the Linux device driver model by not destroying the "struct device" when the actual device gets removed.

Quoted 7 lines

This patchset has been tested by several people, I'm even using this patchset on main laptop every day, and we haven't noticed any issues.

Can you elaborate more about this driver model? We are not removing any ath11k devices, we just want to power down the ath11k (and in the future ath12k) devices for suspend and power up during resume.

Devices (struct dev) for each channels are created once the device (WLAN) enters runtime mode such as (MISSION, SBL etc...). During hibernation, ath11k stack calls mhi_power_down() which essentially resets the device to POR and also the stack powers down the device properly.

In that case, MHI channels do not exist as the device (WLAN) itself is powered down. As per kernel driver model, each struct device is tied to its reference count. And the reference count should be decremented whenever the actual device is not in use. Once the actual device is removed from the system, then the respective struct device has to be destroyed altogether.

Quoted 8 lines

So in this case, even though the channels are not active (present) in the device, the device itself gets powered off, you want MHI stack to keep the struct device active, which is against the model I referenced above.

To fix this issue properly, we need to investigate on how other subsystems are handling this situation (device getting powered down during hibernation), like USB.

To me it all sounds like the probe deferral is not handled properly in mac80211 stack. As you mentioned in the commit message that the dpm_prepare() blocks probing of devices. It gets unblocked and trigerred in dpm_complete():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/main.c#n1131

So if mac80211/ath11k cannot probe the devices at the dpm_complete() stage, then it is definitely an issue that needs to be fixed properly.

- Mani

Quoted 17 lines

- Mani

Quoted 12 lines

Quoted 2 lines

We should try to explore alternate options instead of persisting with this solution.

What other options we have here? At least Baochen is not optimistic that using PM_POST_HIBERNATION as a workaround would work. The issue we have here is that mhi_power_up() doesn't work in the resume handler and that's what we should try to fix, not make workarounds.

Signature · 3 lines

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

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

Signature · 1 line

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

Quoted 97 lines · Manivannan Sadhasivam

Quoted 82 lines · Manivannan Sadhasivam

Quoted 62 lines · Kalle Valo

Quoted 43 lines · Manivannan Sadhasivam

Quoted 41 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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.

Quoted 7 lines

Quoted 4 lines

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]>

Signed-off-by Kalle Valo <[email protected]>

Any reason for reposting this series without discussing the suggestion from Mayank?

Baochen quickly sent me fixes for the v1 review comments, as I have been out of office for some time I didn't want to sit on Baochen's fixes for too long. Better to get them out of the door as soon as possible. I will definitely look at Mayank's proposal but that will take longer.

Quoted 3 lines

As I said in the internal thread, this patch breaks the Linux device driver model by not destroying the "struct device" when the actual device gets removed.

Quoted 7 lines

This patchset has been tested by several people, I'm even using this patchset on main laptop every day, and we haven't noticed any issues.

Can you elaborate more about this driver model? We are not removing any ath11k devices, we just want to power down the ath11k (and in the future ath12k) devices for suspend and power up during resume.

Devices (struct dev) for each channels are created once the device (WLAN) enters runtime mode such as (MISSION, SBL etc...). During hibernation, ath11k stack calls mhi_power_down() which essentially resets the device to POR and also the stack powers down the device properly.

In that case, MHI channels do not exist as the device (WLAN) itself is powered down. As per kernel driver model, each struct device is tied to its reference count. And the reference count should be decremented whenever the actual device is not in use. Once the actual device is removed from the system, then the respective struct device has to be destroyed altogether.

Quoted 8 lines

So in this case, even though the channels are not active (present) in the device, the device itself gets powered off, you want MHI stack to keep the struct device active, which is against the model I referenced above.

To fix this issue properly, we need to investigate on how other subsystems are handling this situation (device getting powered down during hibernation), like USB.

To me it all sounds like the probe deferral is not handled properly in mac80211 stack. As you mentioned in the commit message that the dpm_prepare() blocks probing of devices. It gets unblocked and trigerred in dpm_complete():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/main.c#n1131

Quoted 2 lines

So if mac80211/ath11k cannot probe the devices at the dpm_complete() stage, then it is definitely an issue that needs to be fixed properly.

To clarify, ath11k CAN probe the devices at dpm_complete() stage. The problem is kernel does not wait for all probes to finish, and in that way we will face the issue that user space applications are likely to fail because they get thawed BEFORE WLAN is ready.

Quoted 21 lines

- Mani

Quoted 17 lines

- Mani

Quoted 12 lines

Quoted 2 lines

We should try to explore alternate options instead of persisting with this solution.

What other options we have here? At least Baochen is not optimistic that using PM_POST_HIBERNATION as a workaround would work. The issue we have here is that mhi_power_up() doesn't work in the resume handler and that's what we should try to fix, not make workarounds.

Signature · 3 lines

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

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

Signature · 1 line

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

Quoted 7 lines

Quoted 7 lines

To me it all sounds like the probe deferral is not handled properly in mac80211 stack. As you mentioned in the commit message that the dpm_prepare() blocks probing of devices. It gets unblocked and trigerred in dpm_complete():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/main.c#n1131

So if mac80211/ath11k cannot probe the devices at the dpm_complete() stage, then it is definitely an issue that needs to be fixed properly.

Quoted 5 lines

To clarify, ath11k CAN probe the devices at dpm_complete() stage. The problem is kernel does not wait for all probes to finish, and in that way we will face the issue that user space applications are likely to fail because they get thawed BEFORE WLAN is ready.

Hmm. Please give me some time to reproduce this issue locally. I will get back to this thread with my analysis.

On Thu, Jan 04, 2024 at 11:39:12AM +0530, Manivannan Sadhasivam wrote:

+ Can, Qiang

[...]

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

To me it all sounds like the probe deferral is not handled properly in mac80211 stack. As you mentioned in the commit message that the dpm_prepare() blocks probing of devices. It gets unblocked and trigerred in dpm_complete():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/main.c#n1131

So if mac80211/ath11k cannot probe the devices at the dpm_complete() stage, then it is definitely an issue that needs to be fixed properly.

Quoted 9 lines

Quoted 5 lines

To clarify, ath11k CAN probe the devices at dpm_complete() stage. The problem is kernel does not wait for all probes to finish, and in that way we will face the issue that user space applications are likely to fail because they get thawed BEFORE WLAN is ready.

Hmm. Please give me some time to reproduce this issue locally. I will get back to this thread with my analysis.

We reproduced the issue with the help of PCIe team (thanks Can). What we found out was, during the resume from hibernation the faliure happens in ath11k_core_resume(). Precisely here:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/drivers/net/wireless/ath/ath11k/core.c?h=ath11k-hibernation-support#n850

This code waits for the QMI messages to arrive and eventually timesout. But the impression I got from the start was that the mhi_power_up() always fails during resume. In our investigation, we confirmed that the failure is not happening at the MHI level.No, mhi_power_up() never fails as it only downloads PBL, SBL and waits

I'm not pointing fingers here, but trying to understand why can't you fix ath11k_core_resume() to not timeout? IMO this timeout should be handled as a deferral case.

- Mani

Quoted 28 lines · Manivannan Sadhasivam

On Thu, Jan 04, 2024 at 11:39:12AM +0530, Manivannan Sadhasivam wrote:

+ Can, Qiang

[...]

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

To me it all sounds like the probe deferral is not handled properly in mac80211 stack. As you mentioned in the commit message that the dpm_prepare() blocks probing of devices. It gets unblocked and trigerred in dpm_complete():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/main.c#n1131

So if mac80211/ath11k cannot probe the devices at the dpm_complete() stage, then it is definitely an issue that needs to be fixed properly.

Quoted 9 lines

Quoted 5 lines

To clarify, ath11k CAN probe the devices at dpm_complete() stage. The problem is kernel does not wait for all probes to finish, and in that way we will face the issue that user space applications are likely to fail because they get thawed BEFORE WLAN is ready.

Hmm. Please give me some time to reproduce this issue locally. I will get back to this thread with my analysis.

We reproduced the issue with the help of PCIe team (thanks Can). What we found out was, during the resume from hibernation the faliure happens in ath11k_core_resume(). Precisely here:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/drivers/net/wireless/ath/ath11k/core.c?h=ath11k-hibernation-support#n850

Quoted 5 lines

This code waits for the QMI messages to arrive and eventually timesout. But the impression I got from the start was that the mhi_power_up() always fails during resume. In our investigation, we confirmed that the failure is not happening at the MHI level.No, mhi_power_up() never fails as it only downloads PBL, SBL and waits

for mission mode, no MHI device created hence not affected by the deferred probe. However in addition to PBL/SBL, ath11k also needs to download m3.bin, borad.bin and regdb.bin. Those files are part of WLAN firmware and are downloaded via QMI messages. After mhi_power_up() succeeds ath11k_core_resume() waits for QMI downloading those files. As you know QMI relies on MHI channels, these channels are managed by qcom_mhi_qrtr_driver. Since device probing is deferred, qcom_mhi_qrtr_driver has no chance to run at this stage. As a result ath11k_core_resume() times out.

Quoted 4 lines

I'm not pointing fingers here, but trying to understand why can't you fix ath11k_core_resume() to not timeout? IMO this timeout should be handled as a deferral case.

Let's see what happens if we do it in a deferral way:
1. In ath11k_core_resume() we returns success directly without waiting for QMI downloading other firmware files.
2. Kernel unblocks device probe and schedules a work item to trigger all deferred probing. As a result MHI devices are probed by qcom_mhi_qrtr_driver and finally QMI is online.
3. kernel continues to resume and wake up userspace applications.
4. ath11k gets the message, either by kernel PM notification or something else, that QMI is ready and then downloads other firmware files.

What happens if userspace applications or network stack immediately initiate some WLAN request after resume back? Can ath11k handle such request? The answer is, most likely, no. Because there is no guarantee that QMI finishes downloading before those request.

Quoted 3 lines

- Mani

Quoted 36 lines · Baochen Qiang

Quoted 28 lines · Manivannan Sadhasivam

On Thu, Jan 04, 2024 at 11:39:12AM +0530, Manivannan Sadhasivam wrote:

+ Can, Qiang

[...]

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

To me it all sounds like the probe deferral is not handled properly in mac80211 stack. As you mentioned in the commit message that the dpm_prepare() blocks probing of devices. It gets unblocked and trigerred in dpm_complete():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/main.c#n1131

So if mac80211/ath11k cannot probe the devices at the dpm_complete() stage, then it is definitely an issue that needs to be fixed properly.

Quoted 9 lines

Quoted 5 lines

To clarify, ath11k CAN probe the devices at dpm_complete() stage. The problem is kernel does not wait for all probes to finish, and in that way we will face the issue that user space applications are likely to fail because they get thawed BEFORE WLAN is ready.

Hmm. Please give me some time to reproduce this issue locally. I will get back to this thread with my analysis.

We reproduced the issue with the help of PCIe team (thanks Can). What we found out was, during the resume from hibernation the faliure happens in ath11k_core_resume(). Precisely here:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/drivers/net/wireless/ath/ath11k/core.c?h=ath11k-hibernation-support#n850

Quoted 5 lines

This code waits for the QMI messages to arrive and eventually timesout. But the impression I got from the start was that the mhi_power_up() always fails during resume. In our investigation, we confirmed that the failure is not happening at the MHI level.No, mhi_power_up() never fails as it only downloads PBL, SBL and waits

Quoted 9 lines

for mission mode, no MHI device created hence not affected by the deferred probe. However in addition to PBL/SBL, ath11k also needs to download m3.bin, borad.bin and regdb.bin. Those files are part of WLAN firmware and are downloaded via QMI messages. After mhi_power_up() succeeds ath11k_core_resume() waits for QMI downloading those files. As you know QMI relies on MHI channels, these channels are managed by qcom_mhi_qrtr_driver. Since device probing is deferred, qcom_mhi_qrtr_driver has no chance to run at this stage. As a result ath11k_core_resume() times out.

Thanks for the info, this clarifies the issue in detail.

Quoted 14 lines

Quoted 4 lines

I'm not pointing fingers here, but trying to understand why can't you fix ath11k_core_resume() to not timeout? IMO this timeout should be handled as a deferral case.

Let's see what happens if we do it in a deferral way:
1. In ath11k_core_resume() we returns success directly without waiting for QMI downloading other firmware files.
2. Kernel unblocks device probe and schedules a work item to trigger all deferred probing. As a result MHI devices are probed by qcom_mhi_qrtr_driver and finally QMI is online.
3. kernel continues to resume and wake up userspace applications.
4. ath11k gets the message, either by kernel PM notification or something else, that QMI is ready and then downloads other firmware files.

Quoted 5 lines

What happens if userspace applications or network stack immediately initiate some WLAN request after resume back? Can ath11k handle such request? The answer is, most likely, no. Because there is no guarantee that QMI finishes downloading before those request.

What will happen to userspace if ath11k returns an error like -EBUSY or something? Will the netdev completely go away?

- Mani

Quoted 3 lines

Quoted 3 lines

- Mani

Quoted 63 lines · Manivannan Sadhasivam

Quoted 36 lines · Baochen Qiang

Quoted 28 lines · Manivannan Sadhasivam

On Thu, Jan 04, 2024 at 11:39:12AM +0530, Manivannan Sadhasivam wrote:

+ Can, Qiang

[...]

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

To me it all sounds like the probe deferral is not handled properly in mac80211 stack. As you mentioned in the commit message that the dpm_prepare() blocks probing of devices. It gets unblocked and trigerred in dpm_complete():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/main.c#n1131

So if mac80211/ath11k cannot probe the devices at the dpm_complete() stage, then it is definitely an issue that needs to be fixed properly.

Quoted 9 lines

Quoted 5 lines

To clarify, ath11k CAN probe the devices at dpm_complete() stage. The problem is kernel does not wait for all probes to finish, and in that way we will face the issue that user space applications are likely to fail because they get thawed BEFORE WLAN is ready.

Hmm. Please give me some time to reproduce this issue locally. I will get back to this thread with my analysis.

We reproduced the issue with the help of PCIe team (thanks Can). What we found out was, during the resume from hibernation the faliure happens in ath11k_core_resume(). Precisely here:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/tree/drivers/net/wireless/ath/ath11k/core.c?h=ath11k-hibernation-support#n850

Quoted 5 lines

This code waits for the QMI messages to arrive and eventually timesout. But the impression I got from the start was that the mhi_power_up() always fails during resume. In our investigation, we confirmed that the failure is not happening at the MHI level.No, mhi_power_up() never fails as it only downloads PBL, SBL and waits

Quoted 9 lines

for mission mode, no MHI device created hence not affected by the deferred probe. However in addition to PBL/SBL, ath11k also needs to download m3.bin, borad.bin and regdb.bin. Those files are part of WLAN firmware and are downloaded via QMI messages. After mhi_power_up() succeeds ath11k_core_resume() waits for QMI downloading those files. As you know QMI relies on MHI channels, these channels are managed by qcom_mhi_qrtr_driver. Since device probing is deferred, qcom_mhi_qrtr_driver has no chance to run at this stage. As a result ath11k_core_resume() times out.

Thanks for the info, this clarifies the issue in detail.

Quoted 14 lines

Quoted 4 lines

I'm not pointing fingers here, but trying to understand why can't you fix ath11k_core_resume() to not timeout? IMO this timeout should be handled as a deferral case.

Let's see what happens if we do it in a deferral way:
1. In ath11k_core_resume() we returns success directly without waiting for QMI downloading other firmware files.
2. Kernel unblocks device probe and schedules a work item to trigger all deferred probing. As a result MHI devices are probed by qcom_mhi_qrtr_driver and finally QMI is online.
3. kernel continues to resume and wake up userspace applications.
4. ath11k gets the message, either by kernel PM notification or something else, that QMI is ready and then downloads other firmware files.

Quoted 8 lines

Quoted 5 lines

What happens if userspace applications or network stack immediately initiate some WLAN request after resume back? Can ath11k handle such request? The answer is, most likely, no. Because there is no guarantee that QMI finishes downloading before those request.

What will happen to userspace if ath11k returns an error like -EBUSY or something? Will the netdev completely go away?

It depends, and varies from application to application, we can't make the assumption.

Besides, it doesn't make sense to return -EBUSY or something like that, if ath11k returns success during resume. A WLAN driver is supposed to finish everything, at least get back to the state before suspend, in the resume callback. If it couldn't, report the error.

Ok. So I am getting the feeling that we need to talk to the PM people to get a proper solution. Clearly fixing the MHI code is not the right thing to do. We might need a separate callback that gets registered by the drivers like ath11k to wait for the dependency drivers to get probed.

Can you initiate such a discussion? You can write to [email protected], "Rafael J. Wysocki" <[email protected]> and Pavel Machek <[email protected]>.

From: Baochen Qiang <[email protected]>

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 <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

---
 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(-)

drivers/bus/mhi/host/init.c +1 -0

index 65ceac1837f9..e626b03ffafa 100644
@@ -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] = {

drivers/bus/mhi/host/internal.h +1 -0

index 30ac415a3000..3f45c9c447bd 100644
@@ -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,
 };
 

drivers/bus/mhi/host/pm.c +16 -6

index a2f2feef1476..8833b0248393 100644
@@ -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

Quoted 4 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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

Quoted 131 lines

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 <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

---
  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(-)

drivers/bus/mhi/host/init.c +1 -0

index 65ceac1837f9..e626b03ffafa 100644
@@ -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] = {

drivers/bus/mhi/host/internal.h +1 -0

index 30ac415a3000..3f45c9c447bd 100644
@@ -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,
  };
 

drivers/bus/mhi/host/pm.c +16 -6

index a2f2feef1476..8833b0248393 100644
@@ -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

drivers/bus/mhi/host/pm.c void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful) · line 1218

Quoted 10 lines

drivers/bus/mhi/host/pm.c +3 -1

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

include/linux/mhi.h int mhi_async_power_up(struct mhi_controller *mhi_cntrl); · line 652

Quoted 13 lines

drivers/bus/mhi/host/pm.c +0 -0

  int mhi_sync_power_up(struct mhi_controller *mhi_cntrl)
  {

include/linux/mhi.h +2 -0

index d0f9b522f328..ae092bc8b97e 100644
@@ -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.

include/linux/mhi.h int mhi_async_power_up(struct mhi_controller *mhi_cntrl); · line 656

Quoted 5 lines

include/linux/mhi.h +3 -1

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

include/linux/mhi.h int mhi_async_power_up(struct mhi_controller *mhi_cntrl); · line 661

Quoted 6 lines

include/linux/mhi.h +2 -1

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

include/linux/mhi.h int mhi_async_power_up(struct mhi_controller *mhi_cntrl); · line 668

Quoted 7 lines

include/linux/mhi.h +7 -0

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

include/linux/mhi.h int mhi_async_power_up(struct mhi_controller *mhi_cntrl); · line 669

Quoted 1 line

include/linux/mhi.h +1 -0

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

include/linux/mhi.h int mhi_async_power_up(struct mhi_controller *mhi_cntrl); · line 671

Quoted 2 lines

include/linux/mhi.h +2 -0

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

mhi_prepare_all_for_transfer*()

include/linux/mhi.h int mhi_async_power_up(struct mhi_controller *mhi_cntrl); · line 678

Quoted 7 lines

include/linux/mhi.h +7 -0

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

From: Baochen Qiang <[email protected]>

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 <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

---
 drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
 include/linux/mhi.h         |  20 ++++++-
 2 files changed, 126 insertions(+), 1 deletion(-)

drivers/bus/mhi/host/main.c +0 -0

index d80975f4bba8..3f677fc628ad 100644

drivers/bus/mhi/host/main.c +8 -0

+	if (!mhi_dev->id)
+		return 0;
+
+	ul_chan = mhi_dev->ul_chan;
+	dl_chan = mhi_dev->dl_chan;
+
+	/*
+	 * If execution environment is specified, remove only those devices that

drivers/bus/mhi/host/main.c +16 -0

+	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
+		return 0;
+
+	if (dl_chan->pre_alloc)
+		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
+	else
+		return mhi_prepare_for_transfer(mhi_dev);
+}
+
+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);
+}
+EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
+
 void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
 {
 	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;

drivers/bus/mhi/host/main.c +53 -0

+{
+	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 that are attached to bus */
+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	/* There are cases where there is no MHI client driver matches
+	 * this device, so it is not probed or prepared, no need to
+	 * do unprepare for it.
+	 */
+	if (!mhi_dev->id)
+		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);

include/linux/mhi.h +0 -0

index ae092bc8b97e..dcf62a57056a 100644

include/linux/mhi.h +5 -0

+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
+
 #endif /* _MHI_H_ */

Signature · 1 line

2.39.2

Quoted 24 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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 <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

---
  drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
  include/linux/mhi.h         |  20 ++++++-
  2 files changed, 126 insertions(+), 1 deletion(-)

drivers/bus/mhi/host/main.c +0 -0

index d80975f4bba8..3f677fc628ad 100644
@@ -1669,6 +1669,58 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
  }
  EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
 

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1672

Quoted 1 line

drivers/bus/mhi/host/main.c +1 -0

+static int ____mhi_prepare_for_transfer(struct device *dev, void *data)

"__mhi_prepare_all_for_transfer"

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1676

Quoted 5 lines

drivers/bus/mhi/host/main.c +4 -0

+{
+	struct mhi_device *mhi_dev;
+	struct mhi_chan *ul_chan, *dl_chan;
+	enum mhi_ee_type ee = MHI_EE_MAX;

Reverse Xmas order, please.

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1683

Quoted 7 lines

drivers/bus/mhi/host/main.c +7 -0

+
+	if (dev->bus != &mhi_bus_type)
+		return 0;
+
+	mhi_dev = to_mhi_device(dev);
+
+	/* Only prepare virtual devices that are attached to bus */

"Only prepare virtual devices for the channels". Here and below.

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1689

Quoted 6 lines

drivers/bus/mhi/host/main.c +6 -0

+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	/* There are cases where there is no MHI client driver matches
+	 * this device, we are not allowed to do prepare for it.
+	 */

Use the preferred style for comment:

	/*
	 * ...
	 */

Quoted 8 lines

drivers/bus/mhi/host/main.c +8 -0

+	if (!mhi_dev->id)
+		return 0;
+
+	ul_chan = mhi_dev->ul_chan;
+	dl_chan = mhi_dev->dl_chan;
+
+	/*
+	 * If execution environment is specified, remove only those devices that

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1707

Quoted 10 lines

drivers/bus/mhi/host/main.c +10 -0

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

Remove extra newline.

Quoted 19 lines

drivers/bus/mhi/host/main.c +16 -0

+	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
+		return 0;
+
+	if (dl_chan->pre_alloc)
+		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
+	else
+		return mhi_prepare_for_transfer(mhi_dev);
+}
+
+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);
+}
+EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
+
  void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
  {
  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;

drivers/bus/mhi/host/main.c void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev) · line 1740

Quoted 6 lines

drivers/bus/mhi/host/main.c +2 -0

@@ -1684,3 +1736,58 @@ 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)

__mhi_unprepare_all_from_transfer

Quoted 59 lines

drivers/bus/mhi/host/main.c +53 -0

+{
+	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 that are attached to bus */
+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	/* There are cases where there is no MHI client driver matches
+	 * this device, so it is not probed or prepared, no need to
+	 * do unprepare for it.
+	 */
+	if (!mhi_dev->id)
+		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);

include/linux/mhi.h +0 -0

index ae092bc8b97e..dcf62a57056a 100644

include/linux/mhi.h static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu · line 671

Quoted 5 lines

include/linux/mhi.h +1 -1

@@ -668,7 +668,7 @@ static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu
   * 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_prepare_all_for_transfer() and

This change belongs to previous patch.

include/linux/mhi.h int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir, · line 848

Quoted 11 lines

include/linux/mhi.h +4 -0

   * mhi_unprepare_all_from_transfer().
   *
   * @mhi_cntrl: MHI controller

@@ -842,4 +842,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 - if you are using
+ * mhi_power_down_no_destroy() variant this needs to be called after
+ * calling mhi_power_up().

Add info about what this API does also.

include/linux/mhi.h int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir, · line 857

Quoted 9 lines

include/linux/mhi.h +9 -0

+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_prepare_all_for_transfer(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().

Same as above.

- Mani

Quoted 10 lines

include/linux/mhi.h +5 -0

+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
+
  #endif /* _MHI_H_ */

Signature · 1 line

2.39.2

Quoted 17 lines · Manivannan Sadhasivam

Quoted 16 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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 <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

---
  drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
  include/linux/mhi.h         |  20 ++++++-
  2 files changed, 126 insertions(+), 1 deletion(-)

Quoted 10 lines

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1671

Quoted 8 lines

drivers/bus/mhi/host/main.c +1 -0

index d80975f4bba8..3f677fc628ad 100644
@@ -1669,6 +1669,58 @@ 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(struct device *dev, void *data)

"__mhi_prepare_all_for_transfer"

This is to prepare one single child device, I don't think a name like __mhi_prepare_all_for_transfer (with 'all' inside) make sense, right? How about changing to "mhi_prepare_dev_for_transfer" or "mhi_prepare_single_for_transfer"?

Quoted 71 lines

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1676

Quoted 5 lines

drivers/bus/mhi/host/main.c +4 -0

+{
+	struct mhi_device *mhi_dev;
+	struct mhi_chan *ul_chan, *dl_chan;
+	enum mhi_ee_type ee = MHI_EE_MAX;

Reverse Xmas order, please.

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1683

Quoted 7 lines

drivers/bus/mhi/host/main.c +7 -0

+
+	if (dev->bus != &mhi_bus_type)
+		return 0;
+
+	mhi_dev = to_mhi_device(dev);
+
+	/* Only prepare virtual devices that are attached to bus */

"Only prepare virtual devices for the channels". Here and below.

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1689

Quoted 6 lines

drivers/bus/mhi/host/main.c +6 -0

+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	/* There are cases where there is no MHI client driver matches
+	 * this device, we are not allowed to do prepare for it.
+	 */

Use the preferred style for comment:

	/*
	 * ...
	 */

Quoted 8 lines

drivers/bus/mhi/host/main.c +8 -0

+	if (!mhi_dev->id)
+		return 0;
+
+	ul_chan = mhi_dev->ul_chan;
+	dl_chan = mhi_dev->dl_chan;
+
+	/*
+	 * If execution environment is specified, remove only those devices that

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1707

Quoted 10 lines

drivers/bus/mhi/host/main.c +10 -0

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

Remove extra newline.

Quoted 19 lines

drivers/bus/mhi/host/main.c +16 -0

+	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
+		return 0;
+
+	if (dl_chan->pre_alloc)
+		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
+	else
+		return mhi_prepare_for_transfer(mhi_dev);
+}
+
+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);
+}
+EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
+
  void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
  {
  	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;

Quoted 8 lines

drivers/bus/mhi/host/main.c void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev) · line 1740

Quoted 6 lines

drivers/bus/mhi/host/main.c +2 -0

@@ -1684,3 +1736,58 @@ 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)

__mhi_unprepare_all_from_transfer

same as above.

Quoted 105 lines

Quoted 59 lines

drivers/bus/mhi/host/main.c +53 -0

+{
+	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 that are attached to bus */
+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	/* There are cases where there is no MHI client driver matches
+	 * this device, so it is not probed or prepared, no need to
+	 * do unprepare for it.
+	 */
+	if (!mhi_dev->id)
+		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);

include/linux/mhi.h +0 -0

index ae092bc8b97e..dcf62a57056a 100644

include/linux/mhi.h static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu · line 671

Quoted 5 lines

include/linux/mhi.h +1 -1

@@ -668,7 +668,7 @@ static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu
   * 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_prepare_all_for_transfer() and

This change belongs to previous patch.

include/linux/mhi.h int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir, · line 847

Quoted 10 lines

include/linux/mhi.h +4 -0

   * mhi_unprepare_all_from_transfer().
   *
   * @mhi_cntrl: MHI controller

@@ -842,4 +842,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 - if you are using
+ * mhi_power_down_no_destroy() variant this needs to be called after
+ * calling mhi_power_up().

Add info about what this API does also.

include/linux/mhi.h int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir, · line 857

Quoted 9 lines

include/linux/mhi.h +9 -0

+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_prepare_all_for_transfer(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().

Same as above.

- Mani

Quoted 10 lines

include/linux/mhi.h +5 -0

+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
+
  #endif /* _MHI_H_ */

Signature · 1 line

2.39.2

Quoted 31 lines · Baochen Qiang

Quoted 17 lines · Manivannan Sadhasivam

Quoted 16 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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 <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

---
   drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
   include/linux/mhi.h         |  20 ++++++-
   2 files changed, 126 insertions(+), 1 deletion(-)

Quoted 10 lines

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1671

Quoted 8 lines

drivers/bus/mhi/host/main.c +1 -0

index d80975f4bba8..3f677fc628ad 100644
@@ -1669,6 +1669,58 @@ 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(struct device *dev, void *data)

"__mhi_prepare_all_for_transfer"

Quoted 5 lines

This is to prepare one single child device, I don't think a name like __mhi_prepare_all_for_transfer (with 'all' inside) make sense, right? How about changing to "mhi_prepare_dev_for_transfer" or "mhi_prepare_single_for_transfer"?

I think most of the checks in this function can be moved inside mhi_prepare_for_transfer() API. With that you can just reuse the API without adding a new helper.

For autoqueue channels, you can add another API mhi_prepare_all_for_transfer_autoqueue() just like mhi_prepare_for_transfer_autoqueue() to maintain uniformity.

- Mani

Quoted 188 lines

Quoted 72 lines

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1676

Quoted 6 lines

drivers/bus/mhi/host/main.c +4 -0

+{
+	struct mhi_device *mhi_dev;
+	struct mhi_chan *ul_chan, *dl_chan;
+	enum mhi_ee_type ee = MHI_EE_MAX;

Reverse Xmas order, please.

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1683

Quoted 7 lines

drivers/bus/mhi/host/main.c +7 -0

+
+	if (dev->bus != &mhi_bus_type)
+		return 0;
+
+	mhi_dev = to_mhi_device(dev);
+
+	/* Only prepare virtual devices that are attached to bus */

"Only prepare virtual devices for the channels". Here and below.

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1689

Quoted 6 lines

drivers/bus/mhi/host/main.c +6 -0

+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	/* There are cases where there is no MHI client driver matches
+	 * this device, we are not allowed to do prepare for it.
+	 */

Use the preferred style for comment:

	/*
	 * ...
	 */

Quoted 8 lines

drivers/bus/mhi/host/main.c +8 -0

+	if (!mhi_dev->id)
+		return 0;
+
+	ul_chan = mhi_dev->ul_chan;
+	dl_chan = mhi_dev->dl_chan;
+
+	/*
+	 * If execution environment is specified, remove only those devices that

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1707

Quoted 10 lines

drivers/bus/mhi/host/main.c +10 -0

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

Remove extra newline.

Quoted 19 lines

drivers/bus/mhi/host/main.c +16 -0

+	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
+		return 0;
+
+	if (dl_chan->pre_alloc)
+		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
+	else
+		return mhi_prepare_for_transfer(mhi_dev);
+}
+
+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);
+}
+EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
+
   void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
   {
   	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;

Quoted 8 lines

drivers/bus/mhi/host/main.c void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev) · line 1740

Quoted 6 lines

drivers/bus/mhi/host/main.c +2 -0

@@ -1684,3 +1736,58 @@ 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)

__mhi_unprepare_all_from_transfer

same as above.

Quoted 105 lines

Quoted 59 lines

drivers/bus/mhi/host/main.c +53 -0

+{
+	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 that are attached to bus */
+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	/* There are cases where there is no MHI client driver matches
+	 * this device, so it is not probed or prepared, no need to
+	 * do unprepare for it.
+	 */
+	if (!mhi_dev->id)
+		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);

include/linux/mhi.h +0 -0

index ae092bc8b97e..dcf62a57056a 100644

include/linux/mhi.h static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu · line 671

Quoted 5 lines

include/linux/mhi.h +1 -1

@@ -668,7 +668,7 @@ static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu
    * 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_prepare_all_for_transfer() and

This change belongs to previous patch.

include/linux/mhi.h int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir, · line 847

Quoted 10 lines

include/linux/mhi.h +4 -0

    * mhi_unprepare_all_from_transfer().
    *
    * @mhi_cntrl: MHI controller

@@ -842,4 +842,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 - if you are using
+ * mhi_power_down_no_destroy() variant this needs to be called after
+ * calling mhi_power_up().

Add info about what this API does also.

include/linux/mhi.h int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir, · line 857

Quoted 9 lines

include/linux/mhi.h +9 -0

+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_prepare_all_for_transfer(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().

Same as above.

- Mani

Quoted 10 lines

include/linux/mhi.h +5 -0

+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
+
   #endif /* _MHI_H_ */

Signature · 1 line

2.39.2

Quoted 46 lines · Manivannan Sadhasivam

Quoted 31 lines · Baochen Qiang

Quoted 17 lines · Manivannan Sadhasivam

Quoted 16 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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 <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

---
   drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
   include/linux/mhi.h         |  20 ++++++-
   2 files changed, 126 insertions(+), 1 deletion(-)

Quoted 10 lines

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1671

Quoted 8 lines

drivers/bus/mhi/host/main.c +1 -0

index d80975f4bba8..3f677fc628ad 100644
@@ -1669,6 +1669,58 @@ 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(struct device *dev, void *data)

"__mhi_prepare_all_for_transfer"

Quoted 5 lines

This is to prepare one single child device, I don't think a name like __mhi_prepare_all_for_transfer (with 'all' inside) make sense, right? How about changing to "mhi_prepare_dev_for_transfer" or "mhi_prepare_single_for_transfer"?

I think most of the checks in this function can be moved inside mhi_prepare_for_transfer() API. With that you can just reuse the API without adding a new helper.

For autoqueue channels, you can add another API mhi_prepare_all_for_transfer_autoqueue() just like mhi_prepare_for_transfer_autoqueue() to maintain uniformity.

Quoted 1 line

- Mani

If we do that, we need to call two APIs together, does it make sense? From the view of an MHI user, what we want is an API to prepare all channels, no matter whether a channel is configured as autoqueue or non-autoqueue, we don't care about it.

And besides, I don't think there is a scenario where we need to use them separately. So if we always need to use them together, why not merge them in a single API?

Quoted 189 lines

Quoted 188 lines

Quoted 72 lines

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1676

Quoted 6 lines

drivers/bus/mhi/host/main.c +4 -0

+{
+	struct mhi_device *mhi_dev;
+	struct mhi_chan *ul_chan, *dl_chan;
+	enum mhi_ee_type ee = MHI_EE_MAX;

Reverse Xmas order, please.

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1683

Quoted 7 lines

drivers/bus/mhi/host/main.c +7 -0

+
+	if (dev->bus != &mhi_bus_type)
+		return 0;
+
+	mhi_dev = to_mhi_device(dev);
+
+	/* Only prepare virtual devices that are attached to bus */

"Only prepare virtual devices for the channels". Here and below.

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1689

Quoted 6 lines

drivers/bus/mhi/host/main.c +6 -0

+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	/* There are cases where there is no MHI client driver matches
+	 * this device, we are not allowed to do prepare for it.
+	 */

Use the preferred style for comment:

	/*
	 * ...
	 */

Quoted 8 lines

drivers/bus/mhi/host/main.c +8 -0

+	if (!mhi_dev->id)
+		return 0;
+
+	ul_chan = mhi_dev->ul_chan;
+	dl_chan = mhi_dev->dl_chan;
+
+	/*
+	 * If execution environment is specified, remove only those devices that

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1707

Quoted 10 lines

drivers/bus/mhi/host/main.c +10 -0

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

Remove extra newline.

Quoted 19 lines

drivers/bus/mhi/host/main.c +16 -0

+	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
+		return 0;
+
+	if (dl_chan->pre_alloc)
+		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
+	else
+		return mhi_prepare_for_transfer(mhi_dev);
+}
+
+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);
+}
+EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
+
   void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
   {
   	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;

Quoted 8 lines

drivers/bus/mhi/host/main.c void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev) · line 1740

Quoted 6 lines

drivers/bus/mhi/host/main.c +2 -0

@@ -1684,3 +1736,58 @@ 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)

__mhi_unprepare_all_from_transfer

same as above.

Quoted 105 lines

Quoted 59 lines

drivers/bus/mhi/host/main.c +53 -0

+{
+	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 that are attached to bus */
+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	/* There are cases where there is no MHI client driver matches
+	 * this device, so it is not probed or prepared, no need to
+	 * do unprepare for it.
+	 */
+	if (!mhi_dev->id)
+		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);

include/linux/mhi.h +0 -0

index ae092bc8b97e..dcf62a57056a 100644

include/linux/mhi.h static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu · line 671

Quoted 5 lines

include/linux/mhi.h +1 -1

@@ -668,7 +668,7 @@ static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu
    * 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_prepare_all_for_transfer() and

This change belongs to previous patch.

include/linux/mhi.h int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir, · line 847

Quoted 10 lines

include/linux/mhi.h +4 -0

    * mhi_unprepare_all_from_transfer().
    *
    * @mhi_cntrl: MHI controller

@@ -842,4 +842,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 - if you are using
+ * mhi_power_down_no_destroy() variant this needs to be called after
+ * calling mhi_power_up().

Add info about what this API does also.

include/linux/mhi.h int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir, · line 857

Quoted 9 lines

include/linux/mhi.h +9 -0

+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_prepare_all_for_transfer(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().

Same as above.

- Mani

Quoted 10 lines

include/linux/mhi.h +5 -0

+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
+
   #endif /* _MHI_H_ */

Signature · 1 line

2.39.2

Quoted 49 lines · Baochen Qiang

Quoted 46 lines · Manivannan Sadhasivam

Quoted 31 lines · Baochen Qiang

Quoted 17 lines · Manivannan Sadhasivam

Quoted 16 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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 <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

---
    drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
    include/linux/mhi.h         |  20 ++++++-
    2 files changed, 126 insertions(+), 1 deletion(-)

Quoted 10 lines

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1671

Quoted 8 lines

drivers/bus/mhi/host/main.c +1 -0

index d80975f4bba8..3f677fc628ad 100644
@@ -1669,6 +1669,58 @@ 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(struct device *dev, void *data)

"__mhi_prepare_all_for_transfer"

Quoted 5 lines

This is to prepare one single child device, I don't think a name like __mhi_prepare_all_for_transfer (with 'all' inside) make sense, right? How about changing to "mhi_prepare_dev_for_transfer" or "mhi_prepare_single_for_transfer"?

I think most of the checks in this function can be moved inside mhi_prepare_for_transfer() API. With that you can just reuse the API without adding a new helper.

For autoqueue channels, you can add another API mhi_prepare_all_for_transfer_autoqueue() just like mhi_prepare_for_transfer_autoqueue() to maintain uniformity.

Quoted 6 lines

Quoted 1 line

- Mani

If we do that, we need to call two APIs together, does it make sense? From the view of an MHI user, what we want is an API to prepare all channels, no matter whether a channel is configured as autoqueue or non-autoqueue, we don't care about it.

You are calling this API from a wrong place first up. mhi_{prepare/unprepare}_transfer* APIs are meant to be used by the client drivers like QRTR. Controller drivers should not call them.

What you need here is the hibernation support for QRTR itself and call these APIs from there.

Quoted 5 lines

And besides, I don't think there is a scenario where we need to use them separately. So if we always need to use them together, why not merge them in a single API?

A single controller driver may expose multiple channels and those will bind to multiple client drivers. So only the client drivers should manage the channels, not the controller drivers themselves.

- Mani

Quoted 189 lines

Quoted 189 lines

Quoted 188 lines

Quoted 72 lines

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1676

Quoted 6 lines

drivers/bus/mhi/host/main.c +4 -0

+{
+	struct mhi_device *mhi_dev;
+	struct mhi_chan *ul_chan, *dl_chan;
+	enum mhi_ee_type ee = MHI_EE_MAX;

Reverse Xmas order, please.

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1683

Quoted 7 lines

drivers/bus/mhi/host/main.c +7 -0

+
+	if (dev->bus != &mhi_bus_type)
+		return 0;
+
+	mhi_dev = to_mhi_device(dev);
+
+	/* Only prepare virtual devices that are attached to bus */

"Only prepare virtual devices for the channels". Here and below.

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1689

Quoted 6 lines

drivers/bus/mhi/host/main.c +6 -0

+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	/* There are cases where there is no MHI client driver matches
+	 * this device, we are not allowed to do prepare for it.
+	 */

Use the preferred style for comment:

	/*
	 * ...
	 */

Quoted 8 lines

drivers/bus/mhi/host/main.c +8 -0

+	if (!mhi_dev->id)
+		return 0;
+
+	ul_chan = mhi_dev->ul_chan;
+	dl_chan = mhi_dev->dl_chan;
+
+	/*
+	 * If execution environment is specified, remove only those devices that

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1707

Quoted 10 lines

drivers/bus/mhi/host/main.c +10 -0

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

Remove extra newline.

Quoted 19 lines

drivers/bus/mhi/host/main.c +16 -0

+	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
+		return 0;
+
+	if (dl_chan->pre_alloc)
+		return mhi_prepare_for_transfer_autoqueue(mhi_dev);
+	else
+		return mhi_prepare_for_transfer(mhi_dev);
+}
+
+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);
+}
+EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer);
+
    void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
    {
    	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;

Quoted 8 lines

drivers/bus/mhi/host/main.c void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev) · line 1740

Quoted 6 lines

drivers/bus/mhi/host/main.c +2 -0

@@ -1684,3 +1736,58 @@ 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)

__mhi_unprepare_all_from_transfer

same as above.

Quoted 105 lines

Quoted 59 lines

drivers/bus/mhi/host/main.c +53 -0

+{
+	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 that are attached to bus */
+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	/* There are cases where there is no MHI client driver matches
+	 * this device, so it is not probed or prepared, no need to
+	 * do unprepare for it.
+	 */
+	if (!mhi_dev->id)
+		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);

include/linux/mhi.h +0 -0

index ae092bc8b97e..dcf62a57056a 100644

include/linux/mhi.h static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu · line 671

Quoted 5 lines

include/linux/mhi.h +1 -1

@@ -668,7 +668,7 @@ static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool gracefu
     * 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_prepare_all_for_transfer() and

This change belongs to previous patch.

include/linux/mhi.h int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir, · line 847

Quoted 10 lines

include/linux/mhi.h +4 -0

     * mhi_unprepare_all_from_transfer().
     *
     * @mhi_cntrl: MHI controller

@@ -842,4 +842,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 - if you are using
+ * mhi_power_down_no_destroy() variant this needs to be called after
+ * calling mhi_power_up().

Add info about what this API does also.

include/linux/mhi.h int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir, · line 857

Quoted 9 lines

include/linux/mhi.h +9 -0

+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_prepare_all_for_transfer(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().

Same as above.

- Mani

Quoted 10 lines

include/linux/mhi.h +5 -0

+ *
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_unprepare_all_from_transfer(struct mhi_controller *mhi_cntrl);
+
    #endif /* _MHI_H_ */

Signature · 1 line

2.39.2

Quoted 57 lines · Manivannan Sadhasivam

Quoted 49 lines · Baochen Qiang

Quoted 46 lines · Manivannan Sadhasivam

Quoted 31 lines · Baochen Qiang

Quoted 17 lines · Manivannan Sadhasivam

Quoted 16 lines · Kalle Valo

From: Baochen Qiang <[email protected]>

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 <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

---
    drivers/bus/mhi/host/main.c | 107 ++++++++++++++++++++++++++++++++++++
    include/linux/mhi.h         |  20 ++++++-
    2 files changed, 126 insertions(+), 1 deletion(-)

Quoted 10 lines

drivers/bus/mhi/host/main.c int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev) · line 1671

Quoted 8 lines

drivers/bus/mhi/host/main.c +1 -0

index d80975f4bba8..3f677fc628ad 100644
@@ -1669,6 +1669,58 @@ 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(struct device *dev, void *data)

"__mhi_prepare_all_for_transfer"

Quoted 5 lines

This is to prepare one single child device, I don't think a name like __mhi_prepare_all_for_transfer (with 'all' inside) make sense, right? How about changing to "mhi_prepare_dev_for_transfer" or "mhi_prepare_single_for_transfer"?

I think most of the checks in this function can be moved inside mhi_prepare_for_transfer() API. With that you can just reuse the API without adding a new helper.

For autoqueue channels, you can add another API mhi_prepare_all_for_transfer_autoqueue() just like mhi_prepare_for_transfer_autoqueue() to maintain uniformity.

Quoted 6 lines

Quoted 1 line

- Mani

If we do that, we need to call two APIs together, does it make sense? From the view of an MHI user, what we want is an API to prepare all channels, no matter whether a channel is configured as autoqueue or non-autoqueue, we don't care about it.

Quoted 6 lines

You are calling this API from a wrong place first up. mhi_{prepare/unprepare}_transfer* APIs are meant to be used by the client drivers like QRTR. Controller drivers should not call them.

What you need here is the hibernation support for QRTR itself and call these APIs from there.

OK, I got your point. QRTR is the right place to manage MHI channels, not ath11k it self.
And we even don't need these two APIs if change to do it in QRTR.

Quoted 6 lines

Quoted 5 lines

And besides, I don't think there is a scenario where we need to use them separately. So if we always need to use them together, why not merge them in a single API?

Quoted 3 lines

A single controller driver may expose multiple channels and those will bind to multiple client drivers. So only the client drivers should manage the channels, not the controller drivers themselves.

Exactly.

Great thanks for the proposal, Mani. Will change accordingly in next version.

From: Baochen Qiang <[email protected]>

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 <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

---
  drivers/net/wireless/ath/ath11k/mhi.c | 56 ---------------------------
  1 file changed, 56 deletions(-)

drivers/net/wireless/ath/ath11k/mhi.c +0 -56

index afeabd6ecc67..579af57f7377 100644
@@ -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",

@@ -114,34 +86,6 @@ static struct mhi_controller_config ath11k_mhi_config_qca6390 = {
  };
 
  static struct mhi_channel_config ath11k_mhi_channels_qcn9074[] = {
-	{
-		.num = 0,
-		.name = "LOOPBACK",
-		.num_elements = 32,
-		.event_ring = 1,
-		.dir = DMA_TO_DEVICE,
-		.ee_mask = 0x14,
-		.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 = 1,
-		.dir = DMA_FROM_DEVICE,
-		.ee_mask = 0x14,
-		.pollcfg = 0,
-		.doorbell = MHI_DB_BRST_DISABLE,
-		.lpm_notify = false,
-		.offload_channel = false,
-		.doorbell_mode_switch = false,
-		.auto_queue = false,
-	},
  	{
  		.num = 20,
  		.name = "IPCR",