Re: [PATCH net-next 6/9] e1000e: use pskb_may_pull() in the 82571/2/3 TSO workaround

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] After this patch, two unrelated error paths in e1000e log the
  same message.

--

e1000e: use pskb_may_pull() in the 82571/2/3 TSO workaround

This commit changes the 82571/2/3 TSO workaround in e1000_xmit_frame() to
use pskb_may_pull(skb, len + pull_size) in place of a direct
__pskb_pull_tail() call. With this change the driver no longer calls
__pskb_pull_tail() itself. The error message printed on failure is
updated to match.

> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 844f31ab37ad4..e216868e15cba 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -5859,8 +5859,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
>  			unsigned int pull_size;
>  
>  			pull_size = min_t(unsigned int, 4, skb->data_len);
> -			if (!__pskb_pull_tail(skb, pull_size)) {
> -				e_err("__pskb_pull_tail failed.\n");
> +			if (!pskb_may_pull(skb, len + pull_size)) {
> +				e_err("pskb_may_pull failed.\n");

[Severity: Low]
This isn't a bug, but this TX message now matches a message that
already exists on the jumbo RX path in the same file:

drivers/net/ethernet/intel/e1000e/netdev.c:e1000_clean_jumbo_rx_irq() {
    ...
		/* eth type trans needs skb->data to point to something */
		if (!pskb_may_pull(skb, ETH_HLEN)) {
			e_err("pskb_may_pull failed.\n");
			dev_kfree_skb_irq(skb);
			goto next_desc;
		}
    ...
}

e_err() expands to netdev_err(adapter->netdev, ...). The log line
therefore shows only the device name, not the function or the
direction.

Before this patch the TX site printed "__pskb_pull_tail failed.", so the
two failures could be told apart in dmesg. After it, a failed RX
ETH_HLEN pull and a failed pull in the TX TSO workaround print the same
line.

Could the TX site use a TX-specific message instead, for example
"TSO workaround pskb_may_pull failed.\n"?

>  				dev_kfree_skb_any(skb);
>  				return NETDEV_TX_OK;
>  			}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-b4-pskb-pull-tail-drivers-v1-0-9512b0fb977b%40toxicpanda.com