Re: [Intel-wired-lan] [PATCH iwl-net v2 5/6] ixgbe: fix ITR value overflow in adaptive interrupt throttling

[Simon Horman <horms@kernel.org>]

On Wed, Apr 08, 2026 at 03:11:53PM +0200, Aleksandr Loktionov wrote:
> ixgbe_update_itr() packs a mode flag (IXGBE_ITR_ADAPTIVE_LATENCY,
> bit 7) and a usecs delay (bits [6:0]) into an unsigned int, then
> stores the combined value in ring_container->itr which is declared as
> u8.  Values above 0xFF wrap on truncation, corrupting both the delay
> and the mode flag on the next readback.
> 
> Separate the mode bits from the usecs sub-field; clamp only the usecs
> portion to [0, IXGBE_ITR_ADAPTIVE_LATENCY - 1] (= 0x7F) using min_t()
> so overflow cannot bleed into bit 7.
> 
> Fixes: b4ded8327fea ("ixgbe: Update adaptive ITR algorithm")
> Cc: stable@vger.kernel.org
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
> v1 -> v2:
>  - Add proper [N/M] numbering so patchwork tracks it as part of the set;
>    no code change.
> 
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 210c7b9..9f3ae21 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -2889,8 +2889,9 @@ static void ixgbe_update_itr(struct ixgbe_q_vector *q_vector,
>  	}
>  
>  clear_counts:
> -	/* write back value */
> -	ring_container->itr = itr;
> +	ring_container->itr = (itr & IXGBE_ITR_ADAPTIVE_LATENCY) |
> +		min_t(unsigned int, itr & ~IXGBE_ITR_ADAPTIVE_LATENCY,
> +		      IXGBE_ITR_ADAPTIVE_LATENCY - 1);

* It is not clear to me that the mode flag bit (IXGBE_ITR_ADAPTIVE_LATENCY)
  is always set in itr when reaching this code. But with this patch that
  bit will always be set in ring_container->itr.

* Perhaps no such case exists, but it's not clear to me how this handles a
  case where the usec delay has overflowed into the mode flag bit.

  As a hypothetical example, consider the case where the delay overflows to
  exactly 0x80.  The resulting delay is 0 (both with and without this
  patch).

  I would suggest an approach of keeping the delay and mode bits separate
  during calculation - in separate local variables - and only combining
  them when ring_container->itr is set.

  This may turn out to be more verbose. But I expect it is easier to reason
  with.

* Looking over the code, it looks like the maximum allowed udelay is
  IXGBE_ITR_ADAPTIVE_MAX_USECS (126) rather than
  IXGBE_ITR_ADAPTIVE_LATENCY - 1 (127).

* The calculation does not guard against delay values less
  than IXGBE_ITR_ADAPTIVE_MIN_USECS. Which looking over the code seems
  to be something that matters. (And which occurred in the hypothetical
  example above).

* As itr is an unsigned int, and IXGBE_ITR_ADAPTIVE_LATENCY - 1 is a
  compile time constant, I expect that min() is sufficient.
  IOW, I don't think min_t is needed here.

* It looks like using FIELD_PREP is appropriate to construct
  ring_container->itr. But that may be overkill if you end up with
  something like:

	ring_container->itr = mode | clamp(delay, IXGBE_ITR_ADAPTIVE_MAX_USECS,
					   IXGBE_ITR_ADAPTIVE_MIN_USECS);

>  
>  	/* next update should occur within next jiffy */
>  	ring_container->next_update = next_update + 1;
> -- 
> 2.52.0