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.
>
>...
>
>>>> 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.
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
>
> /* 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.
>>>> +
>>>> + /* 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.
>>>> +
>>>> + /* 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.
>>>> + 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);
>>>> 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
