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

Simon Horman
2026-04-13T13:39:42Z

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