RE: [Patch net-next v6 1/7] r8169: add support for multi irqs

[Javen]

>On Tue, 26 May 2026 16:11:11 +0800 javen wrote:
>> @@ -4820,7 +4838,7 @@ static int rtl_rx(struct net_device *dev, struct
>rtl8169_private *tp, int budget
>>                       goto release_descriptor;
>>               }
>>
>> -             skb = napi_alloc_skb(&tp->napi, pkt_size);
>> +             skb = napi_alloc_skb(&tp->rtl8169_napi[0], pkt_size);
>
>the caller is the NAPI poll function, you should pass that NAPI as arg to rtl_rx()
>already instead of hardcoding [0] in this patch.
>
>>               if (unlikely(!skb)) {
>>                       dev->stats.rx_dropped++;
>>                       goto release_descriptor; @@ -4844,7 +4862,7 @@
>> static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
>>               if (skb->pkt_type == PACKET_MULTICAST)
>>                       dev->stats.multicast++;
>>
>> -             napi_gro_receive(&tp->napi, skb);
>> +             napi_gro_receive(&tp->rtl8169_napi[0], skb);
>>
>>               dev_sw_netstats_rx_add(dev, pkt_size);
>>  release_descriptor:
>
>> +static int rtl8169_set_real_num_queues(struct rtl8169_private *tp) {
>> +     int ret;
>> +
>> +     ret = netif_set_real_num_tx_queues(tp->dev, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return netif_set_real_num_rx_queues(tp->dev, tp->num_rx_rings);
>
>netif_set_real_num_queues() exists, just call it directly instead of adding your
>own helper.
>
>> +}
>> +
>>  static int rtl_jumbo_max(struct rtl8169_private *tp)  {
>>       /* Non-GBit versions don't support jumbo frames */ @@ -5599,6
>> +5669,22 @@ static bool rtl_aspm_is_safe(struct rtl8169_private *tp)
>>       return false;
>>  }
>>
>> +static void r8169_del_napi_action(void *data) {
>> +     struct rtl8169_private *tp = data;
>> +     int i;
>> +
>> +     for (i = 0; i < tp->irq_nvecs; i++)
>> +             netif_napi_del(&tp->rtl8169_napi[i]);
>> +}
>> +
>> +static void r8169_init_napi(struct rtl8169_private *tp) {
>> +     for (int i = 0; i < tp->irq_nvecs; i++)
>> +             netif_napi_add(tp->dev, &tp->rtl8169_napi[i], rtl8169_poll);
>> +     devm_add_action_or_reset(&tp->pci_dev->dev,
>> +r8169_del_napi_action, tp);
>
>devm_add_action_or_reset() can fail (as the AI bots point out) but this whole
>devm_ dance is entirely unnecessary networking stack will automatically
>delete NAPI instances when device is unregistered.

Thanks for your review.

In patch v3, link: https://lore.kernel.org/netdev/20260513115543.1730-2-javen_xu@xxxxxxxxxxxxxx/
I tried to alloc struct rtl8169_napi dynamically for saving memory according to Heiner's suggestion. I agree with his suggestion because only 8127 rss are enabled.
And in this ai review, link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260520031603.700-1-javen_xu%40realsil.com.cn
AI suggested that the lifetime of this devm_kcalloc'd napi array may be compatible with the netdev's napi list. So I add devm_add_action_or_reset in patch v6.
I checked the code and agree that the stack auto-deletes NAPI instances in free_netdev() -> netdev_napi_exit(). However, because devres releases resources in LIFO order:
1. kfree for the NAPI array (allocated via devm_kcalloc) will be called first.
2. free_netdev() (registered via devm_alloc_etherdev) will be called second
When free_netdev() calls netdev_napi_exit() to iterate over dev->napi_list, the NAPI memory has already been freed by devm, which will cause a Use-After-Free. That's why I added the devm action to explicitly remove it before the memory is freed.

So I wanna know what should I do? Whether keep the action in this patch(dynamically allocate napi array) or patch v2(fix the array size), or any other suggestion will be apperaciated.

BRs,
Javen