* [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query
@ 2021-04-29 10:53 Min Hu (Connor)
2021-04-29 10:53 ` [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU Min Hu (Connor)
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Min Hu (Connor) @ 2021-04-29 10:53 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit
From: Huisong Li <lihuisong@huawei.com>
This patchset fixes MTU data type when set MTU, and supports the query
of MTU.
Huisong Li (2):
examples/ethtool: fix data type of MTU
examples/ethtool: support the query of MTU
examples/ethtool/ethtool-app/ethapp.c | 54 ++++++++++++++++++----------
examples/ethtool/lib/rte_ethtool.c | 16 ++++++++---
examples/ethtool/lib/rte_ethtool.h | 16 ++++++++++-
3 files changed, 63 insertions(+), 23 deletions(-)
--
2.8.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU
2021-04-29 10:53 [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query Min Hu (Connor)
@ 2021-04-29 10:53 ` Min Hu (Connor)
2021-11-17 17:49 ` David Marchand
2021-04-29 10:53 ` [dpdk-dev] [PATCH V1 2/2] examples/ethtool: support the query " Min Hu (Connor)
2021-06-28 3:23 ` [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query Min Hu (Connor)
2 siblings, 1 reply; 8+ messages in thread
From: Min Hu (Connor) @ 2021-04-29 10:53 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit
From: Huisong Li <lihuisong@huawei.com>
This patch changes the data type of 'mtu' in rte_ethtool_net_change_mtu()
from 'int' to 'uint16_t'.
Fixes: bda68ab9d1e7 ("examples/ethtool: add user-space ethtool sample application")
Cc: stable@dpdk.org
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
examples/ethtool/ethtool-app/ethapp.c | 6 +-----
examples/ethtool/lib/rte_ethtool.c | 9 +++++----
examples/ethtool/lib/rte_ethtool.h | 2 +-
3 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
index 36a1c37..fc743ce 100644
--- a/examples/ethtool/ethtool-app/ethapp.c
+++ b/examples/ethtool/ethtool-app/ethapp.c
@@ -521,13 +521,9 @@ pcmd_mtu_callback(void *ptr_params,
{
struct pcmd_intstr_params *params = ptr_params;
int stat;
- int new_mtu;
+ uint16_t new_mtu;
char *ptr_parse_end;
- if (!rte_eth_dev_is_valid_port(params->port)) {
- printf("Error: Invalid port number %i\n", params->port);
- return;
- }
new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
if (*ptr_parse_end != '\0' ||
new_mtu < RTE_ETHER_MIN_MTU ||
diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
index 4132516..73193ed 100644
--- a/examples/ethtool/lib/rte_ethtool.c
+++ b/examples/ethtool/lib/rte_ethtool.c
@@ -345,12 +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_change_mtu(uint16_t port_id, int mtu)
+rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
{
- if (mtu < 0 || mtu > UINT16_MAX)
- return -EINVAL;
- return rte_eth_dev_set_mtu(port_id, (uint16_t)mtu);
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+ return rte_eth_dev_set_mtu(port_id, mtu);
}
int
diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
index f177096..fe3250e 100644
--- a/examples/ethtool/lib/rte_ethtool.h
+++ b/examples/ethtool/lib/rte_ethtool.h
@@ -309,7 +309,7 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
* - (-EINVAL) if parameters invalid.
* - others depends on the specific operations implementation.
*/
-int rte_ethtool_net_change_mtu(uint16_t port_id, int mtu);
+int rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu);
/**
* Retrieve the Ethernet device traffic statistics
--
2.8.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [dpdk-dev] [PATCH V1 2/2] examples/ethtool: support the query of MTU
2021-04-29 10:53 [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query Min Hu (Connor)
2021-04-29 10:53 ` [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU Min Hu (Connor)
@ 2021-04-29 10:53 ` Min Hu (Connor)
2021-11-17 17:51 ` David Marchand
2021-06-28 3:23 ` [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query Min Hu (Connor)
2 siblings, 1 reply; 8+ messages in thread
From: Min Hu (Connor) @ 2021-04-29 10:53 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit
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);
}
-
-
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query
2021-04-29 10:53 [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query Min Hu (Connor)
2021-04-29 10:53 ` [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU Min Hu (Connor)
2021-04-29 10:53 ` [dpdk-dev] [PATCH V1 2/2] examples/ethtool: support the query " Min Hu (Connor)
@ 2021-06-28 3:23 ` Min Hu (Connor)
2 siblings, 0 replies; 8+ messages in thread
From: Min Hu (Connor) @ 2021-06-28 3:23 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit
Hi, all,
any comments?
在 2021/4/29 18:53, Min Hu (Connor) 写道:
> From: Huisong Li <lihuisong@huawei.com>
>
> This patchset fixes MTU data type when set MTU, and supports the query
> of MTU.
>
> Huisong Li (2):
> examples/ethtool: fix data type of MTU
> examples/ethtool: support the query of MTU
>
> examples/ethtool/ethtool-app/ethapp.c | 54 ++++++++++++++++++----------
> examples/ethtool/lib/rte_ethtool.c | 16 ++++++++---
> examples/ethtool/lib/rte_ethtool.h | 16 ++++++++++-
> 3 files changed, 63 insertions(+), 23 deletions(-)
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU
2021-04-29 10:53 ` [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU Min Hu (Connor)
@ 2021-11-17 17:49 ` David Marchand
2021-11-18 2:44 ` lihuisong (C)
0 siblings, 1 reply; 8+ messages in thread
From: David Marchand @ 2021-11-17 17:49 UTC (permalink / raw)
To: Min Hu (Connor); +Cc: dev, Yigit, Ferruh
On Thu, Apr 29, 2021 at 12:53 PM Min Hu (Connor) <humin29@huawei.com> wrote:
>
> From: Huisong Li <lihuisong@huawei.com>
>
> This patch changes the data type of 'mtu' in rte_ethtool_net_change_mtu()
> from 'int' to 'uint16_t'.
You did not describe why this change is needed.
>
> Fixes: bda68ab9d1e7 ("examples/ethtool: add user-space ethtool sample application")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> examples/ethtool/ethtool-app/ethapp.c | 6 +-----
> examples/ethtool/lib/rte_ethtool.c | 9 +++++----
> examples/ethtool/lib/rte_ethtool.h | 2 +-
> 3 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
> index 36a1c37..fc743ce 100644
> --- a/examples/ethtool/ethtool-app/ethapp.c
> +++ b/examples/ethtool/ethtool-app/ethapp.c
> @@ -521,13 +521,9 @@ pcmd_mtu_callback(void *ptr_params,
> {
> struct pcmd_intstr_params *params = ptr_params;
> int stat;
> - int new_mtu;
> + uint16_t new_mtu;
strtoul can overflow the stack, when storing to new_mtu some lines below.
You should either change new_mtu to unsigned long int or switch
strtoul to some other integer format helper.
> char *ptr_parse_end;
>
> - if (!rte_eth_dev_is_valid_port(params->port)) {
> - printf("Error: Invalid port number %i\n", params->port);
> - return;
> - }
Adding the check in lib/rte_ethtool.c is ok, but why not keep the
check in the application?
> new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
> if (*ptr_parse_end != '\0' ||
> new_mtu < RTE_ETHER_MIN_MTU ||
With this patch, rte_ethtool_net_change_mtu() now takes a uint16_t.
You should add a check on new_mtu < UINT16_MAX.
> diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
> index 4132516..73193ed 100644
> --- a/examples/ethtool/lib/rte_ethtool.c
> +++ b/examples/ethtool/lib/rte_ethtool.c
> @@ -345,12 +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_change_mtu(uint16_t port_id, int mtu)
> +rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
> {
> - if (mtu < 0 || mtu > UINT16_MAX)
> - return -EINVAL;
> - return rte_eth_dev_set_mtu(port_id, (uint16_t)mtu);
> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> + return rte_eth_dev_set_mtu(port_id, mtu);
> }
>
> int
> diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
> index f177096..fe3250e 100644
> --- a/examples/ethtool/lib/rte_ethtool.h
> +++ b/examples/ethtool/lib/rte_ethtool.h
> @@ -309,7 +309,7 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
> * - (-EINVAL) if parameters invalid.
> * - others depends on the specific operations implementation.
> */
> -int rte_ethtool_net_change_mtu(uint16_t port_id, int mtu);
> +int rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu);
>
> /**
> * Retrieve the Ethernet device traffic statistics
> --
> 2.8.1
>
--
David Marchand
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH V1 2/2] examples/ethtool: support the query of MTU
2021-04-29 10:53 ` [dpdk-dev] [PATCH V1 2/2] examples/ethtool: support the query " Min Hu (Connor)
@ 2021-11-17 17:51 ` David Marchand
2021-11-18 2:58 ` lihuisong (C)
0 siblings, 1 reply; 8+ messages in thread
From: David Marchand @ 2021-11-17 17:51 UTC (permalink / raw)
To: Min Hu (Connor); +Cc: dev, Yigit, Ferruh
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.
>
> -
> -
> 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
>
--
David Marchand
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU
2021-11-17 17:49 ` David Marchand
@ 2021-11-18 2:44 ` lihuisong (C)
0 siblings, 0 replies; 8+ messages in thread
From: lihuisong (C) @ 2021-11-18 2:44 UTC (permalink / raw)
To: David Marchand; +Cc: dev, Yigit, Ferruh, Min Hu (Connor)
在 2021/11/18 1:49, 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 changes the data type of 'mtu' in rte_ethtool_net_change_mtu()
>> from 'int' to 'uint16_t'.
> You did not describe why this change is needed.
Ensure that the input parameter of rte_ethtool_net_change_mtu() API in lib/rte_ethtool.c
re consistent with rte_eth_dev_set_mtu() in ethdev layer.
I will fix commit log.
>
>
>> Fixes: bda68ab9d1e7 ("examples/ethtool: add user-space ethtool sample application")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> examples/ethtool/ethtool-app/ethapp.c | 6 +-----
>> examples/ethtool/lib/rte_ethtool.c | 9 +++++----
>> examples/ethtool/lib/rte_ethtool.h | 2 +-
>> 3 files changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/examples/ethtool/ethtool-app/ethapp.c b/examples/ethtool/ethtool-app/ethapp.c
>> index 36a1c37..fc743ce 100644
>> --- a/examples/ethtool/ethtool-app/ethapp.c
>> +++ b/examples/ethtool/ethtool-app/ethapp.c
>> @@ -521,13 +521,9 @@ pcmd_mtu_callback(void *ptr_params,
>> {
>> struct pcmd_intstr_params *params = ptr_params;
>> int stat;
>> - int new_mtu;
>> + uint16_t new_mtu;
> strtoul can overflow the stack, when storing to new_mtu some lines below.
> You should either change new_mtu to unsigned long int or switch
> strtoul to some other integer format helper.
You are right. I want add an mtu specific cmd parameter structure:
struct pcmd_mtu_params {
cmdline_fixed_string_t cmd;
uint16_t port;
uint16_t mtu_value;
};
MTU size is limited when users enter the value.
Like:
cmdline_parse_token_num_t pcmd_mtu_token_port =
TOKEN_NUM_INITIALIZER(struct pcmd_mtu_params, mtu_value, RTE_UINT16);
What do you think, David?
>
>> char *ptr_parse_end;
>>
>> - if (!rte_eth_dev_is_valid_port(params->port)) {
>> - printf("Error: Invalid port number %i\n", params->port);
>> - return;
>> - }
> Adding the check in lib/rte_ethtool.c is ok, but why not keep the
> check in the application?
All right, let it stay here.
>
>
>> new_mtu = strtoul(params->opt, &ptr_parse_end, 10);
>> if (*ptr_parse_end != '\0' ||
>> new_mtu < RTE_ETHER_MIN_MTU ||
> With this patch, rte_ethtool_net_change_mtu() now takes a uint16_t.
> You should add a check on new_mtu < UINT16_MAX.
>
If we use above new method, this check can be removed.
>> diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
>> index 4132516..73193ed 100644
>> --- a/examples/ethtool/lib/rte_ethtool.c
>> +++ b/examples/ethtool/lib/rte_ethtool.c
>> @@ -345,12 +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_change_mtu(uint16_t port_id, int mtu)
>> +rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu)
>> {
>> - if (mtu < 0 || mtu > UINT16_MAX)
>> - return -EINVAL;
>> - return rte_eth_dev_set_mtu(port_id, (uint16_t)mtu);
>> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> + return rte_eth_dev_set_mtu(port_id, mtu);
>> }
>>
>> int
>> diff --git a/examples/ethtool/lib/rte_ethtool.h b/examples/ethtool/lib/rte_ethtool.h
>> index f177096..fe3250e 100644
>> --- a/examples/ethtool/lib/rte_ethtool.h
>> +++ b/examples/ethtool/lib/rte_ethtool.h
>> @@ -309,7 +309,7 @@ int rte_ethtool_net_validate_addr(uint16_t port_id,
>> * - (-EINVAL) if parameters invalid.
>> * - others depends on the specific operations implementation.
>> */
>> -int rte_ethtool_net_change_mtu(uint16_t port_id, int mtu);
>> +int rte_ethtool_net_change_mtu(uint16_t port_id, uint16_t mtu);
>>
>> /**
>> * Retrieve the Ethernet device traffic statistics
>> --
>> 2.8.1
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH V1 2/2] examples/ethtool: support the query of MTU
2021-11-17 17:51 ` David Marchand
@ 2021-11-18 2:58 ` lihuisong (C)
0 siblings, 0 replies; 8+ messages in thread
From: lihuisong (C) @ 2021-11-18 2:58 UTC (permalink / raw)
To: David Marchand; +Cc: dev, Yigit, Ferruh, Min Hu (Connor)
在 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
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-11-18 2:58 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-29 10:53 [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query Min Hu (Connor)
2021-04-29 10:53 ` [dpdk-dev] [PATCH V1 1/2] examples/ethtool: fix data type of MTU Min Hu (Connor)
2021-11-17 17:49 ` David Marchand
2021-11-18 2:44 ` lihuisong (C)
2021-04-29 10:53 ` [dpdk-dev] [PATCH V1 2/2] examples/ethtool: support the query " Min Hu (Connor)
2021-11-17 17:51 ` David Marchand
2021-11-18 2:58 ` lihuisong (C)
2021-06-28 3:23 ` [dpdk-dev] [PATCH V1 0/2] examples/ethtool: fix MTU set and add MTU query Min Hu (Connor)
DPDK patches and discussions
This inbox may be cloned and mirrored by anyone:
git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
dev@dpdk.org
public-inbox-index dev
Example config snippet for mirrors.
Newsgroup available over NNTP:
nntp://inbox.dpdk.org/inbox.dpdk.dev
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git