Hi Siang
> -----Original Message----- > From: Song, Yoong Siang <[email protected]> > Sent: Friday, December 19, 2025 3:28 AM > To: Nguyen, Anthony L <[email protected]>; Behera, Vivek (DI FA > DSP ICC PRC1) <[email protected]>; Kwapulinski, Piotr > <[email protected]>; Loktionov, Aleksandr > <[email protected]>; Keller, Jacob E <[email protected]>; > Kitszel, Przemyslaw <[email protected]> > Cc: [email protected]; [email protected] > Subject: RE: [Intel-wired-lan] [PATCH iwl-net v8] igc: Fix trigger of > incorrect irq in > igc_xsk_wakeup function > > On Friday, December 19, 2025 6:44 AM, Nguyen, Anthony L > <[email protected]> wrote: > >On 12/17/2025 11:21 PM, Behera, VIVEK wrote: > >> Hello colleagues, > >> > > [...] > > >>>> > >>>> This patch addresses the issue where the igc_xsk_wakeup function > >>>> was > >>> triggering an incorrect IRQ for tx-0 when the i226 is configured > >>> with only 2 combined queues or in an environment with 2 active CPU cores. > >>>> This prevented XDP Zero-copy send functionality in such split IRQ > >>> configurations. > >>>> > >>>> The fix implements the correct logic for extracting q_vectors saved > >>>> during rx > >>> and tx ring allocation and utilizes flags provided by the > >>> ndo_xsk_wakeup API to trigger the appropriate IRQ. > >>>> > > Thank you, Vivek, for the fix. > Overall, your patch looks good to me, just some minor changes are needed. > > >>>> Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy") > >>>> Fixes: 15fd021bc427 ("igc: Add Tx hardware timestamp request for > >>>> AF_XDP zero-copy packet") > >>>> Signed-off-by: Vivek Behera <[email protected]> > >>>> Reviewed-by: Jacob Keller <[email protected]> > >>>> Reviewed-by: Aleksandr loktinov <[email protected]> > >>>> Reviewed-by: Przemyslaw Kitszel <[email protected]> > >>>> Reviewed-by: Tony Nguyen <[email protected]> > > > >Unless you have received a Reviewed-by tag explicitly from the person, > >you shouldn't add it. Okay. Noted > > > >... > > > >>>> drivers/net/ethernet/intel/igc/igc_main.c | 90 > >>>> ++++++++++++++++++----- drivers/net/ethernet/intel/igc/igc_ptp.c | > >>>> 2 +- > >>>> 2 files changed, 73 insertions(+), 19 deletions(-) > >>>> > >>>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c > >>>> b/drivers/net/ethernet/intel/igc/igc_main.c > >>>> index 7aafa60ba0c8..76e4790bd3c0 100644 > >>>> --- a/drivers/net/ethernet/intel/igc/igc_main.c > >>>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c > >>>> @@ -6908,21 +6908,13 @@ static int igc_xdp_xmit(struct net_device > >>>> *dev, int > >>> num_frames, > >>>> return nxmit; > >>>> } > >>>> > >>>> -static void igc_trigger_rxtxq_interrupt(struct igc_adapter *adapter, > >>>> - struct igc_q_vector *q_vector) > >>>> -{ > >>>> - struct igc_hw *hw = &adapter->hw; > >>>> - u32 eics = 0; > >>>> - > >>>> - eics |= q_vector->eims_value; > >>>> - wr32(IGC_EICS, eics); > >>>> -} > >>>> - > >>>> int igc_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags) { > >>>> struct igc_adapter *adapter = netdev_priv(dev); > >>>> + struct igc_hw *hw = &adapter->hw; > >>>> struct igc_q_vector *q_vector; > >>>> struct igc_ring *ring; > >>>> + u32 eics = 0; > >>>> > >>>> if (test_bit(__IGC_DOWN, &adapter->state)) > >>>> return -ENETDOWN; > >>>> @@ -6930,18 +6922,80 @@ 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) > > num_rx_queues and num_tx_queues always having the same value. Thus, it is > safe for you to keep the original queue_id checking at the beginning of the > function, as below, and remove all the other queue_id check in your code. Yes. I see your point. It should be sufficient to have this check right at the beginning of the function since Irrespective of split or combined irqs the number of tx and rx queues is always the same > > if (queue_id >= adapter->num_rx_queues) > return -EINVAL; > > >>>> + 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. */ > > > >nit: This can just be one comment, no need to break it into two comment > >blocks Okay > > > > /* If both TX and RX need to be woken up > > * check if queue pairs are active. > > */ > > > > > >>>> + if ((adapter->flags & IGC_FLAG_QUEUE_PAIRS)) { > >>>> + /* Just get the ring params from Rx */ > >>>> + if (queue_id >= adapter->num_rx_queues) > >>>> + return -EINVAL; > >>>> + ring = adapter->rx_ring[queue_id]; > >>>> + } 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; > > > >This should be RCT, longest argument to shortest, so the structs should > >be first. > > > >>>> + > >>>> + if (queue_id >= adapter->num_rx_queues) > >>>> + return -EINVAL; > >>>> + > >>>> + if (queue_id >= adapter->num_tx_queues) > >>>> + return -EINVAL; > >>>> + > >>>> + /* IRQ trigger preparation for Rx */ > >>>> + ring = adapter->rx_ring[queue_id]; > >>>> + 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; > > IMHO, " eics_rx = ring->q_vector->eims_value " will be more readable, cause > you > just OR with zero. Yes. I agree > > >>>> + > >>>> + /* IRQ trigger preparation for Tx */ > >>>> + ring = adapter->tx_ring[queue_id]; > >>>> + 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; > > IMHO, " eics_tx = ring->q_vector->eims_value " will be more readable, cause > you > just OR with zero. > Same as above > >>>> + > >>>> + /* Check and update napi states for rx and tx */ > >>>> + if (!napi_if_scheduled_mark_missed(rx_napi)) > >>>> + eics |= eics_rx; > >>>> + if (!napi_if_scheduled_mark_missed(tx_napi)) > >>>> + eics |= eics_tx; > >>>> + > >>>> + /* Now we trigger the required irqs for Rx and > >>>> Tx */ > >>>> + if (eics) > >>>> + wr32(IGC_EICS, eics); > >>>> + > >>>> + return 0; > >>>> + } > >>>> + } 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]; > >>>> - > >>>> + } > >>>> + /* Prepare to trigger single irq */ > >>>> if (!ring->xsk_pool) > >>>> return -ENXIO; > >>>> > >>>> - q_vector = adapter->q_vector[queue_id]; > >>>> - if (!napi_if_scheduled_mark_missed(&q_vector->napi)) > >>>> - igc_trigger_rxtxq_interrupt(adapter, q_vector); > >>>> - > >>>> + q_vector = ring->q_vector; > >>>> + if (!napi_if_scheduled_mark_missed(&q_vector->napi)) { > >>>> + eics |= q_vector->eims_value; > > IMHO, "eics = q_vector->eims_value" will be more readable, cause you just OR > with zero. > Same as above > >>>> + wr32(IGC_EICS, eics); > >>>> + } > >>>> return 0; > >>>> } > >>>> > >>>> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c > >>>> b/drivers/net/ethernet/intel/igc/igc_ptp.c > >>>> index b7b46d863bee..6d8c2d639cd7 100644 > >>>> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c > >>>> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c > >>>> @@ -550,7 +550,7 @@ static void igc_ptp_free_tx_buffer(struct > >>>> igc_adapter > >>> *adapter, > >>>> tstamp->buffer_type = 0; > >>>> > >>>> /* Trigger txrx interrupt for transmit completion */ > >>>> - igc_xsk_wakeup(adapter->netdev, > >>>> tstamp->xsk_queue_index, 0); > >>>> + igc_xsk_wakeup(adapter->netdev, tstamp->xsk_queue_index, > >>>> +XDP_WAKEUP_TX); > >>>> > > The preferred limit on the length of a single line is 80 columns, please move > XDP_WAKEUP_TX to new line as below: > > igc_xsk_wakeup(adapter->netdev, tstamp->xsk_queue_index, > XDP_WAKEUP_TX); Okay > > >>>> return; > >>>> } > >>>> -- > >>>> 2.34.1 > >>> > >>> Reviewed-by: Piotr Kwapulinski <[email protected]> > >> > >> Thanks for all the reviews. Are there any suggestions before I add > >> netdev@ and > >linux-kernel@ to the list for further review > > > >For this patch, I don't think I would re-add them. As part of normal > >process (future patches), yes, but this one was asked to be excluded > >while the kinks of the upstream process were worked out. As this is the > >list you're targeting your patch for and I'll be sending this to netdev > >following our validation, it will get its review from netdev at that time. > > > >Also, adding Siang who is one of the authors from the Fixes: > > > > Thank you, Tony, for bringing this patch to my attention. > > >Thanks, > >Tony > >> > >> Regards > >> > >> Vivek > > Thanks & Regards > Siang Thanks Vivek
