[dpdk-dev] [PATCH 0/2] fixes for link speed

[dpdk-dev] [PATCH 0/2] fixes for link speed

[Min Hu (Connor)]

This patchset contains fixes for link speed in testpmd.

Huisong Li (2):
  app/testpmd: add link speed check before port start
  app/testpmd: fix link speed for a specified port

 app/test-pmd/cmdline.c | 48 +++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/2] app/testpmd: add link speed check before port start

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

Currently, to check whether the configured link_speeds is valid, we
have to run "port start". In addition, if the configuration fails,
"port->dev_conf.link_speeds" maintained in testpmd cannot be
restored.

This patch adds the link_speeds check before port start by calling
dev_configure, and resolves these problems.

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

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 5bf1497..09e30cf 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1537,8 +1537,12 @@ cmd_config_speed_all_parsed(void *parsed_result,
 			__rte_unused void *data)
 {
 	struct cmd_config_speed_all *res = parsed_result;
+	uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
+	struct rte_port *port;
 	uint32_t link_speed;
 	portid_t pid;
+	portid_t i;
+	int ret;
 
 	if (!all_ports_stopped()) {
 		printf("Please stop all ports first\n");
@@ -1550,7 +1554,26 @@ cmd_config_speed_all_parsed(void *parsed_result,
 		return;
 
 	RTE_ETH_FOREACH_DEV(pid) {
-		ports[pid].dev_conf.link_speeds = link_speed;
+		port = &ports[pid];
+		old_link_speeds[pid] = port->dev_conf.link_speeds;
+		port->dev_conf.link_speeds = link_speed;
+		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
+					    &port->dev_conf);
+		if (ret < 0) {
+			printf("Failed to check link speeds for port %d, ret = %d.\n",
+				pid, ret);
+			goto roolback;
+		}
+	}
+
+	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
+
+	return;
+
+roolback:
+	for (i = 0; i <= pid; i++) {
+		port = &ports[i];
+		port->dev_conf.link_speeds = old_link_speeds[i];
 	}
 
 	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
@@ -1609,7 +1632,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
 				__rte_unused void *data)
 {
 	struct cmd_config_speed_specific *res = parsed_result;
+	uint32_t old_link_speeds;
+	struct rte_port *port;
 	uint32_t link_speed;
+	int ret;
 
 	if (!all_ports_stopped()) {
 		printf("Please stop all ports first\n");
@@ -1623,7 +1649,17 @@ cmd_config_speed_specific_parsed(void *parsed_result,
 			&link_speed) < 0)
 		return;
 
-	ports[res->id].dev_conf.link_speeds = link_speed;
+	port = &ports[res->id];
+	old_link_speeds = port->dev_conf.link_speeds;
+	port->dev_conf.link_speeds = link_speed;
+	ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
+				    &port->dev_conf);
+	if (ret < 0) {
+		printf("Failed to check link speeds for port %d, ret = %d.\n",
+			res->id, ret);
+		port->dev_conf.link_speeds = old_link_speeds;
+		return;
+	}
 
 	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
 }
-- 
2.7.4

[dpdk-dev] [PATCH 2/2] app/testpmd: fix link speed for a specified port

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

When we use the following cmd to modify the link speed of specified
port: "port config <port_id> speed xxx duplex xxx", we have to stop
all ports. It's not necessary.

Fixes: 82113036e4e5 ("ethdev: redesign link speed config")
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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 09e30cf..31884c5 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1637,13 +1637,13 @@ cmd_config_speed_specific_parsed(void *parsed_result,
 	uint32_t link_speed;
 	int ret;
 
-	if (!all_ports_stopped()) {
-		printf("Please stop all ports first\n");
+	if (port_id_is_invalid(res->id, ENABLED_WARN))
 		return;
-	}
 
-	if (port_id_is_invalid(res->id, ENABLED_WARN))
+	if (!port_is_stopped(res->id)) {
+		printf("Please stop port %d first\n", res->id);
 		return;
+	}
 
 	if (parse_and_check_speed_duplex(res->value1, res->value2,
 			&link_speed) < 0)
-- 
2.7.4

Re: [dpdk-dev] [PATCH 2/2] app/testpmd: fix link speed for a specified port

[Ferruh Yigit]

On 4/19/2021 7:21 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> When we use the following cmd to modify the link speed of specified
> port: "port config <port_id> speed xxx duplex xxx", we have to stop
> all ports. It's not necessary.
> 
> Fixes: 82113036e4e5 ("ethdev: redesign link speed config")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH 1/2] app/testpmd: add link speed check before port start

[Ferruh Yigit]

On 4/19/2021 7:21 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> Currently, to check whether the configured link_speeds is valid, we
> have to run "port start". In addition, if the configuration fails,
> "port->dev_conf.link_speeds" maintained in testpmd cannot be
> restored.
> 
> This patch adds the link_speeds check before port start by calling
> dev_configure, and resolves these problems.
> 

Hi Connor,

I am not sure about this patch, overall design in many commands seems store the 
config changes and mark port or queue as needs reconfiguration 
('cmd_reconfig_device_queue()'), and do the reconfigure during port_start.
Your change is against this overall design.

What is the downside of learning that speed is invalid in port start?
To not able to restore the previous speed is not good, but user can set the 
desired speed again in case of failure.

And do you think adding a check that if the requested speed is in the device 
reported speed capability help on early fail and restore?

> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>   app/test-pmd/cmdline.c | 40 ++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 5bf1497..09e30cf 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1537,8 +1537,12 @@ cmd_config_speed_all_parsed(void *parsed_result,
>   			__rte_unused void *data)
>   {
>   	struct cmd_config_speed_all *res = parsed_result;
> +	uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
> +	struct rte_port *port;
>   	uint32_t link_speed;
>   	portid_t pid;
> +	portid_t i;
> +	int ret;
>   
>   	if (!all_ports_stopped()) {
>   		printf("Please stop all ports first\n");
> @@ -1550,7 +1554,26 @@ cmd_config_speed_all_parsed(void *parsed_result,
>   		return;
>   
>   	RTE_ETH_FOREACH_DEV(pid) {
> -		ports[pid].dev_conf.link_speeds = link_speed;
> +		port = &ports[pid];
> +		old_link_speeds[pid] = port->dev_conf.link_speeds;
> +		port->dev_conf.link_speeds = link_speed;
> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
> +					    &port->dev_conf);
> +		if (ret < 0) {
> +			printf("Failed to check link speeds for port %d, ret = %d.\n",
> +				pid, ret);
> +			goto roolback;
> +		}
> +	}
> +
> +	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> +
> +	return;
> +
> +roolback:
> +	for (i = 0; i <= pid; i++) {
> +		port = &ports[i];
> +		port->dev_conf.link_speeds = old_link_speeds[i];
>   	}
>   
>   	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> @@ -1609,7 +1632,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
>   				__rte_unused void *data)
>   {
>   	struct cmd_config_speed_specific *res = parsed_result;
> +	uint32_t old_link_speeds;
> +	struct rte_port *port;
>   	uint32_t link_speed;
> +	int ret;
>   
>   	if (!all_ports_stopped()) {
>   		printf("Please stop all ports first\n");
> @@ -1623,7 +1649,17 @@ cmd_config_speed_specific_parsed(void *parsed_result,
>   			&link_speed) < 0)
>   		return;
>   
> -	ports[res->id].dev_conf.link_speeds = link_speed;
> +	port = &ports[res->id];
> +	old_link_speeds = port->dev_conf.link_speeds;
> +	port->dev_conf.link_speeds = link_speed;
> +	ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
> +				    &port->dev_conf);
> +	if (ret < 0) {
> +		printf("Failed to check link speeds for port %d, ret = %d.\n",
> +			res->id, ret);
> +		port->dev_conf.link_speeds = old_link_speeds;
> +		return;
> +	}
>   
>   	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>   }
> 

Re: [dpdk-dev] [PATCH 1/2] app/testpmd: add link speed check before port start

[Min Hu (Connor)]

在 2021/4/20 20:11, Ferruh Yigit 写道:
> On 4/19/2021 7:21 AM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> Currently, to check whether the configured link_speeds is valid, we
>> have to run "port start". In addition, if the configuration fails,
>> "port->dev_conf.link_speeds" maintained in testpmd cannot be
>> restored.
>>
>> This patch adds the link_speeds check before port start by calling
>> dev_configure, and resolves these problems.
>>
> 
> Hi Connor,
> 
> I am not sure about this patch, overall design in many commands seems 
> store the config changes and mark port or queue as needs reconfiguration 
> ('cmd_reconfig_device_queue()'), and do the reconfigure during port_start.
> Your change is against this overall design.
> 
Not exactly. DCB configuration in testpmd is also the same operation. I
understand that the DCB configuration needs to be verified in driver
dev_configure before doing the reconfigure during port_start.

> What is the downside of learning that speed is invalid in port start?
> To not able to restore the previous speed is not good, but user can set 
> the desired speed again in case of failure.
> 
I think there are two things:
1/ User may not know the previous configuration.
2/ Generally, if a command fails to be configured, the current
configuration does not聽 take effect and remains in the previous state.
This is achieved by adding a check and recovery here.

> And do you think adding a check that if the requested speed is in the 
> device reported speed capability help on early fail and restore?
> 
It may not be feasible. After all, the definition of 'speed_capa' and
'link_speeds' is not very relevant in the framework.
In addition, the scheme for reporting 'speed_capa' varies from the PMD
driver. Many drivers report all speed capabilities, but the actual state
may not support all of them.
However, if the driver supports the 'link_speeds' configuration, the
driver should verify the link_speeds configuration because the ethdev
layer does not verify the 'link_speeds' configuration.

Above is my personal opinion, look forward to reply.
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   app/test-pmd/cmdline.c | 40 ++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 5bf1497..09e30cf 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -1537,8 +1537,12 @@ cmd_config_speed_all_parsed(void *parsed_result,
>>               __rte_unused void *data)
>>   {
>>       struct cmd_config_speed_all *res = parsed_result;
>> +    uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
>> +    struct rte_port *port;
>>       uint32_t link_speed;
>>       portid_t pid;
>> +    portid_t i;
>> +    int ret;
>>       if (!all_ports_stopped()) {
>>           printf("Please stop all ports first\n");
>> @@ -1550,7 +1554,26 @@ cmd_config_speed_all_parsed(void *parsed_result,
>>           return;
>>       RTE_ETH_FOREACH_DEV(pid) {
>> -        ports[pid].dev_conf.link_speeds = link_speed;
>> +        port = &ports[pid];
>> +        old_link_speeds[pid] = port->dev_conf.link_speeds;
>> +        port->dev_conf.link_speeds = link_speed;
>> +        ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
>> +                        &port->dev_conf);
>> +        if (ret < 0) {
>> +            printf("Failed to check link speeds for port %d, ret = 
>> %d.\n",
>> +                pid, ret);
>> +            goto roolback;
>> +        }
>> +    }
>> +
>> +    cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>> +
>> +    return;
>> +
>> +roolback:
>> +    for (i = 0; i <= pid; i++) {
>> +        port = &ports[i];
>> +        port->dev_conf.link_speeds = old_link_speeds[i];
>>       }
>>       cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>> @@ -1609,7 +1632,10 @@ cmd_config_speed_specific_parsed(void 
>> *parsed_result,
>>                   __rte_unused void *data)
>>   {
>>       struct cmd_config_speed_specific *res = parsed_result;
>> +    uint32_t old_link_speeds;
>> +    struct rte_port *port;
>>       uint32_t link_speed;
>> +    int ret;
>>       if (!all_ports_stopped()) {
>>           printf("Please stop all ports first\n");
>> @@ -1623,7 +1649,17 @@ cmd_config_speed_specific_parsed(void 
>> *parsed_result,
>>               &link_speed) < 0)
>>           return;
>> -    ports[res->id].dev_conf.link_speeds = link_speed;
>> +    port = &ports[res->id];
>> +    old_link_speeds = port->dev_conf.link_speeds;
>> +    port->dev_conf.link_speeds = link_speed;
>> +    ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
>> +                    &port->dev_conf);
>> +    if (ret < 0) {
>> +        printf("Failed to check link speeds for port %d, ret = %d.\n",
>> +            res->id, ret);
>> +        port->dev_conf.link_speeds = old_link_speeds;
>> +        return;
>> +    }
>>       cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>>   }
>>
> 
> .

[dpdk-dev] [PATCH v2 0/2] fixes for link speed

[Min Hu (Connor)]

This patchset contains fixes for link speed in testpmd.

Huisong Li (2):
  app/testpmd: add link speed check before port start
  app/testpmd: fix link speed for a specified port

 app/test-pmd/cmdline.c | 50 ++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 6 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check before port start

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

Currently, to check whether the configured link_speeds is valid, we
have to run "port start". In addition, if the configuration fails,
"port->dev_conf.link_speeds" maintained in testpmd cannot be
restored.

This patch adds the link_speeds check before port start by calling
dev_configure, and resolves these problems.

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

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 5fdcc1c..ddbc629 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1549,8 +1549,12 @@ cmd_config_speed_all_parsed(void *parsed_result,
 			__rte_unused void *data)
 {
 	struct cmd_config_speed_all *res = parsed_result;
+	uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
+	struct rte_port *port;
 	uint32_t link_speed;
 	portid_t pid;
+	portid_t i;
+	int ret;
 
 	if (!all_ports_stopped()) {
 		printf("Please stop all ports first\n");
@@ -1562,7 +1566,26 @@ cmd_config_speed_all_parsed(void *parsed_result,
 		return;
 
 	RTE_ETH_FOREACH_DEV(pid) {
-		ports[pid].dev_conf.link_speeds = link_speed;
+		port = &ports[pid];
+		old_link_speeds[pid] = port->dev_conf.link_speeds;
+		port->dev_conf.link_speeds = link_speed;
+		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
+					    &port->dev_conf);
+		if (ret < 0) {
+			printf("Failed to check link speeds for port %d, ret = %d.\n",
+				pid, ret);
+			goto roolback;
+		}
+	}
+
+	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
+
+	return;
+
+roolback:
+	for (i = 0; i <= pid; i++) {
+		port = &ports[i];
+		port->dev_conf.link_speeds = old_link_speeds[i];
 	}
 
 	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
@@ -1621,7 +1644,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
 				__rte_unused void *data)
 {
 	struct cmd_config_speed_specific *res = parsed_result;
+	uint32_t old_link_speeds;
+	struct rte_port *port;
 	uint32_t link_speed;
+	int ret;
 
 	if (!all_ports_stopped()) {
 		printf("Please stop all ports first\n");
@@ -1635,8 +1661,20 @@ cmd_config_speed_specific_parsed(void *parsed_result,
 			&link_speed) < 0)
 		return;
 
-	ports[res->id].dev_conf.link_speeds = link_speed;
+	port = &ports[res->id];
+	old_link_speeds = port->dev_conf.link_speeds;
+	port->dev_conf.link_speeds = link_speed;
+	ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
+				    &port->dev_conf);
+	if (ret < 0) {
+		printf("Failed to check link speeds for port %d, ret = %d.\n",
+			res->id, ret);
+		port->dev_conf.link_speeds = old_link_speeds;
+	}
 
+	/*
+	 * If the cmd fails to execute, it is necessary to reconfigure device.
+	 */
 	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
 }
 
-- 
2.7.4

[dpdk-dev] [PATCH v2 2/2] app/testpmd: fix link speed for a specified port

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

When we use the following cmd to modify the link speed of specified
port: "port config <port_id> speed xxx duplex xxx", we have to stop
all ports. It's not necessary.

Fixes: 82113036e4e5 ("ethdev: redesign link speed config")
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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index ddbc629..6d80d98 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1649,13 +1649,13 @@ cmd_config_speed_specific_parsed(void *parsed_result,
 	uint32_t link_speed;
 	int ret;
 
-	if (!all_ports_stopped()) {
-		printf("Please stop all ports first\n");
+	if (port_id_is_invalid(res->id, ENABLED_WARN))
 		return;
-	}
 
-	if (port_id_is_invalid(res->id, ENABLED_WARN))
+	if (!port_is_stopped(res->id)) {
+		printf("Please stop port %d first\n", res->id);
 		return;
+	}
 
 	if (parse_and_check_speed_duplex(res->value1, res->value2,
 			&link_speed) < 0)
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 2/2] app/testpmd: fix link speed for a specified port

[Li, Xiaoyun]

> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>
> Sent: Wednesday, April 28, 2021 16:37
> To: dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>
> Subject: [PATCH v2 2/2] app/testpmd: fix link speed for a specified port
> 
> From: Huisong Li <lihuisong@huawei.com>
> 
> When we use the following cmd to modify the link speed of specified
> port: "port config <port_id> speed xxx duplex xxx", we have to stop all ports. It's
> not necessary.
> 
> Fixes: 82113036e4e5 ("ethdev: redesign link speed config")
> 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 | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>

Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check before port start

[Li, Xiaoyun]

> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>
> Sent: Wednesday, April 28, 2021 16:37
> To: dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>
> Subject: [PATCH v2 1/2] app/testpmd: add link speed check before port start
> 
> From: Huisong Li <lihuisong@huawei.com>
> 
> Currently, to check whether the configured link_speeds is valid, we have to run
> "port start". In addition, if the configuration fails, "port->dev_conf.link_speeds"
> maintained in testpmd cannot be restored.
> 
> This patch adds the link_speeds check before port start by calling dev_configure,
> and resolves these problems.

Not sure about this patch. I don't think you can fix the issue you mentioned.
Probably only hns3 does speed check in dev_configure. I don't see this in other drivers, not in i40e/ice/mlx.
I guess it's because if it's not supported speed, it will just be UNKNOWN and user can config again?

BTW, even if this behavior is accepted by others, still some comments below.

> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  app/test-pmd/cmdline.c | 42 ++++++++++++++++++++++++++++++++++++++++-
> -
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> 5fdcc1c..ddbc629 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1549,8 +1549,12 @@ cmd_config_speed_all_parsed(void *parsed_result,
>  			__rte_unused void *data)
>  {
>  	struct cmd_config_speed_all *res = parsed_result;
> +	uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
> +	struct rte_port *port;
>  	uint32_t link_speed;
>  	portid_t pid;
> +	portid_t i;
> +	int ret;
> 
>  	if (!all_ports_stopped()) {
>  		printf("Please stop all ports first\n"); @@ -1562,7 +1566,26 @@
> cmd_config_speed_all_parsed(void *parsed_result,
>  		return;
> 
>  	RTE_ETH_FOREACH_DEV(pid) {
> -		ports[pid].dev_conf.link_speeds = link_speed;
> +		port = &ports[pid];
> +		old_link_speeds[pid] = port->dev_conf.link_speeds;
> +		port->dev_conf.link_speeds = link_speed;
> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
> +					    &port->dev_conf);
> +		if (ret < 0) {
> +			printf("Failed to check link speeds for port %d, ret
> = %d.\n",
> +				pid, ret);
> +			goto roolback;

Why don't you just add restoring all ports speed here and then "break"? No matter one dev fails or not, all ports will do reconfig from your code logic.
And you type rollback wrongly.

> +		}
> +	}
> +
> +	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> +
> +	return;
> +
> +roolback:
> +	for (i = 0; i <= pid; i++) {
> +		port = &ports[i];
> +		port->dev_conf.link_speeds = old_link_speeds[i];
>  	}
> 
>  	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); @@ -1621,7
> +1644,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
>  				__rte_unused void *data)
>  {
>  	struct cmd_config_speed_specific *res = parsed_result;
> +	uint32_t old_link_speeds;
> +	struct rte_port *port;
>  	uint32_t link_speed;
> +	int ret;
> 
>  	if (!all_ports_stopped()) {
>  		printf("Please stop all ports first\n"); @@ -1635,8 +1661,20 @@
> cmd_config_speed_specific_parsed(void *parsed_result,
>  			&link_speed) < 0)
>  		return;
> 
> -	ports[res->id].dev_conf.link_speeds = link_speed;
> +	port = &ports[res->id];
> +	old_link_speeds = port->dev_conf.link_speeds;
> +	port->dev_conf.link_speeds = link_speed;
> +	ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
> +				    &port->dev_conf);
> +	if (ret < 0) {
> +		printf("Failed to check link speeds for port %d, ret = %d.\n",
> +			res->id, ret);
> +		port->dev_conf.link_speeds = old_link_speeds;
> +	}
> 
> +	/*
> +	 * If the cmd fails to execute, it is necessary to reconfigure device.
> +	 */
>  	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);  }
> 
> --
> 2.7.4

Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check before port start

[Huisong Li]

在 2021/4/30 11:19, Li, Xiaoyun 写道:
>> -----Original Message-----
>> From: Min Hu (Connor) <humin29@huawei.com>
>> Sent: Wednesday, April 28, 2021 16:37
>> To: dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>
>> Subject: [PATCH v2 1/2] app/testpmd: add link speed check before port start
>>
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> Currently, to check whether the configured link_speeds is valid, we have to run
>> "port start". In addition, if the configuration fails, "port->dev_conf.link_speeds"
>> maintained in testpmd cannot be restored.
>>
>> This patch adds the link_speeds check before port start by calling dev_configure,
>> and resolves these problems.
> Not sure about this patch. I don't think you can fix the issue you mentioned.
> Probably only hns3 does speed check in dev_configure. I don't see this in other drivers, not in i40e/ice/mlx.
> I guess it's because if it's not supported speed, it will just be UNKNOWN and user can config again?

I think that the validity of the configuration delivered by 
dev_configure is ensured by this interface and cannot be left to the 
backend.

Because it facilitates users to handle abnormal configurations in a 
timely manner. It may be more appropriate for the driver to do this 
check in dev_configure.

In addition, even if other drivers do not add this check in 
dev_configure, this patch does not seem to affect the current behavior 
of these drivers.

> BTW, even if this behavior is accepted by others, still some comments below.
>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   app/test-pmd/cmdline.c | 42 ++++++++++++++++++++++++++++++++++++++++-
>> -
>>   1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>> 5fdcc1c..ddbc629 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -1549,8 +1549,12 @@ cmd_config_speed_all_parsed(void *parsed_result,
>>   			__rte_unused void *data)
>>   {
>>   	struct cmd_config_speed_all *res = parsed_result;
>> +	uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
>> +	struct rte_port *port;
>>   	uint32_t link_speed;
>>   	portid_t pid;
>> +	portid_t i;
>> +	int ret;
>>
>>   	if (!all_ports_stopped()) {
>>   		printf("Please stop all ports first\n"); @@ -1562,7 +1566,26 @@
>> cmd_config_speed_all_parsed(void *parsed_result,
>>   		return;
>>
>>   	RTE_ETH_FOREACH_DEV(pid) {
>> -		ports[pid].dev_conf.link_speeds = link_speed;
>> +		port = &ports[pid];
>> +		old_link_speeds[pid] = port->dev_conf.link_speeds;
>> +		port->dev_conf.link_speeds = link_speed;
>> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
>> +					    &port->dev_conf);
>> +		if (ret < 0) {
>> +			printf("Failed to check link speeds for port %d, ret
>> = %d.\n",
>> +				pid, ret);
>> +			goto roolback;
> Why don't you just add restoring all ports speed here and then "break"? No matter one dev fails or not, all ports will do reconfig from your code logic.
> And you type rollback wrongly.

It cannot exit directly after restoring all ports speed. If the cmd fails to execute, it is necessary to reconfigure device with the correct configuration.
  Because "nb_rx/tx_queues" in dev->data are cleared to zero if dev_configure fails to be executed in PMD driver.

>> +		}
>> +	}
>> +
>> +	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>> +
>> +	return;
>> +
>> +roolback:
>> +	for (i = 0; i <= pid; i++) {
>> +		port = &ports[i];
>> +		port->dev_conf.link_speeds = old_link_speeds[i];
>>   	}
>>
>>   	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); @@ -1621,7
>> +1644,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
>>   				__rte_unused void *data)
>>   {
>>   	struct cmd_config_speed_specific *res = parsed_result;
>> +	uint32_t old_link_speeds;
>> +	struct rte_port *port;
>>   	uint32_t link_speed;
>> +	int ret;
>>
>>   	if (!all_ports_stopped()) {
>>   		printf("Please stop all ports first\n"); @@ -1635,8 +1661,20 @@
>> cmd_config_speed_specific_parsed(void *parsed_result,
>>   			&link_speed) < 0)
>>   		return;
>>
>> -	ports[res->id].dev_conf.link_speeds = link_speed;
>> +	port = &ports[res->id];
>> +	old_link_speeds = port->dev_conf.link_speeds;
>> +	port->dev_conf.link_speeds = link_speed;
>> +	ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
>> +				    &port->dev_conf);
>> +	if (ret < 0) {
>> +		printf("Failed to check link speeds for port %d, ret = %d.\n",
>> +			res->id, ret);
>> +		port->dev_conf.link_speeds = old_link_speeds;
>> +	}
>>
>> +	/*
>> +	 * If the cmd fails to execute, it is necessary to reconfigure device.
>> +	 */
>>   	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);  }
>>
>> --
>> 2.7.4
> .

Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check before port start

[Li, Xiaoyun]

> -----Original Message-----
> From: Huisong Li <lihuisong@huawei.com>
> Sent: Friday, April 30, 2021 12:04
> To: Li, Xiaoyun <xiaoyun.li@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check
> before port start
> 
> 
> 在 2021/4/30 11:19, Li, Xiaoyun 写道:
> >> -----Original Message-----
> >> From: Min Hu (Connor) <humin29@huawei.com>
> >> Sent: Wednesday, April 28, 2021 16:37
> >> To: dev@dpdk.org
> >> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
> >> <xiaoyun.li@intel.com>
> >> Subject: [PATCH v2 1/2] app/testpmd: add link speed check before port
> >> start
> >>
> >> From: Huisong Li <lihuisong@huawei.com>
> >>
> >> Currently, to check whether the configured link_speeds is valid, we
> >> have to run "port start". In addition, if the configuration fails, "port-
> >dev_conf.link_speeds"
> >> maintained in testpmd cannot be restored.
> >>
> >> This patch adds the link_speeds check before port start by calling
> >> dev_configure, and resolves these problems.
> > Not sure about this patch. I don't think you can fix the issue you mentioned.
> > Probably only hns3 does speed check in dev_configure. I don't see this in other
> drivers, not in i40e/ice/mlx.
> > I guess it's because if it's not supported speed, it will just be UNKNOWN and
> user can config again?
> 
> I think that the validity of the configuration delivered by dev_configure is
> ensured by this interface and cannot be left to the backend.
> 
> Because it facilitates users to handle abnormal configurations in a timely
> manner. It may be more appropriate for the driver to do this check in
> dev_configure.

I still think it's not necessary.

> 
> In addition, even if other drivers do not add this check in dev_configure, this
> patch does not seem to affect the current behavior of these drivers.
> 
> > BTW, even if this behavior is accepted by others, still some comments below.
> >
> >> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> >> ---
> >>   app/test-pmd/cmdline.c | 42
> >> ++++++++++++++++++++++++++++++++++++++++-
> >> -
> >>   1 file changed, 40 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> >> 5fdcc1c..ddbc629 100644
> >> --- a/app/test-pmd/cmdline.c
> >> +++ b/app/test-pmd/cmdline.c
> >> @@ -1549,8 +1549,12 @@ cmd_config_speed_all_parsed(void
> *parsed_result,
> >>   			__rte_unused void *data)
> >>   {
> >>   	struct cmd_config_speed_all *res = parsed_result;
> >> +	uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
> >> +	struct rte_port *port;
> >>   	uint32_t link_speed;
> >>   	portid_t pid;
> >> +	portid_t i;
> >> +	int ret;
> >>
> >>   	if (!all_ports_stopped()) {
> >>   		printf("Please stop all ports first\n"); @@ -1562,7 +1566,26 @@
> >> cmd_config_speed_all_parsed(void *parsed_result,
> >>   		return;
> >>
> >>   	RTE_ETH_FOREACH_DEV(pid) {
> >> -		ports[pid].dev_conf.link_speeds = link_speed;
> >> +		port = &ports[pid];
> >> +		old_link_speeds[pid] = port->dev_conf.link_speeds;
> >> +		port->dev_conf.link_speeds = link_speed;
> >> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
> >> +					    &port->dev_conf);
> >> +		if (ret < 0) {
> >> +			printf("Failed to check link speeds for port %d, ret
> >> = %d.\n",
> >> +				pid, ret);
> >> +			goto roolback;
> > Why don't you just add restoring all ports speed here and then "break"? No
> matter one dev fails or not, all ports will do reconfig from your code logic.
> > And you type rollback wrongly.
> 
> It cannot exit directly after restoring all ports speed. If the cmd fails to execute,
> it is necessary to reconfigure device with the correct configuration.
>   Because "nb_rx/tx_queues" in dev->data are cleared to zero if dev_configure
> fails to be executed in PMD driver.

?
cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); is at the end of this cmd. This will re-config all ports.
I don't understand why can't you add a restoring in this if and break this loop and do this reconfig.

> 
> >> +		}
> >> +	}
> >> +
> >> +	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> >> +
> >> +	return;
> >> +
> >> +roolback:
> >> +	for (i = 0; i <= pid; i++) {
> >> +		port = &ports[i];
> >> +		port->dev_conf.link_speeds = old_link_speeds[i];
> >>   	}
> >>
> >>   	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); @@ -1621,7
> >> +1644,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
> >>   				__rte_unused void *data)
> >>   {
> >>   	struct cmd_config_speed_specific *res = parsed_result;
> >> +	uint32_t old_link_speeds;
> >> +	struct rte_port *port;
> >>   	uint32_t link_speed;
> >> +	int ret;
> >>
> >>   	if (!all_ports_stopped()) {
> >>   		printf("Please stop all ports first\n"); @@ -1635,8 +1661,20 @@
> >> cmd_config_speed_specific_parsed(void *parsed_result,
> >>   			&link_speed) < 0)
> >>   		return;
> >>
> >> -	ports[res->id].dev_conf.link_speeds = link_speed;
> >> +	port = &ports[res->id];
> >> +	old_link_speeds = port->dev_conf.link_speeds;
> >> +	port->dev_conf.link_speeds = link_speed;
> >> +	ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
> >> +				    &port->dev_conf);
> >> +	if (ret < 0) {
> >> +		printf("Failed to check link speeds for port %d, ret = %d.\n",
> >> +			res->id, ret);
> >> +		port->dev_conf.link_speeds = old_link_speeds;
> >> +	}
> >>
> >> +	/*
> >> +	 * If the cmd fails to execute, it is necessary to reconfigure device.
> >> +	 */
> >>   	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);  }
> >>
> >> --
> >> 2.7.4
> > .

Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check before port start

[Huisong Li]

在 2021/4/30 12:46, Li, Xiaoyun 写道:
>
>> -----Original Message-----
>> From: Huisong Li <lihuisong@huawei.com>
>> Sent: Friday, April 30, 2021 12:04
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check
>> before port start
>>
>>
>> 在 2021/4/30 11:19, Li, Xiaoyun 写道:
>>>> -----Original Message-----
>>>> From: Min Hu (Connor) <humin29@huawei.com>
>>>> Sent: Wednesday, April 28, 2021 16:37
>>>> To: dev@dpdk.org
>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
>>>> <xiaoyun.li@intel.com>
>>>> Subject: [PATCH v2 1/2] app/testpmd: add link speed check before port
>>>> start
>>>>
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>> Currently, to check whether the configured link_speeds is valid, we
>>>> have to run "port start". In addition, if the configuration fails, "port-
>>> dev_conf.link_speeds"
>>>> maintained in testpmd cannot be restored.
>>>>
>>>> This patch adds the link_speeds check before port start by calling
>>>> dev_configure, and resolves these problems.
>>> Not sure about this patch. I don't think you can fix the issue you mentioned.
>>> Probably only hns3 does speed check in dev_configure. I don't see this in other
>> drivers, not in i40e/ice/mlx.
>>> I guess it's because if it's not supported speed, it will just be UNKNOWN and
>> user can config again?
>>
>> I think that the validity of the configuration delivered by dev_configure is
>> ensured by this interface and cannot be left to the backend.
>>
>> Because it facilitates users to handle abnormal configurations in a timely
>> manner. It may be more appropriate for the driver to do this check in
>> dev_configure.
> I still think it's not necessary.

ok😂

@Ferruh, what do you think?

>
>> In addition, even if other drivers do not add this check in dev_configure, this
>> patch does not seem to affect the current behavior of these drivers.
>>
>>> BTW, even if this behavior is accepted by others, still some comments below.
>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> ---
>>>>    app/test-pmd/cmdline.c | 42
>>>> ++++++++++++++++++++++++++++++++++++++++-
>>>> -
>>>>    1 file changed, 40 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>>>> 5fdcc1c..ddbc629 100644
>>>> --- a/app/test-pmd/cmdline.c
>>>> +++ b/app/test-pmd/cmdline.c
>>>> @@ -1549,8 +1549,12 @@ cmd_config_speed_all_parsed(void
>> *parsed_result,
>>>>    			__rte_unused void *data)
>>>>    {
>>>>    	struct cmd_config_speed_all *res = parsed_result;
>>>> +	uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
>>>> +	struct rte_port *port;
>>>>    	uint32_t link_speed;
>>>>    	portid_t pid;
>>>> +	portid_t i;
>>>> +	int ret;
>>>>
>>>>    	if (!all_ports_stopped()) {
>>>>    		printf("Please stop all ports first\n"); @@ -1562,7 +1566,26 @@
>>>> cmd_config_speed_all_parsed(void *parsed_result,
>>>>    		return;
>>>>
>>>>    	RTE_ETH_FOREACH_DEV(pid) {
>>>> -		ports[pid].dev_conf.link_speeds = link_speed;
>>>> +		port = &ports[pid];
>>>> +		old_link_speeds[pid] = port->dev_conf.link_speeds;
>>>> +		port->dev_conf.link_speeds = link_speed;
>>>> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
>>>> +					    &port->dev_conf);
>>>> +		if (ret < 0) {
>>>> +			printf("Failed to check link speeds for port %d, ret
>>>> = %d.\n",
>>>> +				pid, ret);
>>>> +			goto roolback;
>>> Why don't you just add restoring all ports speed here and then "break"? No
>> matter one dev fails or not, all ports will do reconfig from your code logic.
>>> And you type rollback wrongly.
>> It cannot exit directly after restoring all ports speed. If the cmd fails to execute,
>> it is necessary to reconfigure device with the correct configuration.
>>    Because "nb_rx/tx_queues" in dev->data are cleared to zero if dev_configure
>> fails to be executed in PMD driver.
> ?
> cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); is at the end of this cmd. This will re-config all ports.
> I don't understand why can't you add a restoring in this if and break this loop and do this reconfig.
What do you mean? If a port fails among all ports, only the failed port 
is restored and reconfigured. I suppose that's what you mean?
The cmd "port config all speed xxx duplex xxx" applies to all ports. If 
it fails to be delivered, the user considers that the cmd does not take 
effect.
So it is necessary to restore and reconfigure all ports to the previous 
state.
>
>>>> +		}
>>>> +	}
>>>> +
>>>> +	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>>>> +
>>>> +	return;
>>>> +
>>>> +roolback:
>>>> +	for (i = 0; i <= pid; i++) {
>>>> +		port = &ports[i];
>>>> +		port->dev_conf.link_speeds = old_link_speeds[i];
>>>>    	}
>>>>
>>>>    	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); @@ -1621,7
>>>> +1644,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
>>>>    				__rte_unused void *data)
>>>>    {
>>>>    	struct cmd_config_speed_specific *res = parsed_result;
>>>> +	uint32_t old_link_speeds;
>>>> +	struct rte_port *port;
>>>>    	uint32_t link_speed;
>>>> +	int ret;
>>>>
>>>>    	if (!all_ports_stopped()) {
>>>>    		printf("Please stop all ports first\n"); @@ -1635,8 +1661,20 @@
>>>> cmd_config_speed_specific_parsed(void *parsed_result,
>>>>    			&link_speed) < 0)
>>>>    		return;
>>>>
>>>> -	ports[res->id].dev_conf.link_speeds = link_speed;
>>>> +	port = &ports[res->id];
>>>> +	old_link_speeds = port->dev_conf.link_speeds;
>>>> +	port->dev_conf.link_speeds = link_speed;
>>>> +	ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
>>>> +				    &port->dev_conf);
>>>> +	if (ret < 0) {
>>>> +		printf("Failed to check link speeds for port %d, ret = %d.\n",
>>>> +			res->id, ret);
>>>> +		port->dev_conf.link_speeds = old_link_speeds;
>>>> +	}
>>>>
>>>> +	/*
>>>> +	 * If the cmd fails to execute, it is necessary to reconfigure device.
>>>> +	 */
>>>>    	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);  }
>>>>
>>>> --
>>>> 2.7.4
>>> .

Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check before port start

[Li, Xiaoyun]

> -----Original Message-----
> From: Huisong Li <lihuisong@huawei.com>
> Sent: Tuesday, May 4, 2021 09:46
> To: Li, Xiaoyun <xiaoyun.li@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check
> before port start
> 
> 
> 在 2021/4/30 12:46, Li, Xiaoyun 写道:
> >
> >> -----Original Message-----
> >> From: Huisong Li <lihuisong@huawei.com>
> >> Sent: Friday, April 30, 2021 12:04
> >> To: Li, Xiaoyun <xiaoyun.li@intel.com>
> >> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed
> >> check before port start
> >>
> >>
> >> 在 2021/4/30 11:19, Li, Xiaoyun 写道:
> >>>> -----Original Message-----
> >>>> From: Min Hu (Connor) <humin29@huawei.com>
> >>>> Sent: Wednesday, April 28, 2021 16:37
> >>>> To: dev@dpdk.org
> >>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
> >>>> <xiaoyun.li@intel.com>
> >>>> Subject: [PATCH v2 1/2] app/testpmd: add link speed check before
> >>>> port start
> >>>>
> >>>> From: Huisong Li <lihuisong@huawei.com>
> >>>>
> >>>> Currently, to check whether the configured link_speeds is valid, we
> >>>> have to run "port start". In addition, if the configuration fails,
> >>>> "port-
> >>> dev_conf.link_speeds"
> >>>> maintained in testpmd cannot be restored.
> >>>>
> >>>> This patch adds the link_speeds check before port start by calling
> >>>> dev_configure, and resolves these problems.
> >>> Not sure about this patch. I don't think you can fix the issue you mentioned.
> >>> Probably only hns3 does speed check in dev_configure. I don't see
> >>> this in other
> >> drivers, not in i40e/ice/mlx.
> >>> I guess it's because if it's not supported speed, it will just be
> >>> UNKNOWN and
> >> user can config again?
> >>
> >> I think that the validity of the configuration delivered by
> >> dev_configure is ensured by this interface and cannot be left to the backend.
> >>
> >> Because it facilitates users to handle abnormal configurations in a
> >> timely manner. It may be more appropriate for the driver to do this
> >> check in dev_configure.
> > I still think it's not necessary.
> 
> ok😂
> 
> @Ferruh, what do you think?
> 
> >
> >> In addition, even if other drivers do not add this check in
> >> dev_configure, this patch does not seem to affect the current behavior of
> these drivers.
> >>
> >>> BTW, even if this behavior is accepted by others, still some comments below.
> >>>
> >>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> >>>> ---
> >>>>    app/test-pmd/cmdline.c | 42
> >>>> ++++++++++++++++++++++++++++++++++++++++-
> >>>> -
> >>>>    1 file changed, 40 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> >>>> 5fdcc1c..ddbc629 100644
> >>>> --- a/app/test-pmd/cmdline.c
> >>>> +++ b/app/test-pmd/cmdline.c
> >>>> @@ -1549,8 +1549,12 @@ cmd_config_speed_all_parsed(void
> >> *parsed_result,
> >>>>    			__rte_unused void *data)
> >>>>    {
> >>>>    	struct cmd_config_speed_all *res = parsed_result;
> >>>> +	uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
> >>>> +	struct rte_port *port;
> >>>>    	uint32_t link_speed;
> >>>>    	portid_t pid;
> >>>> +	portid_t i;
> >>>> +	int ret;
> >>>>
> >>>>    	if (!all_ports_stopped()) {
> >>>>    		printf("Please stop all ports first\n"); @@ -1562,7 +1566,26
> >>>> @@ cmd_config_speed_all_parsed(void *parsed_result,
> >>>>    		return;
> >>>>
> >>>>    	RTE_ETH_FOREACH_DEV(pid) {
> >>>> -		ports[pid].dev_conf.link_speeds = link_speed;
> >>>> +		port = &ports[pid];
> >>>> +		old_link_speeds[pid] = port->dev_conf.link_speeds;
> >>>> +		port->dev_conf.link_speeds = link_speed;
> >>>> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
> >>>> +					    &port->dev_conf);
> >>>> +		if (ret < 0) {
> >>>> +			printf("Failed to check link speeds for port %d, ret
> >>>> = %d.\n",
> >>>> +				pid, ret);
> >>>> +			goto roolback;
> >>> Why don't you just add restoring all ports speed here and then
> >>> "break"? No
> >> matter one dev fails or not, all ports will do reconfig from your code logic.
> >>> And you type rollback wrongly.
> >> It cannot exit directly after restoring all ports speed. If the cmd
> >> fails to execute, it is necessary to reconfigure device with the correct
> configuration.
> >>    Because "nb_rx/tx_queues" in dev->data are cleared to zero if
> >> dev_configure fails to be executed in PMD driver.
> > ?
> > cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); is at the end of this cmd.
> This will re-config all ports.
> > I don't understand why can't you add a restoring in this if and break this loop
> and do this reconfig.
> What do you mean? If a port fails among all ports, only the failed port is
> restored and reconfigured. I suppose that's what you mean?
> The cmd "port config all speed xxx duplex xxx" applies to all ports. If it fails to be
> delivered, the user considers that the cmd does not take effect.
> So it is necessary to restore and reconfigure all ports to the previous state.

You still don't understand me. See below. I mean the following.
You don't need to got to rollback. Because "cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);" will config all ports anyway already no matter you restore speeds or not.
Can you read the code carefully? "cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);" is always there and it configs ALL ports.

static void
cmd_config_speed_all_parsed(void *parsed_result,
			__rte_unused struct cmdline *cl,
			__rte_unused void *data)
{
	struct cmd_config_speed_all *res = parsed_result;
	uint32_t link_speed;
	portid_t pid;

	if (!all_ports_stopped()) {
		printf("Please stop all ports first\n");
		return;
	}

	if (parse_and_check_speed_duplex(res->value1, res->value2,
			&link_speed) < 0)
		return;

	RTE_ETH_FOREACH_DEV(pid) {
-		ports[pid].dev_conf.link_speeds = link_speed;
+		port = &ports[pid];
+		old_link_speeds[pid] = port->dev_conf.link_speeds;
+		port->dev_conf.link_speeds = link_speed;
+		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
+					    &port->dev_conf);
+		if (ret < 0) {
+			printf("Failed to check link speeds for port %d, ret = %d.\n",
+				pid, ret);
+			for (i = 0; i <= pid; i++) {
+				port = &ports[i];
+				port->dev_conf.link_speeds = old_link_speeds[i];
+			}
+			break;
+		}
	}

	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
}

> >
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> >>>> +
> >>>> +	return;
> >>>> +
> >>>> +roolback:
> >>>> +	for (i = 0; i <= pid; i++) {
> >>>> +		port = &ports[i];
> >>>> +		port->dev_conf.link_speeds = old_link_speeds[i];
> >>>>    	}
> >>>>
> >>>>    	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); @@ -1621,7
> >>>> +1644,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
> >>>>    				__rte_unused void *data)
> >>>>    {
> >>>>    	struct cmd_config_speed_specific *res = parsed_result;
> >>>> +	uint32_t old_link_speeds;
> >>>> +	struct rte_port *port;
> >>>>    	uint32_t link_speed;
> >>>> +	int ret;
> >>>>
> >>>>    	if (!all_ports_stopped()) {
> >>>>    		printf("Please stop all ports first\n"); @@ -1635,8 +1661,20
> >>>> @@ cmd_config_speed_specific_parsed(void *parsed_result,
> >>>>    			&link_speed) < 0)
> >>>>    		return;
> >>>>
> >>>> -	ports[res->id].dev_conf.link_speeds = link_speed;
> >>>> +	port = &ports[res->id];
> >>>> +	old_link_speeds = port->dev_conf.link_speeds;
> >>>> +	port->dev_conf.link_speeds = link_speed;
> >>>> +	ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
> >>>> +				    &port->dev_conf);
> >>>> +	if (ret < 0) {
> >>>> +		printf("Failed to check link speeds for port %d, ret = %d.\n",
> >>>> +			res->id, ret);
> >>>> +		port->dev_conf.link_speeds = old_link_speeds;
> >>>> +	}
> >>>>
> >>>> +	/*
> >>>> +	 * If the cmd fails to execute, it is necessary to reconfigure device.
> >>>> +	 */
> >>>>    	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);  }
> >>>>
> >>>> --
> >>>> 2.7.4
> >>> .

Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check before port start

[Huisong Li]

在 2021/5/6 10:36, Li, Xiaoyun 写道:
>
>> -----Original Message-----
>> From: Huisong Li <lihuisong@huawei.com>
>> Sent: Tuesday, May 4, 2021 09:46
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check
>> before port start
>>
>>
>> 在 2021/4/30 12:46, Li, Xiaoyun 写道:
>>>> -----Original Message-----
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>> Sent: Friday, April 30, 2021 12:04
>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>
>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed
>>>> check before port start
>>>>
>>>>
>>>> 在 2021/4/30 11:19, Li, Xiaoyun 写道:
>>>>>> -----Original Message-----
>>>>>> From: Min Hu (Connor) <humin29@huawei.com>
>>>>>> Sent: Wednesday, April 28, 2021 16:37
>>>>>> To: dev@dpdk.org
>>>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun
>>>>>> <xiaoyun.li@intel.com>
>>>>>> Subject: [PATCH v2 1/2] app/testpmd: add link speed check before
>>>>>> port start
>>>>>>
>>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>>
>>>>>> Currently, to check whether the configured link_speeds is valid, we
>>>>>> have to run "port start". In addition, if the configuration fails,
>>>>>> "port-
>>>>> dev_conf.link_speeds"
>>>>>> maintained in testpmd cannot be restored.
>>>>>>
>>>>>> This patch adds the link_speeds check before port start by calling
>>>>>> dev_configure, and resolves these problems.
>>>>> Not sure about this patch. I don't think you can fix the issue you mentioned.
>>>>> Probably only hns3 does speed check in dev_configure. I don't see
>>>>> this in other
>>>> drivers, not in i40e/ice/mlx.
>>>>> I guess it's because if it's not supported speed, it will just be
>>>>> UNKNOWN and
>>>> user can config again?
>>>>
>>>> I think that the validity of the configuration delivered by
>>>> dev_configure is ensured by this interface and cannot be left to the backend.
>>>>
>>>> Because it facilitates users to handle abnormal configurations in a
>>>> timely manner. It may be more appropriate for the driver to do this
>>>> check in dev_configure.
>>> I still think it's not necessary.
>> ok😂
>>
>> @Ferruh, what do you think?
>>
>>>> In addition, even if other drivers do not add this check in
>>>> dev_configure, this patch does not seem to affect the current behavior of
>> these drivers.
>>>>> BTW, even if this behavior is accepted by others, still some comments below.
>>>>>
>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>>> ---
>>>>>>     app/test-pmd/cmdline.c | 42
>>>>>> ++++++++++++++++++++++++++++++++++++++++-
>>>>>> -
>>>>>>     1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>>>>>> 5fdcc1c..ddbc629 100644
>>>>>> --- a/app/test-pmd/cmdline.c
>>>>>> +++ b/app/test-pmd/cmdline.c
>>>>>> @@ -1549,8 +1549,12 @@ cmd_config_speed_all_parsed(void
>>>> *parsed_result,
>>>>>>     			__rte_unused void *data)
>>>>>>     {
>>>>>>     	struct cmd_config_speed_all *res = parsed_result;
>>>>>> +	uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
>>>>>> +	struct rte_port *port;
>>>>>>     	uint32_t link_speed;
>>>>>>     	portid_t pid;
>>>>>> +	portid_t i;
>>>>>> +	int ret;
>>>>>>
>>>>>>     	if (!all_ports_stopped()) {
>>>>>>     		printf("Please stop all ports first\n"); @@ -1562,7 +1566,26
>>>>>> @@ cmd_config_speed_all_parsed(void *parsed_result,
>>>>>>     		return;
>>>>>>
>>>>>>     	RTE_ETH_FOREACH_DEV(pid) {
>>>>>> -		ports[pid].dev_conf.link_speeds = link_speed;
>>>>>> +		port = &ports[pid];
>>>>>> +		old_link_speeds[pid] = port->dev_conf.link_speeds;
>>>>>> +		port->dev_conf.link_speeds = link_speed;
>>>>>> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
>>>>>> +					    &port->dev_conf);
>>>>>> +		if (ret < 0) {
>>>>>> +			printf("Failed to check link speeds for port %d, ret
>>>>>> = %d.\n",
>>>>>> +				pid, ret);
>>>>>> +			goto roolback;
>>>>> Why don't you just add restoring all ports speed here and then
>>>>> "break"? No
>>>> matter one dev fails or not, all ports will do reconfig from your code logic.
>>>>> And you type rollback wrongly.
>>>> It cannot exit directly after restoring all ports speed. If the cmd
>>>> fails to execute, it is necessary to reconfigure device with the correct
>> configuration.
>>>>     Because "nb_rx/tx_queues" in dev->data are cleared to zero if
>>>> dev_configure fails to be executed in PMD driver.
>>> ?
>>> cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); is at the end of this cmd.
>> This will re-config all ports.
>>> I don't understand why can't you add a restoring in this if and break this loop
>> and do this reconfig.
>> What do you mean? If a port fails among all ports, only the failed port is
>> restored and reconfigured. I suppose that's what you mean?
>> The cmd "port config all speed xxx duplex xxx" applies to all ports. If it fails to be
>> delivered, the user considers that the cmd does not take effect.
>> So it is necessary to restore and reconfigure all ports to the previous state.
> You still don't understand me. See below. I mean the following.
> You don't need to got to rollback. Because "cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);" will config all ports anyway already no matter you restore speeds or not.
> Can you read the code carefully? "cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);" is always there and it configs ALL ports.
>
> static void
> cmd_config_speed_all_parsed(void *parsed_result,
> 			__rte_unused struct cmdline *cl,
> 			__rte_unused void *data)
> {
> 	struct cmd_config_speed_all *res = parsed_result;
> 	uint32_t link_speed;
> 	portid_t pid;
>
> 	if (!all_ports_stopped()) {
> 		printf("Please stop all ports first\n");
> 		return;
> 	}
>
> 	if (parse_and_check_speed_duplex(res->value1, res->value2,
> 			&link_speed) < 0)
> 		return;
>
> 	RTE_ETH_FOREACH_DEV(pid) {
> -		ports[pid].dev_conf.link_speeds = link_speed;
> +		port = &ports[pid];
> +		old_link_speeds[pid] = port->dev_conf.link_speeds;
> +		port->dev_conf.link_speeds = link_speed;
> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
> +					    &port->dev_conf);
> +		if (ret < 0) {
> +			printf("Failed to check link speeds for port %d, ret = %d.\n",
> +				pid, ret);
> +			for (i = 0; i <= pid; i++) {
> +				port = &ports[i];
> +				port->dev_conf.link_speeds = old_link_speeds[i];
> +			}
> +			break;
> +		}
> 	}
>
> 	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> }

It looks good!  Now we're left with a question about whether we need to 
add this link speed check.

@Ferruh, what do you think?

>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>>>>>> +
>>>>>> +	return;
>>>>>> +
>>>>>> +roolback:
>>>>>> +	for (i = 0; i <= pid; i++) {
>>>>>> +		port = &ports[i];
>>>>>> +		port->dev_conf.link_speeds = old_link_speeds[i];
>>>>>>     	}
>>>>>>
>>>>>>     	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); @@ -1621,7
>>>>>> +1644,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
>>>>>>     				__rte_unused void *data)
>>>>>>     {
>>>>>>     	struct cmd_config_speed_specific *res = parsed_result;
>>>>>> +	uint32_t old_link_speeds;
>>>>>> +	struct rte_port *port;
>>>>>>     	uint32_t link_speed;
>>>>>> +	int ret;
>>>>>>
>>>>>>     	if (!all_ports_stopped()) {
>>>>>>     		printf("Please stop all ports first\n"); @@ -1635,8 +1661,20
>>>>>> @@ cmd_config_speed_specific_parsed(void *parsed_result,
>>>>>>     			&link_speed) < 0)
>>>>>>     		return;
>>>>>>
>>>>>> -	ports[res->id].dev_conf.link_speeds = link_speed;
>>>>>> +	port = &ports[res->id];
>>>>>> +	old_link_speeds = port->dev_conf.link_speeds;
>>>>>> +	port->dev_conf.link_speeds = link_speed;
>>>>>> +	ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
>>>>>> +				    &port->dev_conf);
>>>>>> +	if (ret < 0) {
>>>>>> +		printf("Failed to check link speeds for port %d, ret = %d.\n",
>>>>>> +			res->id, ret);
>>>>>> +		port->dev_conf.link_speeds = old_link_speeds;
>>>>>> +	}
>>>>>>
>>>>>> +	/*
>>>>>> +	 * If the cmd fails to execute, it is necessary to reconfigure device.
>>>>>> +	 */
>>>>>>     	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);  }
>>>>>>
>>>>>> --
>>>>>> 2.7.4
>>>>> .

Re: [dpdk-dev] [PATCH v2 0/2] fixes for link speed

[Andrew Rybchenko]

What is the difference of v2 from v1?
Please, try to add version changelog in the future.

On 4/28/21 11:36 AM, Min Hu (Connor) wrote:
> This patchset contains fixes for link speed in testpmd.
> 
> Huisong Li (2):
>   app/testpmd: add link speed check before port start
>   app/testpmd: fix link speed for a specified port
> 
>  app/test-pmd/cmdline.c | 50 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 44 insertions(+), 6 deletions(-)
> 

Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check before port start

[Andrew Rybchenko]

On 4/30/21 6:19 AM, Li, Xiaoyun wrote:
> 
>> -----Original Message-----
>> From: Min Hu (Connor) <humin29@huawei.com>
>> Sent: Wednesday, April 28, 2021 16:37
>> To: dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>
>> Subject: [PATCH v2 1/2] app/testpmd: add link speed check before port start
>>
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> Currently, to check whether the configured link_speeds is valid, we have to run
>> "port start". In addition, if the configuration fails, "port->dev_conf.link_speeds"
>> maintained in testpmd cannot be restored.
>>
>> This patch adds the link_speeds check before port start by calling dev_configure,
>> and resolves these problems.
> 
> Not sure about this patch. I don't think you can fix the issue you mentioned.
> Probably only hns3 does speed check in dev_configure. I don't see this in other drivers, not in i40e/ice/mlx.

The configuration check may be done by passing the
configuration to HW and HW may reject it.
(Just as a side note that it is not that simple/)

> I guess it's because if it's not supported speed, it will just be UNKNOWN and user can config again?
> 
> BTW, even if this behavior is accepted by others, still some comments below.
> 

[snip]

Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check before port start

[Andrew Rybchenko]

On 4/28/21 11:36 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> Currently, to check whether the configured link_speeds is valid, we
> have to run "port start". In addition, if the configuration fails,
> "port->dev_conf.link_speeds" maintained in testpmd cannot be
> restored.
> 
> This patch adds the link_speeds check before port start by calling
> dev_configure, and resolves these problems.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  app/test-pmd/cmdline.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 5fdcc1c..ddbc629 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1549,8 +1549,12 @@ cmd_config_speed_all_parsed(void *parsed_result,
>  			__rte_unused void *data)
>  {
>  	struct cmd_config_speed_all *res = parsed_result;
> +	uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
> +	struct rte_port *port;
>  	uint32_t link_speed;
>  	portid_t pid;
> +	portid_t i;
> +	int ret;
>  
>  	if (!all_ports_stopped()) {
>  		printf("Please stop all ports first\n");
> @@ -1562,7 +1566,26 @@ cmd_config_speed_all_parsed(void *parsed_result,
>  		return;
>  
>  	RTE_ETH_FOREACH_DEV(pid) {
> -		ports[pid].dev_conf.link_speeds = link_speed;
> +		port = &ports[pid];
> +		old_link_speeds[pid] = port->dev_conf.link_speeds;
> +		port->dev_conf.link_speeds = link_speed;
> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
> +					    &port->dev_conf);
> +		if (ret < 0) {
> +			printf("Failed to check link speeds for port %d, ret = %d.\n",
> +				pid, ret);
> +			goto roolback;
> +		}
> +	}
> +
> +	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> +
> +	return;
> +
> +roolback:
> +	for (i = 0; i <= pid; i++) {
> +		port = &ports[i];
> +		port->dev_conf.link_speeds = old_link_speeds[i];
>  	}
>  
>  	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> @@ -1621,7 +1644,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
>  				__rte_unused void *data)
>  {
>  	struct cmd_config_speed_specific *res = parsed_result;
> +	uint32_t old_link_speeds;
> +	struct rte_port *port;
>  	uint32_t link_speed;
> +	int ret;
>  
>  	if (!all_ports_stopped()) {
>  		printf("Please stop all ports first\n");
> @@ -1635,8 +1661,20 @@ cmd_config_speed_specific_parsed(void *parsed_result,
>  			&link_speed) < 0)
>  		return;
>  
> -	ports[res->id].dev_conf.link_speeds = link_speed;
> +	port = &ports[res->id];
> +	old_link_speeds = port->dev_conf.link_speeds;
> +	port->dev_conf.link_speeds = link_speed;
> +	ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
> +				    &port->dev_conf);
> +	if (ret < 0) {
> +		printf("Failed to check link speeds for port %d, ret = %d.\n",
> +			res->id, ret);
> +		port->dev_conf.link_speeds = old_link_speeds;

In fact, you don't really know here if link speed is a problem
or something else configured while device is stopped.
So, I think there is no point to over-complicate it
with addition of possibly misleading diagnostics.

> +	}
>  
> +	/*
> +	 * If the cmd fails to execute, it is necessary to reconfigure device.
> +	 */
>  	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>  }
>  
> 

Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check before port start

[Huisong Li]

在 2021/6/8 18:49, Andrew Rybchenko 写道:
> On 4/28/21 11:36 AM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> Currently, to check whether the configured link_speeds is valid, we
>> have to run "port start". In addition, if the configuration fails,
>> "port->dev_conf.link_speeds" maintained in testpmd cannot be
>> restored.
>>
>> This patch adds the link_speeds check before port start by calling
>> dev_configure, and resolves these problems.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   app/test-pmd/cmdline.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 5fdcc1c..ddbc629 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -1549,8 +1549,12 @@ cmd_config_speed_all_parsed(void *parsed_result,
>>   			__rte_unused void *data)
>>   {
>>   	struct cmd_config_speed_all *res = parsed_result;
>> +	uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
>> +	struct rte_port *port;
>>   	uint32_t link_speed;
>>   	portid_t pid;
>> +	portid_t i;
>> +	int ret;
>>   
>>   	if (!all_ports_stopped()) {
>>   		printf("Please stop all ports first\n");
>> @@ -1562,7 +1566,26 @@ cmd_config_speed_all_parsed(void *parsed_result,
>>   		return;
>>   
>>   	RTE_ETH_FOREACH_DEV(pid) {
>> -		ports[pid].dev_conf.link_speeds = link_speed;
>> +		port = &ports[pid];
>> +		old_link_speeds[pid] = port->dev_conf.link_speeds;
>> +		port->dev_conf.link_speeds = link_speed;
>> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
>> +					    &port->dev_conf);
>> +		if (ret < 0) {
>> +			printf("Failed to check link speeds for port %d, ret = %d.\n",
>> +				pid, ret);
>> +			goto roolback;
>> +		}
>> +	}
>> +
>> +	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>> +
>> +	return;
>> +
>> +roolback:
>> +	for (i = 0; i <= pid; i++) {
>> +		port = &ports[i];
>> +		port->dev_conf.link_speeds = old_link_speeds[i];
>>   	}
>>   
>>   	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>> @@ -1621,7 +1644,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
>>   				__rte_unused void *data)
>>   {
>>   	struct cmd_config_speed_specific *res = parsed_result;
>> +	uint32_t old_link_speeds;
>> +	struct rte_port *port;
>>   	uint32_t link_speed;
>> +	int ret;
>>   
>>   	if (!all_ports_stopped()) {
>>   		printf("Please stop all ports first\n");
>> @@ -1635,8 +1661,20 @@ cmd_config_speed_specific_parsed(void *parsed_result,
>>   			&link_speed) < 0)
>>   		return;
>>   
>> -	ports[res->id].dev_conf.link_speeds = link_speed;
>> +	port = &ports[res->id];
>> +	old_link_speeds = port->dev_conf.link_speeds;
>> +	port->dev_conf.link_speeds = link_speed;
>> +	ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
>> +				    &port->dev_conf);
>> +	if (ret < 0) {
>> +		printf("Failed to check link speeds for port %d, ret = %d.\n",
>> +			res->id, ret);
>> +		port->dev_conf.link_speeds = old_link_speeds;
> In fact, you don't really know here if link speed is a problem
> or something else configured while device is stopped.
> So, I think there is no point to over-complicate it
> with addition of possibly misleading diagnostics.

Thanks for your reivew.

You're right. There are many configurations in tespmd that need to 
recall dev_configure and are not verified during configuration.

Only one of them is verified, which does not indicate that the failure 
is caused by the current configuration. So let's keep it the way it was.

>> +	}
>>   
>> +	/*
>> +	 * If the cmd fails to execute, it is necessary to reconfigure device.
>> +	 */
>>   	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>>   }
>>   
>>
> .

Re: [dpdk-dev] [PATCH v2 0/2] fixes for link speed

[Huisong Li]

在 2021/6/8 18:29, Andrew Rybchenko 写道:
> What is the difference of v2 from v1?
> Please, try to add version changelog in the future.
ok😁
> On 4/28/21 11:36 AM, Min Hu (Connor) wrote:
>> This patchset contains fixes for link speed in testpmd.
>>
>> Huisong Li (2):
>>    app/testpmd: add link speed check before port start
>>    app/testpmd: fix link speed for a specified port
>>
>>   app/test-pmd/cmdline.c | 50 ++++++++++++++++++++++++++++++++++------
>>   1 file changed, 44 insertions(+), 6 deletions(-)
>>
> .

Re: [dpdk-dev] [PATCH v2 0/2] fixes for link speed

[Min Hu (Connor)]

Hi, all,
	any comments?

在 2021/4/28 16:36, Min Hu (Connor) 写道:
> This patchset contains fixes for link speed in testpmd.
> 
> Huisong Li (2):
>    app/testpmd: add link speed check before port start
>    app/testpmd: fix link speed for a specified port
> 
>   app/test-pmd/cmdline.c | 50 ++++++++++++++++++++++++++++++++++------
>   1 file changed, 44 insertions(+), 6 deletions(-)
> 

Re: [dpdk-dev] [PATCH v2 0/2] fixes for link speed

[Singh, Aman Deep]

Hi Min Hu,

On 6/28/2021 8:55 AM, Min Hu (Connor) wrote:
> Hi, all,
>     any comments?

As part of this series-
[PATCH v2 1/2]:  As per Huisong Li "So let's keep it the way it was."
[PATCH v2 2/2]: Has been acked.

>
> 在 2021/4/28 16:36, Min Hu (Connor) 写道:
>> This patchset contains fixes for link speed in testpmd.
>>
>> Huisong Li (2):
>>    app/testpmd: add link speed check before port start
>>    app/testpmd: fix link speed for a specified port
>>
>>   app/test-pmd/cmdline.c | 50 ++++++++++++++++++++++++++++++++++------
>>   1 file changed, 44 insertions(+), 6 deletions(-)
>>

Re: [dpdk-dev] [PATCH v2 0/2] fixes for link speed

[Min Hu (Connor)]

Hi, Singh

在 2021/6/29 14:24, Singh, Aman Deep 写道:
> Hi Min Hu,
> 
> On 6/28/2021 8:55 AM, Min Hu (Connor) wrote:
>> Hi, all,
>>     any comments?
> 
> As part of this series-
> [PATCH v2 1/2]:  As per Huisong Li "So let's keep it the way it was."
This patch can be abandoned.

> [PATCH v2 2/2]: Has been acked.
This patch could be merged, thanks。
> 
>>
>> 在 2021/4/28 16:36, Min Hu (Connor) 写道:
>>> This patchset contains fixes for link speed in testpmd.
>>>
>>> Huisong Li (2):
>>>    app/testpmd: add link speed check before port start
>>>    app/testpmd: fix link speed for a specified port
>>>
>>>   app/test-pmd/cmdline.c | 50 ++++++++++++++++++++++++++++++++++------
>>>   1 file changed, 44 insertions(+), 6 deletions(-)
>>>
> .

Re: [dpdk-dev] [PATCH v2 2/2] app/testpmd: fix link speed for a specified port

[Andrew Rybchenko]

On 4/29/21 11:42 AM, Li, Xiaoyun wrote:
>> -----Original Message-----
>> From: Min Hu (Connor) <humin29@huawei.com>
>> Sent: Wednesday, April 28, 2021 16:37
>> To: dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>
>> Subject: [PATCH v2 2/2] app/testpmd: fix link speed for a specified port
>>
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> When we use the following cmd to modify the link speed of specified
>> port: "port config <port_id> speed xxx duplex xxx", we have to stop all ports. It's
>> not necessary.
>>
>> Fixes: 82113036e4e5 ("ethdev: redesign link speed config")
>> 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 | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
> 

From v1:
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied the patch only, thanks.

Re: [dpdk-dev] [PATCH v2 0/2] fixes for link speed

[Andrew Rybchenko]

On 6/29/21 10:31 AM, Min Hu (Connor) wrote:
> Hi, Singh
> 
> 在 2021/6/29 14:24, Singh, Aman Deep 写道:
>> Hi Min Hu,
>>
>> On 6/28/2021 8:55 AM, Min Hu (Connor) wrote:
>>> Hi, all,
>>>     any comments?
>>
>> As part of this series-
>> [PATCH v2 1/2]:  As per Huisong Li "So let's keep it the way it was."
> This patch can be abandoned.
> 
>> [PATCH v2 2/2]: Has been acked.
> This patch could be merged, thanks。

Done.