Re: [PATCH 4/4] bus: mhi: Fix broken runtime PM design

Mon, 01 Dec 2025 10:34:07 -0800 

Hi Krishna

On 12/1/2025 4:43 AM, Krishna Chaitanya Chundru wrote:

The current MHI runtime PM design is flawed, as the MHI core attempts to
manage power references internally via mhi_queue() and related paths.
This is problematic because the controller drivers do not have the
knowledge of the client PM status due to the broken PM topology. So when
they runtime suspend the controller, the client drivers could no longer
function.

To address this, in the new design, the client drivers reports their own
runtime PM status now and the PM framework makes sure that the parent
(controller driver) and other components up in the chain remain active.
This leverages the standard parent-child PM relationship.

Since MHI creates a mhi_dev device without an associated driver, we
explicitly enable runtime PM on it and mark it with
pm_runtime_no_callbacks() to indicate the PM core that no callbacks
exist for this device. This is only needed for MHI controller, since
the controller driver uses the bus device just like PCI device.

Also Update the MHI core to explicitly manage runtime PM references in
__mhi_device_get_sync() and mhi_device_put() to ensure the controller
does not enter suspend while a client device is active.

Why does this needed here ?
Isn't it MHI client driver take care of allowing suspend ?
Do you think we should remove mhi_device_get_sync() and mhi_device_put() API interfaces as well ? And let controller/client driver takes care of calling get/sync directly ? 

How are you handling cases for M0 and M3 suspend ?
Do we need to tie runtime usage with M0 (pm_runtime_get) and M3 (pm_runtime_put) ? 

Regards,
Mayank


Signed-off-by: Krishna Chaitanya Chundru <[email protected]>
---
  drivers/bus/mhi/host/internal.h |  6 +++---
  drivers/bus/mhi/host/main.c     | 28 ++++------------------------
  drivers/bus/mhi/host/pm.c       | 18 ++++++++----------
  3 files changed, 15 insertions(+), 37 deletions(-)

diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
index 
61e03298e898e6dd02d2a977cddc4c87b21e3a6c..d6a3168bb3ecc34eab1036c0e74f8d70cf422fed
 100644
--- a/drivers/bus/mhi/host/internal.h
+++ b/drivers/bus/mhi/host/internal.h
@@ -355,9 +355,9 @@ static inline bool mhi_is_active(struct mhi_controller 
*mhi_cntrl)
  static inline void mhi_trigger_resume(struct mhi_controller *mhi_cntrl)
  {
        pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
-       pm_runtime_get(mhi_cntrl->cntrl_dev);
-       pm_runtime_mark_last_busy(mhi_cntrl->cntrl_dev);
-       pm_runtime_put(mhi_cntrl->cntrl_dev);
+       pm_runtime_get(&mhi_cntrl->mhi_dev->dev);
+       pm_runtime_mark_last_busy(&mhi_cntrl->mhi_dev->dev);
+       pm_runtime_put(&mhi_cntrl->mhi_dev->dev);
  }
/* Register access methods */ 
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index 
7ac1162a0a81ae11245a2bbd9bf6fd6c0f86fbc1..85a9a5a62a6d3f92b0e9dc35b13fd867db89dd95
 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -427,6 +427,8 @@ void mhi_create_devices(struct mhi_controller *mhi_cntrl)
                if (ret)
                        put_device(&mhi_dev->dev);
        }
+       pm_runtime_no_callbacks(&mhi_cntrl->mhi_dev->dev);
+       devm_pm_runtime_set_active_enabled(&mhi_cntrl->mhi_dev->dev);
  }
irqreturn_t mhi_irq_handler(int irq_number, void *dev) 
@@ -658,12 +660,8 @@ static int parse_xfer_event(struct mhi_controller 
*mhi_cntrl,
                        /* notify client */
                        mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result);
- if (mhi_chan->dir == DMA_TO_DEVICE) { 
+                       if (mhi_chan->dir == DMA_TO_DEVICE)
                                atomic_dec(&mhi_cntrl->pending_pkts);
-                               /* Release the reference got from mhi_queue() */
-                               pm_runtime_mark_last_busy(mhi_cntrl->cntrl_dev);
-                               pm_runtime_put(mhi_cntrl->cntrl_dev);
-                       }
/* 
                         * Recycle the buffer if buffer is pre-allocated,
@@ -1152,12 +1150,6 @@ static int mhi_queue(struct mhi_device *mhi_dev, struct 
mhi_buf_info *buf_info,
read_lock_irqsave(&mhi_cntrl->pm_lock, flags); - /* Packet is queued, take a usage ref to exit M3 if necessary 
-        * for host->device buffer, balanced put is done on buffer completion
-        * for device->host buffer, balanced put is after ringing the DB
-        */
-       pm_runtime_get(mhi_cntrl->cntrl_dev);
-
        /* Assert dev_wake (to exit/prevent M1/M2)*/
        mhi_cntrl->wake_toggle(mhi_cntrl);
@@ -1167,11 +1159,6 @@ static int mhi_queue(struct mhi_device *mhi_dev, struct mhi_buf_info *buf_info, 
        if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)))
                mhi_ring_chan_db(mhi_cntrl, mhi_chan);
- if (dir == DMA_FROM_DEVICE) { 
-               pm_runtime_mark_last_busy(mhi_cntrl->cntrl_dev);
-               pm_runtime_put(mhi_cntrl->cntrl_dev);
-       }
-
        read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags);
return ret; 
@@ -1377,7 +1364,6 @@ static int mhi_update_channel_state(struct mhi_controller 
*mhi_cntrl,
        ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
        if (ret)
                return ret;
-       pm_runtime_get(mhi_cntrl->cntrl_dev);
reinit_completion(&mhi_chan->completion); 
        ret = mhi_send_cmd(mhi_cntrl, mhi_chan, cmd);
@@ -1408,8 +1394,6 @@ static int mhi_update_channel_state(struct mhi_controller 
*mhi_cntrl,
trace_mhi_channel_command_end(mhi_cntrl, mhi_chan, to_state, TPS("Updated")); 
  exit_channel_update:
-       pm_runtime_mark_last_busy(mhi_cntrl->cntrl_dev);
-       pm_runtime_put(mhi_cntrl->cntrl_dev);
        mhi_device_put(mhi_cntrl->mhi_dev);
return ret; 
@@ -1592,12 +1576,8 @@ static void mhi_reset_data_chan(struct mhi_controller 
*mhi_cntrl,
        while (tre_ring->rp != tre_ring->wp) {
                struct mhi_buf_info *buf_info = buf_ring->rp;
- if (mhi_chan->dir == DMA_TO_DEVICE) { 
+               if (mhi_chan->dir == DMA_TO_DEVICE)
                        atomic_dec(&mhi_cntrl->pending_pkts);
-                       /* Release the reference got from mhi_queue() */
-                       pm_runtime_mark_last_busy(mhi_cntrl->cntrl_dev);
-                       pm_runtime_put(mhi_cntrl->cntrl_dev);
-               }
if (!buf_info->pre_mapped) 
                        mhi_cntrl->unmap_single(mhi_cntrl, buf_info);
diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
index 
b4ef115189b505c3450ff0949ad2d09f3ed53386..fd690e8af693109ed8c55248db0ea153f9e69423
 100644
--- a/drivers/bus/mhi/host/pm.c
+++ b/drivers/bus/mhi/host/pm.c
@@ -429,6 +429,7 @@ static int mhi_pm_mission_mode_transition(struct 
mhi_controller *mhi_cntrl)
if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) { 
                ret = -EIO;
+               read_unlock_bh(&mhi_cntrl->pm_lock);
                goto error_mission_mode;
        }
@@ -459,11 +460,9 @@ static int mhi_pm_mission_mode_transition(struct mhi_controller *mhi_cntrl) 
         */
        mhi_create_devices(mhi_cntrl);
- read_lock_bh(&mhi_cntrl->pm_lock); error_mission_mode: 
-       mhi_cntrl->wake_put(mhi_cntrl, false);
-       read_unlock_bh(&mhi_cntrl->pm_lock);
+       mhi_device_put(mhi_cntrl->mhi_dev);
return ret; 
  }
@@ -1038,9 +1037,11 @@ int __mhi_device_get_sync(struct mhi_controller 
*mhi_cntrl)
                read_unlock_bh(&mhi_cntrl->pm_lock);
                return -EIO;
        }
+       read_unlock_bh(&mhi_cntrl->pm_lock);
+
+       pm_runtime_get_sync(&mhi_cntrl->mhi_dev->dev);
+       read_lock_bh(&mhi_cntrl->pm_lock);
        mhi_cntrl->wake_get(mhi_cntrl, true);
-       if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
-               mhi_trigger_resume(mhi_cntrl);
        read_unlock_bh(&mhi_cntrl->pm_lock);
ret = wait_event_timeout(mhi_cntrl->state_event, 
@@ -1049,9 +1050,7 @@ int __mhi_device_get_sync(struct mhi_controller 
*mhi_cntrl)
                                 msecs_to_jiffies(mhi_cntrl->timeout_ms));
if (!ret || MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) { 
-               read_lock_bh(&mhi_cntrl->pm_lock);
-               mhi_cntrl->wake_put(mhi_cntrl, false);
-               read_unlock_bh(&mhi_cntrl->pm_lock);
+               mhi_device_put(mhi_cntrl->mhi_dev);
                return -EIO;
        }
@@ -1339,11 +1338,10 @@ void mhi_device_put(struct mhi_device *mhi_dev) mhi_dev->dev_wake--; 
        read_lock_bh(&mhi_cntrl->pm_lock);
-       if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
-               mhi_trigger_resume(mhi_cntrl);
mhi_cntrl->wake_put(mhi_cntrl, false); 
        read_unlock_bh(&mhi_cntrl->pm_lock);
+       pm_runtime_put(&mhi_cntrl->mhi_dev->dev);
  }
  EXPORT_SYMBOL_GPL(mhi_device_put);

Reply via email to