[PATCH v2 0/2] wifi: ath11k/ath12k: Fix MHI leak on power up failure

[PATCH v2 0/2] wifi: ath11k/ath12k: Fix MHI leak on power up failure

[Vladimir Zapolskiy]

If MHI power up fails, for instance due to a missing firmware file, the
MHI device context and the RDDM image table allocated by
mhi_prepare_for_power_up() are leaked on probe error path in both ath11k
and ath12k drivers.

The fixes keep MHI prepared on the power up error path, so a firmware
crash dump collection in the reset work can still read the RDDM image,
and release it on driver unregistration or before MHI is prepared again.

Changes from v1 to v2:
* fixed double unprepare by introducing new "MHI prepared" flag (Baochen),
* added a similar ath12 memory leak fix.

Link to v1:
* https://lore.kernel.org/linux-wireless/20261028104336.1686334-1-vz@kernel.org/

Vladimir Zapolskiy (2):
  wifi: ath11k: Unprepare MHI on power up failure
  wifi: ath12k: Unprepare MHI on power up failure

 drivers/net/wireless/ath/ath11k/mhi.c |  9 ++++++++-
 drivers/net/wireless/ath/ath11k/pci.h |  1 +
 drivers/net/wireless/ath/ath12k/mhi.c | 14 ++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

-- 
2.51.0

[PATCH v2 1/2] wifi: ath11k: Unprepare MHI on power up failure

[Vladimir Zapolskiy]

If MHI power up fails, for instance due to a missing firmware file,
ath11k_core_init() does not power down the device on this error,
the MHI controller stays prepared, and the MHI device context and
the RDDM image table allocated by mhi_prepare_for_power_up() are
not released, here is a report from kmemleak:

  % cat /sys/kernel/debug/kmemleak
  unreferenced object 0xffff00079156a140 (size 64):
    comm "(udev-worker)", pid 595, jiffies 4294893781
    hex dump (first 32 bytes):
      00 d0 f1 80 00 80 ff ff 00 50 f1 80 00 80 ff ff  .........P......
      00 d0 f7 80 00 80 ff ff 00 30 fc ff 00 00 00 00  .........0......
    backtrace (crc 62662f0a):
      kmemleak_alloc+0x34/0x40
      __kmalloc_cache_noprof+0x224/0x3b8
      mhi_prepare_for_power_up+0x58/0x548 [mhi]
      ath11k_mhi_start+0x30/0xb8 [ath11k_pci]
      ath11k_pci_power_up+0xcc/0x188 [ath11k_pci]
      ath11k_core_init+0x8c/0x190 [ath11k]
      ath11k_pci_probe+0x4e0/0x870 [ath11k_pci]
      local_pci_probe+0x40/0xa8
      pci_call_probe+0x64/0x190
      pci_device_probe+0xac/0x138
      really_probe+0xbc/0x298
      __driver_probe_device+0x16c/0x19c
      driver_probe_device+0x3c/0x114
      __driver_attach+0x98/0x19c
      bus_for_each_dev+0x74/0xd0
      driver_attach+0x24/0x30
  unreferenced object 0xffff0007916fb800 (size 512):
    comm "(udev-worker)", pid 595, jiffies 4294893781
    hex dump (first 32 bytes):
      00 60 04 81 00 80 ff ff 0a 00 00 00 00 00 00 00  .`..............
      00 10 e0 82 00 80 ff ff 00 00 00 00 00 00 00 00  ................
    backtrace (crc 52e9e075):
      kmemleak_alloc+0x34/0x40
      __kmalloc_noprof+0x280/0x440
      mhi_alloc_bhie_table+0x54/0x168 [mhi]
      mhi_prepare_for_power_up+0x318/0x548 [mhi]
      ath11k_mhi_start+0x30/0xb8 [ath11k_pci]
      ath11k_pci_power_up+0xcc/0x188 [ath11k_pci]
      ath11k_core_init+0x8c/0x190 [ath11k]
      ath11k_pci_probe+0x4e0/0x870 [ath11k_pci]
      local_pci_probe+0x40/0xa8
      pci_call_probe+0x64/0x190
      pci_device_probe+0xac/0x138
      really_probe+0xbc/0x298
      __driver_probe_device+0x16c/0x19c
      driver_probe_device+0x3c/0x114
      __driver_attach+0x98/0x19c
      bus_for_each_dev+0x74/0xd0

Track the prepared state of MHI in a new ab_pci flag, and unprepare it
in ath11k_mhi_unregister() if it is still set. The prepared state is
intentionally kept on power up failure, because the RDDM image may still
be read by a firmware crash dump collection in the reset work. After a
failed power up in the reset or resume path the device stays bound, and
ath11k_mhi_stop() releases the resources on the next power down, as
before.

Also unprepare MHI in ath11k_mhi_stop() only if the flag is set. If
mhi_prepare_for_power_up() fails in the reset or resume path, it
releases the device context on its own error path, and the unconditional
mhi_unprepare_after_power_down() in a later ath11k_mhi_stop() frees the
command ring buffers again and dereferences the NULL device context.

Fixes: 3df6d74aedfd ("ath11k: mhi: use mhi_sync_power_up()")
Signed-off-by: Vladimir Zapolskiy <vz@kernel.org>
---
 drivers/net/wireless/ath/ath11k/mhi.c | 9 ++++++++-
 drivers/net/wireless/ath/ath11k/pci.h | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
index a6c9ff112c68..7cc652292d6f 100644
--- a/drivers/net/wireless/ath/ath11k/mhi.c
+++ b/drivers/net/wireless/ath/ath11k/mhi.c
@@ -425,6 +425,10 @@ void ath11k_mhi_unregister(struct ath11k_pci *ab_pci)
 {
 	struct mhi_controller *mhi_ctrl = ab_pci->mhi_ctrl;
 
+	/* MHI stays prepared after a failed power up */
+	if (test_and_clear_bit(ATH11K_PCI_MHI_PREPARED, &ab_pci->flags))
+		mhi_unprepare_after_power_down(mhi_ctrl);
+
 	mhi_unregister_controller(mhi_ctrl);
 	kfree(mhi_ctrl->irq);
 	mhi_free_controller(mhi_ctrl);
@@ -443,6 +447,8 @@ int ath11k_mhi_start(struct ath11k_pci *ab_pci)
 		return ret;
 	}
 
+	set_bit(ATH11K_PCI_MHI_PREPARED, &ab_pci->flags);
+
 	ret = mhi_sync_power_up(ab_pci->mhi_ctrl);
 	if (ret) {
 		ath11k_warn(ab, "failed to power up mhi: %d", ret);
@@ -463,7 +469,8 @@ void ath11k_mhi_stop(struct ath11k_pci *ab_pci, bool is_suspend)
 	else
 		mhi_power_down(ab_pci->mhi_ctrl, true);
 
-	mhi_unprepare_after_power_down(ab_pci->mhi_ctrl);
+	if (test_and_clear_bit(ATH11K_PCI_MHI_PREPARED, &ab_pci->flags))
+		mhi_unprepare_after_power_down(ab_pci->mhi_ctrl);
 }
 
 int ath11k_mhi_suspend(struct ath11k_pci *ab_pci)
diff --git a/drivers/net/wireless/ath/ath11k/pci.h b/drivers/net/wireless/ath/ath11k/pci.h
index 1e3005a4b64c..1e811245c1a5 100644
--- a/drivers/net/wireless/ath/ath11k/pci.h
+++ b/drivers/net/wireless/ath/ath11k/pci.h
@@ -55,6 +55,7 @@
 
 enum ath11k_pci_flags {
 	ATH11K_PCI_ASPM_RESTORE,
+	ATH11K_PCI_MHI_PREPARED,
 };
 
 struct ath11k_pci {
-- 
2.51.0

[PATCH v2 2/2] wifi: ath12k: Unprepare MHI on power up failure

[Vladimir Zapolskiy]

If MHI power up fails, ath12k_mhi_start() returns with MHI still left in
the ATH12K_MHI_INIT state, and on this error ath12k_core_soc_create()
frees the MHI controller data, but does not power down the device and
does not free the MHI device context and the RDDM image table allocated
earlier by mhi_prepare_for_power_up(), this is reported by kmemleak:

  unreferenced object 0xffff00081c048100 (size 64):
    comm "(udev-worker)", pid 556, jiffies 4294893304
    hex dump (first 32 bytes):
      00 50 1d 80 00 80 ff ff 00 d0 df 80 00 80 ff ff  .P..............
      00 d0 05 81 00 80 ff ff 00 d0 fd ff 00 00 00 00  ................
    backtrace (crc 87372616):
      kmemleak_alloc+0x3c/0x50
      __kmalloc_cache_noprof+0x248/0x430
      mhi_prepare_for_power_up+0x60/0x558 [mhi]
      ath12k_mhi_start+0x44/0x1c8 [ath12k]
      ath12k_pci_power_up+0x1f4/0x398 [ath12k]
      ath12k_core_init+0x128/0x510 [ath12k]
      ath12k_pci_probe+0x814/0xe30 [ath12k]
      local_pci_probe+0x48/0xb8
      pci_call_probe+0x6c/0x1a0
      pci_device_probe+0xb4/0x148
      really_probe+0xc4/0x2a8
      __driver_probe_device+0x174/0x1b0
      driver_probe_device+0x44/0x128
      __driver_attach+0xa0/0x1b0
      bus_for_each_dev+0x7c/0xe8
      driver_attach+0x2c/0x40

After a failed power up in the reset or resume path the device stays
bound, but ath12k_pci_power_down() does not stop MHI, because
ATH12K_PCI_FLAG_INIT_DONE is not set. So the resources are leaked on
removal as well, and the stale ATH12K_MHI_INIT state bit makes every
later ath12k_mhi_start() fail until the device is rebound.

Release a prepared but not powered up MHI controller in
ath12k_mhi_unregister() and before preparing it again in
ath12k_mhi_start(). It is intentionally not released on the power up
error path, because the RDDM image may still be read by a firmware
crash dump collection in the reset work.

Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
Signed-off-by: Vladimir Zapolskiy <vz@kernel.org>
---
 drivers/net/wireless/ath/ath12k/mhi.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/wireless/ath/ath12k/mhi.c b/drivers/net/wireless/ath/ath12k/mhi.c
index ee87f00bc5de..1b8d47eb5984 100644
--- a/drivers/net/wireless/ath/ath12k/mhi.c
+++ b/drivers/net/wireless/ath/ath12k/mhi.c
@@ -276,10 +276,22 @@ int ath12k_mhi_register(struct ath12k_pci *ab_pci)
 	return ret;
 }
 
+/* MHI stays prepared after a failed power up, release it here */
+static void ath12k_mhi_unprepare_stale(struct ath12k_pci *ab_pci)
+{
+	if (!test_bit(ATH12K_MHI_INIT, &ab_pci->mhi_state) ||
+	    test_bit(ATH12K_MHI_POWER_ON, &ab_pci->mhi_state))
+		return;
+
+	mhi_unprepare_after_power_down(ab_pci->mhi_ctrl);
+	clear_bit(ATH12K_MHI_INIT, &ab_pci->mhi_state);
+}
+
 void ath12k_mhi_unregister(struct ath12k_pci *ab_pci)
 {
 	struct mhi_controller *mhi_ctrl = ab_pci->mhi_ctrl;
 
+	ath12k_mhi_unprepare_stale(ab_pci);
 	mhi_unregister_controller(mhi_ctrl);
 	kfree(mhi_ctrl->irq);
 	mhi_free_controller(mhi_ctrl);
@@ -485,6 +497,8 @@ int ath12k_mhi_start(struct ath12k_pci *ab_pci)
 
 	ab_pci->mhi_ctrl->timeout_ms = MHI_TIMEOUT_DEFAULT_MS;
 
+	ath12k_mhi_unprepare_stale(ab_pci);
+
 	ret = ath12k_mhi_set_state(ab_pci, ATH12K_MHI_INIT);
 	if (ret)
 		goto out;
-- 
2.51.0