Re: [dpdk-dev] [PATCH V1 2/2] examples/ethtool: support the query of MTU

["lihuisong (C)" <lihuisong@huawei.com>]

在 2021/11/18 1:51, David Marchand 写道:
> On Thu, Apr 29, 2021 at 12:53 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> This patch supports the query of MTU.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   examples/ethtool/ethtool-app/ethapp.c | 48 +++++++++++++++++++++++++----------
>>   examples/ethtool/lib/rte_ethtool.c    |  7 +++++
>>   examples/ethtool/lib/rte_ethtool.h    | 14 ++++++++++
>>   3 files changed, 56 insertions(+), 13 deletions(-)
>>
>> diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
>> index fc743ce..aed4bc5 100644
>> --- a/examples/ethtool/ethtool-app/ethapp.c
>> +++ b/examples/ethtool/ethtool-app/ethapp.c
>> @@ -517,31 +517,41 @@ pcmd_macaddr_callback(void *ptr_params,
>>   static void
>>   pcmd_mtu_callback(void *ptr_params,
>>          __rte_unused struct cmdline *ctx,
>> -       __rte_unused void *ptr_data)
>> +       void *ptr_data)
>>   {
>>          struct pcmd_intstr_params *params = ptr_params;
>>          int stat;
>>          uint16_t new_mtu;
>>          char *ptr_parse_end;
>>
>> -       new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
>> -       if (*ptr_parse_end != '\0' ||
>> -                       new_mtu < RTE_ETHER_MIN_MTU ||
>> -                       new_mtu > RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
>> -               printf("Port %i: Invalid MTU value\n", params->port);
>> +       if (ptr_data == NULL) {
>> +               new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
>> +               if (*ptr_parse_end != '\0' ||
>> +                               new_mtu < RTE_ETHER_MIN_MTU ||
>> +                               new_mtu > RTE_ETHER_MAX_JUMBO_FRAME_LEN) {
>> +                       printf("Port %i: Invalid MTU value\n", params->port);
>> +                       return;
>> +               }
>> +               stat = rte_ethtool_net_change_mtu(params->port, new_mtu);
>> +               if (stat == 0)
>> +                       printf("Port %i: MTU set to %i\n", params->port,
>> +                               new_mtu);
>> +               else if (stat == -ENOTSUP)
>> +                       printf("Port %i: Operation not supported\n",
>> +                               params->port);
>> +               else
>> +                       printf("Port %i: Error setting MTU\n", params->port);
>> +
>>                  return;
>>          }
>> -       stat = rte_ethtool_net_change_mtu(params->port, new_mtu);
>> -       if (stat == 0)
>> -               printf("Port %i: MTU set to %i\n", params->port, new_mtu);
>> -       else if (stat == -ENOTSUP)
>> +
>> +       stat = rte_ethtool_net_get_mtu(params->port, &new_mtu);
>> +       if (stat)
>>                  printf("Port %i: Operation not supported\n", params->port);
>>          else
>> -               printf("Port %i: Error setting MTU\n", params->port);
>> +               printf("Port %i: Current MTU: %i\n", params->port, new_mtu);
>>   }
>
> Please separate as two helpers, there is no code shared between set
> and get, afaics.
>
The main purpose of my modification is to be consistent with other 
command styles.

Like: pcmd_ringparam & pcmd_ringparam_set and pcmd_macaddr & 
pcmd_macaddr_get.

Do you mean to use a separate interface to implement MTU query?

>> -
>> -
>>   static void pcmd_portstats_callback(__rte_unused void *ptr_params,
>>          __rte_unused struct cmdline *ctx,
>>          __rte_unused void *ptr_data)
>> @@ -799,6 +809,17 @@ cmdline_parse_inst_t pcmd_macaddr = {
>>                  NULL
>>          },
>>   };
>> +cmdline_parse_inst_t pcmd_mtu_get = {
>> +       .f = pcmd_mtu_callback,
>> +       .data = (void *)0x01,
>> +       .help_str = "mtu <port_id>\n"
>> +               "     Get MTU",
>> +       .tokens = {
>> +               (void *)&pcmd_mtu_token_cmd,
>> +               (void *)&pcmd_intstr_token_port,
>> +               NULL
>> +       },
>> +};
>>   cmdline_parse_inst_t pcmd_mtu = {
>>          .f = pcmd_mtu_callback,
>>          .data = NULL,
>> @@ -879,6 +900,7 @@ cmdline_parse_ctx_t list_prompt_commands[] = {
>>          (cmdline_parse_inst_t *)&pcmd_link,
>>          (cmdline_parse_inst_t *)&pcmd_macaddr_get,
>>          (cmdline_parse_inst_t *)&pcmd_macaddr,
>> +       (cmdline_parse_inst_t *)&pcmd_mtu_get,
>>          (cmdline_parse_inst_t *)&pcmd_mtu,
>>          (cmdline_parse_inst_t *)&pcmd_open,
>>          (cmdline_parse_inst_t *)&pcmd_pause_noopt,
>> diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
>> index 73193ed..e69b2c1 100644
>> --- a/examples/ethtool/lib/rte_ethtool.c
>> +++ b/examples/ethtool/lib/rte_ethtool.c
>> @@ -345,6 +345,13 @@ rte_ethtool_net_validate_addr(uint16_t port_id __rte_unused,
>>          return rte_is_valid_assigned_ether_addr(addr);
>>   }
>>
>> +int
>> +rte_ethtool_net_get_mtu(uint16_t port_id, uint16_t *mtu)
>> +{
>> +       RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +       return rte_eth_dev_get_mtu(port_id, mtu);
>> +}
>>
>>   int
>>   rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
>> diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
>> index fe3250e..434ad1d 100644
>> --- a/examples/ethtool/lib/rte_ethtool.h
>> +++ b/examples/ethtool/lib/rte_ethtool.h
>> @@ -23,6 +23,7 @@
>>    * rte_ethtool_net_stop:            net_device_ops::ndo_stop
>>    * rte_ethtool_net_set_mac_addr:    net_device_ops::ndo_set_mac_address
>>    * rte_ethtool_net_validate_addr:   net_device_ops::ndo_validate_addr
>> + * rte_ethtool_net_get_mtu:         net_device_ops::ndo_get_mtu
>>    * rte_ethtool_net_change_mtu:      net_device_ops::ndo_change_mtu
>>    * rte_ethtool_net_get_stats64:     net_device_ops::ndo_get_stats64
>>    * rte_ethtool_net_vlan_rx_add_vid  net_device_ops::ndo_vlan_rx_add_vid
>> @@ -296,6 +297,19 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
>>                                  struct rte_ether_addr *addr);
>>
>>   /**
>> + * Retrieve the MTU of an Ethernet device.
>> + *
>> + * @param port_id
>> + *   The port identifier of the Ethernet device.
>> + * @param mtu
>> + *   A pointer to a uint16_t where the retrieved MTU is to be stored.
>> + * @return
>> + *   - (0) if successful.
>> + *   - (-ENODEV) if *port_id* invalid.
>> + */
>> +int rte_ethtool_net_get_mtu(uint16_t port_id, uint16_t *mtu);
>> +
>> +/**
>>    * Setting the Ethernet device maximum Tx unit.
>>    *
>>    * @param port_id
>> --
>> 2.8.1
>>
>