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