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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 08.05.2026 14:17, 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>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 158 ++++++++++++++++++++--
>  1 file changed, 147 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 29fcd469eebb..bb72a2030f44 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -77,6 +77,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
> @@ -452,8 +453,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)
> @@ -584,6 +590,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_desc_bit {
> @@ -750,6 +759,7 @@ struct rtl8169_rx_ring {
>  struct rtl8169_napi {
>  	struct napi_struct napi;
>  	void *priv;
> +	int index;

napi_struct has a member index for the same purpose,
can't you use this one?

>  };
>  
>  struct rtl8169_irq {
> @@ -787,6 +797,7 @@ struct rtl8169_private {
>  	u8 hw_curr_isr_ver;
>  	u8 irq_nvecs;
>  	bool recheck_desc_ownbit;
> +	unsigned int features;
>  	int irq;
>  	struct clk *clk;
>  
> @@ -1685,26 +1696,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)
> +			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)
> @@ -5080,6 +5101,44 @@ 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 rtl8169_napi *napi = dev_instance;

Name napi is typically used for the napi_struct. This may cause confusion.
Better reflect in the name that this is about a custom struct.

> +	struct rtl8169_private *tp = napi->priv;
> +	int message_id = napi->index;
> +
> +	rtl8169_disable_hw_interrupt_msix(tp, message_id);
> +

Why do you disable interrupts here, and not directly before the
call to napi_schedule()?

> +	rtl8169_clear_hw_isr(tp, message_id);
> +
> +	if (message_id == MSIX_ID_VEC_MAP_LINKCHG) {
> +		phy_mac_interrupt(tp->phydev);
> +		rtl8169_enable_hw_interrupt_msix(tp, message_id);
> +		return IRQ_HANDLED;
> +	}
> +
> +	tp->recheck_desc_ownbit = true;
> +
> +	napi_schedule(&napi->napi);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int rtl8169_request_irq(struct rtl8169_private *tp)
>  {
>  	struct net_device *dev = tp->dev;
> @@ -5089,8 +5148,11 @@ static int rtl8169_request_irq(struct rtl8169_private *tp)
>  
>  	for (int i = 0; i < tp->irq_nvecs; i++) {
>  		irq = &tp->irq_tbl[i];
> +		if (tp->features & RTL_VEC_MAP_ENABLE && tp->hw_curr_isr_ver > 1)

What's the difference between this check and tp->rss_enable?

> +			irq->handler = rtl8169_interrupt_msix;
> +		else
> +			irq->handler = rtl8169_interrupt;

AFAIK indirect calls are expensive, what may hurt in a hot path.

>  		napi = &tp->r8169napi[i];
> -		irq->handler = rtl8169_interrupt;
>  		rc = pci_request_irq(tp->pci_dev, i, irq->handler,
>  				     NULL, napi, "%s-%d", dev->name, i);
>  		if (rc)
> @@ -5539,10 +5601,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)
> @@ -5573,6 +5641,9 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
>  	tp->irq = pci_irq_vector(pdev, 0);
>  	tp->irq_nvecs = nvecs;
>  
> +	if (nvecs > 1)
> +		tp->features |= RTL_VEC_MAP_ENABLE;
> +
>  	return 0;
>  }
>  
> @@ -5839,6 +5910,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;
> +	const int message_id = r8169_napi->index;
> +	struct net_device *dev = tp->dev;
> +	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;
> +	const int message_id = r8169_napi->index;
> +	int tx_ring_idx = message_id - 8;
> +	struct net_device *dev = tp->dev;
> +	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 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++) {
> @@ -5846,8 +5964,25 @@ 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 >= R8127_MAX_RX_QUEUES &&
> +					 i < (R8127_MAX_RX_QUEUES +
> +					 R8127_MAX_TX_QUEUES))
> +					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;
>  	}
>  }
>  
> @@ -5975,6 +6110,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (!tp->rss_support) {
>  		netif_napi_add(dev, &tp->r8169napi[0].napi, rtl8169_poll);
>  		tp->r8169napi[0].priv = tp;
> +		tp->r8169napi[0].index = 0;
>  	} else {
>  		r8169_init_napi(tp);
>  	}