This patch implements the rx checksum offload feature
including NETIF_F_IP_CSUM NETIF_F_IPV6_CSUM and NETIF_F_RXCSUM
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
ChangeLog:
v2 -> v3:
- Remove .ndo_fix_features() suggested by Jakub Kicinski.
v2: https://lore.kernel.org/all/20250218085829.3172126-1-shaojijie@huawei.com/
v1 -> v2:
- Use !! to convert integer to boolean, suggested by Simon Horman.
v1: https://lore.kernel.org/all/20250213035529.2402283-1-shaojijie@huawei.com/
---
.../net/ethernet/hisilicon/hibmcge/hbg_main.c | 13 ++++++++++++
.../net/ethernet/hisilicon/hibmcge/hbg_txrx.c | 20 +++++++++++++++----
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c
index 969df6020942..688f408de84c 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c
@@ -14,6 +14,9 @@
#include "hbg_txrx.h"
#include "hbg_debugfs.h"
+#define HBG_SUPPORT_FEATURES (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | \
+ NETIF_F_RXCSUM)
+
static void hbg_all_irq_enable(struct hbg_priv *priv, bool enabled)
{
struct hbg_irq_info *info;
@@ -419,6 +422,9 @@ static int hbg_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
if (ret)
return ret;
+ /* set default features */
+ netdev->features |= HBG_SUPPORT_FEATURES;
+ netdev->hw_features |= HBG_SUPPORT_FEATURES;
netdev->priv_flags |= IFF_UNICAST_FLT;
netdev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS;
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c
index 35dd3512d00e..2c7657e8ba27 100644
--- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c
+++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c
@@ -202,8 +202,11 @@ static int hbg_napi_tx_recycle(struct napi_struct *napi, int budget)
}
static bool hbg_rx_check_l3l4_error(struct hbg_priv *priv,
- struct hbg_rx_desc *desc)
+ struct hbg_rx_desc *desc,
+ struct sk_buff *skb)
{
+ bool rx_checksum_offload = !!(priv->netdev->features & NETIF_F_RXCSUM);
+
if (likely(!FIELD_GET(HBG_RX_DESC_W4_L3_ERR_CODE_M, desc->word4) &&
!FIELD_GET(HBG_RX_DESC_W4_L4_ERR_CODE_M, desc->word4)))
return true;
@@ -215,6 +218,10 @@ static bool hbg_rx_check_l3l4_error(struct hbg_priv *priv,
priv->stats.rx_desc_l3_wrong_head_cnt++;
return false;
case HBG_L3_CSUM_ERR:
+ if (!rx_checksum_offload) {
+ skb->ip_summed = CHECKSUM_NONE;
+ break;
+ }
priv->stats.rx_desc_l3_csum_err_cnt++;
return false;
case HBG_L3_LEN_ERR:
@@ -238,6 +245,10 @@ static bool hbg_rx_check_l3l4_error(struct hbg_priv *priv,
priv->stats.rx_desc_l4_len_err_cnt++;
return false;
case HBG_L4_CSUM_ERR:
+ if (!rx_checksum_offload) {
+ skb->ip_summed = CHECKSUM_NONE;
+ break;
+ }
priv->stats.rx_desc_l4_csum_err_cnt++;
return false;
case HBG_L4_ZERO_PORT_NUM:
@@ -322,7 +333,8 @@ static void hbg_update_rx_protocol_stats(struct hbg_priv *priv,
hbg_update_rx_ip_protocol_stats(priv, desc);
}
-static bool hbg_rx_pkt_check(struct hbg_priv *priv, struct hbg_rx_desc *desc)
+static bool hbg_rx_pkt_check(struct hbg_priv *priv, struct hbg_rx_desc *desc,
+ struct sk_buff *skb)
{
if (unlikely(FIELD_GET(HBG_RX_DESC_W2_PKT_LEN_M, desc->word2) >
priv->dev_specs.max_frame_len)) {
@@ -342,7 +354,7 @@ static bool hbg_rx_pkt_check(struct hbg_priv *priv, struct hbg_rx_desc *desc)
return false;
}
- if (unlikely(!hbg_rx_check_l3l4_error(priv, desc))) {
+ if (unlikely(!hbg_rx_check_l3l4_error(priv, desc, skb))) {
priv->stats.rx_desc_l3l4_err_cnt++;
return false;
}
@@ -413,7 +425,7 @@ static int hbg_napi_rx_poll(struct napi_struct *napi, int budget)
rx_desc = (struct hbg_rx_desc *)buffer->skb->data;
pkt_len = FIELD_GET(HBG_RX_DESC_W2_PKT_LEN_M, rx_desc->word2);
- if (unlikely(!hbg_rx_pkt_check(priv, rx_desc))) {
+ if (unlikely(!hbg_rx_pkt_check(priv, rx_desc, buffer->skb))) {
hbg_buffer_free(buffer);
goto next_buffer;
}
--
2.33.0On Fri, 21 Feb 2025 19:55:22 +0800 Jijie Shao wrote: > +#define HBG_SUPPORT_FEATURES (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | \ these are tx not rx > + NETIF_F_RXCSUM) I don't see you setting the checksum to anything other than NONE
on 2025/2/25 11:09, Jakub Kicinski wrote: > On Fri, 21 Feb 2025 19:55:22 +0800 Jijie Shao wrote: >> +#define HBG_SUPPORT_FEATURES (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | \ > these are tx not rx Yes, the processing of the driver tx checksum is received along with the xmit() function, and here it's just set features. I may need to explain this in the commit log. > >> + NETIF_F_RXCSUM) > I don't see you setting the checksum to anything other than NONE When receiving packets, MAC checks the checksum by default. This behavior cannot be disabled. If the checksum is incorrect, the MAC notifies the driver through the descriptor. If checksum offload is enabled, the driver drops the packet. Otherwise, the driver set the checksum to NONE and sends the packet to the stack. Thanks, Jijie Shao
On Tue, 25 Feb 2025 17:00:45 +0800 Jijie Shao wrote: > >> + NETIF_F_RXCSUM) > > I don't see you setting the checksum to anything other than NONE > > When receiving packets, MAC checks the checksum by default. This behavior cannot be disabled. > If the checksum is incorrect, the MAC notifies the driver through the descriptor. > > If checksum offload is enabled, the driver drops the packet. > Otherwise, the driver set the checksum to NONE and sends the packet to the stack. Dropping packets with bad csum is not correct. Packets where device validated L4 csum should have csum set to UNNECESSARY, most likely. Please read the comment in skbuff.h
on 2025/2/26 0:23, Jakub Kicinski wrote: > On Tue, 25 Feb 2025 17:00:45 +0800 Jijie Shao wrote: >>>> + NETIF_F_RXCSUM) >>> I don't see you setting the checksum to anything other than NONE >> When receiving packets, MAC checks the checksum by default. This behavior cannot be disabled. >> If the checksum is incorrect, the MAC notifies the driver through the descriptor. >> >> If checksum offload is enabled, the driver drops the packet. >> Otherwise, the driver set the checksum to NONE and sends the packet to the stack. > Dropping packets with bad csum is not correct. > Packets where device validated L4 csum should have csum set > to UNNECESSARY, most likely. Please read the comment in skbuff.h Hi, is it ok below: rx checksum offload enable: device check ok -> CHECKSUM_UNNECESSARY -> stack device check fail -> drop rx checksum offload disable: device check ok -> CHECKSUM_NONE -> stack device check fail -> CHECKSUM_NONE -> stack Thanks Jijie Shao
On Thu, 27 Feb 2025 19:28:25 +0800 Jijie Shao wrote: > rx checksum offload enable: > device check ok -> CHECKSUM_UNNECESSARY -> stack > device check fail -> drop Don't drop packets on csum validation failure. The stack can easily handle packets with bad csum. And users will monitor stack metrics for csum errors. Plus devices are wrong more often than the stack.
on 2025/2/27 22:47, Jakub Kicinski wrote: > On Thu, 27 Feb 2025 19:28:25 +0800 Jijie Shao wrote: >> rx checksum offload enable: >> device check ok -> CHECKSUM_UNNECESSARY -> stack >> device check fail -> drop > Don't drop packets on csum validation failure. > The stack can easily handle packets with bad csum. > And users will monitor stack metrics for csum errors. > Plus devices are wrong more often than the stack. OK, I'll modify this in the v4 and modify the statistics as well. Thanks, Jijie Shao
© 2016 - 2025 Red Hat, Inc.