Re: [PATCH ath-next v2] wifi: ath12k: Fix NAPI resource leaks in AHB external IRQ paths

[Aaradhana Sahu <aaradhana.sahu@oss.qualcomm.com>]

On 10/8/2026 10:46 PM, Rameshkumar Sundaram wrote:
> On 10/1/2026 10:24 AM, Aaradhana Sahu wrote:
>> Currently, NAPI instances and their associated netdevs allocated for AHB
>> external IRQ groups are not released when external IRQ configuration
>> fails. They are also not freed when arch_init() fails after
>> ath12k_ahb_config_irq() succeeds during probe, or when the driver is
>> unloaded.
>>
>> Fix this by adding a common cleanup helper to release the NAPI instances
>> and associated netdevs. Call it from the external IRQ configuration error
>> path, probe error path, and driver cleanup path.
>>
>> Tested-on: IPQ5332 hw1.0 AHB WLAN.WBE.1.6-01275-QCAHKSWPL_SILICONZ-1
>>
>> Fixes: 6cee30f0da75 ("wifi: ath12k: add AHB driver support for IPQ5332")
>> Fixes: 614c23e24ee8 ("wifi: ath12k: Support arch-specific DP device allocation")
>> Signed-off-by: Aaradhana Sahu <aaradhana.sahu@oss.qualcomm.com>
>> ---
>> v2:
>>    -Remove unnecessary irq_grp->napi_ndev NULL check.
>> ---
>>   drivers/net/wireless/ath/ath12k/ahb.c | 41 ++++++++++++++++++++++++---
>>   1 file changed, 37 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/ahb.c b/drivers/net/wireless/ath/ath12k/ahb.c
>> index 85360365aa4b..9b77632bd1bf 100644
>> --- a/drivers/net/wireless/ath/ath12k/ahb.c
>> +++ b/drivers/net/wireless/ath/ath12k/ahb.c
>> @@ -191,6 +191,26 @@ static void __ath12k_ahb_ext_irq_disable(struct ath12k_base *ab)
>>       }
>>   }
>>   +static void ath12k_ahb_free_napi_for_group(struct ath12k_ext_irq_grp *irq_grp)
>> +{
>> +    netif_napi_del(&irq_grp->napi);
>> +    free_netdev(irq_grp->napi_ndev);
>> +    irq_grp->napi_ndev = NULL;
>> +}
>> +
>> +static void ath12k_ahb_free_napi(struct ath12k_base *ab, int num_ext_irq_grp)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < num_ext_irq_grp; i++)
>> +        ath12k_ahb_free_napi_for_group(&ab->ext_irq_grp[i]);
>> +}
>> +
>> +static void ath12k_ahb_free_ext_napi(struct ath12k_base *ab)
>> +{
>> +    ath12k_ahb_free_napi(ab, ATH12K_EXT_IRQ_GRP_NUM_MAX);
>> +}
>> +
>>   static void ath12k_ahb_ext_grp_enable(struct ath12k_ext_irq_grp *irq_grp)
>>   {
>>       int i;
>> @@ -622,8 +642,10 @@ static int ath12k_ahb_config_ext_irq(struct ath12k_base *ab)
>>           irq_grp->grp_id = i;
>>             irq_grp->napi_ndev = alloc_netdev_dummy(0);
>> -        if (!irq_grp->napi_ndev)
>> -            return -ENOMEM;
>> +        if (!irq_grp->napi_ndev) {
>> +            ret = -ENOMEM;
>> +            goto free_napi_ndev;
>> +        }
>>             netif_napi_add(irq_grp->napi_ndev, &irq_grp->napi,
>>                      ath12k_ahb_ext_grp_napi_poll);
>> @@ -675,12 +697,19 @@ static int ath12k_ahb_config_ext_irq(struct ath12k_base *ab)
> 
> 
> platform_get_irq_byname() can return a negative error code. Should we handle that explicitly before calling devm_request_irq()?
> 
> Something like:
> 
>  irq = platform_get_irq_byname(ab->pdev, irq_name[irq_idx]);
>   if (irq < 0) {
>     ath12k_warn(ab, "failed to retrieve irq number for %s\n", irq_name[irq_idx]);
>         ret = irq;
>         ath12k_ahb_free_napi_for_group(irq_grp);
>         goto free_napi_ndev;
>   }
> 

You're right, thanks for pointing this out. This patch focuses on fixing the NAPI resource leaks.
Since handling the IRQ retrieval error is an existing issue, would it be okay to address it in a
separate patch?

> 
>>                              ath12k_ahb_ext_interrupt_handler,
>>                              IRQF_TRIGGER_RISING,
>>                              irq_name[irq_idx], irq_grp);
>> -            if (ret)
>> +            if (ret) {
>>                   ath12k_warn(ab, "failed request_irq for %d\n", irq);
>> +                ath12k_ahb_free_napi_for_group(irq_grp);
>> +                goto free_napi_ndev;
>> +            }
>>           }
>>       }
>>         return 0;
>> +
>> +free_napi_ndev:
>> +    ath12k_ahb_free_napi(ab, i);
>> +    return ret;
>>   }
>>     static int ath12k_ahb_config_irq(struct ath12k_base *ab)
>> @@ -1353,7 +1382,7 @@ static int ath12k_ahb_probe(struct platform_device *pdev)
>>       ret = ab_ahb->device_family_ops->arch_init(ab);
>>       if (ret) {
>>           ath12k_err(ab, "AHB arch_init failed %d\n", ret);
>> -        goto err_rproc_deconfigure;
>> +        goto err_free_irq;
>>       }
>>         ret = ath12k_core_init(ab);
>> @@ -1367,6 +1396,9 @@ static int ath12k_ahb_probe(struct platform_device *pdev)
>>   err_deinit_arch:
>>       ab_ahb->device_family_ops->arch_deinit(ab);
>>   +err_free_irq:
>> +    ath12k_ahb_free_ext_napi(ab);
>> +
>>   err_rproc_deconfigure:
>>       mutex_lock(&ath12k_rproc_info_lock);
>>       ath12k_ahb_deconfigure_rproc(ab);
>> @@ -1409,6 +1441,7 @@ static void ath12k_ahb_free_resources(struct ath12k_base *ab)
>>       struct platform_device *pdev = ab->pdev;
>>       struct ath12k_ahb *ab_ahb = ath12k_ab_to_ahb(ab);
>>   +    ath12k_ahb_free_ext_napi(ab);
>>       ath12k_hal_srng_deinit(ab);
>>       ath12k_ce_free_pipes(ab);
>>       ath12k_ahb_resource_deinit(ab);
>>
>> base-commit: b64812cb6cec118f6c59a4ec2bd4848db17f6bb2
> 
>