Re: [Intel-wired-lan] [PATCH iwl-net v5] igb: Fix trigger of incorrect irq in igb_xsk_wakeup

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Wed, Jan 14, 2026 at 08:19:37AM +0000, Behera, VIVEK wrote:

(...)

> > >
> > > diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h
> > > b/drivers/net/ethernet/intel/igb/e1000_defines.h
> > > index fa028928482f..9357564a2d58 100644
> > > --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
> > > +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
> > > @@ -443,6 +443,7 @@
> > >  #define E1000_ICS_LSC       E1000_ICR_LSC       /* Link Status Change */
> > >  #define E1000_ICS_RXDMT0    E1000_ICR_RXDMT0    /* rx desc min. threshold
> > */
> > >  #define E1000_ICS_DRSTA     E1000_ICR_DRSTA     /* Device Reset Aserted */
> > > +#define E1000_ICS_TXDW      E1000_ICR_TXDW	/* Transmit desc written
> > back */
> > >
> > >  /* Extended Interrupt Cause Set */
> > >  /* E1000_EITR_CNT_IGNR is only for 82576 and newer */ diff --git
> > > a/drivers/net/ethernet/intel/igb/igb_xsk.c
> > > b/drivers/net/ethernet/intel/igb/igb_xsk.c
> > > index 30ce5fbb5b77..6e51b5b6f131 100644
> > > --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> > > +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> > > @@ -529,6 +529,13 @@ int igb_xsk_wakeup(struct net_device *dev, u32 qid, u32
> > flags)
> > >  	struct igb_adapter *adapter = netdev_priv(dev);
> > >  	struct e1000_hw *hw = &adapter->hw;
> > >  	struct igb_ring *ring;
> > > +	struct igb_q_vector *q_vector;
> > > +	struct napi_struct *rx_napi;
> > > +	struct napi_struct *tx_napi;
> > > +	bool trigger_irq_tx = false;
> > > +	bool trigger_irq_rx = false;
> > > +	u32 eics_tx = 0;
> > > +	u32 eics_rx = 0;
> > >  	u32 eics = 0;
> > >
> > >  	if (test_bit(__IGB_DOWN, &adapter->state)) @@ -536,27 +543,65 @@ int
> > > igb_xsk_wakeup(struct net_device *dev, u32 qid, u32 flags)
> > >
> > >  	if (!igb_xdp_is_enabled(adapter))
> > >  		return -EINVAL;
> > > -
> > > -	if (qid >= adapter->num_tx_queues)
> > > +	/* Check if queue_id is valid. Tx and Rx queue numbers are always same */
> > > +	if (qid >= adapter->num_rx_queues)
> > >  		return -EINVAL;
> > > -
> > > -	ring = adapter->tx_ring[qid];
> > > -
> > > -	if (test_bit(IGB_RING_FLAG_TX_DISABLED, &ring->flags))
> > > -		return -ENETDOWN;
> > > -
> > > -	if (!READ_ONCE(ring->xsk_pool))
> > > +	/* Check if flags are valid */
> > > +	if (!(flags & XDP_WAKEUP_RX) && !(flags & XDP_WAKEUP_TX))
> > >  		return -EINVAL;
> > > -
> > > -	if (!napi_if_scheduled_mark_missed(&ring->q_vector->napi)) {
> > > -		/* Cause software interrupt */
> > > +	if (flags & XDP_WAKEUP_RX) {
> > > +		/* IRQ trigger preparation for Rx */
> > > +		ring = adapter->rx_ring[qid];
> > > +		if (!READ_ONCE(ring->xsk_pool))
> > > +			return -ENXIO;
> > > +		q_vector = ring->q_vector;
> > > +		rx_napi = &q_vector->napi;
> > > +		/* Extend the BIT mask for eics */
> > > +		eics_rx = ring->q_vector->eims_value;
> > > +		trigger_irq_rx = true;
> > > +	}
> > > +	if (flags & XDP_WAKEUP_TX) {
> > > +		if (adapter->flags & IGB_FLAG_QUEUE_PAIRS) {
> > > +		/* In queue-pair mode, rx_ring and tx_ring share the same q_vector,
> > > +		 * so a single IRQ trigger will wake both RX and TX processing
> > > +		 */
> > > +		} else {
> > > +			/* IRQ trigger preparation for Tx */
> > > +			ring = adapter->tx_ring[qid];
> > > +			if (test_bit(IGB_RING_FLAG_TX_DISABLED, &ring->flags))
> > > +				return -ENETDOWN;
> > > +
> > > +			if (!READ_ONCE(ring->xsk_pool))
> > > +				return -ENXIO;
> > > +			q_vector = ring->q_vector;
> > > +			tx_napi = &q_vector->napi;
> > > +			/* Extend the BIT mask for eics */
> > > +			eics_tx = ring->q_vector->eims_value;
> > > +			trigger_irq_tx = true;
> > > +		}
> > > +	}
> > > +	/* All error checks are finished. Check and update napi states for rx and tx */
> > > +	if (trigger_irq_rx) {
> > > +		if (!napi_if_scheduled_mark_missed(rx_napi))
> > > +			eics |= eics_rx;
> > > +	}
> > > +	if (trigger_irq_tx) {
> > > +		if (!napi_if_scheduled_mark_missed(tx_napi))
> > > +			eics |= eics_tx;
> > > +	}
> > > +	/* Now we trigger the required irqs for Rx and Tx */
> > > +	if ((trigger_irq_rx) || (trigger_irq_tx)) {
> > >  		if (adapter->flags & IGB_FLAG_HAS_MSIX) {
> > > -			eics |= ring->q_vector->eims_value;
> > >  			wr32(E1000_EICS, eics);
> > >  		} else {
> > > -			wr32(E1000_ICS, E1000_ICS_RXDMT0);
> > > +			if ((trigger_irq_rx) && (trigger_irq_tx))
> > > +				wr32(E1000_ICS,
> > > +				     E1000_ICS_RXDMT0 | E1000_ICS_TXDW);
> > > +			else if (trigger_irq_rx)
> > > +				wr32(E1000_ICS, E1000_ICS_RXDMT0);
> > > +			else
> > > +				wr32(E1000_ICS, E1000_ICS_TXDW);
> > 
> > My understanding is something below would be sufficient. Bits set on E1000_ICS are
> > not handled in any way so we don't have to distinguish between rx/tx, it's just the
> > matter of irq trigger and napi schedule.
> > 
> Hi see my comments below
> > -----------------8<-----------------
> > 
> > diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c
> > b/drivers/net/ethernet/intel/igb/igb_xsk.c
> > index 30ce5fbb5b77..0aba7afd6a03 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> > @@ -524,12 +524,26 @@ bool igb_xmit_zc(struct igb_ring *tx_ring, struct
> > xsk_buff_pool *xsk_pool)
> >  	return nb_pkts < budget;
> >  }
> > 
> > +static void igb_sw_irq(struct igb_q_vector *q_vector) {
> > +	u32 eics = 0;
> > +
> > +	if (!napi_if_scheduled_mark_missed(&q_vector->napi)) {
> > +		/* Cause software interrupt */
> > +		if (adapter->flags & IGB_FLAG_HAS_MSIX) {
> > +			eics |= ring->q_vector->eims_value;
> > +			wr32(E1000_EICS, eics);
> > +		} else {
> > +			wr32(E1000_ICS, E1000_ICS_RXDMT0);
> So here it is sufficient to rely on the E1000_ICS_RXDMT0 bit to trigger the correct irq (Tx and Rx)?
>  I remember I received a review comment from Intel point to E1000_ICS_TXDW as being the correct bit of triggering TX for non MSIX case. 
> I can't really evaluate this since I don't have a setup to test this. But okay 

I don't see in irq handlers that we do any specific handling for txdw vs
rxdmt0. It's rather a matter of getting an irq here.

> > +		}
> > +	}
> > +}
> > +
> >  int igb_xsk_wakeup(struct net_device *dev, u32 qid, u32 flags)  {
> >  	struct igb_adapter *adapter = netdev_priv(dev);
> >  	struct e1000_hw *hw = &adapter->hw;
> >  	struct igb_ring *ring;
> > -	u32 eics = 0;
> > 
> >  	if (test_bit(__IGB_DOWN, &adapter->state))
> >  		return -ENETDOWN;
> > @@ -548,14 +562,15 @@ int igb_xsk_wakeup(struct net_device *dev, u32 qid, u32
> > flags)
> >  	if (!READ_ONCE(ring->xsk_pool))
> >  		return -EINVAL;
> > 
> > -	if (!napi_if_scheduled_mark_missed(&ring->q_vector->napi)) {
> > -		/* Cause software interrupt */
> > -		if (adapter->flags & IGB_FLAG_HAS_MSIX) {
> > -			eics |= ring->q_vector->eims_value;
> > -			wr32(E1000_EICS, eics);
> > -		} else {
> > -			wr32(E1000_ICS, E1000_ICS_RXDMT0);
> > -		}
> > +	if (flags & XDP_WAKEUP_TX)
> > +		igb_sw_irq(ring->q_vector);
> > +
> > +	if (flags & XDP_WAKEUP_RX) {
> > +		ring = adapter->rx_ring[qid];
> > +		/* for !IGB_FLAG_QUEUE_PAIRS, this will be NOP as NAPI has
> > +		 * been already marked with NAPIF_STATE_MISSED
> > +		 */
> I think you meant for the case IGB_FLAG_QUEUE_PAIRS. Since when the queue pairs are not active 
> the Tx AND Rx queues don't share the same qvector and consequently not the same NAPI

yes, correct

> > +		igb_sw_irq(ring->q_vector);
> Okay so you would be triggering soft irq's in two steps if both TX and RX flags are set. 
> Honestly, I have tried to avoid doing this in my patch.  Which is the reason why I wait to finish all the error checks,
> Napi updates before triggering the required irq vectors by writing to eics with a single write. 
> But okay the other approach also works 



>  
> >  	}
> > 
> >  	return 0;
> > 
> > ----------------->8-----------------
> > 
> > >  		}
> > >  	}
> > > -
> > >  	return 0;
> > >  }
> > > --
> > > 2.34.1
> > >
> I think the strategy of triggering interrupts in one step after performing all the necessary checks is what might make this approach look complex. 
> IMHO the one step strategy is better and more intuitive.
> Unfortunately, there isn't a reference here to go by since none of the xsk_wakeup hooks implemented in the kernel care about the flags 
> I can submit a v6 of the patch based the one step approach with further simplifications. v6 would also include review suggestions I received for v5. 
> Like this I can also submit the next version to the igc patch. It follows the same logic as the igb 
> I have our regression tests with RTC testbench and our Siemens Profinet RT tester running with these patches with I210 and I226
> 
> Alternatively, you could submit patches following the igb and igc following the two-step logic. 

How about we meet the half way and something below? that would include
your request of having a single write to E1000_ICS.


diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
index 30ce5fbb5b77..432b4c7c1850 100644
--- a/drivers/net/ethernet/intel/igb/igb_xsk.c
+++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
@@ -524,6 +524,17 @@ bool igb_xmit_zc(struct igb_ring *tx_ring, struct xsk_buff_pool *xsk_pool)
 	return nb_pkts < budget;
 }
 
+static u32 igb_sw_irq_prep(struct igb_q_vector *q_vector)
+{
+	u32 eics = 0;
+
+	if (!napi_if_scheduled_mark_missed(&q_vector->napi))
+		eics = adapter->flags & IGB_FLAG_HAS_MSIX ?
+			q_vector->eims_value : 1;
+
+	return eics;
+}
+
 int igb_xsk_wakeup(struct net_device *dev, u32 qid, u32 flags)
 {
 	struct igb_adapter *adapter = netdev_priv(dev);
@@ -548,14 +559,23 @@ int igb_xsk_wakeup(struct net_device *dev, u32 qid, u32 flags)
 	if (!READ_ONCE(ring->xsk_pool))
 		return -EINVAL;
 
-	if (!napi_if_scheduled_mark_missed(&ring->q_vector->napi)) {
-		/* Cause software interrupt */
-		if (adapter->flags & IGB_FLAG_HAS_MSIX) {
-			eics |= ring->q_vector->eims_value;
+	if (flags & XDP_WAKEUP_TX)
+		eics |= igb_sw_irq_prep(ring->q_vector);
+
+	if (flags & XDP_WAKEUP_RX) {
+		ring = adapter->rx_ring[qid];
+		/* for IGB_FLAG_QUEUE_PAIRS, this will be NOP as NAPI has
+		 * been already marked with NAPIF_STATE_MISSED
+		 */
+		eics |= igb_sw_irq_prep(ring->q_vector);
+	}
+
+	/* Cause software interrupt */
+	if (eics) {
+		if (adapter->flags & IGB_FLAG_HAS_MSIX)
 			wr32(E1000_EICS, eics);
-		} else {
+		else
 			wr32(E1000_ICS, E1000_ICS_RXDMT0);
-		}
 	}
 
 	return 0;

> 
> Regards
> 
> Vivek