Re: [PATCH iwl-next 1/8] e1000e: add jumbo Rx CRC stripping

[Matt Vollrath <tactii@gmail.com>]

On 9/3/26 06:27, Simon Horman wrote:
> 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/#/patchset/20260830232146.36948-2-tactii@gmail.com?part=1
> ---
> e1000e: add jumbo Rx CRC stripping
>      
> When the hardware is configured to not strip the FCS (CrcStripping=0),
> the jumbo Rx path would deliver frames with the FCS still attached.
> Follow the standard path's convention by stripping the FCS at EOP and
> leaving it out of the byte count.
> 
>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>> index 844f31ab37ad4..599600ad695cd 100644
>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>> @@ -1631,13 +1631,23 @@ static bool e1000_clean_jumbo_rx_irq(struct e1000_ring *rx_ring, int *work_done,
>>   			}
>>   		}
>>   
>> +		/* strip the Ethernet CRC; it may span fragments */
>> +		if (!(adapter->flags2 & FLAG2_CRC_STRIPPING) &&
>> +		    !(netdev->features & NETIF_F_RXFCS))
>> +			pskb_trim(skb, skb->len - 4);
>> +
>>   		/* Receive Checksum Offload */
>>   		e1000_rx_checksum(adapter, staterr, skb);
>>   
>>   		e1000_rx_hash(netdev, rx_desc->wb.lower.hi_dword.rss, skb);
>>   
>> -		/* probably a little skewed due to removing CRC */
>>   		total_rx_bytes += skb->len;
>> +		/* If configured to store CRC, keep the FCS bytes out of the
>> +		 * total_rx_bytes counter
>> +		 */
>> +		if (!(adapter->flags2 & FLAG2_CRC_STRIPPING) &&
>> +		    (netdev->features & NETIF_F_RXFCS))
>> +			total_rx_bytes -= 4;
> 
> [Severity: Medium]
> Could these lockless evaluations of adapter->flags2 and netdev->features get
> out of sync if userspace changes features via ethtool while a packet is being
> processed?

Yes, this could be prevented by bringing down the adapter before changing bits
in e1000_set_features().

> 
> If a concurrent change occurs between the first check and this second check,
> it appears it could lead to an inconsistent state where a packet's payload is
> incorrectly truncated by 4 bytes, or where the byte statistics are overcounted
> or undercounted by 4 bytes.
> 
>>   		total_rx_packets++;
>>   
>>   		/* eth type trans needs skb->data to point to something */