From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: net: mhi_net: Hold runtime PM during active data path operations
Date: Mon, 25 May 2026 19:03:04 +1000
Message-ID: <review-patch3-20260522-mhi_runtimepm-v2-3-fbebf41a82bb@oss.qualcomm.com>
In-Reply-To: <20260522-mhi_runtimepm-v2-3-fbebf41a82bb@oss.qualcomm.com>
References: <20260522-mhi_runtimepm-v2-0-fbebf41a82bb@oss.qualcomm.com> <20260522-mhi_runtimepm-v2-3-fbebf41a82bb@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

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