diff mbox series

[v1,1/2] net/hinic: fix outer_l3_len parse error

Message ID 39bc11d96f40e26afc0d1799def59003b697987a.1604115055.git.cloud.wangxiaoyun@huawei.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series fix some csum errors |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Wangxiaoyun (Cloud) Oct. 31, 2020, 3:38 a.m. UTC 
  This patch fixes outer_l3_len parse error when
PKT_TX_OUTER_IP_CKSUM is not set, which does not affect
checksum function, just be consistent with mbuf meta
information description.

Fixes: 8c8b61234ffd ("net/hinic: refactor checksum functions")
Cc: stable@dpdk.org
Signed-off-by: Xiaoyun wang <cloud.wangxiaoyun@huawei.com>
---
 drivers/net/hinic/hinic_pmd_tx.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

Comments

Ferruh Yigit Nov. 2, 2020, 5:08 p.m. UTC | #1 
On 10/31/2020 3:38 AM, Xiaoyun wang wrote:
> This patch fixes outer_l3_len parse error when
> PKT_TX_OUTER_IP_CKSUM is not set, which does not affect
> checksum function, just be consistent with mbuf meta
> information description.
> 
> Fixes: 8c8b61234ffd ("net/hinic: refactor checksum functions")
> Cc: stable@dpdk.org
> Signed-off-by: Xiaoyun wang <cloud.wangxiaoyun@huawei.com>
> ---
>   drivers/net/hinic/hinic_pmd_tx.c | 25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/hinic/hinic_pmd_tx.c b/drivers/net/hinic/hinic_pmd_tx.c
> index 2dd4fe1..125627e 100644
> --- a/drivers/net/hinic/hinic_pmd_tx.c
> +++ b/drivers/net/hinic/hinic_pmd_tx.c
> @@ -779,26 +779,25 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf *mbuf,
>   {
>   	struct rte_ether_hdr *eth_hdr;
>   	struct rte_vlan_hdr *vlan_hdr;
> -	struct rte_ipv4_hdr *ip4h;
> -	u16 pkt_type;
> -	u8 *hdr;
> +	struct rte_ipv4_hdr *ipv4_hdr;
> +	u16 eth_type;
>   
> -	hdr = (u8 *)rte_pktmbuf_mtod(mbuf, u8*);
> -	eth_hdr = (struct rte_ether_hdr *)hdr;
> -	pkt_type = rte_be_to_cpu_16(eth_hdr->ether_type);
> +	eth_hdr = rte_pktmbuf_mtod(mbuf, struct rte_ether_hdr *);
> +	eth_type = rte_be_to_cpu_16(eth_hdr->ether_type);
>   
> -	if (pkt_type == RTE_ETHER_TYPE_VLAN) {
> +	if (eth_type == RTE_ETHER_TYPE_VLAN) {
>   		off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
> -		vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
> -		pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
> +		vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
> +		eth_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
>   	} else {
>   		off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
>   	}
>   
> -	if (pkt_type == RTE_ETHER_TYPE_IPV4) {
> -		ip4h = (struct rte_ipv4_hdr *)(hdr + off_info->outer_l2_len);
> -		off_info->outer_l3_len = rte_ipv4_hdr_len(ip4h);
> -	} else if (pkt_type == RTE_ETHER_TYPE_IPV6) {
> +	if (eth_type == RTE_ETHER_TYPE_IPV4) {
> +		ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf, struct rte_ipv4_hdr *,
> +						   off_info->outer_l2_len);
> +		off_info->outer_l3_len = rte_ipv4_hdr_len(ipv4_hdr);
> +	} else if (eth_type == RTE_ETHER_TYPE_IPV6) {
>   		/* not support ipv6 extension header */
>   		off_info->outer_l3_len = sizeof(struct rte_ipv6_hdr);
>   	}
> 


The actual fix is following [1] and rest is refactoring, right?
It is hard to catch the actual fix with refactoring, can you please describe the 
actual problem and fix in the commit log to clarify it?



[1]
  @@ -789,7 +789,7 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf 
*mbuf,

          if (pkt_type == RTE_ETHER_TYPE_VLAN) {
                  off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
  -               vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
  +               vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
                  pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
          } else {
                  off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
Ferruh Yigit Nov. 2, 2020, 5:26 p.m. UTC | #2 
On 11/2/2020 5:08 PM, Ferruh Yigit wrote:
> On 10/31/2020 3:38 AM, Xiaoyun wang wrote:
>> This patch fixes outer_l3_len parse error when
>> PKT_TX_OUTER_IP_CKSUM is not set, which does not affect
>> checksum function, just be consistent with mbuf meta
>> information description.
>>
>> Fixes: 8c8b61234ffd ("net/hinic: refactor checksum functions")
>> Cc: stable@dpdk.org
>> Signed-off-by: Xiaoyun wang <cloud.wangxiaoyun@huawei.com>
>> ---
>>   drivers/net/hinic/hinic_pmd_tx.c | 25 ++++++++++++-------------
>>   1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/hinic/hinic_pmd_tx.c b/drivers/net/hinic/hinic_pmd_tx.c
>> index 2dd4fe1..125627e 100644
>> --- a/drivers/net/hinic/hinic_pmd_tx.c
>> +++ b/drivers/net/hinic/hinic_pmd_tx.c
>> @@ -779,26 +779,25 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf 
>> *mbuf,
>>   {
>>       struct rte_ether_hdr *eth_hdr;
>>       struct rte_vlan_hdr *vlan_hdr;
>> -    struct rte_ipv4_hdr *ip4h;
>> -    u16 pkt_type;
>> -    u8 *hdr;
>> +    struct rte_ipv4_hdr *ipv4_hdr;
>> +    u16 eth_type;
>> -    hdr = (u8 *)rte_pktmbuf_mtod(mbuf, u8*);
>> -    eth_hdr = (struct rte_ether_hdr *)hdr;
>> -    pkt_type = rte_be_to_cpu_16(eth_hdr->ether_type);
>> +    eth_hdr = rte_pktmbuf_mtod(mbuf, struct rte_ether_hdr *);
>> +    eth_type = rte_be_to_cpu_16(eth_hdr->ether_type);
>> -    if (pkt_type == RTE_ETHER_TYPE_VLAN) {
>> +    if (eth_type == RTE_ETHER_TYPE_VLAN) {
>>           off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
>> -        vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
>> -        pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
>> +        vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
>> +        eth_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
>>       } else {
>>           off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
>>       }
>> -    if (pkt_type == RTE_ETHER_TYPE_IPV4) {
>> -        ip4h = (struct rte_ipv4_hdr *)(hdr + off_info->outer_l2_len);
>> -        off_info->outer_l3_len = rte_ipv4_hdr_len(ip4h);
>> -    } else if (pkt_type == RTE_ETHER_TYPE_IPV6) {
>> +    if (eth_type == RTE_ETHER_TYPE_IPV4) {
>> +        ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf, struct rte_ipv4_hdr *,
>> +                           off_info->outer_l2_len);
>> +        off_info->outer_l3_len = rte_ipv4_hdr_len(ipv4_hdr);
>> +    } else if (eth_type == RTE_ETHER_TYPE_IPV6) {
>>           /* not support ipv6 extension header */
>>           off_info->outer_l3_len = sizeof(struct rte_ipv6_hdr);
>>       }
>>
> 
> 
> The actual fix is following [1] and rest is refactoring, right?
> It is hard to catch the actual fix with refactoring, can you please describe the 
> actual problem and fix in the commit log to clarify it?
> 
> 
> 
> [1]
>   @@ -789,7 +789,7 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf 
> *mbuf,
> 
>           if (pkt_type == RTE_ETHER_TYPE_VLAN) {
>                   off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
>   -               vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
>   +               vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
>                   pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
>           } else {
>                   off_info->outer_l2_len = ETHER_LEN_NO_VLAN;

Added following to the commit log while merging, please correct it if it is 
wrong/missing:

     The outer_l3_len is calculated wrong because 'vlan_hdr' is calculated
     wrong, 'vlan_hdr' fixed and code refactored.
Wangxiaoyun (Cloud) Nov. 4, 2020, 2:19 a.m. UTC | #3 
Yes,the actual fix is following as this patch:

commit 8c8b61234ffd9a283cecbf9751942cbdb87d68f6
Author: Xiaoyun Wang <cloud.wangxiaoyun@huawei.com>
Date:   Sat Jul 25 16:15:33 2020 +0800

     net/hinic: refactor checksum functions

     Encapsulate different types of packet checksum preprocessing
     into functions.


在 2020/11/3 1:08, Ferruh Yigit 写道:
> On 10/31/2020 3:38 AM, Xiaoyun wang wrote:
>> This patch fixes outer_l3_len parse error when
>> PKT_TX_OUTER_IP_CKSUM is not set, which does not affect
>> checksum function, just be consistent with mbuf meta
>> information description.
>>
>> Fixes: 8c8b61234ffd ("net/hinic: refactor checksum functions")
>> Cc: stable@dpdk.org
>> Signed-off-by: Xiaoyun wang <cloud.wangxiaoyun@huawei.com>
>> ---
>>   drivers/net/hinic/hinic_pmd_tx.c | 25 ++++++++++++-------------
>>   1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/hinic/hinic_pmd_tx.c b/drivers/net/hinic/hinic_pmd_tx.c
>> index 2dd4fe1..125627e 100644
>> --- a/drivers/net/hinic/hinic_pmd_tx.c
>> +++ b/drivers/net/hinic/hinic_pmd_tx.c
>> @@ -779,26 +779,25 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf *mbuf,
>>   {
>>       struct rte_ether_hdr *eth_hdr;
>>       struct rte_vlan_hdr *vlan_hdr;
>> -    struct rte_ipv4_hdr *ip4h;
>> -    u16 pkt_type;
>> -    u8 *hdr;
>> +    struct rte_ipv4_hdr *ipv4_hdr;
>> +    u16 eth_type;
>> -    hdr = (u8 *)rte_pktmbuf_mtod(mbuf, u8*);
>> -    eth_hdr = (struct rte_ether_hdr *)hdr;
>> -    pkt_type = rte_be_to_cpu_16(eth_hdr->ether_type);
>> +    eth_hdr = rte_pktmbuf_mtod(mbuf, struct rte_ether_hdr *);
>> +    eth_type = rte_be_to_cpu_16(eth_hdr->ether_type);
>> -    if (pkt_type == RTE_ETHER_TYPE_VLAN) {
>> +    if (eth_type == RTE_ETHER_TYPE_VLAN) {
>>           off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
>> -        vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
>> -        pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
>> +        vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
>> +        eth_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
>>       } else {
>>           off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
>>       }
>> -    if (pkt_type == RTE_ETHER_TYPE_IPV4) {
>> -        ip4h = (struct rte_ipv4_hdr *)(hdr + off_info->outer_l2_len);
>> -        off_info->outer_l3_len = rte_ipv4_hdr_len(ip4h);
>> -    } else if (pkt_type == RTE_ETHER_TYPE_IPV6) {
>> +    if (eth_type == RTE_ETHER_TYPE_IPV4) {
>> +        ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf, struct rte_ipv4_hdr *,
>> +                           off_info->outer_l2_len);
>> +        off_info->outer_l3_len = rte_ipv4_hdr_len(ipv4_hdr);
>> +    } else if (eth_type == RTE_ETHER_TYPE_IPV6) {
>>           /* not support ipv6 extension header */
>>           off_info->outer_l3_len = sizeof(struct rte_ipv6_hdr);
>>       }
>>
> 
> 
> The actual fix is following [1] and rest is refactoring, right?
> It is hard to catch the actual fix with refactoring, can you please describe the actual problem and fix in the commit log to clarify it?
> 
> 
> 
> [1]
>   @@ -789,7 +789,7 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf *mbuf,
> 
>           if (pkt_type == RTE_ETHER_TYPE_VLAN) {
>                   off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
>   -               vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
>   +               vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
>                   pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
>           } else {
>                   off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
> .
Ferruh Yigit Nov. 4, 2020, 11:05 a.m. UTC | #4 
On 11/4/2020 2:19 AM, Wangxiaoyun (Cloud) wrote:

<please don't top post, moved reply to down>

> 在 2020/11/3 1:08, Ferruh Yigit 写道:
>> On 10/31/2020 3:38 AM, Xiaoyun wang wrote:
>>> This patch fixes outer_l3_len parse error when
>>> PKT_TX_OUTER_IP_CKSUM is not set, which does not affect
>>> checksum function, just be consistent with mbuf meta
>>> information description.
>>>
>>> Fixes: 8c8b61234ffd ("net/hinic: refactor checksum functions")
>>> Cc: stable@dpdk.org
>>> Signed-off-by: Xiaoyun wang <cloud.wangxiaoyun@huawei.com>
>>> ---
>>>   drivers/net/hinic/hinic_pmd_tx.c | 25 ++++++++++++-------------
>>>   1 file changed, 12 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/net/hinic/hinic_pmd_tx.c b/drivers/net/hinic/hinic_pmd_tx.c
>>> index 2dd4fe1..125627e 100644
>>> --- a/drivers/net/hinic/hinic_pmd_tx.c
>>> +++ b/drivers/net/hinic/hinic_pmd_tx.c
>>> @@ -779,26 +779,25 @@ static inline void hinic_analyze_tx_info(struct 
>>> rte_mbuf *mbuf,
>>>   {
>>>       struct rte_ether_hdr *eth_hdr;
>>>       struct rte_vlan_hdr *vlan_hdr;
>>> -    struct rte_ipv4_hdr *ip4h;
>>> -    u16 pkt_type;
>>> -    u8 *hdr;
>>> +    struct rte_ipv4_hdr *ipv4_hdr;
>>> +    u16 eth_type;
>>> -    hdr = (u8 *)rte_pktmbuf_mtod(mbuf, u8*);
>>> -    eth_hdr = (struct rte_ether_hdr *)hdr;
>>> -    pkt_type = rte_be_to_cpu_16(eth_hdr->ether_type);
>>> +    eth_hdr = rte_pktmbuf_mtod(mbuf, struct rte_ether_hdr *);
>>> +    eth_type = rte_be_to_cpu_16(eth_hdr->ether_type);
>>> -    if (pkt_type == RTE_ETHER_TYPE_VLAN) {
>>> +    if (eth_type == RTE_ETHER_TYPE_VLAN) {
>>>           off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
>>> -        vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
>>> -        pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
>>> +        vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
>>> +        eth_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
>>>       } else {
>>>           off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
>>>       }
>>> -    if (pkt_type == RTE_ETHER_TYPE_IPV4) {
>>> -        ip4h = (struct rte_ipv4_hdr *)(hdr + off_info->outer_l2_len);
>>> -        off_info->outer_l3_len = rte_ipv4_hdr_len(ip4h);
>>> -    } else if (pkt_type == RTE_ETHER_TYPE_IPV6) {
>>> +    if (eth_type == RTE_ETHER_TYPE_IPV4) {
>>> +        ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf, struct rte_ipv4_hdr *,
>>> +                           off_info->outer_l2_len);
>>> +        off_info->outer_l3_len = rte_ipv4_hdr_len(ipv4_hdr);
>>> +    } else if (eth_type == RTE_ETHER_TYPE_IPV6) {
>>>           /* not support ipv6 extension header */
>>>           off_info->outer_l3_len = sizeof(struct rte_ipv6_hdr);
>>>       }
>>>
>>
>>
>> The actual fix is following [1] and rest is refactoring, right?
>> It is hard to catch the actual fix with refactoring, can you please describe 
>> the actual problem and fix in the commit log to clarify it?
>>
>>
>>
>> [1]
>>   @@ -789,7 +789,7 @@ static inline void hinic_analyze_tx_info(struct rte_mbuf 
>> *mbuf,
>>
>>           if (pkt_type == RTE_ETHER_TYPE_VLAN) {
>>                   off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
>>   -               vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
>>   +               vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
>>                   pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
>>           } else {
>>                   off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
>> .
 > Yes,the actual fix is following as this patch:
 >
 > commit 8c8b61234ffd9a283cecbf9751942cbdb87d68f6
 > Author: Xiaoyun Wang <cloud.wangxiaoyun@huawei.com>
 > Date:   Sat Jul 25 16:15:33 2020 +0800
 >
 >      net/hinic: refactor checksum functions
 >
 >      Encapsulate different types of packet checksum preprocessing
 >      into functions.
 >
 >

I am not asking the original patch that introduced the problem.
In this patch, the single line I put above is the only required change, right?
diff mbox series

Patch

diff --git a/drivers/net/hinic/hinic_pmd_tx.c b/drivers/net/hinic/hinic_pmd_tx.c
index 2dd4fe1..125627e 100644
--- a/drivers/net/hinic/hinic_pmd_tx.c
+++ b/drivers/net/hinic/hinic_pmd_tx.c
@@ -779,26 +779,25 @@  static inline void hinic_analyze_tx_info(struct rte_mbuf *mbuf,
 {
 	struct rte_ether_hdr *eth_hdr;
 	struct rte_vlan_hdr *vlan_hdr;
-	struct rte_ipv4_hdr *ip4h;
-	u16 pkt_type;
-	u8 *hdr;
+	struct rte_ipv4_hdr *ipv4_hdr;
+	u16 eth_type;
 
-	hdr = (u8 *)rte_pktmbuf_mtod(mbuf, u8*);
-	eth_hdr = (struct rte_ether_hdr *)hdr;
-	pkt_type = rte_be_to_cpu_16(eth_hdr->ether_type);
+	eth_hdr = rte_pktmbuf_mtod(mbuf, struct rte_ether_hdr *);
+	eth_type = rte_be_to_cpu_16(eth_hdr->ether_type);
 
-	if (pkt_type == RTE_ETHER_TYPE_VLAN) {
+	if (eth_type == RTE_ETHER_TYPE_VLAN) {
 		off_info->outer_l2_len = ETHER_LEN_WITH_VLAN;
-		vlan_hdr = (struct rte_vlan_hdr *)(hdr + 1);
-		pkt_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
+		vlan_hdr = (struct rte_vlan_hdr *)(eth_hdr + 1);
+		eth_type = rte_be_to_cpu_16(vlan_hdr->eth_proto);
 	} else {
 		off_info->outer_l2_len = ETHER_LEN_NO_VLAN;
 	}
 
-	if (pkt_type == RTE_ETHER_TYPE_IPV4) {
-		ip4h = (struct rte_ipv4_hdr *)(hdr + off_info->outer_l2_len);
-		off_info->outer_l3_len = rte_ipv4_hdr_len(ip4h);
-	} else if (pkt_type == RTE_ETHER_TYPE_IPV6) {
+	if (eth_type == RTE_ETHER_TYPE_IPV4) {
+		ipv4_hdr = rte_pktmbuf_mtod_offset(mbuf, struct rte_ipv4_hdr *,
+						   off_info->outer_l2_len);
+		off_info->outer_l3_len = rte_ipv4_hdr_len(ipv4_hdr);
+	} else if (eth_type == RTE_ETHER_TYPE_IPV6) {
 		/* not support ipv6 extension header */
 		off_info->outer_l3_len = sizeof(struct rte_ipv6_hdr);
 	}