diff mbox series

[net-next,3/7] net: hibmcge: Add rx checksum offload supported in this module

Message ID 20250213035529.2402283-4-shaojijie@huawei.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Support some enhances features for the HIBMCGE driver | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 94 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jijie Shao Feb. 13, 2025, 3:55 a.m. UTC 
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>
---
 .../net/ethernet/hisilicon/hibmcge/hbg_main.c | 13 ++++++++++++
 .../net/ethernet/hisilicon/hibmcge/hbg_txrx.c | 20 +++++++++++++++----
 2 files changed, 29 insertions(+), 4 deletions(-)

Comments

Simon Horman Feb. 17, 2025, 3:40 p.m. UTC | #1 
On Thu, Feb 13, 2025 at 11:55:25AM +0800, Jijie Shao wrote:
> 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>

...

> diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c
> index 8c631a9bcb6b..aa1d128a863b 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;

nit: I think this would be better expressed in a way that
     rx_checksum_offload is assigned a boolean value (completely untested).

	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;
Jijie Shao Feb. 18, 2025, 2:16 a.m. UTC | #2 
on 2025/2/17 23:40, Simon Horman wrote:
> On Thu, Feb 13, 2025 at 11:55:25AM +0800, Jijie Shao wrote:
>> 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>
> ...
>
>> diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c
>> index 8c631a9bcb6b..aa1d128a863b 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;
> nit: I think this would be better expressed in a way that
>       rx_checksum_offload is assigned a boolean value (completely untested).
>
> 	bool rx_checksum_offload = !!(priv->netdev->features & NETIF_F_RXCSUM);

Okay, I'll modify it in v2.

Thanks
Jijie Shao

>
>> +
>>   	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;
Kalesh Anakkur Purayil Feb. 18, 2025, 2:46 a.m. UTC | #3 
On Tue, Feb 18, 2025 at 7:47 AM Jijie Shao <shaojijie@huawei.com> wrote:
>
>
> on 2025/2/17 23:40, Simon Horman wrote:
> > On Thu, Feb 13, 2025 at 11:55:25AM +0800, Jijie Shao wrote:
> >> 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>
> > ...
> >
> >> diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c
> >> index 8c631a9bcb6b..aa1d128a863b 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;
> > nit: I think this would be better expressed in a way that
> >       rx_checksum_offload is assigned a boolean value (completely untested).
> >
> >       bool rx_checksum_offload = !!(priv->netdev->features & NETIF_F_RXCSUM);
>
> Okay, I'll modify it in v2.

Maybe you can remove " in this module" from the patch title as it is
implicit. This comment/suggestion applies to all patches in this
series.
>
> Thanks
> Jijie Shao
>
> >
> >> +
> >>      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;
>
Jijie Shao Feb. 18, 2025, 8:27 a.m. UTC | #4 
on 2025/2/18 10:46, Kalesh Anakkur Purayil wrote:
> On Tue, Feb 18, 2025 at 7:47 AM Jijie Shao <shaojijie@huawei.com> wrote:
>>
>> on 2025/2/17 23:40, Simon Horman wrote:
>>> On Thu, Feb 13, 2025 at 11:55:25AM +0800, Jijie Shao wrote:
>>>> 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>
>>> ...
>>>
>>>> diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c
>>>> index 8c631a9bcb6b..aa1d128a863b 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;
>>> nit: I think this would be better expressed in a way that
>>>        rx_checksum_offload is assigned a boolean value (completely untested).
>>>
>>>        bool rx_checksum_offload = !!(priv->netdev->features & NETIF_F_RXCSUM);
>> Okay, I'll modify it in v2.
> Maybe you can remove " in this module" from the patch title as it is
> implicit. This comment/suggestion applies to all patches in this
> series.

Sorry this may not have any bad effect,
so I don't plan to change it in V2.
If anyone else thinks it should be modified,
I will modify it.

Thanks a lot
Jijie Shao

>> Thanks
>> Jijie Shao
>>
>>>> +
>>>>       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;
>
Simon Horman Feb. 18, 2025, 1:42 p.m. UTC | #5 
On Tue, Feb 18, 2025 at 04:27:28PM +0800, Jijie Shao wrote:
> 
> on 2025/2/18 10:46, Kalesh Anakkur Purayil wrote:
> > On Tue, Feb 18, 2025 at 7:47 AM Jijie Shao <shaojijie@huawei.com> wrote:
> > > 
> > > on 2025/2/17 23:40, Simon Horman wrote:
> > > > On Thu, Feb 13, 2025 at 11:55:25AM +0800, Jijie Shao wrote:
> > > > > 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>
> > > > ...
> > > > 
> > > > > diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c
> > > > > index 8c631a9bcb6b..aa1d128a863b 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;
> > > > nit: I think this would be better expressed in a way that
> > > >        rx_checksum_offload is assigned a boolean value (completely untested).
> > > > 
> > > >        bool rx_checksum_offload = !!(priv->netdev->features & NETIF_F_RXCSUM);
> > > Okay, I'll modify it in v2.
> > Maybe you can remove " in this module" from the patch title as it is
> > implicit. This comment/suggestion applies to all patches in this
> > series.
> 
> Sorry this may not have any bad effect,
> so I don't plan to change it in V2.
> If anyone else thinks it should be modified,
> I will modify it.

I agree that a shorter subject would be better.
Jijie Shao Feb. 19, 2025, 1:15 a.m. UTC | #6 
on 2025/2/18 21:42, Simon Horman wrote:
> On Tue, Feb 18, 2025 at 04:27:28PM +0800, Jijie Shao wrote:
>> on 2025/2/18 10:46, Kalesh Anakkur Purayil wrote:
>>> On Tue, Feb 18, 2025 at 7:47 AM Jijie Shao <shaojijie@huawei.com> wrote:
>>>> on 2025/2/17 23:40, Simon Horman wrote:
>>>>> On Thu, Feb 13, 2025 at 11:55:25AM +0800, Jijie Shao wrote:
>>>>>> 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>
>>>>> ...
>>>>>
>>>>>> diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_txrx.c
>>>>>> index 8c631a9bcb6b..aa1d128a863b 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;
>>>>> nit: I think this would be better expressed in a way that
>>>>>         rx_checksum_offload is assigned a boolean value (completely untested).
>>>>>
>>>>>         bool rx_checksum_offload = !!(priv->netdev->features & NETIF_F_RXCSUM);
>>>> Okay, I'll modify it in v2.
>>> Maybe you can remove " in this module" from the patch title as it is
>>> implicit. This comment/suggestion applies to all patches in this
>>> series.
>> Sorry this may not have any bad effect,
>> so I don't plan to change it in V2.
>> If anyone else thinks it should be modified,
>> I will modify it.
> I agree that a shorter subject would be better.

ok,
v2 has been sent, I'll modify it if need to send v3.

Thanks,
Jijie Shao
diff mbox series

Patch

diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c
index 4667e51ccc2d..813f1a1c900f 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;
@@ -263,6 +266,12 @@  static void hbg_net_get_stats(struct net_device *netdev,
 	stats->rx_crc_errors += h_stats->rx_fcs_error_cnt;
 }
 
+static netdev_features_t hbg_net_fix_features(struct net_device *netdev,
+					      netdev_features_t features)
+{
+	return features & HBG_SUPPORT_FEATURES;
+}
+
 static const struct net_device_ops hbg_netdev_ops = {
 	.ndo_open		= hbg_net_open,
 	.ndo_stop		= hbg_net_stop,
@@ -273,6 +282,7 @@  static const struct net_device_ops hbg_netdev_ops = {
 	.ndo_tx_timeout		= hbg_net_tx_timeout,
 	.ndo_set_rx_mode	= hbg_net_set_rx_mode,
 	.ndo_get_stats64	= hbg_net_get_stats,
+	.ndo_fix_features	= hbg_net_fix_features,
 };
 
 static void hbg_service_task(struct work_struct *work)
@@ -419,6 +429,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 8c631a9bcb6b..aa1d128a863b 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;
 	}
@@ -419,7 +431,7 @@  static int hbg_napi_rx_poll(struct napi_struct *napi, int budget)
 			goto next_buffer;
 		}
 
-		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;
 		}