Re: [PATCH iwl-net v2] e1000e: work around TSO erratum that corrupts data and hangs Tx

[David Laight <david.laight.linux@gmail.com>]

On Wed, 21 Oct 2026 12:49:16 +0200
André Brait <andrebrait@gmail.com> wrote:

> On Wed, Oct 21, 2026 at 10:57 AM David Laight
> <david.laight.linux@gmail.com> wrote:
> 
> > > Apply it to all e1000e parts: limit the first data descriptor of a TSO
> > > packet to 3 * (max_read_request - 4) bytes (372 on the I219, where
> > > pcie_get_readrq() returns 128). The rest of the linear part goes into
> > > the next descriptor, which e1000_xmit_frame() already reserves;  
> >
> > Aren't you using an additional descriptor over and above those the code
> > might already use?
> > So either it was reserving one too many, or you need to reserve another one.  
> 
> You're right that the split can use one more descriptor than before.
> The count already covers it, though: the bare count++ in
> e1000_xmit_frame() is never used by e1000_tx_map().

You could just comment that that count++ is in case the linear part
has the extra fragment.
(Indeed the other count++ could be unconditional as well, you aren't
going to notice if a transmit fails to fit by one descriptor.)

> It comes from e1000,
> where that descriptor pays for the 4-byte TSO sentinel ("premature desc
> write-backs in TSO mode"); e1000e dropped the sentinel but kept the
> count++.

Put that in the commit message.

> With this patch the linear part takes
> 1 + DIV_ROUND_UP(len - 372, tx_fifo_limit) descriptors instead of
> DIV_ROUND_UP(len, tx_fifo_limit), at most one more, so it uses exactly
> that spare.
> 
> If we want the count to say so, I see two ways:
> 
> a) Count the split explicitly and keep the spare:
> 
> if ((mss) || (skb->ip_summed == CHECKSUM_PARTIAL))
> count++;
> count++;
> + /* the headers go in a first buffer of at most tx_hdr_buf_max */
> + if (mss && len > adapter->tx_hdr_buf_max)
> + count++;
> count += DIV_ROUND_UP(len, adapter->tx_fifo_limit);
> 
>    The split pays for its own descriptor and the old spare stays a
>    spare.
> 
> b) Replace the spare with the explicit count:
> 
> if ((mss) || (skb->ip_summed == CHECKSUM_PARTIAL))
> count++;
> - count++;
> + if (mss && len > adapter->tx_hdr_buf_max)
> + count++;
> count += DIV_ROUND_UP(len, adapter->tx_fifo_limit);

c) Adjust the DIV_ROUND_UP() for the linear part.
   I think:
    DIV_ROUND_UP(len + adapter->tx_fifo_limit - 372, adapter->tx_fifo_limit)
   is right.

Although I wonder if you really want to be doing all those divides?
I'm guessing that tx_fifo_limit is usually 24k (and the actual buffer larger).
So the result is almost always 1.
(Indeed, does TSO and checksum insertion work if the packets are longer
than the real buffer?)

> 
>    The count becomes exact, and one lower for every other packet.
> 
> Neither fixes a bug I have seen: the reservation already covers the
> split, and in about 3.5 hours of test traffic on the patched driver the
> ring reached its stop threshold over 300,000 times without a stall. b
> also changes the reservation for every other packet, which I haven't
> tested. So I'd keep the patch as it is, the smallest change that fixes
> the bug, and leave the descriptor accounting for a separate cleanup.

It'll only matter if you are transmitting packets that need more descriptors
than the post-transmit check.
Otherwise that check disables transmit first and you don't bounce off the
'just enough space' check.

> 
> If you'd rather have the dependency explicit, I'm happy to fold a) into
> a v3; it only reserves one more descriptor for those TSO packets.
> 
> > I'm not actually sure all the logic there is actually needed.
> > The code calculates the number of descriptors the packet is expected to need
> > and gets the packet requeued if there aren't enough, but there is also the
> > 'post transmit' check that stops the transmit if there isn't enough room
> > for a full length packet.
> > If the latter is valid you don't need the former.
> > Although a sanity check that the ring isn't being wrapped might not do any
> > harm.  
> 
> The post-transmit check reserves (MAX_SKB_FRAGS + 1) *
> DIV_ROUND_UP(PAGE_SIZE, tx_fifo_limit) + 4, which assumes no buffer is
> larger than a page.

That limit is constant (for the device) - so could be pre-calculated.

It has to be said that I've always thought that check should be for a
'reasonable packet' whereas the docs sort of imply it should be for the
largest packet and the driver should never really return 'I couldn't do it'.

David

> The linear part and high-order frags can be, so the
> per-packet count is still needed.
> 
> Andre