RE: [PATCH iwl-net v1] i40e: fix set_ringparam error path freeing live Tx rings

> -----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