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

[Aaradhana Sahu]

On 10/10/2026 2:11 AM, Jeff Johnson wrote:
> On 10/8/2026 8:38 PM, Aaradhana Sahu wrote:
>> On 10/8/2026 10:46 PM, Rameshkumar Sundaram wrote:
>>> On 10/1/2026 10:24 AM, Aaradhana Sahu wrote:
> 
>>>> @@ -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?
> 
> Yes, please handle as a separate patch.
> 
> My review agent notes:
> 
> There are three platform_get_irq_byname() call sites in ahb.c:
> 
>   ┌──────┬────────────────────────────────┬─────────────────────────────────┐
>   │ Line │            Location            │      Checks return value?       │
>   ├──────┼────────────────────────────────┼─────────────────────────────────┤
>   │ 692  │ ext IRQ loop                   │ No                              │
>   │      │ (ath12k_ahb_config_ext_irq)    │                                 │
>   ├──────┼────────────────────────────────┼─────────────────────────────────┤
>   │ 730  │ CE IRQ loop                    │ No                              │
>   │      │ (ath12k_ahb_config_irq)        │                                 │
>   ├──────┼────────────────────────────────┼─────────────────────────────────┤
>   │      │                                │ Yes — if                        │
>   │ 819  │ userpd IRQ                     │ (ab_ahb->userpd_irq_num[i] < 0) │
>   │      │                                │  return ...                     │
>   └──────┴────────────────────────────────┴─────────────────────────────────┘
> 
> So the driver is already inconsistent: userpd checks, CE and ext don't.
> 
> That said, the ext and CE sites aren't silently broken — devm_request_irq()
> with a negative IRQ number hits irq_to_desc(), gets NULL, and returns -EINVAL,
> which the existing if (ret) check catches. The practical impact is a
> confusing error message ("failed request_irq") when the real failure was
> platform_get_irq_byname() itself.
> 
> And further suggest it should be a separate patch. The reasons:
> 
> 1. This bug is pre-existing at both line 692 and line 730 — it wasn't
> introduced by the NAPI leak patch. Adding the check to only line 692 (as the
> reviewer suggests) leaves line 730 unfixed and makes the code more
> inconsistent than before.
> 2. A correct fix needs to cover both sites together, with a consistent error
> message and error-path handling.
> 3. This patch already has a well-defined scope (NAPI cleanup). Adding
> unrelated error-handling hygiene muddies the Fixes: attribution and makes it
> harder to backport.

Thanks for the clarification. I'll address the IRQ retrieval error handling 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)