RE: [RFC Patch net-next v2 4/8] r8169: add support for new interrupt mapping

[Javen <javen_xu@realsil.com.cn>]

>On 29/04/2026 08:07, javen wrote:
>> From: Javen Xu <javen_xu@realsil.com.cn>
>>
>> To support RSS, the number of hardware interrupt bits should match the
>> interrupt of software. So we add support for new interrupt mapping here.
>> ISR_VER_MAP_REG is the hardware register to indicate interrupt status.
>> IMR_SET_VEC_MAP_REG is interrupt mask which is set to enable irq.
>>
>> Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
>
>[...]
>
>>
>>               napi = &tp->r8169napi[i];
>>               snprintf(irq->name, len, "%s-%d", dev->name, i); @@
>> -5664,10 +5717,17 @@ static const struct net_device_ops rtl_netdev_ops
>> = {
>>
>>   static void rtl_set_irq_mask(struct rtl8169_private *tp)
>>   {
>> -     tp->irq_mask = RxOK | RxErr | TxOK | TxErr | LinkChg;
>> +     if (tp->features & RTL_VEC_MAP_ENABLE) {
>> +             tp->irq_mask = ISRIMR_LINKCHG;
>> +             tp->irq_mask |= ISRIMR_TOK_Q0;
>
>       nit: you can set it in one line
>
>> +             for (int i = 0; i < tp->num_rx_rings; i++)
>> +                     tp->irq_mask |= ISRIMR_ROK_Q0 << i;
>> +     } else {
>> +             tp->irq_mask = RxOK | RxErr | TxOK | TxErr | LinkChg;
>>
>> -     if (tp->mac_version <= RTL_GIGA_MAC_VER_06)
>> -             tp->irq_mask |= SYSErr | RxFIFOOver;
>> +             if (tp->mac_version <= RTL_GIGA_MAC_VER_06)
>> +                     tp->irq_mask |= SYSErr | RxFIFOOver;
>> +     }
>>   }
>>
>>   static int rtl_alloc_irq(struct rtl8169_private *tp) @@ -5695,6
>> +5755,16 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
>>       if (nvecs < 0)
>>               nvecs = pci_alloc_irq_vectors(pdev, 1, 1,
>> PCI_IRQ_ALL_TYPES);
>>
>> +     tp->features &= ~RTL_VEC_MAP_ENABLE;
>> +
>> +     if (nvecs > 0) {
>> +             tp->irq_nvecs = nvecs;
>> +             tp->irq = pci_irq_vector(pdev, 0);
>> +             if (nvecs > 1)
>> +                     tp->features |= RTL_VEC_MAP_ENABLE;
>> +             return 0;
>> +     }
>> +
>>       tp->irq = pdev->irq;
>>       tp->irq_nvecs = 1;
>
>now these 2 lines are not needed, because in success they are never executed,
>but in error path they provide wrong information.
>
>the whole can be rewritten with error path in case both tries of
>pci_alloc_irq_vectors failed and the common code for success path:
>
>        if (nvecs < 0)
>                nvecs = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
>
>        if (nvecs < 0)
>                return nvecs;
>
>        tp->irq_nvecs = nvecs;
>        tp->irq = pci_irq_vector(pdev, 0);
>
>        if (nvecs > 1)
>                tp->features |= RTL_VEC_MAP_ENABLE;
>
>        return 0;
>
>>
>> @@ -5965,6 +6035,53 @@ static bool rtl_aspm_is_safe(struct
>rtl8169_private *tp)
>>       return false;
>>   }
>>
>> +static int rtl8169_poll_msix_rx(struct napi_struct *napi, int budget)
>> +{
>> +     struct rtl8169_napi *r8169_napi = container_of(napi, struct rtl8169_napi,
>napi);
>> +     struct rtl8169_private *tp = r8169_napi->priv;
>> +     struct net_device *dev = tp->dev;
>> +     const int message_id = r8169_napi->index;
>
>reverse xmass tree, please
>
>> +     int work_done = 0;
>> +
>> +     if (message_id < tp->num_rx_rings)
>> +             work_done += rtl_rx(dev, tp, &tp->rx_ring[message_id],
>> + budget);
>> +
>> +     if (work_done < budget && napi_complete_done(napi, work_done))
>> +             rtl8169_enable_hw_interrupt_msix(tp, message_id);
>> +
>> +     return work_done;
>> +}
>> +
>> +static int rtl8169_poll_msix_tx(struct napi_struct *napi, int budget)
>> +{
>> +     struct rtl8169_napi *r8169_napi = container_of(napi, struct rtl8169_napi,
>napi);
>> +     struct rtl8169_private *tp = r8169_napi->priv;
>> +     struct net_device *dev = tp->dev;
>> +     unsigned int work_done = 0;
>> +     const int message_id = r8169_napi->index;
>> +     int tx_ring_idx = message_id - 8;
>
>ditto
>
>> +
>> +     if (tx_ring_idx >= 0)
>> +             rtl_tx(dev, tp, budget);
>> +
>> +     if (work_done < budget && napi_complete_done(napi, work_done))
>> +             rtl8169_enable_hw_interrupt_msix(tp, message_id);
>> +
>> +     return work_done;
>> +}
>> +
>> +static int rtl8169_poll_msix_other(struct napi_struct *napi, int
>> +budget) {
>> +     struct rtl8169_napi *r8169_napi = container_of(napi, struct rtl8169_napi,
>napi);
>> +     struct rtl8169_private *tp = r8169_napi->priv;
>> +     const int message_id = r8169_napi->index;
>> +
>> +     napi_complete_done(napi, budget);
>> +     rtl8169_enable_hw_interrupt_msix(tp, message_id);
>> +
>> +     return 1;
>> +}
>> +
>>   static void r8169_init_napi(struct rtl8169_private *tp)
>>   {
>>       for (int i = 0; i < tp->irq_nvecs; i++) { @@ -5972,6 +6089,20 @@
>> static void r8169_init_napi(struct rtl8169_private *tp)
>>               int (*poll)(struct napi_struct *napi, int budget);
>>
>>               poll = rtl8169_poll;
>> +             if (tp->features & RTL_VEC_MAP_ENABLE) {
>> +                     switch (tp->hw_curr_isr_ver) {
>> +                     case 6:
>> +                             if (i < R8127_MAX_RX_QUEUES)
>> +                                     poll = rtl8169_poll_msix_rx;
>> +                             else if (i > 7 && i < 16)
>
>        magic constants?
>
>> +                                     poll = rtl8169_poll_msix_tx;
>> +                             else
>> +                                     poll = rtl8169_poll_msix_other;
>> +                             break;
>> +                     default:
>> +                             break;
>> +                     }
>> +             }
>>               netif_napi_add(tp->dev, &r8169napi->napi, poll);
>>               r8169napi->priv = tp;
>>               r8169napi->index = i;

Thanks for your review. I have applied all the comments locally. Once the ongoing discussion concludes, I will send the next version.

BRs,
Javen