Hi Jacob, Thank you very much for taking the time to review the patch and for the `Reviewed-by:` tag! I really appreciate your feedback.
Just wanted to let you know that I had already sent out `v2` of this patch, which included the PTP TX wakeup fix and addressed some `checkpatch.pl` comments. For `v3`, I will incorporate your suggestions and send it out shortly. Thanks again, Vivek Behera -----Original Message----- From: Jacob Keller <[email protected]> Sent: Tuesday, December 9, 2025 12:54 AM To: Behera, Vivek (DI FA DSP ICC PRC1) <[email protected]>; Tony Nguyen <[email protected]>; Przemek Kitszel <[email protected]>; Andrew Lunn <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]> Cc: [email protected]; [email protected]; [email protected] Subject: Re: [PATCH] igc: Fix trigger of incorrect irq in igc_xsk_wakeup function On 12/5/2025 4:39 AM, Behera, VIVEK wrote: > From 4e3ebdc0af6baa83ccfc17c61c1eb61408095ffd Mon Sep 17 00:00:00 2001 > From: Vivek Behera <[email protected]> > Date: Fri, 5 Dec 2025 10:26:05 +0100 > Subject: [PATCH] igc: Fix trigger of incorrect irq in igc_xsk_wakeup > function > > When the i226 is configured to use only 2 combined queues using > ethtool or in an environment with only 2 active CPU cores the 4 irq > lines are used in a split configuration with one irq assigned to each > of the two rx and tx queues (see console output below) > > sudo ethtool -l enp1s0 > Channel parameters for enp1s0: > Pre-set maximums: > RX: n/a > TX: n/a > Other: 1 > Combined: 4 > Current hardware settings: > RX: n/a > TX: n/a > Other: 1 > Combined: 2 > eddx@mvs:~$ cat /proc/interrupts | grep enp1s0 > 147: 1 0 IR-PCI-MSIX-0000:01:00.0 0-edge enp1s0 > 148: 8 0 IR-PCI-MSIX-0000:01:00.0 1-edge enp1s0-rx-0 > 149: 0 0 IR-PCI-MSIX-0000:01:00.0 2-edge enp1s0-rx-1 > 150: 26 0 IR-PCI-MSIX-0000:01:00.0 3-edge enp1s0-tx-0 > 151: 0 0 IR-PCI-MSIX-0000:01:00.0 4-edge enp1s0-tx-1 > > While testing with the RTC Testbench it was noticed using the bpftrace > that the igc_xsk_wakeup when triggered by xsk_sendmsg was triggering > the incorrect irq for tx-0(see trace below) > > TIMESTAMP: 456992309829 | FUNCTION: igc_xsk_wakeup | ENTRY: > RtcTxThread (PID: 945) - queue_id: 0 > TIMESTAMP: 456992317157 | FUNCTION: igc_poll | ENTRY: irq/148-enp1s0- > (PID: 948) > TIMESTAMP: 456993309408 | FUNCTION: igc_xsk_wakeup | ENTRY: > RtcTxThread (PID: 945) - queue_id: 0 > TIMESTAMP: 456993316591 | FUNCTION: igc_poll | ENTRY: irq/148-enp1s0- > (PID: 948) > TIMESTAMP: 456994309630 | FUNCTION: igc_xsk_wakeup | ENTRY: > RtcTxThread (PID: 945) - queue_id: 0 > TIMESTAMP: 456994316674 | FUNCTION: igc_poll | ENTRY: irq/148-enp1s0- > (PID: 948) > TIMESTAMP: 456995309493 | FUNCTION: igc_xsk_wakeup | ENTRY: > RtcTxThread (PID: 945) - queue_id: 0 > TIMESTAMP: 456995316593 | FUNCTION: igc_poll | ENTRY: irq/148-enp1s0- > (PID: 948) > > Due to this bug no XDP Zc send is possible in this split irq configuration. > This patch implements the correct logic of extracting the q_vectors > saved duirng the rx and tx ring allocation. > Furthermore the patch includes usage of flags provided by the > ndo_xsk_wakeup api to trigger the required irq. With this patch > correct irqs are triggered > > cat /proc/interrupts | grep enp1s0 > 161: 1 0 0 0 IR-PCI-MSIX-0000:01:00.0 > 0-edge enp1s0 > 162: 2 0 0 0 IR-PCI-MSIX-0000:01:00.0 > 1-edge enp1s0-rx-0 > 163: 359 0 0 0 IR-PCI-MSIX-0000:01:00.0 > 2-edge enp1s0-rx-1 > 164: 872005 0 0 0 IR-PCI-MSIX-0000:01:00.0 > 3-edge enp1s0-tx-0 > 165: 71 0 0 0 IR-PCI-MSIX-0000:01:00.0 > 4-edge enp1s0-tx-1 > > TIMESTAMP: 149658589239205 | FUNCTION: igc_xsk_wakeup | ENTRY: > RtcTxThread (PID: 10633) - queue_id: 0 > TIMESTAMP: 149658589244662 | FUNCTION: igc_poll | ENTRY: > irq/164-enp1s0- (PID: 10593) > TIMESTAMP: 149658589293396 | FUNCTION: igc_poll | ENTRY: > irq/164-enp1s0- (PID: 10593) > TIMESTAMP: 149658589295357 | FUNCTION: xsk_tx_completed | ENTRY: > irq/164-enp1s0- (PID: 10593) - num_entries: 61 > TIMESTAMP: 149658589342151 | FUNCTION: igc_poll | ENTRY: > irq/164-enp1s0- (PID: 10593) > TIMESTAMP: 149658589343881 | FUNCTION: xsk_tx_completed | ENTRY: > irq/164-enp1s0- (PID: 10593) - num_entries: 3 > TIMESTAMP: 149658589391394 | FUNCTION: igc_poll | ENTRY: > irq/164-enp1s0- (PID: 10593) > TIMESTAMP: 149658590239215 | FUNCTION: igc_xsk_wakeup | ENTRY: > RtcTxThread (PID: 10633) - queue_id: 0 > I appreciate the detailed outline of how to configure the system so this fails, and the steps taken to verify the change fixes the issue. > Signed-off-by: Vivek Behera <[email protected]> This is a bug fix, so it should be targeted at net. You will need a Fixes tag associating which commit this fixes as well. Alternatively, since this is for an Intel networking driver and you sent it to Intel Wired LAN, it would be "iwl-net" so that it gets picked up by Tony for testing along with our other igc changes. > --- > drivers/net/ethernet/intel/igc/igc_main.c | 31 +++++++++++++++++++---- > 1 file changed, 26 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c > b/drivers/net/ethernet/intel/igc/igc_main.c > index 7aafa60ba0c8..0cfcd20a2536 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -6930,21 +6930,42 @@ int igc_xsk_wakeup(struct net_device *dev, u32 > queue_id, u32 flags) > if (!igc_xdp_is_enabled(adapter)) > return -ENXIO; > - if (queue_id >= adapter->num_rx_queues) > + if ((flags & XDP_WAKEUP_RX) && (flags & XDP_WAKEUP_TX)) { > + /* If both TX and RX need to be woken up queue pair > per IRQ is needed */ > + if (!(adapter->flags & IGC_FLAG_QUEUE_PAIRS)) > + return -EINVAL; /* igc queue pairs are not > activated. > + * Can't trigger irq > + */ We only have to check for queue pairs if we want to wake both. Makes sense. > + /* Just get the ring params from Rx */ > + if (queue_id >= adapter->num_rx_queues) > + return -EINVAL; > + ring = adapter->rx_ring[queue_id]; > + } else if (flags & XDP_WAKEUP_TX) { > + if (queue_id >= adapter->num_tx_queues) > + return -EINVAL; > + /* Get the ring params from Tx */ > + ring = adapter->tx_ring[queue_id]; > + } else if (flags & XDP_WAKEUP_RX) { > + if (queue_id >= adapter->num_rx_queues) > + return -EINVAL; > + /* Get the ring params from Rx */ > + ring = adapter->rx_ring[queue_id]; > + } else { > + /* Invalid Flags */ > return -EINVAL; > - > - ring = adapter->rx_ring[queue_id]; > + } > if (!ring->xsk_pool) > return -ENXIO; > - > - q_vector = adapter->q_vector[queue_id]; > + /* Retrieve the q_vector saved in the ring */ > + q_vector = ring->q_vector; > if (!napi_if_scheduled_mark_missed(&q_vector->napi)) > igc_trigger_rxtxq_interrupt(adapter, q_vector); The actual code changes seem correct to me, so you may add my review tag on a version which has the Fixes and the appropriate tree tag i.e. [iwl-net] or [PATCH iwl-net]. Reviewed-by: Jacob Keller <[email protected]> Thanks, Jake
