On 4/28/21 11:36 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuis...@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 <lihuis...@huawei.com>
> Signed-off-by: Min Hu (Connor) <humi...@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);
> }
>
>
