RE: [Patch net-next v3 3/7] r8169: add support for new interrupt mapping

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

>On 13.05.2026 13:55, 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>
>> ---
>> Changes in v2:
>>  - no changes
>>
>> Changes in v3:
>>  - init index in napi_struct and get message_id from index
>>  - move rtl8169_disable_hw_interrupt_msix directly before the call to
>>    napi_schedule()
>>  - change the condition in rtl8169_request_irq when RTL_VEC_MAP_ENABLE
>>    enabled, use rtl8169_interrupt_msix
>> ---
>>  drivers/net/ethernet/realtek/r8169_main.c | 165
>> ++++++++++++++++++++--
>>  1 file changed, 151 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
>> b/drivers/net/ethernet/realtek/r8169_main.c
>> index 9dab0fbcca61..f259cc0cee37 100644
>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>> @@ -78,6 +78,7 @@
>>  #define R8169_TX_STOP_THRS   (MAX_SKB_FRAGS + 1)
>>  #define R8169_TX_START_THRS  (2 * R8169_TX_STOP_THRS)  #define
>> R8169_MAX_RX_QUEUES  8
>> +#define R8127_MAX_TX_QUEUES  8
>>  #define R8169_MAX_MSIX_VEC   32
>>  #define R8127_MAX_RX_QUEUES  8
>>  #define R8169_DEFAULT_RX_QUEUES      1
>> @@ -451,8 +452,13 @@ enum rtl8125_registers {
>>       RSS_CTRL_8125           = 0x4500,
>>       Q_NUM_CTRL_8125         = 0x4800,
>>       EEE_TXIDLE_TIMER_8125   = 0x6048,
>> +     IMR_CLEAR_VEC_MAP_REG   = 0x0d00,
>> +     ISR_VEC_MAP_REG         = 0x0d04,
>> +     IMR_SET_VEC_MAP_REG     = 0x0d0c,
>>  };
>>
>> +#define MSIX_ID_VEC_MAP_LINKCHG      29
>> +#define RTL_VEC_MAP_ENABLE   BIT(0)
>>  #define LEDSEL_MASK_8125     0x23f
>>
>>  #define RX_VLAN_INNER_8125   BIT(22)
>> @@ -583,6 +589,9 @@ enum rtl_register_content {
>>
>>       /* magic enable v2 */
>>       MagicPacket_v2  = (1 << 16),    /* Wake up when receives a Magic Packet
>*/
>> +#define      ISRIMR_LINKCHG  BIT(29)
>> +#define      ISRIMR_TOK_Q0   BIT(8)
>> +#define      ISRIMR_ROK_Q0   BIT(0)
>>  };
>>
>>  enum rtl_isr_version {
>> @@ -778,6 +787,7 @@ struct rtl8169_private {
>>       enum rtl_isr_version hw_curr_isr_ver;
>>       u8 irq_nvecs;
>>       bool recheck_desc_ownbit;
>> +     unsigned int features;
>
>Why do you add an extra bitmap here? Why not use bool xxx:1 like for other
>flags?
>
>>       int irq;
>>       struct clk *clk;
>>
>> @@ -1676,26 +1686,36 @@ static u32 rtl_get_events(struct
>> rtl8169_private *tp)
>>
>>  static void rtl_ack_events(struct rtl8169_private *tp, u32 bits)  {
>> -     if (rtl_is_8125(tp))
>> +     if (rtl_is_8125(tp)) {
>>               RTL_W32(tp, IntrStatus_8125, bits);
>> -     else
>> +             if (tp->features & RTL_VEC_MAP_ENABLE)
>
>Looks to me like this check is equivalent to checking hw_curr_isr_ver >
>RTL_ISR_VER_DEFAULT, or?
>If yes, then this additional flag doesn't seem to be needed.

I will remove this flag, and replace it with check if (tp->irq_nvecs > 1), I think this is more clearly. When we allocate irq more than 1, just enable vector interrupt mapping.

>
>> +                     RTL_W32(tp, ISR_VEC_MAP_REG, 0xffffffff);
>> +     } else {
>>               RTL_W16(tp, IntrStatus, bits);
>> +     }
>>  }
>>
>>  static void rtl_irq_disable(struct rtl8169_private *tp)  {
>> -     if (rtl_is_8125(tp))
>> +     if (rtl_is_8125(tp)) {
>>               RTL_W32(tp, IntrMask_8125, 0);
>> -     else
>> +             if (tp->features & RTL_VEC_MAP_ENABLE)
>> +                     RTL_W32(tp, IMR_CLEAR_VEC_MAP_REG, 0xffffffff);
>> +     } else {
>>               RTL_W16(tp, IntrMask, 0);
>> +     }
>>  }
>>
>>  static void rtl_irq_enable(struct rtl8169_private *tp)  {
>> -     if (rtl_is_8125(tp))
>> -             RTL_W32(tp, IntrMask_8125, tp->irq_mask);
>> -     else
>> +     if (rtl_is_8125(tp)) {
>> +             if (tp->features & RTL_VEC_MAP_ENABLE)
>> +                     RTL_W32(tp, IMR_SET_VEC_MAP_REG, tp->irq_mask);
>> +             else
>> +                     RTL_W32(tp, IntrMask_8125, tp->irq_mask);
>> +     } else {
>>               RTL_W16(tp, IntrMask, tp->irq_mask);
>> +     }
>>  }
>>
>>  static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp) @@
>> -5070,6 +5090,42 @@ static void rtl8169_free_irq(struct rtl8169_private *tp)
>>       }
>>  }
>>
>> +static void rtl8169_disable_hw_interrupt_msix(struct rtl8169_private
>> +*tp, int message_id) {
>> +     RTL_W32(tp, IMR_CLEAR_VEC_MAP_REG, BIT(message_id)); }
>> +
>> +static void rtl8169_clear_hw_isr(struct rtl8169_private *tp, int
>> +message_id) {
>> +     RTL_W32(tp, ISR_VEC_MAP_REG, BIT(message_id)); }
>> +
>> +static void rtl8169_enable_hw_interrupt_msix(struct rtl8169_private
>> +*tp, int message_id) {
>> +     RTL_W32(tp, IMR_SET_VEC_MAP_REG, BIT(message_id)); }
>> +
>> +static irqreturn_t rtl8169_interrupt_msix(int irq, void
>> +*dev_instance) {
>> +     struct napi_struct *napi = dev_instance;
>> +     struct net_device *dev = napi->dev;
>> +     struct rtl8169_private *tp = netdev_priv(dev);
>> +     int message_id = napi->index;
>> +
>> +     rtl8169_clear_hw_isr(tp, message_id);
>> +
>> +     if (message_id == MSIX_ID_VEC_MAP_LINKCHG) {
>> +             phy_mac_interrupt(tp->phydev);
>> +             return IRQ_HANDLED;
>> +     }
>> +
>> +     tp->recheck_desc_ownbit = true;
>> +     rtl8169_disable_hw_interrupt_msix(tp, message_id);
>> +     napi_schedule(napi);
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>>  static int rtl8169_request_irq(struct rtl8169_private *tp)  {
>>       struct net_device *dev = tp->dev; @@ -5078,8 +5134,12 @@ static
>> int rtl8169_request_irq(struct rtl8169_private *tp)
>>
>>       for (int i = 0; i < tp->irq_nvecs; i++) {
>>               napi = &tp->rtl8169_napi[i];
>> -             rc = pci_request_irq(tp->pci_dev, i, rtl8169_interrupt,
>> -                                  NULL, napi, "%s-%d", dev->name, i);
>> +             if (tp->features & RTL_VEC_MAP_ENABLE)
>> +                     rc = pci_request_irq(tp->pci_dev, i, rtl8169_interrupt_msix,
>> +                                          NULL, napi, "%s-%d", dev->name, i);
>> +             else
>> +                     rc = pci_request_irq(tp->pci_dev, i, rtl8169_interrupt,
>> +                                          NULL, napi, "%s-%d",
>> + dev->name, i);
>>               if (rc)
>>                       break;
>>       }
>> @@ -5523,10 +5583,16 @@ 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 | ISRIMR_TOK_Q0;
>> +             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) @@ -5555,6
>> +5621,8 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
>>               return nvecs;
>>
>>       tp->irq_nvecs = nvecs;
>> +     if (nvecs > 1)
>> +             tp->features |= RTL_VEC_MAP_ENABLE;
>>
>>       return 0;
>>  }
>> @@ -5822,10 +5890,79 @@ 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 net_device *dev = napi->dev;
>> +     struct rtl8169_private *tp = netdev_priv(dev);
>> +     const int message_id = napi->index;
>> +     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 net_device *dev = napi->dev;
>> +     struct rtl8169_private *tp = netdev_priv(dev);
>> +     const int message_id = napi->index;
>> +     int tx_ring_idx = message_id - 8;
>> +     unsigned int work_done = 0;
>> +
>> +     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 net_device *dev = napi->dev;
>> +     struct rtl8169_private *tp = netdev_priv(dev);
>> +     const int message_id = napi - tp->rtl8169_napi;
>
>Why not use napi->index here too?
>
>> +
>> +     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++)
>> -             netif_napi_add(tp->dev, &tp->rtl8169_napi[i], rtl8169_poll);
>> +     for (int i = 0; i < tp->irq_nvecs; i++) {
>> +             if (tp->features & RTL_VEC_MAP_ENABLE) {
>> +                     switch (tp->hw_curr_isr_ver) {
>> +                     case RTL_ISR_VER_8127:
>
>A comment describing the RTL8127 MSI-X vector layout would be helpful here.
>Otherwise the following is hard to understand.
>
>> +                             if (i < R8127_MAX_RX_QUEUES)
>> +                                     netif_napi_add(tp->dev, &tp->rtl8169_napi[i],
>> +                                                    rtl8169_poll_msix_rx);
>> +                             else if (i >= R8127_MAX_RX_QUEUES &&
>> +                                      i < (R8127_MAX_RX_QUEUES +
>> +                                      R8127_MAX_TX_QUEUES))
>> +                                     netif_napi_add(tp->dev, &tp->rtl8169_napi[i],
>> +                                                    rtl8169_poll_msix_tx);
>> +                             else
>> +                                     netif_napi_add(tp->dev, &tp->rtl8169_napi[i],
>> +                                                    rtl8169_poll_msix_other);
>> +                             break;
>> +                     default:
>> +                             netif_napi_add(tp->dev, &tp->rtl8169_napi[i],
>> +                                            rtl8169_poll);
>> +                             break;
>> +                     }
>> +             } else {
>> +                     netif_napi_add(tp->dev, &tp->rtl8169_napi[i], rtl8169_poll);
>> +             }
>
>This seems to be unnecessarily complex and can be simplified.
>
>> +             tp->rtl8169_napi[i].index = i;
>> +     }
>>  }
>>
>>  static int rtl_init_one(struct pci_dev *pdev, const struct
>> pci_device_id *ent)