> -----Original Message-----
> From: Simon Horman <[email protected]>
> Sent: Thursday, July 16, 2026 12:42 PM
> To: Loktionov, Aleksandr <[email protected]>
> Cc: Simon Horman <[email protected]>; [email protected];
> Nguyen, Anthony L <[email protected]>; [email protected]
> Subject: Re: [PATCH iwl-net v1] i40e: fix set_ringparam error path
> freeing live Tx rings
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> --- Patch [1]: [PATCH iwl-net v1] i40e: fix set_ringparam error path
> freeing live Tx rings --- commit
> 7042e7b9f58a75bdb26437aed842a7b6780df035
> Author: Aleksandr Loktionov <[email protected]>
>
> i40e: fix set_ringparam error path freeing live Tx rings
>
> This commit fixes an issue in the i40e_set_ringparam() error path
> where it mistakenly frees live, NAPI-active TX rings instead of the
> temporary rings allocated for a configuration change. It does this by
> passing the address of the temporary rings instead of the live ring
> pointer.
>
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > index 3da9ec49cc745..6d2b076049f7e 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> > @@ -2249,7 +2249,7 @@ static int i40e_set_ringparam(struct
> net_device *netdev,
> > if (tx_rings) {
> > for (i = 0; i < tx_alloc_queue_pairs; i++) {
> > if (i40e_active_tx_ring_index(vsi, i))
> > - i40e_free_tx_resources(vsi->tx_rings[i]);
> > + i40e_free_tx_resources(&tx_rings[i]);
>
> [Severity: High]
> This is a pre-existing issue, but does calling
> i40e_free_tx_resources() on the cloned &tx_rings[i] inadvertently
> reset the Byte Queue Limits (BQL) state on the live TX queues?
>
> Because the temporary rings are shallow copies of the live rings, they
> inherit the netdev and queue_index fields. When
> i40e_free_tx_resources() cleans up the temporary rings, it calls
> i40e_clean_tx_ring():
>
> drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_clean_tx_ring() {
> ...
> netdev_tx_reset_queue(txring_txq(tx_ring));
> ...
> }
>
> Since i40e_down() has not yet been called on this error path, the live
> queue is still actively transmitting. Locklessly resetting BQL here
> zeroes num_queued. When the hardware subsequently completes the
> already-queued packets, won't dql_completed() underflow and trigger a
> BUG_ON()?
>
> The same issue also seems to be present in the earlier error path if
> i40e_setup_tx_descriptors() fails.
>
> > }
> > kfree(tx_rings);
> > tx_rings = NULL;
Thank you for your review.
I'll re-spin v2 asap
Alex
