* [PATCH iwl-net v1 0/2] Rework ice Tx checksum @ 2026-04-28 7:06 Michal Swiatkowski 2026-04-28 7:06 ` [PATCH iwl-net v1 1/2] ice: always do GCS if hardware supports it Michal Swiatkowski 2026-04-28 7:06 ` [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 Michal Swiatkowski 0 siblings, 2 replies; 10+ messages in thread From: Michal Swiatkowski @ 2026-04-28 7:06 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, jramaseu, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov, Michal Swiatkowski Hi, Jakub find the problem with how ice handles Tx checksum offload [1]. The issue is that IP_CSUM/IPV6_CSUM should support only basic packet types. Kernel assumes that if there are extensions headers and there is no HW_CSUM software fallback should be applied. Here is a patch that introduced that [2]. However, software fallback can't be applied when TSO offload happens. That is the situation that this patchset is fixing. When TSO is enabled and IP_CSUM/IPV6_CSUM is on, kernel is assuming hardware will be able to calculate the checksum, but later on the extension headers are checked and if there is no HW_CSUM the skb_bad_offload() is called. Following the documentation ice driver is capable of HW_CSUM support, so switch from IP_CSUM/IPV6_CSUM to HW_CSUM. Other Intel drivers (even previous like i40e, ixgbe) also supports HW_CSUM. The HW_CSUM was used to indicate that GCS (Intel name for checksumming offload using additional descriptor) is support, which is variation of HW_CSUM. Even without GCS th HW_CSUM is supported. First patch is switching to use custom Tx ring flags to allow HW_CSUM usage. [1] https://lore.kernel.org/netdev/20260310150557.1138437-1-jramaseu@redhat.com/ [2] https://lore.kernel.org/all/5fbeecfc311ea182aa1d1c771725ab8b4cac515e.1729778144.git.benoit.monin@gmx.fr/ Michal Swiatkowski (2): ice: always do GCS if hardware supports it ice: use NETIF_F_HW_CSUM instead of IP/IPV6 drivers/net/ethernet/intel/ice/ice_lib.c | 4 ++++ drivers/net/ethernet/intel/ice/ice_main.c | 21 +-------------------- drivers/net/ethernet/intel/ice/ice_txrx.c | 2 +- drivers/net/ethernet/intel/ice/ice_txrx.h | 1 + 4 files changed, 7 insertions(+), 21 deletions(-) -- 2.49.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH iwl-net v1 1/2] ice: always do GCS if hardware supports it 2026-04-28 7:06 [PATCH iwl-net v1 0/2] Rework ice Tx checksum Michal Swiatkowski @ 2026-04-28 7:06 ` Michal Swiatkowski 2026-04-28 7:06 ` [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 Michal Swiatkowski 1 sibling, 0 replies; 10+ messages in thread From: Michal Swiatkowski @ 2026-04-28 7:06 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, jramaseu, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov, Michal Swiatkowski There is no need to check for NETIF_HW_CSUM. If the code reach calculating checksum it means that correct checksum flags are set, because kernel is checking that when setting ip->summed. Instead of netdev feature flag use Tx ring flag to check if the hardware can use special descriptor for checksum calculating. Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> --- drivers/net/ethernet/intel/ice/ice_lib.c | 4 ++++ drivers/net/ethernet/intel/ice/ice_txrx.c | 2 +- drivers/net/ethernet/intel/ice/ice_txrx.h | 1 + 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c index 837b71b7b2b7..033fabc22f58 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -1415,6 +1415,10 @@ static int ice_vsi_alloc_rings(struct ice_vsi *vsi) set_bit(ICE_TX_RING_FLAGS_VLAN_L2TAG2, ring->flags); else set_bit(ICE_TX_RING_FLAGS_VLAN_L2TAG1, ring->flags); + + if (ice_is_feature_supported(pf, ICE_F_GCS)) + set_bit(ICE_TX_RING_FLAGS_GCS, ring->flags); + WRITE_ONCE(vsi->tx_rings[i], ring); } diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c index 4ca1a0602307..25b19a5d817e 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx.c +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c @@ -1743,7 +1743,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off) l3_len = l4.hdr - ip.hdr; offset |= (l3_len / 4) << ICE_TX_DESC_LEN_IPLEN_S; - if ((tx_ring->netdev->features & NETIF_F_HW_CSUM) && + if (test_bit(ICE_TX_RING_FLAGS_GCS, tx_ring->flags) && !(first->tx_flags & ICE_TX_FLAGS_TSO) && !skb_csum_is_sctp(skb)) { /* Set GCS */ diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h index 5e517f219379..15dbd5100912 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx.h +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h @@ -217,6 +217,7 @@ enum ice_tx_ring_flags { ICE_TX_RING_FLAGS_VLAN_L2TAG1, ICE_TX_RING_FLAGS_VLAN_L2TAG2, ICE_TX_RING_FLAGS_TXTIME, + ICE_TX_RING_FLAGS_GCS, ICE_TX_RING_FLAGS_NBITS, }; -- 2.49.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 2026-04-28 7:06 [PATCH iwl-net v1 0/2] Rework ice Tx checksum Michal Swiatkowski 2026-04-28 7:06 ` [PATCH iwl-net v1 1/2] ice: always do GCS if hardware supports it Michal Swiatkowski @ 2026-04-28 7:06 ` Michal Swiatkowski 2026-04-28 8:34 ` Loktionov, Aleksandr ` (2 more replies) 1 sibling, 3 replies; 10+ messages in thread From: Michal Swiatkowski @ 2026-04-28 7:06 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, jramaseu, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov, Michal Swiatkowski The hardware is capable of calculating checksum for IPV6 packets with extension header. To not drop such packets switch from IP/IPV6 checksum to HW_CSUM. HW_CSUM is also used in previous generation (i40e). Previously HW_CSUM was used to indicate that hardware supports general checksum. Drop it assuming that if the hardware supports it, it is used. Disabling offload for E830 in case of TSO isn't needed anymore as the check for TSO is done in Tx path just before preparation of the special GCS descriptor. The commit from Fixes didn't introduce a bug, it just shown that the driver is doing sth wrong with the checksum features. Suggested-by: Jakub Ramaseuski <jramaseu@redhat.com> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Fixes: 04c20a9356f2 ("net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension") Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> --- drivers/net/ethernet/intel/ice/ice_main.c | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 15550216fbf0..0f2f949af536 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -3500,9 +3500,8 @@ void ice_set_netdev_features(struct net_device *netdev) NETIF_F_RXHASH; csumo_features = NETIF_F_RXCSUM | - NETIF_F_IP_CSUM | NETIF_F_SCTP_CRC | - NETIF_F_IPV6_CSUM; + NETIF_F_HW_CSUM; vlano_features = NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_CTAG_TX | @@ -3564,12 +3563,6 @@ void ice_set_netdev_features(struct net_device *netdev) /* Allow core to manage IRQs affinity */ netif_set_affinity_auto(netdev); - /* Mutual exclusivity for TSO and GCS is enforced by the set features - * ndo callback. - */ - if (ice_is_feature_supported(pf, ICE_F_GCS)) - netdev->hw_features |= NETIF_F_HW_CSUM; - netif_set_tso_max_size(netdev, ICE_MAX_TSO_SIZE); } @@ -6489,18 +6482,6 @@ ice_set_features(struct net_device *netdev, netdev_features_t features) if (changed & NETIF_F_LOOPBACK) ret = ice_set_loopback(vsi, !!(features & NETIF_F_LOOPBACK)); - /* Due to E830 hardware limitations, TSO (NETIF_F_ALL_TSO) with GCS - * (NETIF_F_HW_CSUM) is not supported. - */ - if (ice_is_feature_supported(pf, ICE_F_GCS) && - ((features & NETIF_F_HW_CSUM) && (features & NETIF_F_ALL_TSO))) { - if (netdev->features & NETIF_F_HW_CSUM) - dev_err(ice_pf_to_dev(pf), "To enable TSO, you must first disable HW checksum.\n"); - else - dev_err(ice_pf_to_dev(pf), "To enable HW checksum, you must first disable TSO.\n"); - return -EIO; - } - return ret; } -- 2.49.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 2026-04-28 7:06 ` [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 Michal Swiatkowski @ 2026-04-28 8:34 ` Loktionov, Aleksandr 2026-04-30 15:29 ` Simon Horman 2026-05-04 23:53 ` [Intel-wired-lan] " Jacob Keller 2 siblings, 0 replies; 10+ messages in thread From: Loktionov, Aleksandr @ 2026-04-28 8:34 UTC (permalink / raw) To: Michal Swiatkowski, intel-wired-lan@lists.osuosl.org Cc: netdev@vger.kernel.org, jramaseu@redhat.com, Nguyen, Anthony L, Kitszel, Przemyslaw > -----Original Message----- > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > Sent: Tuesday, April 28, 2026 9:07 AM > To: intel-wired-lan@lists.osuosl.org > Cc: netdev@vger.kernel.org; jramaseu@redhat.com; Nguyen, Anthony L > <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw > <przemyslaw.kitszel@intel.com>; Loktionov, Aleksandr > <aleksandr.loktionov@intel.com>; Michal Swiatkowski > <michal.swiatkowski@linux.intel.com> > Subject: [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of > IP/IPV6 > > The hardware is capable of calculating checksum for IPV6 packets with > extension header. To not drop such packets switch from IP/IPV6 > checksum to HW_CSUM. I'd recommend "To not drop" -> "To avoid dropping" > > HW_CSUM is also used in previous generation (i40e). > > Previously HW_CSUM was used to indicate that hardware supports general > checksum. Drop it assuming that if the hardware supports it, it is > used. > > Disabling offload for E830 in case of TSO isn't needed anymore as the > check for TSO is done in Tx path just before preparation of the > special GCS descriptor. > > The commit from Fixes didn't introduce a bug, it just shown that the > driver is doing sth wrong with the checksum features. > Except commit message nits Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > Suggested-by: Jakub Ramaseuski <jramaseu@redhat.com> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Fixes: 04c20a9356f2 ("net: skip offload for NETIF_F_IPV6_CSUM if ipv6 > header contains extension") > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > --- > drivers/net/ethernet/intel/ice/ice_main.c | 21 +-------------------- > 1 file changed, 1 insertion(+), 20 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c > b/drivers/net/ethernet/intel/ice/ice_main.c > index 15550216fbf0..0f2f949af536 100644 > --- a/drivers/net/ethernet/intel/ice/ice_main.c > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > @@ -3500,9 +3500,8 @@ void ice_set_netdev_features(struct net_device > *netdev) > NETIF_F_RXHASH; > > csumo_features = NETIF_F_RXCSUM | > - NETIF_F_IP_CSUM | > NETIF_F_SCTP_CRC | > - NETIF_F_IPV6_CSUM; > + NETIF_F_HW_CSUM; > > vlano_features = NETIF_F_HW_VLAN_CTAG_FILTER | > NETIF_F_HW_VLAN_CTAG_TX | > @@ -3564,12 +3563,6 @@ void ice_set_netdev_features(struct net_device > *netdev) > /* Allow core to manage IRQs affinity */ > netif_set_affinity_auto(netdev); > > - /* Mutual exclusivity for TSO and GCS is enforced by the set > features > - * ndo callback. > - */ > - if (ice_is_feature_supported(pf, ICE_F_GCS)) > - netdev->hw_features |= NETIF_F_HW_CSUM; > - > netif_set_tso_max_size(netdev, ICE_MAX_TSO_SIZE); } > > @@ -6489,18 +6482,6 @@ ice_set_features(struct net_device *netdev, > netdev_features_t features) > if (changed & NETIF_F_LOOPBACK) > ret = ice_set_loopback(vsi, !!(features & > NETIF_F_LOOPBACK)); > > - /* Due to E830 hardware limitations, TSO (NETIF_F_ALL_TSO) with > GCS > - * (NETIF_F_HW_CSUM) is not supported. > - */ > - if (ice_is_feature_supported(pf, ICE_F_GCS) && > - ((features & NETIF_F_HW_CSUM) && (features & > NETIF_F_ALL_TSO))) { > - if (netdev->features & NETIF_F_HW_CSUM) > - dev_err(ice_pf_to_dev(pf), "To enable TSO, you > must first disable HW checksum.\n"); > - else > - dev_err(ice_pf_to_dev(pf), "To enable HW > checksum, you must first disable TSO.\n"); > - return -EIO; > - } > - > return ret; > } > > -- > 2.49.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 2026-04-28 7:06 ` [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 Michal Swiatkowski 2026-04-28 8:34 ` Loktionov, Aleksandr @ 2026-04-30 15:29 ` Simon Horman 2026-04-30 16:21 ` Simon Horman 2026-05-04 23:53 ` [Intel-wired-lan] " Jacob Keller 2 siblings, 1 reply; 10+ messages in thread From: Simon Horman @ 2026-04-30 15:29 UTC (permalink / raw) To: michal.swiatkowski Cc: 'Simon Horman', intel-wired-lan, netdev, jramaseu, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov From: 'Simon Horman' <horms@kernel.org> 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 --- ice: use NETIF_F_HW_CSUM instead of IP/IPV6 This commit replaces the IP and IPV6 checksum offload flags with HW_CSUM in the ice driver to allow hardware checksumming for IPv6 packets with extension headers. It also removes the mutual exclusivity check between TSO and GCS. > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c > index 5f92377d4dfc..33f252a6a22f 100644 > --- a/drivers/net/ethernet/intel/ice/ice_main.c > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > @@ -3491,9 +3491,8 @@ void ice_set_netdev_features(struct net_device *netdev) > NETIF_F_RXHASH; > > csumo_features = NETIF_F_RXCSUM | > - NETIF_F_IP_CSUM | > NETIF_F_SCTP_CRC | > - NETIF_F_IPV6_CSUM; > + NETIF_F_HW_CSUM; By advertising NETIF_F_HW_CSUM, the network stack assumes the driver can handle checksums for any protocol and passes non-IP packets (e.g., from AF_PACKET injected frames) to the driver with CHECKSUM_PARTIAL. Since ice_tx_csum() strictly parses the L3 header and expects IPv4 or IPv6, returning -1 for other protocols, will this regression cause non-IP packets to be silently dropped instead of falling back to software checksums in the kernel? Additionally, skb_csum_hwoffload_help() relies on NETIF_F_IPV6_CSUM to safely fall back to software checksums when IPv6 extension headers are present. Does using NETIF_F_HW_CSUM bypass this safeguard, potentially causing legacy hardware (like E810) or E830 without GCS descriptors to compute incorrect pseudo-headers and silently corrupt IPv6 packets with extension headers? Finally, for protocols other than TCP, UDP, or SCTP, the driver falls back to calling skb_checksum_help(skb) in the default case of ice_tx_csum(). However, the driver appears to ignore the return value of skb_checksum_help(). If skb_checksum_help() fails (e.g., due to -ENOMEM during pskb_expand_head()), could this result in the packet being transmitted with an uncomputed checksum? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 2026-04-30 15:29 ` Simon Horman @ 2026-04-30 16:21 ` Simon Horman 2026-05-04 8:09 ` Michal Swiatkowski 0 siblings, 1 reply; 10+ messages in thread From: Simon Horman @ 2026-04-30 16:21 UTC (permalink / raw) To: michal.swiatkowski Cc: intel-wired-lan, netdev, jramaseu, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov On Thu, Apr 30, 2026 at 04:29:49PM +0100, Simon Horman wrote: > From: 'Simon Horman' <horms@kernel.org> > > 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 Sorry, the email I am responding to was supposed to come with some commentary - it seems my method for adding it is broken :( So I'm adding it below (as best as I can remember what it was). > --- > ice: use NETIF_F_HW_CSUM instead of IP/IPV6 > > This commit replaces the IP and IPV6 checksum offload flags with HW_CSUM > in the ice driver to allow hardware checksumming for IPv6 packets with > extension headers. It also removes the mutual exclusivity check between > TSO and GCS. > > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c > > index 5f92377d4dfc..33f252a6a22f 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_main.c > > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > > @@ -3491,9 +3491,8 @@ void ice_set_netdev_features(struct net_device *netdev) > > NETIF_F_RXHASH; > > > > csumo_features = NETIF_F_RXCSUM | > > - NETIF_F_IP_CSUM | > > NETIF_F_SCTP_CRC | > > - NETIF_F_IPV6_CSUM; > > + NETIF_F_HW_CSUM; > > By advertising NETIF_F_HW_CSUM, the network stack assumes the driver can handle > checksums for any protocol and passes non-IP packets (e.g., from AF_PACKET > injected frames) to the driver with CHECKSUM_PARTIAL. > > Since ice_tx_csum() strictly parses the L3 header and expects IPv4 or IPv6, > returning -1 for other protocols, will this regression cause non-IP packets to > be silently dropped instead of falling back to software checksums in the kernel? The problem above seems to be a legitimate concern, if a corner case. It seems to also exist in i40e. > Additionally, skb_csum_hwoffload_help() relies on NETIF_F_IPV6_CSUM to > safely fall back to software checksums when IPv6 extension headers are > present. > > Does using NETIF_F_HW_CSUM bypass this safeguard, potentially causing > legacy hardware (like E810) or E830 without GCS descriptors to compute > incorrect pseudo-headers and silently corrupt IPv6 packets with extension > headers? The issue above seems to be a false positive as the ice driver doesn't rely on the behaviour described. Rather ice_tx_csum() uses ipv6_skip_exthdr() to correctly handle extension headers. > > Finally, for protocols other than TCP, UDP, or SCTP, the driver falls back to > calling skb_checksum_help(skb) in the default case of ice_tx_csum(). > > However, the driver appears to ignore the return value of skb_checksum_help(). > > If skb_checksum_help() fails (e.g., due to -ENOMEM during pskb_expand_head()), > could this result in the packet being transmitted with an uncomputed checksum? The problem above seems real to me, but it also seems to be pre-existing. So I don't think it should delay progress of this patch. If it is a problem, it also seems to be present in i40e. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 2026-04-30 16:21 ` Simon Horman @ 2026-05-04 8:09 ` Michal Swiatkowski 0 siblings, 0 replies; 10+ messages in thread From: Michal Swiatkowski @ 2026-05-04 8:09 UTC (permalink / raw) To: Simon Horman Cc: michal.swiatkowski, intel-wired-lan, netdev, jramaseu, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov On Thu, Apr 30, 2026 at 05:21:49PM +0100, Simon Horman wrote: > On Thu, Apr 30, 2026 at 04:29:49PM +0100, Simon Horman wrote: > > From: 'Simon Horman' <horms@kernel.org> > > > > 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 > > Sorry, the email I am responding to was supposed to come with > some commentary - it seems my method for adding it is broken :( > So I'm adding it below (as best as I can remember what it was). > Thanks for adding the comments. > > --- > > ice: use NETIF_F_HW_CSUM instead of IP/IPV6 > > > > This commit replaces the IP and IPV6 checksum offload flags with HW_CSUM > > in the ice driver to allow hardware checksumming for IPv6 packets with > > extension headers. It also removes the mutual exclusivity check between > > TSO and GCS. > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c > > > index 5f92377d4dfc..33f252a6a22f 100644 > > > --- a/drivers/net/ethernet/intel/ice/ice_main.c > > > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > > > @@ -3491,9 +3491,8 @@ void ice_set_netdev_features(struct net_device *netdev) > > > NETIF_F_RXHASH; > > > > > > csumo_features = NETIF_F_RXCSUM | > > > - NETIF_F_IP_CSUM | > > > NETIF_F_SCTP_CRC | > > > - NETIF_F_IPV6_CSUM; > > > + NETIF_F_HW_CSUM; > > > > By advertising NETIF_F_HW_CSUM, the network stack assumes the driver can handle > > checksums for any protocol and passes non-IP packets (e.g., from AF_PACKET > > injected frames) to the driver with CHECKSUM_PARTIAL. > > > > Since ice_tx_csum() strictly parses the L3 header and expects IPv4 or IPv6, > > returning -1 for other protocols, will this regression cause non-IP packets to > > be silently dropped instead of falling back to software checksums in the kernel? > > The problem above seems to be a legitimate concern, if a corner case. > It seems to also exist in i40e. > I think there is a lack of skb_checksum_help() before one error return from ice_tx_csum(). I will add it. > > Additionally, skb_csum_hwoffload_help() relies on NETIF_F_IPV6_CSUM to > > safely fall back to software checksums when IPv6 extension headers are > > present. > > > > Does using NETIF_F_HW_CSUM bypass this safeguard, potentially causing > > legacy hardware (like E810) or E830 without GCS descriptors to compute > > incorrect pseudo-headers and silently corrupt IPv6 packets with extension > > headers? > > The issue above seems to be a false positive as the ice driver > doesn't rely on the behaviour described. Rather ice_tx_csum() > uses ipv6_skip_exthdr() to correctly handle extension headers. > Yeah, righ. > > > > Finally, for protocols other than TCP, UDP, or SCTP, the driver falls back to > > calling skb_checksum_help(skb) in the default case of ice_tx_csum(). > > > > However, the driver appears to ignore the return value of skb_checksum_help(). > > > > If skb_checksum_help() fails (e.g., due to -ENOMEM during pskb_expand_head()), > > could this result in the packet being transmitted with an uncomputed checksum? > > The problem above seems real to me, but it also seems to be pre-existing. > So I don't think it should delay progress of this patch. > > If it is a problem, it also seems to be present in i40e. idpf, igbe too. It should return error in such case and drop the packet. What do you think, should I add the patch for that here or send it separately? To be honest I prefer sending it separately as this is in already exsisting code and touch more than one driver. Thanks ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 2026-04-28 7:06 ` [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 Michal Swiatkowski 2026-04-28 8:34 ` Loktionov, Aleksandr 2026-04-30 15:29 ` Simon Horman @ 2026-05-04 23:53 ` Jacob Keller 2026-05-05 4:35 ` Michal Swiatkowski 2 siblings, 1 reply; 10+ messages in thread From: Jacob Keller @ 2026-05-04 23:53 UTC (permalink / raw) To: Michal Swiatkowski, intel-wired-lan Cc: netdev, jramaseu, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov On 4/28/2026 12:06 AM, Michal Swiatkowski wrote: > The hardware is capable of calculating checksum for IPV6 packets with > extension header. To not drop such packets switch from IP/IPV6 checksum > to HW_CSUM. > > HW_CSUM is also used in previous generation (i40e). > > Previously HW_CSUM was used to indicate that hardware supports general > checksum. Drop it assuming that if the hardware supports it, it is used. > > Disabling offload for E830 in case of TSO isn't needed anymore as the > check for TSO is done in Tx path just before preparation of the special > GCS descriptor. > > The commit from Fixes didn't introduce a bug, it just shown that the > driver is doing sth wrong with the checksum features. > > Suggested-by: Jakub Ramaseuski <jramaseu@redhat.com> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Fixes: 04c20a9356f2 ("net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension") > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > --- Am I correct in thinking that this supersedes (really, properly fixes) the patch "ice: enable NETIF_F_HW_CSUM for GSO packets" at https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20260310150557.1138437-1-jramaseu@redhat.com/ ? Thanks, Jake ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 2026-05-04 23:53 ` [Intel-wired-lan] " Jacob Keller @ 2026-05-05 4:35 ` Michal Swiatkowski 2026-05-05 5:16 ` Jacob Keller 0 siblings, 1 reply; 10+ messages in thread From: Michal Swiatkowski @ 2026-05-05 4:35 UTC (permalink / raw) To: Jacob Keller Cc: Michal Swiatkowski, intel-wired-lan, netdev, jramaseu, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov On Mon, May 04, 2026 at 04:53:12PM -0700, Jacob Keller wrote: > On 4/28/2026 12:06 AM, Michal Swiatkowski wrote: > > The hardware is capable of calculating checksum for IPV6 packets with > > extension header. To not drop such packets switch from IP/IPV6 checksum > > to HW_CSUM. > > > > HW_CSUM is also used in previous generation (i40e). > > > > Previously HW_CSUM was used to indicate that hardware supports general > > checksum. Drop it assuming that if the hardware supports it, it is used. > > > > Disabling offload for E830 in case of TSO isn't needed anymore as the > > check for TSO is done in Tx path just before preparation of the special > > GCS descriptor. > > > > The commit from Fixes didn't introduce a bug, it just shown that the > > driver is doing sth wrong with the checksum features. > > > > Suggested-by: Jakub Ramaseuski <jramaseu@redhat.com> > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > Fixes: 04c20a9356f2 ("net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension") > > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > --- > Am I correct in thinking that this supersedes (really, properly fixes) > the patch "ice: enable NETIF_F_HW_CSUM for GSO packets" at > https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20260310150557.1138437-1-jramaseu@redhat.com/ > ? > > Thanks, > Jake Yes, exactly. I think I linked it in cover letter, but maybe I should do it also here. Thanks ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 2026-05-05 4:35 ` Michal Swiatkowski @ 2026-05-05 5:16 ` Jacob Keller 0 siblings, 0 replies; 10+ messages in thread From: Jacob Keller @ 2026-05-05 5:16 UTC (permalink / raw) To: Michal Swiatkowski Cc: intel-wired-lan, netdev, jramaseu, anthony.l.nguyen, przemyslaw.kitszel, aleksandr.loktionov On 5/4/2026 9:35 PM, Michal Swiatkowski wrote: > On Mon, May 04, 2026 at 04:53:12PM -0700, Jacob Keller wrote: >> On 4/28/2026 12:06 AM, Michal Swiatkowski wrote: >>> The hardware is capable of calculating checksum for IPV6 packets with >>> extension header. To not drop such packets switch from IP/IPV6 checksum >>> to HW_CSUM. >>> >>> HW_CSUM is also used in previous generation (i40e). >>> >>> Previously HW_CSUM was used to indicate that hardware supports general >>> checksum. Drop it assuming that if the hardware supports it, it is used. >>> >>> Disabling offload for E830 in case of TSO isn't needed anymore as the >>> check for TSO is done in Tx path just before preparation of the special >>> GCS descriptor. >>> >>> The commit from Fixes didn't introduce a bug, it just shown that the >>> driver is doing sth wrong with the checksum features. >>> >>> Suggested-by: Jakub Ramaseuski <jramaseu@redhat.com> >>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >>> Fixes: 04c20a9356f2 ("net: skip offload for NETIF_F_IPV6_CSUM if ipv6 header contains extension") >>> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> >>> --- >> Am I correct in thinking that this supersedes (really, properly fixes) >> the patch "ice: enable NETIF_F_HW_CSUM for GSO packets" at >> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20260310150557.1138437-1-jramaseu@redhat.com/ >> ? >> >> Thanks, >> Jake > > Yes, exactly. I think I linked it in cover letter, but maybe I should do > it also here. > > Thanks I think its fine, I just wanted to make sure I was correct in marking that patch as rejected in patchwork. Thanks, Jake ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-05-05 5:16 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-04-28 7:06 [PATCH iwl-net v1 0/2] Rework ice Tx checksum Michal Swiatkowski 2026-04-28 7:06 ` [PATCH iwl-net v1 1/2] ice: always do GCS if hardware supports it Michal Swiatkowski 2026-04-28 7:06 ` [PATCH iwl-net v1 2/2] ice: use NETIF_F_HW_CSUM instead of IP/IPV6 Michal Swiatkowski 2026-04-28 8:34 ` Loktionov, Aleksandr 2026-04-30 15:29 ` Simon Horman 2026-04-30 16:21 ` Simon Horman 2026-05-04 8:09 ` Michal Swiatkowski 2026-05-04 23:53 ` [Intel-wired-lan] " Jacob Keller 2026-05-05 4:35 ` Michal Swiatkowski 2026-05-05 5:16 ` Jacob Keller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox