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/internal.h |  1 +
  drivers/bus/mhi/host/pm.c       | 26 +++++++++++++++++++-------
  include/linux/mhi.h             | 29 +++++++++++++++++++++++++++--
  3 files changed, 47 insertions(+), 9 deletions(-)

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

index 2e139e76de4c..d1033af285e2 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 8a4362d75fc4..e0e429adbda6 100644
@@ -453,7 +453,8 @@ static int mhi_pm_mission_mode_transition(struct mhi_controller *mhi_cntrl)
  }
 
  /* Handle shutdown transitions */
-static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl)
+static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
+				      bool destroy_device)
  {
  	enum mhi_pm_state cur_state;
  	struct mhi_event *mhi_event;

@@ -515,8 +516,10 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl)
  	dev_dbg(dev, "Waiting for all pending threads to complete\n");
  	wake_up_all(&mhi_cntrl->state_event);
 
-	dev_dbg(dev, "Reset all active channels and remove MHI devices\n");
-	device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL, mhi_destroy_device);
+	if (destroy_device) {
+		dev_dbg(dev, "Reset all active channels and remove MHI devices\n");
+		device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL, mhi_destroy_device);
+	}
 
  	mutex_lock(&mhi_cntrl->pm_mutex);
 

@@ -801,7 +804,10 @@ void mhi_pm_st_worker(struct work_struct *work)
  			mhi_pm_sys_error_transition(mhi_cntrl);
  			break;
  		case DEV_ST_TRANSITION_DISABLE:
-			mhi_pm_disable_transition(mhi_cntrl);
+			mhi_pm_disable_transition(mhi_cntrl, false);
+			break;
+		case DEV_ST_TRANSITION_DISABLE_DESTROY_DEVICE:
+			mhi_pm_disable_transition(mhi_cntrl, true);
  			break;
  		default:
  			break;

@@ -1154,7 +1160,8 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
  }
  EXPORT_SYMBOL_GPL(mhi_async_power_up);
 
-void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
+void __mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful,
+		      bool destroy_device)
  {
  	enum mhi_pm_state cur_state, transition_state;
  	struct device *dev = &mhi_cntrl->mhi_dev->dev;

@@ -1190,14 +1197,19 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
  	write_unlock_irq(&mhi_cntrl->pm_lock);
  	mutex_unlock(&mhi_cntrl->pm_mutex);
 
-	mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_DISABLE);
+	if (destroy_device)
+		mhi_queue_state_transition(mhi_cntrl,
+					   DEV_ST_TRANSITION_DISABLE_DESTROY_DEVICE);
+	else
+		mhi_queue_state_transition(mhi_cntrl,
+					   DEV_ST_TRANSITION_DISABLE);
 
  	/* Wait for shutdown to complete */
  	flush_work(&mhi_cntrl->st_worker);
 
  	disable_irq(mhi_cntrl->irq[0]);
  }
-EXPORT_SYMBOL_GPL(mhi_power_down);
+EXPORT_SYMBOL_GPL(__mhi_power_down);
 
  int mhi_sync_power_up(struct mhi_controller *mhi_cntrl)
  {

include/linux/mhi.h +27 -2

index 039943ec4d4e..85edc9c5df88 100644
@@ -644,12 +644,37 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl);
   */
  int mhi_sync_power_up(struct mhi_controller *mhi_cntrl);
 
+void __mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful,
+		    bool destroy_device);
+
  /**
- * mhi_power_down - Start MHI power down sequence
+ * mhi_power_down - Start MHI power down sequence. See also
+ * mhi_power_down_no_destroy() which is a variant of this for suspend.
+ *
   * @mhi_cntrl: MHI controller
   * @graceful: Link is still accessible, so do a graceful shutdown process
   */
-void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful);
+static inline void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
+{
+	__mhi_power_down(mhi_cntrl, graceful, true);
+}
+
+/**
+ * mhi_power_down_no_destroy - Start MHI power down sequence but don't
+ * destroy struct devices. This is a variant for mhi_power_down() and is a
+ * workaround to make it possible to use mhi_power_up() in a resume
+ * handler. When using this variant the caller must also call
+ * mhi_prepare_all_for_transfer_autoqueue() and
+ * mhi_unprepare_all_from_transfer().
+ *
+ * @mhi_cntrl: MHI controller
+ * @graceful: Link is still accessible, so do a graceful shutdown process
+ */
+static inline void mhi_power_down_no_destroy(struct mhi_controller *mhi_cntrl,
+					     bool graceful)
+{
+	__mhi_power_down(mhi_cntrl, graceful, false);
+}
 
  /**
   * mhi_unprepare_after_power_down - Free any allocated memory after power down

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 | 91 +++++++++++++++++++++++++++++++++++++
  include/linux/mhi.h         | 18 ++++++++
  2 files changed, 109 insertions(+)

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

index dcf627b36e82..9bcf8a49c000 100644
@@ -1667,6 +1667,49 @@ int mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
  }
  EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
 
+static int __mhi_prepare_for_transfer_autoqueue(struct device *dev, void *data)
+{

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

+	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
+		return 0;
+
+	ul_chan = mhi_dev->ul_chan;
+	dl_chan = mhi_dev->dl_chan;
+
+	/*
+	 * If execution environment is specified, remove only those devices that
+	 * started in them based on ee_mask for the channels as we move on to a
+	 * different execution environment
+	 */
+	if (data)
+		ee = *(enum mhi_ee_type *)data;
+
+	if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
+		return 0;
+
+
+	if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
+		return 0;

Quoted 30 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 | 91 +++++++++++++++++++++++++++++++++++++ include/linux/mhi.h         | 18 ++++++++

  2 files changed, 109 insertions(+)

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

index dcf627b36e82..9bcf8a49c000 100644
@@ -1667,6 +1667,49 @@ int mhi_prepare_for_transfer_autoqueue(struct 
 mhi_device *mhi_dev)
   }
   EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
   +static int __mhi_prepare_for_transfer_autoqueue(struct device *dev, void *data)
+{
+    struct mhi_device *mhi_dev;
+    struct mhi_chan *ul_chan, *dl_chan;
+    enum mhi_ee_type ee = MHI_EE_MAX;
+

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

Quoted 6 lines

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

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

"that are"?

Quoted 27 lines

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

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

devices that
+     * started in them based on ee_mask for the channels as we move on to a
+     * different execution environment
+     */
+    if (data)
+        ee = *(enum mhi_ee_type *)data;

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

+
+    if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
+        return 0;
+
+
+    if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
+        return 0;
+
+    return mhi_prepare_for_transfer_autoqueue(mhi_dev);
+}
+
+int mhi_prepare_all_for_transfer_autoqueue(struct mhi_controller 

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

Quoted 6 lines

*mhi_cntrl)

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

+{
+    return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
+                     __mhi_prepare_for_transfer_autoqueue);
+}
+EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer_autoqueue);

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

Quoted 31 lines · Jeffrey Hugo

Quoted 30 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 | 91 +++++++++++++++++++++++++++++++++++++ include/linux/mhi.h         | 18 ++++++++

  2 files changed, 109 insertions(+)

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

index dcf627b36e82..9bcf8a49c000 100644
@@ -1667,6 +1667,49 @@ int mhi_prepare_for_transfer_autoqueue(struct 
 mhi_device *mhi_dev)
   }
   EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
   +static int __mhi_prepare_for_transfer_autoqueue(struct device *dev, void *data)
+{
+    struct mhi_device *mhi_dev;
+    struct mhi_chan *ul_chan, *dl_chan;
+    enum mhi_ee_type ee = MHI_EE_MAX;
+

Quoted 9 lines

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

Quoted 6 lines

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

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

"that are"?

It means MHI devices with a type of MHI_DEVICE_XFER. See also mhi_destroy_device();

Quoted 29 lines

Quoted 29 lines

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

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

devices that
+     * started in them based on ee_mask for the channels as we move on to a
+     * different execution environment
+     */
+    if (data)
+        ee = *(enum mhi_ee_type *)data;

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

+
+    if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
+        return 0;
+
+
+    if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
+        return 0;
+
+    return mhi_prepare_for_transfer_autoqueue(mhi_dev);
+}
+
+int mhi_prepare_all_for_transfer_autoqueue(struct mhi_controller 

Quoted 8 lines

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

Quoted 6 lines

*mhi_cntrl)

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

+{
+    return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
+                     __mhi_prepare_for_transfer_autoqueue);
+}
+EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer_autoqueue);

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

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

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

mhi_prepare_for_transfer_dev(struct device *dev, ...)

{

...

dl_chan = mhi_dev->dl_chan;

...

if (dl_chan->pre_alloc)

       mhi_prepare_for_transfer_autoqueue(dev);

else

       mhi_prepare_for_transfer(dev);

}

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

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

Quoted 43 lines · Baochen Qiang

Quoted 41 lines · Jeffrey Hugo

Quoted 27 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 | 91
+++++++++++++++++++++++++++++++++++++
  include/linux/mhi.h         | 18 ++++++++

  2 files changed, 109 insertions(+)

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

index dcf627b36e82..9bcf8a49c000 100644
@@ -1667,6 +1667,49 @@ int 
 mhi_prepare_for_transfer_autoqueue(struct mhi_device *mhi_dev)
   }
   EXPORT_SYMBOL_GPL(mhi_prepare_for_transfer_autoqueue);
   +static int __mhi_prepare_for_transfer_autoqueue(struct device *dev, void *data)
+{

drivers/bus/mhi/host/main.c int · line 1681

Quoted 10 lines

drivers/bus/mhi/host/main.c +10 -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 prepare virtual devices thats attached to bus */

"that are"?

Quoted 1 line

It means MHI devices with a type of MHI_DEVICE_XFER. See also mhi_destroy_device();

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

Quoted 39 lines

Quoted 30 lines

Quoted 30 lines

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

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

devices that
+     * started in them based on ee_mask for the channels as we move on to a
+     * different execution environment
+     */
+    if (data)
+        ee = *(enum mhi_ee_type *)data;

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

+
+    if (ul_chan && ee != MHI_EE_MAX && !(ul_chan->ee_mask & BIT(ee)))
+        return 0;
+
+
+    if (dl_chan && ee != MHI_EE_MAX && !(dl_chan->ee_mask & BIT(ee)))
+        return 0;
+
+    return mhi_prepare_for_transfer_autoqueue(mhi_dev);
+}
+
+int mhi_prepare_all_for_transfer_autoqueue(struct mhi_controller 

Quoted 8 lines

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

Quoted 6 lines

*mhi_cntrl)

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

+{
+    return device_for_each_child(&mhi_cntrl->mhi_dev->dev, NULL,
+                     __mhi_prepare_for_transfer_autoqueue);
+}
+EXPORT_SYMBOL_GPL(mhi_prepare_all_for_transfer_autoqueue);

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

Quoted 1 line

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

Seems ok.

Quoted 32 lines

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

mhi_prepare_for_transfer_dev(struct device *dev, ...)

{

...

dl_chan = mhi_dev->dl_chan;

...

if (dl_chan->pre_alloc)

        mhi_prepare_for_transfer_autoqueue(dev);

else

        mhi_prepare_for_transfer(dev);

}

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

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

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

Quoted 5 lines · Kalle Valo

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:

Quoted 2 lines

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

This feels like just masking a real issue.

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

-Jeff

Quoted 9 lines · Jeffrey Hugo

Quoted 7 lines · Kalle Valo

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

Quoted 5 lines

This feels like just masking a real issue.

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

-Jeff

That is because, with patch 'bus: mhi: host: add new interfaces to handle MHI channels directly' in this patch set, ath11k is able to call mhi_unprepare_all_from_transfer(), which will reset all channels.

Quoted 13 lines · Baochen Qiang

Quoted 11 lines · Jeffrey Hugo

Quoted 7 lines · Kalle Valo

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

This feels like just masking a real issue.

Quoted 4 lines

Quoted 3 lines

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

-Jeff

That is because, with patch 'bus: mhi: host: add new interfaces to handle MHI channels directly' in this patch set, ath11k is able to call mhi_unprepare_all_from_transfer(), which will reset all channels.

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

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

Quoted 16 lines · Baochen Qiang

Quoted 15 lines · Jeffrey Hugo

Quoted 7 lines · Kalle Valo

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

This feels like just masking a real issue.

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

Quoted 5 lines

Quoted 1 line

-Jeff

That is because, with patch 'bus: mhi: host: add new interfaces to handle MHI channels directly' in this patch set, ath11k is able to call mhi_unprepare_all_from_transfer(), which will reset all channels.

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

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

Quoted 24 lines · Jeffrey Hugo

Quoted 21 lines · Baochen Qiang

Quoted 9 lines · Jeffrey Hugo

Quoted 7 lines · Kalle Valo

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

Quoted 7 lines

This feels like just masking a real issue.

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

-Jeff

That is because, with patch 'bus: mhi: host: add new interfaces to handle MHI channels directly' in this patch set, ath11k is able to call mhi_unprepare_all_from_transfer(), which will reset all channels.

Quoted 4 lines

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

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

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

Quoted 30 lines · Kalle Valo

Quoted 28 lines · Jeffrey Hugo

Quoted 16 lines · Baochen Qiang

Quoted 15 lines · Jeffrey Hugo

Quoted 7 lines · Kalle Valo

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

This feels like just masking a real issue.

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

Quoted 5 lines

Quoted 1 line

-Jeff

That is because, with patch 'bus: mhi: host: add new interfaces to handle MHI channels directly' in this patch set, ath11k is able to call mhi_unprepare_all_from_transfer(), which will reset all channels.

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

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

Quoted 4 lines

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

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