* [PATCH v2 0/6] bus: mhi: Fix broken runtime PM design
@ 2026-05-22 10:00 Krishna Chaitanya Chundru
2026-05-22 10:00 ` [PATCH v2 1/6] bus: mhi: Replace controller runtime_get/put callbacks with direct PM runtime APIs Krishna Chaitanya Chundru
` (6 more replies)
0 siblings, 7 replies; 15+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-05-22 10:00 UTC (permalink / raw)
To: Manivannan Sadhasivam, Jeff Hugo, Carl Vanderlip, Oded Gabbay,
Jeff Johnson, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Loic Poulain,
Sergey Ryazanov, Johannes Berg
Cc: mhi, linux-arm-msm, linux-kernel, dri-devel, linux-wireless,
ath11k, ath12k, netdev, mayank.rana, quic_vbadigan,
vivek.pernamitta, Krishna Chaitanya Chundru, Konrad Dybcio
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.
NOTE: As we have dependecies with other subsystems, Mani can you take
these series through MHI tree if other maintainers give a ack for this
series. To all the maintainers please ack to this series after reviewing
so that Mani can take this through MHI branch.
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
Changes in v2:
- Rewrite commit messages (Bjorn Andersson)
- Remove autosuspend and use normal runtime get/put API's as there is
already autosuspend in controller driver which waits significant time.
- Add pm_runtime_get()/get_sync() error handling.
- Add rumtime pm for wwan and qrtr.
- Link to v1: https://lore.kernel.org/r/20251201-mhi_runtimepm-v1-0-fab94399ca75@oss.qualcomm.com
---
Krishna Chaitanya Chundru (6):
bus: mhi: Replace controller runtime_get/put callbacks with direct PM runtime APIs
bus: mhi: Drop controller runtime PM callback indirection
net: mhi_net: Hold runtime PM during active data path operations
net: qrtr: Hold runtime PM during active data path operations
net: wwan: Hold runtime PM during active data path operations
bus: mhi: host: Fix runtime PM ownership between clients and controller
drivers/accel/qaic/mhi_controller.c | 11 -------
drivers/bus/mhi/host/init.c | 4 ++-
drivers/bus/mhi/host/internal.h | 7 ++--
drivers/bus/mhi/host/main.c | 21 ++----------
drivers/bus/mhi/host/pci_generic.c | 24 ++------------
drivers/bus/mhi/host/pm.c | 18 +++++------
drivers/net/mhi_net.c | 39 +++++++++++++++++++++++
drivers/net/wireless/ath/ath11k/mhi.c | 10 ------
drivers/net/wireless/ath/ath12k/mhi.c | 11 -------
drivers/net/wwan/mhi_wwan_ctrl.c | 60 ++++++++++++++++++++++++++++++++++-
drivers/net/wwan/mhi_wwan_mbim.c | 44 ++++++++++++++++++++++++-
include/linux/mhi.h | 4 ---
net/qrtr/mhi.c | 57 +++++++++++++++++++++++++++++++--
13 files changed, 216 insertions(+), 94 deletions(-)
---
base-commit: a293ec25d59dd96309058c70df5a4dd0f889a1e4
change-id: 20251128-mhi_runtimepm-f7aed52cc557
Best regards,
--
Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/6] bus: mhi: Replace controller runtime_get/put callbacks with direct PM runtime APIs
2026-05-22 10:00 [PATCH v2 0/6] bus: mhi: Fix broken runtime PM design Krishna Chaitanya Chundru
@ 2026-05-22 10:00 ` Krishna Chaitanya Chundru
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
2026-05-22 10:00 ` [PATCH v2 2/6] bus: mhi: Drop controller runtime PM callback indirection Krishna Chaitanya Chundru
` (5 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-05-22 10:00 UTC (permalink / raw)
To: Manivannan Sadhasivam, Jeff Hugo, Carl Vanderlip, Oded Gabbay,
Jeff Johnson, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Loic Poulain,
Sergey Ryazanov, Johannes Berg
Cc: mhi, linux-arm-msm, linux-kernel, dri-devel, linux-wireless,
ath11k, ath12k, netdev, mayank.rana, quic_vbadigan,
vivek.pernamitta, Krishna Chaitanya Chundru, Konrad Dybcio
Remove the runtime_get() and runtime_put() function pointers from the MHI
controller and replace their call sites with direct calls to
pm_runtime_get() and pm_runtime_put(). Also add pm_runtime_mark_last_busy()
before each pm_runtime_put() call to properly update the last busy
timestamp for autosuspend.
The removed callbacks provided no additional logic beyond wrapping the PM
runtime APIs, so eliminate the indirection and the requirement for drivers
to implement these no-op callbacks.
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
drivers/bus/mhi/host/init.c | 1 -
drivers/bus/mhi/host/internal.h | 7 +++++--
drivers/bus/mhi/host/main.c | 19 ++++++++++++-------
3 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index 0a728ca2c494..9f3ee4a14418 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -927,7 +927,6 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
int ret, i;
if (!mhi_cntrl || !mhi_cntrl->cntrl_dev || !mhi_cntrl->regs ||
- !mhi_cntrl->runtime_get || !mhi_cntrl->runtime_put ||
!mhi_cntrl->status_cb || !mhi_cntrl->read_reg ||
!mhi_cntrl->write_reg || !mhi_cntrl->nr_irqs ||
!mhi_cntrl->irq || !mhi_cntrl->reg_len)
diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
index 7b0ee5e3a12d..a7493aabc6fa 100644
--- a/drivers/bus/mhi/host/internal.h
+++ b/drivers/bus/mhi/host/internal.h
@@ -7,6 +7,8 @@
#ifndef _MHI_INT_H
#define _MHI_INT_H
+#include <linux/pm_runtime.h>
+
#include "../common.h"
extern const struct bus_type mhi_bus_type;
@@ -352,8 +354,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);
- mhi_cntrl->runtime_get(mhi_cntrl);
- mhi_cntrl->runtime_put(mhi_cntrl);
+ pm_runtime_get(mhi_cntrl->cntrl_dev);
+ pm_runtime_mark_last_busy(mhi_cntrl->cntrl_dev);
+ pm_runtime_put(mhi_cntrl->cntrl_dev);
}
/* Register access methods */
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index 53c0ffe30070..71919c2e9462 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -661,7 +661,8 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
if (mhi_chan->dir == DMA_TO_DEVICE) {
atomic_dec(&mhi_cntrl->pending_pkts);
/* Release the reference got from mhi_queue() */
- mhi_cntrl->runtime_put(mhi_cntrl);
+ pm_runtime_mark_last_busy(mhi_cntrl->cntrl_dev);
+ pm_runtime_put(mhi_cntrl->cntrl_dev);
}
read_lock_bh(&mhi_chan->lock);
@@ -1138,7 +1139,7 @@ static int mhi_queue(struct mhi_device *mhi_dev, struct mhi_buf_info *buf_info,
* for host->device buffer, balanced put is done on buffer completion
* for device->host buffer, balanced put is after ringing the DB
*/
- mhi_cntrl->runtime_get(mhi_cntrl);
+ pm_runtime_get(mhi_cntrl->cntrl_dev);
/* Assert dev_wake (to exit/prevent M1/M2)*/
mhi_cntrl->wake_toggle(mhi_cntrl);
@@ -1149,8 +1150,10 @@ 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)
- mhi_cntrl->runtime_put(mhi_cntrl);
+ 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);
@@ -1352,7 +1355,7 @@ static int mhi_update_channel_state(struct mhi_controller *mhi_cntrl,
ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
if (ret)
return ret;
- mhi_cntrl->runtime_get(mhi_cntrl);
+ pm_runtime_get(mhi_cntrl->cntrl_dev);
reinit_completion(&mhi_chan->completion);
ret = mhi_send_cmd(mhi_cntrl, mhi_chan, cmd);
@@ -1383,7 +1386,8 @@ 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:
- mhi_cntrl->runtime_put(mhi_cntrl);
+ 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;
@@ -1524,7 +1528,8 @@ static void mhi_reset_data_chan(struct mhi_controller *mhi_cntrl,
if (mhi_chan->dir == DMA_TO_DEVICE) {
atomic_dec(&mhi_cntrl->pending_pkts);
/* Release the reference got from mhi_queue() */
- mhi_cntrl->runtime_put(mhi_cntrl);
+ pm_runtime_mark_last_busy(mhi_cntrl->cntrl_dev);
+ pm_runtime_put(mhi_cntrl->cntrl_dev);
}
if (!buf_info->pre_mapped)
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/6] bus: mhi: Drop controller runtime PM callback indirection
2026-05-22 10:00 [PATCH v2 0/6] bus: mhi: Fix broken runtime PM design Krishna Chaitanya Chundru
2026-05-22 10:00 ` [PATCH v2 1/6] bus: mhi: Replace controller runtime_get/put callbacks with direct PM runtime APIs Krishna Chaitanya Chundru
@ 2026-05-22 10:00 ` Krishna Chaitanya Chundru
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
2026-05-22 10:00 ` [PATCH v2 3/6] net: mhi_net: Hold runtime PM during active data path operations Krishna Chaitanya Chundru
` (4 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-05-22 10:00 UTC (permalink / raw)
To: Manivannan Sadhasivam, Jeff Hugo, Carl Vanderlip, Oded Gabbay,
Jeff Johnson, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Loic Poulain,
Sergey Ryazanov, Johannes Berg
Cc: mhi, linux-arm-msm, linux-kernel, dri-devel, linux-wireless,
ath11k, ath12k, netdev, mayank.rana, quic_vbadigan,
vivek.pernamitta, Krishna Chaitanya Chundru, Konrad Dybcio
The MHI controller interface exposes runtime_get and runtime_put callbacks
to abstract runtime PM handling from the MHI core. This indirection is
unnecessary since the MHI core can directly use the generic pm_runtime_*
APIs, and the existing implementations are either no-ops or trivial
wrappers around those same APIs.
Remove the runtime_get and runtime_put function pointers from struct
mhi_controller and update all users in the MHI host stack to call the
standard runtime PM helpers directly.
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
drivers/accel/qaic/mhi_controller.c | 11 -----------
drivers/bus/mhi/host/pci_generic.c | 24 +++---------------------
drivers/net/wireless/ath/ath11k/mhi.c | 10 ----------
drivers/net/wireless/ath/ath12k/mhi.c | 11 -----------
include/linux/mhi.h | 4 ----
5 files changed, 3 insertions(+), 57 deletions(-)
diff --git a/drivers/accel/qaic/mhi_controller.c b/drivers/accel/qaic/mhi_controller.c
index 4d787f77ce41..68cabfd2df2d 100644
--- a/drivers/accel/qaic/mhi_controller.c
+++ b/drivers/accel/qaic/mhi_controller.c
@@ -776,15 +776,6 @@ static void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *addr,
writel_relaxed(val, addr);
}
-static int mhi_runtime_get(struct mhi_controller *mhi_cntrl)
-{
- return 0;
-}
-
-static void mhi_runtime_put(struct mhi_controller *mhi_cntrl)
-{
-}
-
static void mhi_status_cb(struct mhi_controller *mhi_cntrl, enum mhi_callback reason)
{
struct qaic_device *qdev = pci_get_drvdata(to_pci_dev(mhi_cntrl->cntrl_dev));
@@ -845,8 +836,6 @@ struct mhi_controller *qaic_mhi_register_controller(struct pci_dev *pci_dev, voi
mhi_cntrl->iova_start = 0;
mhi_cntrl->iova_stop = PHYS_ADDR_MAX - 1;
mhi_cntrl->status_cb = mhi_status_cb;
- mhi_cntrl->runtime_get = mhi_runtime_get;
- mhi_cntrl->runtime_put = mhi_runtime_put;
mhi_cntrl->read_reg = mhi_read_reg;
mhi_cntrl->write_reg = mhi_write_reg;
mhi_cntrl->regs = mhi_bar;
diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
index 750da3dbb4c6..cbd2b442df83 100644
--- a/drivers/bus/mhi/host/pci_generic.c
+++ b/drivers/bus/mhi/host/pci_generic.c
@@ -1220,23 +1220,6 @@ static int mhi_pci_get_irqs(struct mhi_controller *mhi_cntrl,
return 0;
}
-static int mhi_pci_runtime_get(struct mhi_controller *mhi_cntrl)
-{
- /* The runtime_get() MHI callback means:
- * Do whatever is requested to leave M3.
- */
- return pm_runtime_get(mhi_cntrl->cntrl_dev);
-}
-
-static void mhi_pci_runtime_put(struct mhi_controller *mhi_cntrl)
-{
- /* The runtime_put() MHI callback means:
- * Device can be moved in M3 state.
- */
- pm_runtime_mark_last_busy(mhi_cntrl->cntrl_dev);
- pm_runtime_put(mhi_cntrl->cntrl_dev);
-}
-
static void mhi_pci_recovery_work(struct work_struct *work)
{
struct mhi_pci_device *mhi_pdev = container_of(work, struct mhi_pci_device,
@@ -1324,7 +1307,7 @@ static int mhi_pci_generic_edl_trigger(struct mhi_controller *mhi_cntrl)
}
pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
- mhi_cntrl->runtime_get(mhi_cntrl);
+ pm_runtime_get(mhi_cntrl->cntrl_dev);
ret = mhi_get_channel_doorbell_offset(mhi_cntrl, &val);
if (ret)
@@ -1338,7 +1321,8 @@ static int mhi_pci_generic_edl_trigger(struct mhi_controller *mhi_cntrl)
mhi_soc_reset(mhi_cntrl);
err_get_chdb:
- mhi_cntrl->runtime_put(mhi_cntrl);
+ 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;
@@ -1385,8 +1369,6 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
mhi_cntrl->read_reg = mhi_pci_read_reg;
mhi_cntrl->write_reg = mhi_pci_write_reg;
mhi_cntrl->status_cb = mhi_pci_status_cb;
- mhi_cntrl->runtime_get = mhi_pci_runtime_get;
- mhi_cntrl->runtime_put = mhi_pci_runtime_put;
mhi_cntrl->mru = info->mru_default;
mhi_cntrl->name = info->name;
diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
index f994233df2bb..db163a708064 100644
--- a/drivers/net/wireless/ath/ath11k/mhi.c
+++ b/drivers/net/wireless/ath/ath11k/mhi.c
@@ -226,14 +226,6 @@ static int ath11k_mhi_get_msi(struct ath11k_pci *ab_pci)
return 0;
}
-static int ath11k_mhi_op_runtime_get(struct mhi_controller *mhi_cntrl)
-{
- return 0;
-}
-
-static void ath11k_mhi_op_runtime_put(struct mhi_controller *mhi_cntrl)
-{
-}
static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
{
@@ -380,8 +372,6 @@ int ath11k_mhi_register(struct ath11k_pci *ab_pci)
mhi_ctrl->sbl_size = SZ_512K;
mhi_ctrl->seg_len = SZ_512K;
mhi_ctrl->fbc_download = true;
- mhi_ctrl->runtime_get = ath11k_mhi_op_runtime_get;
- mhi_ctrl->runtime_put = ath11k_mhi_op_runtime_put;
mhi_ctrl->status_cb = ath11k_mhi_op_status_cb;
mhi_ctrl->read_reg = ath11k_mhi_op_read_reg;
mhi_ctrl->write_reg = ath11k_mhi_op_write_reg;
diff --git a/drivers/net/wireless/ath/ath12k/mhi.c b/drivers/net/wireless/ath/ath12k/mhi.c
index ee87f00bc5de..9122837e5206 100644
--- a/drivers/net/wireless/ath/ath12k/mhi.c
+++ b/drivers/net/wireless/ath/ath12k/mhi.c
@@ -100,15 +100,6 @@ static int ath12k_mhi_get_msi(struct ath12k_pci *ab_pci)
return 0;
}
-static int ath12k_mhi_op_runtime_get(struct mhi_controller *mhi_cntrl)
-{
- return 0;
-}
-
-static void ath12k_mhi_op_runtime_put(struct mhi_controller *mhi_cntrl)
-{
-}
-
static char *ath12k_mhi_op_callback_to_str(enum mhi_callback reason)
{
switch (reason) {
@@ -256,8 +247,6 @@ int ath12k_mhi_register(struct ath12k_pci *ab_pci)
mhi_ctrl->sbl_size = SZ_512K;
mhi_ctrl->seg_len = SZ_512K;
mhi_ctrl->fbc_download = true;
- mhi_ctrl->runtime_get = ath12k_mhi_op_runtime_get;
- mhi_ctrl->runtime_put = ath12k_mhi_op_runtime_put;
mhi_ctrl->status_cb = ath12k_mhi_op_status_cb;
mhi_ctrl->read_reg = ath12k_mhi_op_read_reg;
mhi_ctrl->write_reg = ath12k_mhi_op_write_reg;
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index fb3ba639f4f8..46ac60d01846 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -361,8 +361,6 @@ struct mhi_controller_config {
* @wake_get: CB function to assert device wake (optional)
* @wake_put: CB function to de-assert device wake (optional)
* @wake_toggle: CB function to assert and de-assert device wake (optional)
- * @runtime_get: CB function to controller runtime resume (required)
- * @runtime_put: CB function to decrement pm usage (required)
* @map_single: CB function to create TRE buffer
* @unmap_single: CB function to destroy TRE buffer
* @read_reg: Read a MHI register via the physical link (required)
@@ -441,8 +439,6 @@ struct mhi_controller {
void (*wake_get)(struct mhi_controller *mhi_cntrl, bool override);
void (*wake_put)(struct mhi_controller *mhi_cntrl, bool override);
void (*wake_toggle)(struct mhi_controller *mhi_cntrl);
- int (*runtime_get)(struct mhi_controller *mhi_cntrl);
- void (*runtime_put)(struct mhi_controller *mhi_cntrl);
int (*map_single)(struct mhi_controller *mhi_cntrl,
struct mhi_buf_info *buf);
void (*unmap_single)(struct mhi_controller *mhi_cntrl,
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 3/6] net: mhi_net: Hold runtime PM during active data path operations
2026-05-22 10:00 [PATCH v2 0/6] bus: mhi: Fix broken runtime PM design Krishna Chaitanya Chundru
2026-05-22 10:00 ` [PATCH v2 1/6] bus: mhi: Replace controller runtime_get/put callbacks with direct PM runtime APIs Krishna Chaitanya Chundru
2026-05-22 10:00 ` [PATCH v2 2/6] bus: mhi: Drop controller runtime PM callback indirection Krishna Chaitanya Chundru
@ 2026-05-22 10:00 ` Krishna Chaitanya Chundru
2026-05-22 20:09 ` Loic Poulain
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
2026-05-22 10:00 ` [PATCH v2 4/6] net: qrtr: " Krishna Chaitanya Chundru
` (3 subsequent siblings)
6 siblings, 2 replies; 15+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-05-22 10:00 UTC (permalink / raw)
To: Manivannan Sadhasivam, Jeff Hugo, Carl Vanderlip, Oded Gabbay,
Jeff Johnson, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Loic Poulain,
Sergey Ryazanov, Johannes Berg
Cc: mhi, linux-arm-msm, linux-kernel, dri-devel, linux-wireless,
ath11k, ath12k, netdev, mayank.rana, quic_vbadigan,
vivek.pernamitta, Krishna Chaitanya Chundru
The mhi_net driver does not coordinate with runtime PM, which allows the
underlying MHI controller to be runtime-suspended while transmit, receive,
or RX buffer refill operations are in progress. This can lead to stalled
transfers or failed queueing once runtime PM is enabled in the MHI core.
Add runtime PM reference counting to the mhi_net data path to keep the
controller active for the duration of TX, RX, and buffer management
operations. Enable runtime PM during probe and take/release references
explicitly around these critical paths.
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
drivers/net/mhi_net.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/drivers/net/mhi_net.c b/drivers/net/mhi_net.c
index ae169929a9d8..5d7f9ccdb17b 100644
--- a/drivers/net/mhi_net.c
+++ b/drivers/net/mhi_net.c
@@ -9,6 +9,7 @@
#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/netdevice.h>
+#include <linux/pm_runtime.h>
#include <linux/skbuff.h>
#include <linux/u64_stats_sync.h>
@@ -76,11 +77,19 @@ static netdev_tx_t mhi_ndo_xmit(struct sk_buff *skb, struct net_device *ndev)
struct mhi_device *mdev = mhi_netdev->mdev;
int err;
+ err = pm_runtime_get(&mdev->dev);
+ if (err < 0 && err != -EINPROGRESS) {
+ dev_err(&mdev->dev, "pm_runtime_get failed %d\n", err);
+ pm_runtime_put_noidle(&mdev->dev);
+ goto exit_drop;
+ }
+
err = mhi_queue_skb(mdev, DMA_TO_DEVICE, skb, skb->len, MHI_EOT);
if (unlikely(err)) {
net_err_ratelimited("%s: Failed to queue TX buf (%d)\n",
ndev->name, err);
dev_kfree_skb_any(skb);
+ pm_runtime_put(&mdev->dev);
goto exit_drop;
}
@@ -251,6 +260,7 @@ static void mhi_net_ul_callback(struct mhi_device *mhi_dev,
/* MHI layer stopping/resetting the UL channel */
if (mhi_res->transaction_status == -ENOTCONN) {
u64_stats_update_end(&mhi_netdev->stats.tx_syncp);
+ pm_runtime_put(&mdev->dev);
return;
}
@@ -261,6 +271,7 @@ static void mhi_net_ul_callback(struct mhi_device *mhi_dev,
}
u64_stats_update_end(&mhi_netdev->stats.tx_syncp);
+ pm_runtime_put(&mdev->dev);
if (netif_queue_stopped(ndev) && !mhi_queue_is_full(mdev, DMA_TO_DEVICE))
netif_wake_queue(ndev);
}
@@ -277,6 +288,12 @@ static void mhi_net_rx_refill_work(struct work_struct *work)
size = mhi_netdev->mru ? mhi_netdev->mru : READ_ONCE(ndev->mtu);
+ err = pm_runtime_resume_and_get(&mdev->dev);
+ if (err) {
+ dev_err(&mdev->dev, "pm_runtime_resume_and_get failed %d\n", err);
+ return;
+ }
+
while (!mhi_queue_is_full(mdev, DMA_FROM_DEVICE)) {
skb = netdev_alloc_skb(ndev, size);
if (unlikely(!skb))
@@ -296,6 +313,7 @@ static void mhi_net_rx_refill_work(struct work_struct *work)
cond_resched();
}
+ pm_runtime_put(&mdev->dev);
/* If we're still starved of rx buffers, reschedule later */
if (mhi_get_free_desc_count(mdev, DMA_FROM_DEVICE) == mhi_netdev->rx_queue_sz)
schedule_delayed_work(&mhi_netdev->rx_refill, HZ / 2);
@@ -362,20 +380,41 @@ static int mhi_net_probe(struct mhi_device *mhi_dev,
SET_NETDEV_DEV(ndev, &mhi_dev->dev);
+ pm_runtime_no_callbacks(&mhi_dev->dev);
+ err = devm_pm_runtime_set_active_enabled(&mhi_dev->dev);
+ if (err)
+ return err;
+
+ err = pm_runtime_resume_and_get(&mhi_dev->dev);
+ if (err) {
+ dev_err(&mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", err);
+ return err;
+ }
+
err = mhi_net_newlink(mhi_dev, ndev);
if (err) {
free_netdev(ndev);
+ pm_runtime_put(&mhi_dev->dev);
return err;
}
+ pm_runtime_put(&mhi_dev->dev);
return 0;
}
static void mhi_net_remove(struct mhi_device *mhi_dev)
{
struct mhi_net_dev *mhi_netdev = dev_get_drvdata(&mhi_dev->dev);
+ int err;
+
+ err = pm_runtime_resume_and_get(&mhi_dev->dev);
+ if (err)
+ dev_err(&mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", err);
mhi_net_dellink(mhi_dev, mhi_netdev->ndev);
+
+ if (!err)
+ pm_runtime_put(&mhi_dev->dev);
}
static const struct mhi_device_info mhi_hwip0 = {
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 4/6] net: qrtr: Hold runtime PM during active data path operations
2026-05-22 10:00 [PATCH v2 0/6] bus: mhi: Fix broken runtime PM design Krishna Chaitanya Chundru
` (2 preceding siblings ...)
2026-05-22 10:00 ` [PATCH v2 3/6] net: mhi_net: Hold runtime PM during active data path operations Krishna Chaitanya Chundru
@ 2026-05-22 10:00 ` Krishna Chaitanya Chundru
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
2026-05-22 10:00 ` [PATCH v2 5/6] net: wwan: " Krishna Chaitanya Chundru
` (2 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-05-22 10:00 UTC (permalink / raw)
To: Manivannan Sadhasivam, Jeff Hugo, Carl Vanderlip, Oded Gabbay,
Jeff Johnson, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Loic Poulain,
Sergey Ryazanov, Johannes Berg
Cc: mhi, linux-arm-msm, linux-kernel, dri-devel, linux-wireless,
ath11k, ath12k, netdev, mayank.rana, quic_vbadigan,
vivek.pernamitta, Krishna Chaitanya Chundru
The QRTR MHI transport driver does not coordinate with runtime PM, which
allows the underlying MHI controller to be runtime-suspended while
transmit, receive, or RX buffer refill operations are in progress. This can
lead to stalled transfers or failed buffer queueing once runtime PM is
enabled in the MHI core.
Add runtime PM reference counting around TX, RX, and buffer management
operations to keep the controller active for the duration of each critical
section. Enable runtime PM during probe and take explicit references around
these data path operations.
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
net/qrtr/mhi.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 54 insertions(+), 3 deletions(-)
diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
index 80e341d2f8a4..cba7a5daf7a4 100644
--- a/net/qrtr/mhi.c
+++ b/net/qrtr/mhi.c
@@ -6,6 +6,7 @@
#include <linux/mhi.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
+#include <linux/pm_runtime.h>
#include <linux/skbuff.h>
#include <net/sock.h>
@@ -38,11 +39,20 @@ static void qcom_mhi_qrtr_dl_callback(struct mhi_device *mhi_dev,
if (rc == -EINVAL)
dev_err(qdev->dev, "invalid ipcrouter packet\n");
+ rc = pm_runtime_get(&qdev->mhi_dev->dev);
+ if (rc < 0 && rc != -EINPROGRESS) {
+ dev_err(&mhi_dev->dev, "pm_runtime_get failed %d\n", rc);
+ pm_runtime_put_noidle(&qdev->mhi_dev->dev);
+ return;
+ }
+
/* Done with the buffer, now recycle it for future use */
rc = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, mhi_res->buf_addr,
mhi_dev->mhi_cntrl->buffer_len, MHI_EOT);
if (rc)
dev_err(&mhi_dev->dev, "Failed to recycle the buffer: %d\n", rc);
+
+ pm_runtime_put(&mhi_dev->dev);
}
/* From QRTR to MHI */
@@ -54,6 +64,8 @@ static void qcom_mhi_qrtr_ul_callback(struct mhi_device *mhi_dev,
if (skb->sk)
sock_put(skb->sk);
consume_skb(skb);
+
+ pm_runtime_put(&mhi_dev->dev);
}
/* Send data over MHI */
@@ -69,13 +81,21 @@ static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
if (rc)
goto free_skb;
+ rc = pm_runtime_resume_and_get(&qdev->mhi_dev->dev);
+ if (rc) {
+ dev_err(&qdev->mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", rc);
+ goto free_skb;
+ }
+
rc = mhi_queue_skb(qdev->mhi_dev, DMA_TO_DEVICE, skb, skb->len,
MHI_EOT);
if (rc)
- goto free_skb;
+ goto runtime_put;
return rc;
+runtime_put:
+ pm_runtime_put(&qdev->mhi_dev->dev);
free_skb:
if (skb->sk)
sock_put(skb->sk);
@@ -90,20 +110,30 @@ static int qcom_mhi_qrtr_queue_dl_buffers(struct mhi_device *mhi_dev)
void *buf;
int ret;
+ ret = pm_runtime_resume_and_get(&mhi_dev->dev);
+ if (ret) {
+ dev_err(&mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", ret);
+ return ret;
+ }
+
free_desc = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
while (free_desc--) {
buf = devm_kmalloc(&mhi_dev->dev, mhi_dev->mhi_cntrl->buffer_len, GFP_KERNEL);
- if (!buf)
+ if (!buf) {
+ pm_runtime_put(&mhi_dev->dev);
return -ENOMEM;
+ }
ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, buf, mhi_dev->mhi_cntrl->buffer_len,
MHI_EOT);
if (ret) {
dev_err(&mhi_dev->dev, "Failed to queue buffer: %d\n", ret);
+ pm_runtime_put(&mhi_dev->dev);
return ret;
}
}
+ pm_runtime_put(&mhi_dev->dev);
return 0;
}
@@ -121,12 +151,22 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
qdev->dev = &mhi_dev->dev;
qdev->ep.xmit = qcom_mhi_qrtr_send;
+ pm_runtime_no_callbacks(&mhi_dev->dev);
+ rc = devm_pm_runtime_set_active_enabled(&mhi_dev->dev);
+ if (rc)
+ return rc;
+
+ rc = pm_runtime_resume_and_get(&mhi_dev->dev);
+ if (rc) {
+ dev_err(&mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", rc);
+ return rc;
+ }
dev_set_drvdata(&mhi_dev->dev, qdev);
/* start channels */
rc = mhi_prepare_for_transfer(mhi_dev);
if (rc)
- return rc;
+ goto runtime_put;
rc = qrtr_endpoint_register(&qdev->ep, QRTR_EP_NID_AUTO);
if (rc)
@@ -138,12 +178,15 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n");
+ pm_runtime_put(&mhi_dev->dev);
return 0;
err_unregister:
qrtr_endpoint_unregister(&qdev->ep);
err_unprepare:
mhi_unprepare_from_transfer(mhi_dev);
+runtime_put:
+ pm_runtime_put(&mhi_dev->dev);
return rc;
}
@@ -151,10 +194,18 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
static void qcom_mhi_qrtr_remove(struct mhi_device *mhi_dev)
{
struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
+ int err;
+
+ err = pm_runtime_resume_and_get(&mhi_dev->dev);
+ if (err)
+ dev_err(&mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", err);
qrtr_endpoint_unregister(&qdev->ep);
mhi_unprepare_from_transfer(mhi_dev);
dev_set_drvdata(&mhi_dev->dev, NULL);
+
+ if (!err)
+ pm_runtime_put(&mhi_dev->dev);
}
static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = {
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 5/6] net: wwan: Hold runtime PM during active data path operations
2026-05-22 10:00 [PATCH v2 0/6] bus: mhi: Fix broken runtime PM design Krishna Chaitanya Chundru
` (3 preceding siblings ...)
2026-05-22 10:00 ` [PATCH v2 4/6] net: qrtr: " Krishna Chaitanya Chundru
@ 2026-05-22 10:00 ` Krishna Chaitanya Chundru
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
2026-05-22 10:00 ` [PATCH v2 6/6] bus: mhi: host: Fix runtime PM ownership between clients and controller Krishna Chaitanya Chundru
2026-05-25 9:03 ` Claude review: bus: mhi: Fix broken runtime PM design Claude Code Review Bot
6 siblings, 1 reply; 15+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-05-22 10:00 UTC (permalink / raw)
To: Manivannan Sadhasivam, Jeff Hugo, Carl Vanderlip, Oded Gabbay,
Jeff Johnson, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Loic Poulain,
Sergey Ryazanov, Johannes Berg
Cc: mhi, linux-arm-msm, linux-kernel, dri-devel, linux-wireless,
ath11k, ath12k, netdev, mayank.rana, quic_vbadigan,
vivek.pernamitta, Krishna Chaitanya Chundru
The mhi_wwan_ctrl and mhi_wwan_mbim drivers do not coordinate with runtime
PM, which allows the underlying MHI controller to be runtime suspended
while transmit, receive, or RX buffer refill operations are in progress.
This can lead to stalled transfers or failed queueing once runtime PM is
enabled in the MHI core.
Enable runtime PM during probe and take explicit references around TX, RX,
and buffer management operations so the controller remains active for the
duration of each critical path.
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
drivers/net/wwan/mhi_wwan_ctrl.c | 60 +++++++++++++++++++++++++++++++++++++++-
drivers/net/wwan/mhi_wwan_mbim.c | 44 ++++++++++++++++++++++++++++-
2 files changed, 102 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
index fa73861db6ad..0fe0f24c0df4 100644
--- a/drivers/net/wwan/mhi_wwan_ctrl.c
+++ b/drivers/net/wwan/mhi_wwan_ctrl.c
@@ -4,6 +4,7 @@
#include <linux/mhi.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
+#include <linux/pm_runtime.h>
#include <linux/wwan.h>
/* MHI wwan flags */
@@ -79,6 +80,13 @@ static void mhi_wwan_ctrl_refill_work(struct work_struct *work)
{
struct mhi_wwan_dev *mhiwwan = container_of(work, struct mhi_wwan_dev, rx_refill);
struct mhi_device *mhi_dev = mhiwwan->mhi_dev;
+ int err;
+
+ err = pm_runtime_resume_and_get(&mhi_dev->dev);
+ if (err) {
+ dev_err(&mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", err);
+ return;
+ }
while (mhi_wwan_rx_budget_dec(mhiwwan)) {
struct sk_buff *skb;
@@ -102,17 +110,27 @@ static void mhi_wwan_ctrl_refill_work(struct work_struct *work)
break;
}
}
+ pm_runtime_put(&mhi_dev->dev);
}
static int mhi_wwan_ctrl_start(struct wwan_port *port)
{
struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
+ struct mhi_device *mhi_dev = mhiwwan->mhi_dev;
int ret;
+ ret = pm_runtime_resume_and_get(&mhi_dev->dev);
+ if (ret) {
+ dev_err(&mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", ret);
+ return ret;
+ }
+
/* Start mhi device's channel(s) */
ret = mhi_prepare_for_transfer(mhiwwan->mhi_dev);
- if (ret)
+ if (ret) {
+ pm_runtime_put(&mhi_dev->dev);
return ret;
+ }
/* Don't allocate more buffers than MHI channel queue size */
mhiwwan->rx_budget = mhi_get_free_desc_count(mhiwwan->mhi_dev, DMA_FROM_DEVICE);
@@ -123,12 +141,15 @@ static int mhi_wwan_ctrl_start(struct wwan_port *port)
mhi_wwan_ctrl_refill_work(&mhiwwan->rx_refill);
}
+ pm_runtime_put(&mhi_dev->dev);
return 0;
}
static void mhi_wwan_ctrl_stop(struct wwan_port *port)
{
struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
+ struct mhi_device *mhi_dev = mhiwwan->mhi_dev;
+ int err;
spin_lock_bh(&mhiwwan->rx_lock);
clear_bit(MHI_WWAN_RX_REFILL, &mhiwwan->flags);
@@ -136,12 +157,20 @@ static void mhi_wwan_ctrl_stop(struct wwan_port *port)
cancel_work_sync(&mhiwwan->rx_refill);
+ err = pm_runtime_resume_and_get(&mhi_dev->dev);
+ if (err)
+ dev_err(&mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", err);
+
mhi_unprepare_from_transfer(mhiwwan->mhi_dev);
+
+ if (!err)
+ pm_runtime_put(&mhi_dev->dev);
}
static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
{
struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
+ struct mhi_device *mhi_dev = mhiwwan->mhi_dev;
int ret;
if (skb->len > mhiwwan->mtu)
@@ -150,6 +179,12 @@ static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
if (!test_bit(MHI_WWAN_UL_CAP, &mhiwwan->flags))
return -EOPNOTSUPP;
+ ret = pm_runtime_resume_and_get(&mhi_dev->dev);
+ if (ret) {
+ dev_err(&mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", ret);
+ return ret;
+ }
+
/* Queue the packet for MHI transfer and check fullness of the queue */
spin_lock_bh(&mhiwwan->tx_lock);
ret = mhi_queue_skb(mhiwwan->mhi_dev, DMA_TO_DEVICE, skb, skb->len, MHI_EOT);
@@ -157,6 +192,9 @@ static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
wwan_port_txoff(port);
spin_unlock_bh(&mhiwwan->tx_lock);
+ if (ret)
+ pm_runtime_put(&mhi_dev->dev);
+
return ret;
}
@@ -179,6 +217,8 @@ static void mhi_ul_xfer_cb(struct mhi_device *mhi_dev,
/* MHI core has done with the buffer, release it */
consume_skb(skb);
+ pm_runtime_put(&mhi_dev->dev);
+
/* There is likely new slot available in the MHI queue, re-allow TX */
spin_lock_bh(&mhiwwan->tx_lock);
if (!mhi_queue_is_full(mhiwwan->mhi_dev, DMA_TO_DEVICE))
@@ -217,11 +257,26 @@ static int mhi_wwan_ctrl_probe(struct mhi_device *mhi_dev,
struct mhi_controller *cntrl = mhi_dev->mhi_cntrl;
struct mhi_wwan_dev *mhiwwan;
struct wwan_port *port;
+ int err;
mhiwwan = kzalloc_obj(*mhiwwan);
if (!mhiwwan)
return -ENOMEM;
+ pm_runtime_no_callbacks(&mhi_dev->dev);
+ err = devm_pm_runtime_set_active_enabled(&mhi_dev->dev);
+ if (err) {
+ kfree(mhiwwan);
+ return err;
+ }
+
+ err = pm_runtime_resume_and_get(&mhi_dev->dev);
+ if (err) {
+ dev_err(&mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", err);
+ kfree(mhiwwan);
+ return err;
+ }
+
mhiwwan->mhi_dev = mhi_dev;
mhiwwan->mtu = MHI_WWAN_MAX_MTU;
INIT_WORK(&mhiwwan->rx_refill, mhi_wwan_ctrl_refill_work);
@@ -240,11 +295,14 @@ static int mhi_wwan_ctrl_probe(struct mhi_device *mhi_dev,
&wwan_pops, NULL, mhiwwan);
if (IS_ERR(port)) {
kfree(mhiwwan);
+ pm_runtime_put(&mhi_dev->dev);
return PTR_ERR(port);
}
mhiwwan->wwan_port = port;
+ pm_runtime_put(&mhi_dev->dev);
+
return 0;
};
diff --git a/drivers/net/wwan/mhi_wwan_mbim.c b/drivers/net/wwan/mhi_wwan_mbim.c
index 1d7e3ad900c1..56e660ecfcb4 100644
--- a/drivers/net/wwan/mhi_wwan_mbim.c
+++ b/drivers/net/wwan/mhi_wwan_mbim.c
@@ -20,6 +20,7 @@
#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/netdevice.h>
+#include <linux/pm_runtime.h>
#include <linux/skbuff.h>
#include <linux/u64_stats_sync.h>
#include <linux/usb.h>
@@ -153,9 +154,18 @@ static netdev_tx_t mhi_mbim_ndo_xmit(struct sk_buff *skb, struct net_device *nde
{
struct mhi_mbim_link *link = wwan_netdev_drvpriv(ndev);
struct mhi_mbim_context *mbim = link->mbim;
+ struct mhi_device *mhi_dev = mbim->mdev;
unsigned long flags;
int err = -ENOMEM;
+ err = pm_runtime_get(&mhi_dev->dev);
+ if (err < 0 && err != -EINPROGRESS) {
+ dev_err(&mhi_dev->dev, "pm_runtime_get Failed %d\n", err);
+ pm_runtime_put_noidle(&mhi_dev->dev);
+ dev_kfree_skb_any(skb);
+ return NETDEV_TX_OK;
+ }
+
/* Serialize MHI channel queuing and MBIM seq */
spin_lock_irqsave(&mbim->tx_lock, flags);
@@ -184,6 +194,7 @@ static netdev_tx_t mhi_mbim_ndo_xmit(struct sk_buff *skb, struct net_device *nde
return NETDEV_TX_OK;
exit_drop:
+ pm_runtime_put(&mhi_dev->dev);
u64_stats_update_begin(&link->tx_syncp);
u64_stats_inc(&link->tx_dropped);
u64_stats_update_end(&link->tx_syncp);
@@ -396,6 +407,10 @@ static void mhi_net_rx_refill_work(struct work_struct *work)
struct mhi_device *mdev = mbim->mdev;
int err;
+ err = pm_runtime_get(&mdev->dev);
+ if (err < 0 && err != -EINPROGRESS)
+ dev_err(&mdev->dev, "pm_runtime_get Failed %d\n", err);
+
while (!mhi_queue_is_full(mdev, DMA_FROM_DEVICE)) {
struct sk_buff *skb = alloc_skb(mbim->mru, GFP_KERNEL);
@@ -415,6 +430,8 @@ static void mhi_net_rx_refill_work(struct work_struct *work)
cond_resched();
}
+ pm_runtime_put(&mdev->dev);
+
/* If we're still starved of rx buffers, reschedule later */
if (mhi_get_free_desc_count(mdev, DMA_FROM_DEVICE) == mbim->rx_queue_sz)
schedule_delayed_work(&mbim->rx_refill, HZ / 2);
@@ -501,6 +518,7 @@ static void mhi_mbim_ul_callback(struct mhi_device *mhi_dev,
/* MHI layer stopping/resetting the UL channel */
if (mhi_res->transaction_status == -ENOTCONN) {
u64_stats_update_end(&link->tx_syncp);
+ pm_runtime_put(&mhi_dev->dev);
return;
}
@@ -511,6 +529,8 @@ static void mhi_mbim_ul_callback(struct mhi_device *mhi_dev,
}
u64_stats_update_end(&link->tx_syncp);
+ pm_runtime_put(&mhi_dev->dev);
+
if (netif_queue_stopped(ndev) && !mhi_queue_is_full(mbim->mdev, DMA_TO_DEVICE))
netif_wake_queue(ndev);
}
@@ -614,6 +634,17 @@ static int mhi_mbim_probe(struct mhi_device *mhi_dev, const struct mhi_device_id
if (!mbim)
return -ENOMEM;
+ pm_runtime_no_callbacks(&mhi_dev->dev);
+ err = devm_pm_runtime_set_active_enabled(&mhi_dev->dev);
+ if (err)
+ return err;
+
+ err = pm_runtime_get(&mhi_dev->dev);
+ if (err < 0 && err != -EINPROGRESS) {
+ dev_err(&mhi_dev->dev, "pm_runtime_get Failed %d\n", err);
+ return err;
+ }
+
spin_lock_init(&mbim->tx_lock);
dev_set_drvdata(&mhi_dev->dev, mbim);
mbim->mdev = mhi_dev;
@@ -623,8 +654,12 @@ static int mhi_mbim_probe(struct mhi_device *mhi_dev, const struct mhi_device_id
/* Start MHI channels */
err = mhi_prepare_for_transfer(mhi_dev);
- if (err)
+ if (err) {
+ pm_runtime_put(&mhi_dev->dev);
return err;
+ }
+
+ pm_runtime_put(&mhi_dev->dev);
/* Number of transfer descriptors determines size of the queue */
mbim->rx_queue_sz = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
@@ -637,12 +672,19 @@ static void mhi_mbim_remove(struct mhi_device *mhi_dev)
{
struct mhi_mbim_context *mbim = dev_get_drvdata(&mhi_dev->dev);
struct mhi_controller *cntrl = mhi_dev->mhi_cntrl;
+ int err;
+
+ err = pm_runtime_get(&mhi_dev->dev);
+ if (err < 0 && err != -EINPROGRESS)
+ dev_err(&mhi_dev->dev, "pm_runtime_get Failed %d\n", err);
mhi_unprepare_from_transfer(mhi_dev);
cancel_delayed_work_sync(&mbim->rx_refill);
wwan_unregister_ops(&cntrl->mhi_dev->dev);
kfree_skb(mbim->skbagg_head);
dev_set_drvdata(&mhi_dev->dev, NULL);
+
+ pm_runtime_put(&mhi_dev->dev);
}
static const struct mhi_device_id mhi_mbim_id_table[] = {
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 6/6] bus: mhi: host: Fix runtime PM ownership between clients and controller
2026-05-22 10:00 [PATCH v2 0/6] bus: mhi: Fix broken runtime PM design Krishna Chaitanya Chundru
` (4 preceding siblings ...)
2026-05-22 10:00 ` [PATCH v2 5/6] net: wwan: " Krishna Chaitanya Chundru
@ 2026-05-22 10:00 ` Krishna Chaitanya Chundru
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
2026-05-25 9:03 ` Claude review: bus: mhi: Fix broken runtime PM design Claude Code Review Bot
6 siblings, 1 reply; 15+ messages in thread
From: Krishna Chaitanya Chundru @ 2026-05-22 10:00 UTC (permalink / raw)
To: Manivannan Sadhasivam, Jeff Hugo, Carl Vanderlip, Oded Gabbay,
Jeff Johnson, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Loic Poulain,
Sergey Ryazanov, Johannes Berg
Cc: mhi, linux-arm-msm, linux-kernel, dri-devel, linux-wireless,
ath11k, ath12k, netdev, mayank.rana, quic_vbadigan,
vivek.pernamitta, Krishna Chaitanya Chundru
The current MHI runtime PM model allows the core to take power references
directly on the controller device without visibility into client driver
activity. As a result, a controller driver may runtime-suspend the
hardware while one or more MHI client devices are still actively in use.
This happens because the MHI core historically managed runtime PM
internally, rather than relying on standard parent-child PM dependency
tracking. The controller driver therefore has no way to infer whether MHI
clients still require the controller to remain active.
Fix this by enabling runtime PM on the MHI bus device (mhi_dev) and routing
runtime PM references through it. Client drivers now hold runtime PM
references on their own mhi_dev, allowing the PM framework to naturally
propagate activity to the controller via the device hierarchy.
The existing mhi_device_get_sync() and mhi_device_put() helpers are
retained, as they serve as the synchronization point between the MHI power
state machine and runtime PM, combining runtime PM reference management
with MHI wake handling.
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
drivers/bus/mhi/host/init.c | 3 +++
drivers/bus/mhi/host/internal.h | 6 +++---
drivers/bus/mhi/host/main.c | 26 ++------------------------
drivers/bus/mhi/host/pm.c | 18 ++++++++----------
4 files changed, 16 insertions(+), 37 deletions(-)
diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index 9f3ee4a14418..002f3abb8103 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -1034,6 +1034,9 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
mhi_cntrl->mhi_dev = mhi_dev;
+ pm_runtime_no_callbacks(&mhi_cntrl->mhi_dev->dev);
+ devm_pm_runtime_set_active_enabled(&mhi_cntrl->mhi_dev->dev);
+
mhi_create_debugfs(mhi_cntrl);
return 0;
diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
index a7493aabc6fa..434ed4705d25 100644
--- a/drivers/bus/mhi/host/internal.h
+++ b/drivers/bus/mhi/host/internal.h
@@ -354,9 +354,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 71919c2e9462..f0b09657de29 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -658,12 +658,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);
- }
read_lock_bh(&mhi_chan->lock);
}
@@ -1135,12 +1131,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);
@@ -1150,11 +1140,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;
@@ -1355,7 +1340,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);
@@ -1386,8 +1370,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;
@@ -1525,12 +1507,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 f799503c8f36..278fc2ffb820 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);
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/6] net: mhi_net: Hold runtime PM during active data path operations
2026-05-22 10:00 ` [PATCH v2 3/6] net: mhi_net: Hold runtime PM during active data path operations Krishna Chaitanya Chundru
@ 2026-05-22 20:09 ` Loic Poulain
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
1 sibling, 0 replies; 15+ messages in thread
From: Loic Poulain @ 2026-05-22 20:09 UTC (permalink / raw)
To: Krishna Chaitanya Chundru
Cc: Manivannan Sadhasivam, Jeff Hugo, Carl Vanderlip, Oded Gabbay,
Jeff Johnson, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Sergey Ryazanov,
Johannes Berg, mhi, linux-arm-msm, linux-kernel, dri-devel,
linux-wireless, ath11k, ath12k, netdev, mayank.rana,
quic_vbadigan, vivek.pernamitta
Hi Krishna,
On Fri, May 22, 2026 at 12:01 PM Krishna Chaitanya Chundru
<krishna.chundru@oss.qualcomm.com> wrote:
>
> The mhi_net driver does not coordinate with runtime PM, which allows the
> underlying MHI controller to be runtime-suspended while transmit, receive,
> or RX buffer refill operations are in progress. This can lead to stalled
> transfers or failed queueing once runtime PM is enabled in the MHI core.
>
> Add runtime PM reference counting to the mhi_net data path to keep the
> controller active for the duration of TX, RX, and buffer management
> operations. Enable runtime PM during probe and take/release references
> explicitly around these critical paths.
>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
> drivers/net/mhi_net.c | 39 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/net/mhi_net.c b/drivers/net/mhi_net.c
> index ae169929a9d8..5d7f9ccdb17b 100644
> --- a/drivers/net/mhi_net.c
> +++ b/drivers/net/mhi_net.c
> @@ -9,6 +9,7 @@
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
> #include <linux/netdevice.h>
> +#include <linux/pm_runtime.h>
> #include <linux/skbuff.h>
> #include <linux/u64_stats_sync.h>
>
> @@ -76,11 +77,19 @@ static netdev_tx_t mhi_ndo_xmit(struct sk_buff *skb, struct net_device *ndev)
> struct mhi_device *mdev = mhi_netdev->mdev;
> int err;
>
> + err = pm_runtime_get(&mdev->dev);
> + if (err < 0 && err != -EINPROGRESS) {
> + dev_err(&mdev->dev, "pm_runtime_get failed %d\n", err);
> + pm_runtime_put_noidle(&mdev->dev);
> + goto exit_drop;
> + }
> +
I am wondering what the value is in pushing this PM responsibility to
each individual MHI client driver and requiring every MHI operation to
be bracketed with runtime PM handling.
What does the client driver know here that the MHI core itself cannot
handle centrally? It feels like ensuring the controller is
runtime-active during transfer could be handled generically in the
framework instead of duplicating the same logic in every client.
> err = mhi_queue_skb(mdev, DMA_TO_DEVICE, skb, skb->len, MHI_EOT);
> if (unlikely(err)) {
> net_err_ratelimited("%s: Failed to queue TX buf (%d)\n",
> ndev->name, err);
> dev_kfree_skb_any(skb);
> + pm_runtime_put(&mdev->dev);
> goto exit_drop;
> }
>
> @@ -251,6 +260,7 @@ static void mhi_net_ul_callback(struct mhi_device *mhi_dev,
> /* MHI layer stopping/resetting the UL channel */
> if (mhi_res->transaction_status == -ENOTCONN) {
> u64_stats_update_end(&mhi_netdev->stats.tx_syncp);
> + pm_runtime_put(&mdev->dev);
> return;
> }
>
> @@ -261,6 +271,7 @@ static void mhi_net_ul_callback(struct mhi_device *mhi_dev,
> }
> u64_stats_update_end(&mhi_netdev->stats.tx_syncp);
>
> + pm_runtime_put(&mdev->dev);
> if (netif_queue_stopped(ndev) && !mhi_queue_is_full(mdev, DMA_TO_DEVICE))
> netif_wake_queue(ndev);
> }
> @@ -277,6 +288,12 @@ static void mhi_net_rx_refill_work(struct work_struct *work)
>
> size = mhi_netdev->mru ? mhi_netdev->mru : READ_ONCE(ndev->mtu);
>
> + err = pm_runtime_resume_and_get(&mdev->dev);
> + if (err) {
> + dev_err(&mdev->dev, "pm_runtime_resume_and_get failed %d\n", err);
> + return;
> + }
> +
> while (!mhi_queue_is_full(mdev, DMA_FROM_DEVICE)) {
> skb = netdev_alloc_skb(ndev, size);
> if (unlikely(!skb))
> @@ -296,6 +313,7 @@ static void mhi_net_rx_refill_work(struct work_struct *work)
> cond_resched();
> }
>
> + pm_runtime_put(&mdev->dev);
> /* If we're still starved of rx buffers, reschedule later */
> if (mhi_get_free_desc_count(mdev, DMA_FROM_DEVICE) == mhi_netdev->rx_queue_sz)
> schedule_delayed_work(&mhi_netdev->rx_refill, HZ / 2);
> @@ -362,20 +380,41 @@ static int mhi_net_probe(struct mhi_device *mhi_dev,
>
> SET_NETDEV_DEV(ndev, &mhi_dev->dev);
>
> + pm_runtime_no_callbacks(&mhi_dev->dev);
> + err = devm_pm_runtime_set_active_enabled(&mhi_dev->dev);
> + if (err)
> + return err;
> +
> + err = pm_runtime_resume_and_get(&mhi_dev->dev);
> + if (err) {
> + dev_err(&mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", err);
> + return err;
> + }
> +
> err = mhi_net_newlink(mhi_dev, ndev);
> if (err) {
> free_netdev(ndev);
> + pm_runtime_put(&mhi_dev->dev);
> return err;
> }
>
> + pm_runtime_put(&mhi_dev->dev);
> return 0;
> }
>
> static void mhi_net_remove(struct mhi_device *mhi_dev)
> {
> struct mhi_net_dev *mhi_netdev = dev_get_drvdata(&mhi_dev->dev);
> + int err;
> +
> + err = pm_runtime_resume_and_get(&mhi_dev->dev);
> + if (err)
> + dev_err(&mhi_dev->dev, "pm_runtime_resume_and_get failed %d\n", err);
>
> mhi_net_dellink(mhi_dev, mhi_netdev->ndev);
> +
> + if (!err)
> + pm_runtime_put(&mhi_dev->dev);
> }
>
> static const struct mhi_device_info mhi_hwip0 = {
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Claude review: bus: mhi: Fix broken runtime PM design
2026-05-22 10:00 [PATCH v2 0/6] bus: mhi: Fix broken runtime PM design Krishna Chaitanya Chundru
` (5 preceding siblings ...)
2026-05-22 10:00 ` [PATCH v2 6/6] bus: mhi: host: Fix runtime PM ownership between clients and controller Krishna Chaitanya Chundru
@ 2026-05-25 9:03 ` Claude Code Review Bot
6 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 9:03 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: bus: mhi: Fix broken runtime PM design
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Patches: 8
Reviewed: 2026-05-25T19:03:03.597455
---
This series attempts to fix a fundamental design flaw in MHI runtime PM by replacing controller-level callback indirection (`runtime_get/put`) with standard `pm_runtime_*` APIs, and pushing PM ownership from the MHI core down to client drivers via proper parent-child PM hierarchy. The design direction is correct and well-motivated.
However, the series has several issues ranging from inconsistent API usage across client drivers (patches 3-5) to a **dev_wake counter imbalance bug in patch 6** that could cause incorrect power state tracking. The inconsistent error handling between `mhi_wwan_ctrl` (which uses `pm_runtime_resume_and_get` and conditional cleanup) and `mhi_wwan_mbim` (which uses raw `pm_runtime_get` and unconditional cleanup) within the *same patch* suggests insufficient cross-checking.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
* Claude review: bus: mhi: Replace controller runtime_get/put callbacks with direct PM runtime APIs
2026-05-22 10:00 ` [PATCH v2 1/6] bus: mhi: Replace controller runtime_get/put callbacks with direct PM runtime APIs Krishna Chaitanya Chundru
@ 2026-05-25 9:03 ` Claude Code Review Bot
0 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 9:03 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Clean mechanical replacement. The change from `mhi_cntrl->runtime_get(mhi_cntrl)` / `mhi_cntrl->runtime_put(mhi_cntrl)` to `pm_runtime_get(mhi_cntrl->cntrl_dev)` / `pm_runtime_put(mhi_cntrl->cntrl_dev)` with `pm_runtime_mark_last_busy()` before each put is correct.
One minor concern: in `mhi_queue()`, the `pm_runtime_get()` (async variant) is used:
```c
- mhi_cntrl->runtime_get(mhi_cntrl);
+ pm_runtime_get(mhi_cntrl->cntrl_dev);
```
The original `mhi_pci_runtime_get` (removed in patch 2) also called `pm_runtime_get()`, so this is a faithful conversion. The comment above this call site documents the get/put balancing scheme for TX vs RX paths, which remains intact.
No issues found.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
* Claude review: bus: mhi: Drop controller runtime PM callback indirection
2026-05-22 10:00 ` [PATCH v2 2/6] bus: mhi: Drop controller runtime PM callback indirection Krishna Chaitanya Chundru
@ 2026-05-25 9:03 ` Claude Code Review Bot
0 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 9:03 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
Clean removal of the `runtime_get`/`runtime_put` function pointers from `struct mhi_controller` and all driver-side implementations. The removed implementations were either no-ops (qaic, ath11k, ath12k) or trivial wrappers (pci_generic).
The `mhi_pci_generic_edl_trigger` conversion is correct - it was the one remaining caller that went through the callbacks, and it's properly switched to direct API calls.
One observation: there's a stray blank line left behind in `ath11k/mhi.c`:
```c
-static void ath11k_mhi_op_runtime_put(struct mhi_controller *mhi_cntrl)
-{
-}
static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
```
This leaves a double blank line. Minor style nit.
No functional issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
* Claude review: net: mhi_net: Hold runtime PM during active data path operations
2026-05-22 10:00 ` [PATCH v2 3/6] net: mhi_net: Hold runtime PM during active data path operations Krishna Chaitanya Chundru
2026-05-22 20:09 ` Loic Poulain
@ 2026-05-25 9:03 ` Claude Code Review Bot
1 sibling, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 9:03 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Issue 1 - pm_runtime_put ordering in UL callback:**
```c
+ pm_runtime_put(&mdev->dev);
if (netif_queue_stopped(ndev) && !mhi_queue_is_full(mdev, DMA_TO_DEVICE))
netif_wake_queue(ndev);
```
The `pm_runtime_put` is placed *before* the `mhi_queue_is_full` check. While `mhi_queue_is_full` likely only inspects software ring state and doesn't touch hardware registers, it's better practice to put after all usage of `mdev` is complete. If autosuspend fires immediately (e.g., no autosuspend delay configured), the device state could change under you.
**Issue 2 - Asymmetric probe/remove PM handling:**
In probe, the pattern is get-then-put bracketing the initialization:
```c
+ err = pm_runtime_resume_and_get(&mhi_dev->dev);
...
err = mhi_net_newlink(mhi_dev, ndev);
...
+ pm_runtime_put(&mhi_dev->dev);
```
But in remove:
```c
+ err = pm_runtime_resume_and_get(&mhi_dev->dev);
+ if (err)
+ dev_err(...)
mhi_net_dellink(mhi_dev, mhi_netdev->ndev);
+ if (!err)
+ pm_runtime_put(&mhi_dev->dev);
```
The remove path proceeds with `mhi_net_dellink` even if `pm_runtime_resume_and_get` fails. This is reasonable since removal must complete, but the conditional `pm_runtime_put` only on success could leave the refcount unbalanced in practice. Consider using `pm_runtime_put_noidle` in the error case, or just always calling put since the device is being torn down anyway.
**Issue 3 - Noisy error logging:**
```c
+ err = pm_runtime_get(&mdev->dev);
+ if (err < 0 && err != -EINPROGRESS) {
+ dev_err(&mdev->dev, "pm_runtime_get failed %d\n", err);
```
Using `dev_err` for a runtime PM failure on the hot data path (xmit) can be very noisy. Consider `dev_err_ratelimited` or `dev_dbg`.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
* Claude review: net: qrtr: Hold runtime PM during active data path operations
2026-05-22 10:00 ` [PATCH v2 4/6] net: qrtr: " Krishna Chaitanya Chundru
@ 2026-05-25 9:03 ` Claude Code Review Bot
0 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 9:03 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Issue 1 - devm_kmalloc for RX buffers:**
This isn't introduced by this patch, but the PM changes interact with it: `qcom_mhi_qrtr_queue_dl_buffers` uses `devm_kmalloc` for DL buffers. These persist for the lifetime of the device and are never freed on the error path within this function. The PM wrapping is correct, but the underlying memory management concern remains.
**Issue 2 - PM in DL callback:**
```c
+ rc = pm_runtime_get(&qdev->mhi_dev->dev);
+ if (rc < 0 && rc != -EINPROGRESS) {
+ dev_err(&mhi_dev->dev, "pm_runtime_get failed %d\n", rc);
+ pm_runtime_put_noidle(&qdev->mhi_dev->dev);
+ return;
+ }
+
/* Done with the buffer, now recycle it for future use */
rc = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, ...);
```
The DL callback fires because data was received - meaning the device was necessarily active to deliver the interrupt/data. Taking a PM reference here to requeue the buffer is correct, but returning without requeuing the buffer on PM failure means that buffer is permanently lost from the RX ring. This will eventually starve the RX path. A better approach might be to attempt the requeue regardless (since the device clearly is active if the callback is running).
**Issue 3 - Same probe/remove asymmetry as patch 3.**
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
* Claude review: net: wwan: Hold runtime PM during active data path operations
2026-05-22 10:00 ` [PATCH v2 5/6] net: wwan: " Krishna Chaitanya Chundru
@ 2026-05-25 9:03 ` Claude Code Review Bot
0 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 9:03 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Issue 1 - Inconsistent PM API usage between the two drivers in the same patch:**
`mhi_wwan_ctrl` uses `pm_runtime_resume_and_get` (synchronous, proper error returns) throughout:
```c
+ ret = pm_runtime_resume_and_get(&mhi_dev->dev);
+ if (ret) {
```
But `mhi_wwan_mbim` uses raw `pm_runtime_get` (async) in several places:
```c
+ err = pm_runtime_get(&mhi_dev->dev);
+ if (err < 0 && err != -EINPROGRESS) {
```
This inconsistency within a single patch is concerning. The `mhi_wwan_ctrl` approach is generally more correct for non-atomic contexts like probe, remove, and workqueues. The xmit path is the only place where async `pm_runtime_get` is justified (because xmit might be in atomic context).
**Issue 2 - mhi_wwan_mbim rx_refill_work missing error handling:**
```c
+ err = pm_runtime_get(&mdev->dev);
+ if (err < 0 && err != -EINPROGRESS)
+ dev_err(&mdev->dev, "pm_runtime_get Failed %d\n", err);
+
while (!mhi_queue_is_full(mdev, DMA_FROM_DEVICE)) {
```
On PM failure, it logs an error but **proceeds into the loop anyway**, queueing buffers on a potentially suspended device. Compare with `mhi_wwan_ctrl_refill_work` which properly returns on error. This is a bug.
**Issue 3 - mhi_mbim_remove unconditional pm_runtime_put:**
```c
+ err = pm_runtime_get(&mhi_dev->dev);
+ if (err < 0 && err != -EINPROGRESS)
+ dev_err(...);
...
+ pm_runtime_put(&mhi_dev->dev);
```
The `pm_runtime_put` is called unconditionally, even if `pm_runtime_get` failed. Compare with `mhi_wwan_ctrl_stop` which conditionally puts only on success. If `pm_runtime_get` returned an error other than `-EINPROGRESS`, calling `pm_runtime_put` would decrement the usage count below zero.
**Issue 4 - mhi_mbim_probe leaks `mbim` allocation on PM failure:**
```c
mbim = kzalloc(sizeof(*mbim), GFP_KERNEL);
if (!mbim)
return -ENOMEM;
+ pm_runtime_no_callbacks(&mhi_dev->dev);
+ err = devm_pm_runtime_set_active_enabled(&mhi_dev->dev);
+ if (err)
+ return err;
```
If `devm_pm_runtime_set_active_enabled` fails, `mbim` is leaked (no `kfree`). The `mhi_wwan_ctrl` side handles this correctly by calling `kfree(mhiwwan)` on failure.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
* Claude review: bus: mhi: host: Fix runtime PM ownership between clients and controller
2026-05-22 10:00 ` [PATCH v2 6/6] bus: mhi: host: Fix runtime PM ownership between clients and controller Krishna Chaitanya Chundru
@ 2026-05-25 9:03 ` Claude Code Review Bot
0 siblings, 0 replies; 15+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 9:03 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the most complex patch and has the most concerning issues.
**Issue 1 (Bug) - dev_wake counter imbalance:**
`__mhi_device_get_sync` is modified to call `mhi_device_put` on error:
```c
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;
}
```
But `mhi_device_put` does `mhi_dev->dev_wake--` before calling `wake_put`. The matching increment `mhi_dev->dev_wake++` only exists in the **exported** `mhi_device_get_sync()`, not in the **internal** `__mhi_device_get_sync()`. When `__mhi_device_get_sync` is called directly (as in `mhi_pm_mission_mode_transition`), there's no `dev_wake++`, so calling `mhi_device_put` in the error path will decrement `dev_wake` below its original value, corrupting the counter.
Similarly, in `mhi_pm_mission_mode_transition`:
```c
error_mission_mode:
- mhi_cntrl->wake_put(mhi_cntrl, false);
- read_unlock_bh(&mhi_cntrl->pm_lock);
+ mhi_device_put(mhi_cntrl->mhi_dev);
```
This calls `mhi_device_put` to undo `__mhi_device_get_sync`, but again, `dev_wake--` in `mhi_device_put` has no matching `dev_wake++`.
**Fix suggestion:** Either create a new internal cleanup helper that does `wake_put + pm_runtime_put` without the `dev_wake--`, or explicitly do the `dev_wake++` in `__mhi_device_get_sync` to match.
**Issue 2 - Race window in __mhi_device_get_sync:**
```c
+ 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);
```
Between dropping the lock and reacquiring it after `pm_runtime_get_sync`, the PM state could transition to an error state. The code doesn't re-check for error state after reacquiring the lock before calling `wake_get`. If the device entered an error state during `pm_runtime_get_sync`, calling `wake_get` on a dead device could be problematic.
**Issue 3 - Lost wakeup event in mhi_device_put:**
The original `mhi_device_put` checked for suspend state and called `mhi_trigger_resume`, which includes `pm_wakeup_event`:
```c
- if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
- mhi_trigger_resume(mhi_cntrl);
```
This `pm_wakeup_event` path is now removed entirely. The assumption is that `pm_runtime_put` is sufficient, but `pm_wakeup_event` serves a different purpose (keeping the system awake, not just the device). If there are system suspend paths that depend on this wakeup event, removing it silently could cause regressions.
**Issue 4 - devm_pm_runtime_set_active_enabled return value ignored:**
```c
+ pm_runtime_no_callbacks(&mhi_cntrl->mhi_dev->dev);
+ devm_pm_runtime_set_active_enabled(&mhi_cntrl->mhi_dev->dev);
```
The return value of `devm_pm_runtime_set_active_enabled` is not checked. This function can fail (e.g., if `pm_runtime_set_active` or `devm_add_action_or_reset` fails). Compare with the client driver patches (3-5) which all check the return value. This should be checked and propagated.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2026-05-25 9:03 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-22 10:00 [PATCH v2 0/6] bus: mhi: Fix broken runtime PM design Krishna Chaitanya Chundru
2026-05-22 10:00 ` [PATCH v2 1/6] bus: mhi: Replace controller runtime_get/put callbacks with direct PM runtime APIs Krishna Chaitanya Chundru
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
2026-05-22 10:00 ` [PATCH v2 2/6] bus: mhi: Drop controller runtime PM callback indirection Krishna Chaitanya Chundru
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
2026-05-22 10:00 ` [PATCH v2 3/6] net: mhi_net: Hold runtime PM during active data path operations Krishna Chaitanya Chundru
2026-05-22 20:09 ` Loic Poulain
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
2026-05-22 10:00 ` [PATCH v2 4/6] net: qrtr: " Krishna Chaitanya Chundru
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
2026-05-22 10:00 ` [PATCH v2 5/6] net: wwan: " Krishna Chaitanya Chundru
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
2026-05-22 10:00 ` [PATCH v2 6/6] bus: mhi: host: Fix runtime PM ownership between clients and controller Krishna Chaitanya Chundru
2026-05-25 9:03 ` Claude review: " Claude Code Review Bot
2026-05-25 9:03 ` Claude review: bus: mhi: Fix broken runtime PM design Claude Code Review Bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox