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);
>  }
>  
> 

Reply via email to