Re: [PATCH] wifi: ath11k: Unprepare MHI on power up failure

[Vladimir Zapolskiy <vz@kernel.org>]

Hi Baochen.

On 10/29/26 07:38, Baochen Qiang wrote:
> 
> 
> On 10/28/2026 6:43 PM, Vladimir Zapolskiy wrote:
>> If MHI power up fails, for instance it happens due to a missing firmware,
>> ath11k_mhi_start() returns without undoing mhi_prepare_for_power_up().
>> Then the failed probe or power up unwinding frees the MHI controller,
>> but the MHI device context and the RDDM image table allocated by
>> mhi_prepare_for_power_up() are leaked along with their DMA buffers:
>>
>>    % 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
>>
>> The MHI device is already powered down by mhi_sync_power_up() on
>> failure, but mhi_unprepare_after_power_down() still has to be
>> called on error path to release the allocated resources.
>>
>> Fixes: 3df6d74aedfd ("ath11k: mhi: use mhi_sync_power_up()")
>> Signed-off-by: Vladimir Zapolskiy <vz@kernel.org>
>> ---
>>   drivers/net/wireless/ath/ath11k/mhi.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
>> index a6c9ff112c68..76c194573ec7 100644
>> --- a/drivers/net/wireless/ath/ath11k/mhi.c
>> +++ b/drivers/net/wireless/ath/ath11k/mhi.c
>> @@ -446,6 +446,7 @@ int ath11k_mhi_start(struct ath11k_pci *ab_pci)
>>   	ret = mhi_sync_power_up(ab_pci->mhi_ctrl);
>>   	if (ret) {
>>   		ath11k_warn(ab, "failed to power up mhi: %d", ret);
>> +		mhi_unprepare_after_power_down(ab_pci->mhi_ctrl);
> 
> For the probe path itself this is fine: when probe fails the driver is not bound, so no
> remove or shutdown follows and this is the only unprepare.
> 
> The problem is that in runtime, ath11k_hif_power_up() is also called from reset and resume:
> 
> reset:
> 	ath11k_core_reset() {
> 		...
> 		ath11k_hif_power_up()
> 	}
> 
> resume:
> 	ath11k_core_resume_early() {
> 		...
> 		ath11k_hif_power_up()
> 	}
> 
> The later teardown in rmmod path or shutdown path then calls power_down unconditionally,
> regardless of whether the last power_up succeeded:
> 
> rmmod:
>    mhi_unprepare_after_power_down()   <- 2nd call
>    ath11k_mhi_stop()
>    ath11k_pci_power_down()
>    ath11k_hif_power_down()
>    ath11k_core_deinit()
>    ath11k_pci_remove()
> 
> shutdown:
>    mhi_unprepare_after_power_down()   <- 2nd call
>    ath11k_mhi_stop()
>    ath11k_pci_power_down()
>    ath11k_pci_shutdown()
> 
> so the unprepare is called twice, while in the second call we can hit NULL pointer
> dereference or double free ...
> 
> This comes from the fact the existing code does not handle power up failure correctly in
> reset and resume. I don't see a proper solution to this, maybe we need a new flag telling
> us whether the unprepare has been done already ?

Thank you for your review!

Due to my check the analysis is correct, and apparently the proposed solution
will work also, but I'd prefer to introduce a "prepared" flag set right after
mhi_sync_power_up() successful return rather than to toggle "unprepared" flag.

By the way, there is a memory leak on error path in ath12 driver also, but
the rootcause and its solution are different, there is a missing state set to
ATH12K_MHI_DEINIT, if ATH12K_MHI_POWER_ON fails in ath12k_mhi_start().

After the testing I'll prepare a changeset including both fixes for review.

-- 
Best wishes,
Vladimir