diff mbox series

[2/2] app/testpmd: fix incorrect MTU verification

Message ID 20220406084537.16799-3-humin29@huawei.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series bugfix for testpmd |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

humin (Q) April 6, 2022, 8:45 a.m. UTC 
  From: Huisong Li <lihuisong@huawei.com>

The macro RTE_ETHER_MIN_LEN isn't the minimum value of MTU. But testpmd
used it when execute 'port config mtu 0 xx' cmd. This patch fix it.

Fixes: 1bb4a528c41f ("ethdev: fix max Rx packet length")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 app/test-pmd/cmdline.c |  4 ---
 app/test-pmd/config.c  | 55 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 4 deletions(-)

Comments

Singh, Aman Deep April 25, 2022, 4:25 p.m. UTC | #1 
On 4/6/2022 2:15 PM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
>
> The macro RTE_ETHER_MIN_LEN isn't the minimum value of MTU. But testpmd
> used it when execute 'port config mtu 0 xx' cmd. This patch fix it.
>
> Fixes: 1bb4a528c41f ("ethdev: fix max Rx packet length")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>   app/test-pmd/cmdline.c |  4 ---
>   app/test-pmd/config.c  | 55 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 6ffea8e21a..91e4090582 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -2050,10 +2050,6 @@ cmd_config_mtu_parsed(void *parsed_result,
>   {
>   	struct cmd_config_mtu_result *res = parsed_result;
>   
> -	if (res->value < RTE_ETHER_MIN_LEN) {
> -		fprintf(stderr, "mtu cannot be less than %d\n", RTE_ETHER_MIN_LEN);
> -		return;
> -	}
>   	port_mtu_set(res->port_id, res->value);
>   }
>   
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index bd689f9f86..1b1e738f83 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1254,6 +1254,57 @@ port_reg_set(portid_t port_id, uint32_t reg_off, uint32_t reg_v)
>   	display_port_reg_value(port_id, reg_off, reg_v);
>   }
>   
> +static uint32_t
> +eth_dev_get_overhead_len(uint32_t max_rx_pktlen, uint16_t max_mtu)
> +{
> +	uint32_t overhead_len;
> +
> +	if (max_mtu != UINT16_MAX && max_rx_pktlen > max_mtu)
> +		overhead_len = max_rx_pktlen - max_mtu;
> +	else
> +		overhead_len = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
> +
> +	return overhead_len;
> +}
> +
> +static int
> +eth_dev_validate_mtu(uint16_t port_id, uint16_t mtu)
> +{
> +	struct rte_eth_dev_info dev_info;
> +	uint32_t overhead_len;
> +	uint32_t frame_size;
> +	int ret;
> +
> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (mtu < dev_info.min_mtu) {
> +		fprintf(stderr,
> +			"MTU (%u) < device min MTU (%u) for port_id %u\n",
> +			mtu, dev_info.min_mtu, port_id);
> +		return -EINVAL;
> +	}
> +	if (mtu > dev_info.max_mtu) {
> +		fprintf(stderr,
> +			"MTU (%u) > device max MTU (%u) for port_id %u\n",
> +			mtu, dev_info.max_mtu, port_id);
> +		return -EINVAL;
> +	}
> +
> +	overhead_len = eth_dev_get_overhead_len(dev_info.max_rx_pktlen,
> +			dev_info.max_mtu);
> +	frame_size = mtu + overhead_len;
> +	if (frame_size > dev_info.max_rx_pktlen) {
> +		fprintf(stderr,
> +			"Frame size (%u) > device max frame size (%u) for port_id %u\n",
> +			frame_size, dev_info.max_rx_pktlen, port_id);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>   void
>   port_mtu_set(portid_t port_id, uint16_t mtu)
>   {
> @@ -1263,6 +1314,10 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>   	if (port_id_is_invalid(port_id, ENABLED_WARN))
>   		return;
>   
> +	diag = eth_dev_validate_mtu(port_id, mtu);
> +	if (diag != 0)
> +		return;
> +
>   	if (port->need_reconfig == 0) {
>   		diag = rte_eth_dev_set_mtu(port_id, mtu);
>   		if (diag != 0) {
I just wanted to know if these added functions eth_dev_validate_mtu() &  
eth_dev_get_overhead_len()
are copy of ethdev library API's in file "rte_ethdev.c", which get 
called by rte_eth_dev_set_mtu.
Is our intent, is to call these twice ?
lihuisong (C) April 26, 2022, 1:38 a.m. UTC | #2 
在 2022/4/26 0:25, Singh, Aman Deep 写道:
>
> On 4/6/2022 2:15 PM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> The macro RTE_ETHER_MIN_LEN isn't the minimum value of MTU. But testpmd
>> used it when execute 'port config mtu 0 xx' cmd. This patch fix it.
>>
>> Fixes: 1bb4a528c41f ("ethdev: fix max Rx packet length")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   app/test-pmd/cmdline.c |  4 ---
>>   app/test-pmd/config.c  | 55 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 55 insertions(+), 4 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 6ffea8e21a..91e4090582 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -2050,10 +2050,6 @@ cmd_config_mtu_parsed(void *parsed_result,
>>   {
>>       struct cmd_config_mtu_result *res = parsed_result;
>>   -    if (res->value < RTE_ETHER_MIN_LEN) {
>> -        fprintf(stderr, "mtu cannot be less than %d\n", 
>> RTE_ETHER_MIN_LEN);
>> -        return;
>> -    }
>>       port_mtu_set(res->port_id, res->value);
>>   }
>>   diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index bd689f9f86..1b1e738f83 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -1254,6 +1254,57 @@ port_reg_set(portid_t port_id, uint32_t 
>> reg_off, uint32_t reg_v)
>>       display_port_reg_value(port_id, reg_off, reg_v);
>>   }
>>   +static uint32_t
>> +eth_dev_get_overhead_len(uint32_t max_rx_pktlen, uint16_t max_mtu)
>> +{
>> +    uint32_t overhead_len;
>> +
>> +    if (max_mtu != UINT16_MAX && max_rx_pktlen > max_mtu)
>> +        overhead_len = max_rx_pktlen - max_mtu;
>> +    else
>> +        overhead_len = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
>> +
>> +    return overhead_len;
>> +}
>> +
>> +static int
>> +eth_dev_validate_mtu(uint16_t port_id, uint16_t mtu)
>> +{
>> +    struct rte_eth_dev_info dev_info;
>> +    uint32_t overhead_len;
>> +    uint32_t frame_size;
>> +    int ret;
>> +
>> +    ret = rte_eth_dev_info_get(port_id, &dev_info);
>> +    if (ret != 0)
>> +        return ret;
>> +
>> +    if (mtu < dev_info.min_mtu) {
>> +        fprintf(stderr,
>> +            "MTU (%u) < device min MTU (%u) for port_id %u\n",
>> +            mtu, dev_info.min_mtu, port_id);
>> +        return -EINVAL;
>> +    }
>> +    if (mtu > dev_info.max_mtu) {
>> +        fprintf(stderr,
>> +            "MTU (%u) > device max MTU (%u) for port_id %u\n",
>> +            mtu, dev_info.max_mtu, port_id);
>> +        return -EINVAL;
>> +    }
>> +
>> +    overhead_len = eth_dev_get_overhead_len(dev_info.max_rx_pktlen,
>> +            dev_info.max_mtu);
>> +    frame_size = mtu + overhead_len;
>> +    if (frame_size > dev_info.max_rx_pktlen) {
>> +        fprintf(stderr,
>> +            "Frame size (%u) > device max frame size (%u) for 
>> port_id %u\n",
>> +            frame_size, dev_info.max_rx_pktlen, port_id);
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   void
>>   port_mtu_set(portid_t port_id, uint16_t mtu)
>>   {
>> @@ -1263,6 +1314,10 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>>       if (port_id_is_invalid(port_id, ENABLED_WARN))
>>           return;
>>   +    diag = eth_dev_validate_mtu(port_id, mtu);
>> +    if (diag != 0)
>> +        return;
>> +
>>       if (port->need_reconfig == 0) {
>>           diag = rte_eth_dev_set_mtu(port_id, mtu);
>>           if (diag != 0) {
> I just wanted to know if these added functions eth_dev_validate_mtu() 
> &  eth_dev_get_overhead_len()
> are copy of ethdev library API's in file "rte_ethdev.c", which get 
> called by rte_eth_dev_set_mtu.
> Is our intent, is to call these twice ?
If port->need_reconfig is 1, rte_eth_dev_set_mtu doesn't be called, and
MTU value is saved to port->dev_conf.rxmode.mtu. This dev_conf.rxmode.mtu
will be set to driver in dev_configure(). This check is performed to
prevent dev_configure() failure. That's what I think.
> .
Ferruh Yigit May 12, 2022, 4:37 p.m. UTC | #3 
On 4/26/2022 2:38 AM, lihuisong (C) wrote:
> 
> 在 2022/4/26 0:25, Singh, Aman Deep 写道:
>>
>> On 4/6/2022 2:15 PM, Min Hu (Connor) wrote:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> The macro RTE_ETHER_MIN_LEN isn't the minimum value of MTU. But testpmd
>>> used it when execute 'port config mtu 0 xx' cmd. This patch fix it.
>>>
>>> Fixes: 1bb4a528c41f ("ethdev: fix max Rx packet length")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

<...>

>>> @@ -1263,6 +1314,10 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>>>       if (port_id_is_invalid(port_id, ENABLED_WARN))
>>>           return;
>>>   +    diag = eth_dev_validate_mtu(port_id, mtu);
>>> +    if (diag != 0)
>>> +        return;
>>> +
>>>       if (port->need_reconfig == 0) {
>>>           diag = rte_eth_dev_set_mtu(port_id, mtu);
>>>           if (diag != 0) {
>> I just wanted to know if these added functions eth_dev_validate_mtu() 
>> &  eth_dev_get_overhead_len()
>> are copy of ethdev library API's in file "rte_ethdev.c", which get 
>> called by rte_eth_dev_set_mtu.
>> Is our intent, is to call these twice ?
> If port->need_reconfig is 1, rte_eth_dev_set_mtu doesn't be called, and
> MTU value is saved to port->dev_conf.rxmode.mtu. This dev_conf.rxmode.mtu
> will be set to driver in dev_configure(). This check is performed to
> prevent dev_configure() failure. That's what I think.
>> .

Testpmd stores the MTU value and process it during start_port(), which 
may postpone any possible error in rte_eth_dev_configure(), so I think
it is OK to duplicate the check in app level.

Acked-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
diff mbox series

Patch

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6ffea8e21a..91e4090582 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2050,10 +2050,6 @@  cmd_config_mtu_parsed(void *parsed_result,
 {
 	struct cmd_config_mtu_result *res = parsed_result;
 
-	if (res->value < RTE_ETHER_MIN_LEN) {
-		fprintf(stderr, "mtu cannot be less than %d\n", RTE_ETHER_MIN_LEN);
-		return;
-	}
 	port_mtu_set(res->port_id, res->value);
 }
 
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index bd689f9f86..1b1e738f83 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1254,6 +1254,57 @@  port_reg_set(portid_t port_id, uint32_t reg_off, uint32_t reg_v)
 	display_port_reg_value(port_id, reg_off, reg_v);
 }
 
+static uint32_t
+eth_dev_get_overhead_len(uint32_t max_rx_pktlen, uint16_t max_mtu)
+{
+	uint32_t overhead_len;
+
+	if (max_mtu != UINT16_MAX && max_rx_pktlen > max_mtu)
+		overhead_len = max_rx_pktlen - max_mtu;
+	else
+		overhead_len = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
+
+	return overhead_len;
+}
+
+static int
+eth_dev_validate_mtu(uint16_t port_id, uint16_t mtu)
+{
+	struct rte_eth_dev_info dev_info;
+	uint32_t overhead_len;
+	uint32_t frame_size;
+	int ret;
+
+	ret = rte_eth_dev_info_get(port_id, &dev_info);
+	if (ret != 0)
+		return ret;
+
+	if (mtu < dev_info.min_mtu) {
+		fprintf(stderr,
+			"MTU (%u) < device min MTU (%u) for port_id %u\n",
+			mtu, dev_info.min_mtu, port_id);
+		return -EINVAL;
+	}
+	if (mtu > dev_info.max_mtu) {
+		fprintf(stderr,
+			"MTU (%u) > device max MTU (%u) for port_id %u\n",
+			mtu, dev_info.max_mtu, port_id);
+		return -EINVAL;
+	}
+
+	overhead_len = eth_dev_get_overhead_len(dev_info.max_rx_pktlen,
+			dev_info.max_mtu);
+	frame_size = mtu + overhead_len;
+	if (frame_size > dev_info.max_rx_pktlen) {
+		fprintf(stderr,
+			"Frame size (%u) > device max frame size (%u) for port_id %u\n",
+			frame_size, dev_info.max_rx_pktlen, port_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 void
 port_mtu_set(portid_t port_id, uint16_t mtu)
 {
@@ -1263,6 +1314,10 @@  port_mtu_set(portid_t port_id, uint16_t mtu)
 	if (port_id_is_invalid(port_id, ENABLED_WARN))
 		return;
 
+	diag = eth_dev_validate_mtu(port_id, mtu);
+	if (diag != 0)
+		return;
+
 	if (port->need_reconfig == 0) {
 		diag = rte_eth_dev_set_mtu(port_id, mtu);
 		if (diag != 0) {