Hi Tony > -----Original Message----- > From: Tony Nguyen <[email protected]> > Sent: Friday, December 19, 2025 10:07 PM > To: Behera, Vivek (DI FA DSP ICC PRC1) <[email protected]>; > [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected] > Cc: [email protected] > Subject: Re: [PATCH iwl-net v2] igb: Fix trigger of incorrect irq in > igb_xsk_wakeup > > > > On 12/18/2025 11:05 PM, Behera, VIVEK wrote: > > > > > >> -----Original Message----- > >> From: Behera, Vivek (DI FA DSP ICC PRC1) <[email protected]> > >> Sent: Monday, December 15, 2025 12:54 PM > >> To: [email protected]; [email protected]; > >> [email protected]; [email protected]; > >> [email protected]; [email protected]; > >> [email protected] > >> Cc: [email protected]; Behera, Vivek (DI FA DSP ICC > >> PRC1) <[email protected]> > >> Subject: [PATCH iwl-net v2] igb: Fix trigger of incorrect irq in > >> igb_xsk_wakeup > >> > >> The current implementation in the igb_xsk_wakeup expects the Rx and > >> Tx queues to share the same irq. This would lead to triggering of > >> incorrect irq in split irq configuration. > >> This patch addresses this issue which could impact environments with > >> 2 active cpu cores or when the number of queues is reduced to 2 or > >> less > >> > >> cat /proc/interrupts | grep eno2 > >> 167: 0 0 0 0 IR-PCI-MSIX-0000:08:00.0 > >> 0-edge eno2 > >> 168: 0 0 0 0 IR-PCI-MSIX-0000:08:00.0 > >> 1-edge eno2-rx-0 > >> 169: 0 0 0 0 IR-PCI-MSIX-0000:08:00.0 > >> 2-edge eno2-rx-1 > >> 170: 0 0 0 0 IR-PCI-MSIX-0000:08:00.0 > >> 3-edge eno2-tx-0 > >> 171: 0 0 0 0 IR-PCI-MSIX-0000:08:00.0 > >> 4-edge eno2-tx-1 > >> > >> Furthermore it uses the flags input argument to trigger either rx, tx > >> or both rx and tx irqs as specified in the ndo_xsk_wakeup api > >> documentation > >> > >> Fixes: 80f6ccf9f116 ("igb: Introduce XSK data structures and > >> helpers") > >> Signed-off-by: Vivek Behera <[email protected]> > >> Reviewed-by: Aleksandr loktinov <[email protected]> > >> --- > >> v1: > >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flor > >> > e.kernel.o%2F&data=05%7C02%7Cvivek.behera%40siemens.com%7C1540e21c5 > 79 > >> > 548f5fddc08de3f429f20%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7 > C63 > >> > 9017752565580869%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRyd > WUsIlYi > >> > OiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C > 0% > >> > 7C%7C%7C&sdata=MPBbmZMTBMzxIthA3AnmBB9tQhJiY8bUj4S8JtNsV4s%3D > &reserve > >> d=0 > >> rg%2Fintel-wired-lan%2F20251212131454.124116-1- > >> > vivek.behera%40siemens.com%2F&data=05%7C02%7Cvivek.behera%40siemens > >> > .com%7C1199ec6c63494235f90408de3bd0eefa%7C38ae3bcd95794fd4addab42e1 > >> > 495d55a%7C1%7C0%7C639013965726377756%7CUnknown%7CTWFpbGZsb3d8 > >> > eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoi > >> > TWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=uTarFq1Uj3h0H1aZeN > >> 06HWf0ts3BsMJkfPq9pPBegrI%3D&reserved=0 > >> > >> changelog: > >> v1 > >> - Inital description of the Bug and fixes made in the patch > >> > >> v1 -> v2 > >> - Handling of RX and TX Wakeup in igc_xsk_wakeup for a split IRQ > >> configuration > >> - Review by Aleksander: Modified sequence to complete all error > >> checks for rx and tx > >> before updating napi states and triggering irqs > >> - Corrected trigger of TX and RX interrupts over E1000_ICS (non msix > >> use case) > >> - Added define for Tx interrupt trigger bit mask for E1000_ICS > >> --- > >> .../net/ethernet/intel/igb/e1000_defines.h | 1 + > >> drivers/net/ethernet/intel/igb/igb_xsk.c | 98 +++++++++++++++++-- > >> 2 files changed, 92 insertions(+), 7 deletions(-) > >> > >> 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..d146ab629ef0 100644 > >> --- a/drivers/net/ethernet/intel/igb/igb_xsk.c > >> +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c > >> @@ -529,6 +529,7 @@ 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; > >> u32 eics = 0; > >> > >> if (test_bit(__IGB_DOWN, &adapter->state)) @@ -537,13 +538,91 > >> @@ 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) > >> + if ((flags & XDP_WAKEUP_RX) && (flags & XDP_WAKEUP_TX)) { > >> + /* If both TX and RX need to be woken up */ > >> + /* check if queue pairs are active. */ > >> + if (adapter->flags & IGB_FLAG_QUEUE_PAIRS) { > >> + /* Extract ring params from Rx. */ > >> + if (qid >= adapter->num_rx_queues) > >> + return -EINVAL; > >> + ring = adapter->rx_ring[qid]; > >> + } else { > >> + /* Two irqs for Rx AND Tx need to be triggered */ > >> + u32 eics_tx = 0; > >> + u32 eics_rx = 0; > >> + struct napi_struct *rx_napi; > >> + struct napi_struct *tx_napi; > >> + bool trigger_irq_tx = false; > >> + bool trigger_irq_rx = false; > >> + > >> + if (qid >= adapter->num_rx_queues) > >> + return -EINVAL; > >> + > >> + if (qid >= adapter->num_tx_queues) > >> + return -EINVAL; > >> + > >> + /* IRQ trigger preparation for Rx */ > >> + ring = adapter->rx_ring[qid]; > >> + if (!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; > >> + > >> + /* IRQ trigger preparation for Tx */ > >> + ring = adapter->tx_ring[qid]; > >> + if (test_bit(IGB_RING_FLAG_TX_DISABLED, &ring- > >>> flags)) > >> + return -ENETDOWN; > >> + > >> + if (!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; > >> + > >> + /* Check and update napi states for rx and tx */ > >> + if (!napi_if_scheduled_mark_missed(rx_napi)) { > >> + trigger_irq_rx = true; > >> + eics |= eics_rx; > >> + } > >> + if (!napi_if_scheduled_mark_missed(tx_napi)) { > >> + trigger_irq_tx = true; > >> + 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 { > >> + 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); > >> + } > >> + } > >> + return 0; > >> + } > >> + } else if (flags & XDP_WAKEUP_TX) { > >> + if (qid >= adapter->num_tx_queues) > >> + return -EINVAL; > >> + /* Get the ring params from Tx */ > >> + ring = adapter->tx_ring[qid]; > >> + if (test_bit(IGB_RING_FLAG_TX_DISABLED, &ring->flags)) > >> + return -ENETDOWN; > >> + } else if (flags & XDP_WAKEUP_RX) { > >> + if (qid >= adapter->num_rx_queues) > >> + return -EINVAL; > >> + /* Get the ring params from Rx */ > >> + ring = adapter->rx_ring[qid]; > >> + } else { > >> + /* Invalid Flags */ > >> 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)) > >> return -EINVAL; > >> @@ -554,7 +633,12 @@ int igb_xsk_wakeup(struct net_device *dev, u32 > >> qid, > >> u32 flags) > >> eics |= ring->q_vector->eims_value; > >> wr32(E1000_EICS, eics); > >> } else { > >> - wr32(E1000_ICS, E1000_ICS_RXDMT0); > >> + if ((flags & XDP_WAKEUP_RX) && (flags & > >> XDP_WAKEUP_TX)) > >> + wr32(E1000_ICS, E1000_ICS_RXDMT0 | > >> E1000_ICS_TXDW); > >> + else if (flags & XDP_WAKEUP_RX) > >> + wr32(E1000_ICS, E1000_ICS_RXDMT0); > >> + else > >> + wr32(E1000_ICS, E1000_ICS_TXDW); > >> } > >> } > >> > >> -- > >> 2.34.1 > > > > Hi Intel Colleagues, > > > > Would you be submitting this patch to netdev list after your internal > > validation > just like the igc patch? > > Hi Vivek, > > Yes, this will follow the same path as the igc patch. > > As these drivers, and this patch, are similar many of the recent comments on > the > igc patch apply here as well. Could you apply the applicable changes/updates > from igc to here as well?
Sure. I will include feedback and suggestions submitted for the igc patch here as well > > Thanks, > Tony > > > Regards > > > > Vivek
