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

[Jeff Johnson <jeff.johnson@oss.qualcomm.com>]

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.
> 
>>
>>>                              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)