Re: [PATCH iwl-net] e1000: report RX checksum offload as on when it is enabled

Re: [PATCH iwl-net] e1000: report RX checksum offload as on when it is enabled

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

[PATCH iwl-net] e1000: report RX checksum offload as on when it is enabled

[Jiaming Fan]

e1000_probe() copies hw_features into features before it adds
NETIF_F_RXCSUM to hw_features, so the feature is off by default. The
XsumRX module parameter is on by default, though: e1000_check_options()
sets adapter->rx_csum, e1000_configure_rx() then enables TCP/UDP
checksum offload in RXCSUM, and e1000_rx_checksum() uses the result
without looking at the features.

So ethtool reports rx-checksumming as off while the hardware verifies
checksums and the stack trusts the result, and "ethtool -K ethX rx off"
does nothing: the requested features equal the current ones, so
e1000_set_features() is never called and adapter->rx_csum stays true.
Offload can only be disabled by turning it on and then off again.
Before commit e97d3207c5e6 ("e1000: convert to ndo_fix_features"),
ethtool reported adapter->rx_csum, which was on by default.

Set NETIF_F_RXCSUM after e1000_check_options() when RX checksum offload
is enabled. With XsumRX=0 the feature stays off, as before.

Fixes: e97d3207c5e6 ("e1000: convert to ndo_fix_features")
Signed-off-by: Jiaming Fan <fan.jiaming@hotmail.com>
---
Tested on QEMU's 82540EM model. QEMU does not emulate RX checksum
offload on this model, so I traced the driver's MMIO writes to RXCSUM
instead:

                          before            after
  probe/open              0x200, "off"      0x200, "on"
  ethtool -K rx off       no write, "off"   0x0, "off"
  ethtool -K rx on        0x200, "on"       0x200, "on"
  ethtool -K rx off       0x0, "off"        0x0, "off"

With e1000.XsumRX=0 the patched driver writes 0x0 at open and reports
"off", as before.

 drivers/net/ethernet/intel/e1000/e1000_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index d7e279b..a52fff0 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1111,6 +1111,14 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	e1000_check_options(adapter);
 
+	/* The XsumRX option (enabled by default) decides whether RX checksum
+	 * offload starts enabled. Reflect it in the features so that ethtool
+	 * reports the real state and e1000_set_features() sees a change when
+	 * the user turns it off.
+	 */
+	if (hw->mac_type >= e1000_82543 && adapter->rx_csum)
+		netdev->features |= NETIF_F_RXCSUM;
+
 	/* Initial Wake on LAN setting
 	 * If APM wake is enabled in the EEPROM,
 	 * enable the ACPI Magic Packet filter
-- 
2.56.0