[PATCH 0/3] Support administrative link up and link down

[PATCH 0/3] Support administrative link up and link down

[Ouyang Changchun]

This patch series contain the following 3 items:
1. Add API to support administrative link up and down.
2. Implement the functionality of administrative link up and down in IXGBE PMD.
3. Add command in testpmd to test the functionality of administrative link up and down of PMD.

Ouyang Changchun (3):
  Add API for supporting administrative link up and down.
  Implement the functionality of administrative link up and down in
    IXGBE PMD.
  Add command line to test the functionality of administrative link up
    and down of PMD in testpmd.

 app/test-pmd/cmdline.c              | 78 +++++++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.c              | 14 +++++++
 app/test-pmd/testpmd.h              |  2 +
 lib/librte_ether/rte_ethdev.c       | 38 ++++++++++++++++++
 lib/librte_ether/rte_ethdev.h       | 34 ++++++++++++++++
 lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 58 +++++++++++++++++++++++++++
 6 files changed, 224 insertions(+)

-- 
1.9.0

[PATCH 1/3] ether: Add API to support administrative link up and down

[Ouyang Changchun]

This patch addes API to support administrative link up and down.

Signed-off-by: Ouyang Changchun <changchun.ouyang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_ether/rte_ethdev.c | 38 ++++++++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.h | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 0ddedfb..06a0896 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -796,6 +796,44 @@ rte_eth_dev_stop(uint8_t port_id)
 	(*dev->dev_ops->dev_stop)(dev);
 }
 
+int
+rte_eth_dev_admin_link_up(uint8_t port_id)
+{
+	struct rte_eth_dev *dev;
+
+	/* This function is only safe when called from the primary process
+	 * in a multi-process setup*/
+	PROC_PRIMARY_OR_ERR_RET(-E_RTE_SECONDARY);
+
+	if (port_id >= nb_ports) {
+		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+		return (-EINVAL);
+	}
+	dev = &rte_eth_devices[port_id];
+
+	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_admin_link_up, -ENOTSUP);
+	return (*dev->dev_ops->dev_admin_link_up)(dev);
+}
+
+int
+rte_eth_dev_admin_link_down(uint8_t port_id)
+{
+	struct rte_eth_dev *dev;
+
+	/* This function is only safe when called from the primary process
+	 * in a multi-process setup*/
+	PROC_PRIMARY_OR_ERR_RET(-E_RTE_SECONDARY);
+
+	if (port_id >= nb_ports) {
+		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
+		return (-EINVAL);
+	}
+	dev = &rte_eth_devices[port_id];
+
+	FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_admin_link_down, -ENOTSUP);
+	return (*dev->dev_ops->dev_admin_link_down)(dev);
+}
+
 void
 rte_eth_dev_close(uint8_t port_id)
 {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 2be6e4f..d33ff93 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -891,6 +891,12 @@ typedef int  (*eth_dev_start_t)(struct rte_eth_dev *dev);
 typedef void (*eth_dev_stop_t)(struct rte_eth_dev *dev);
 /**< @internal Function used to stop a configured Ethernet device. */
 
+typedef int  (*eth_dev_admin_link_up_t)(struct rte_eth_dev *dev);
+/**< @internal Function used to link up a configured Ethernet device administratively. */
+
+typedef int  (*eth_dev_admin_link_down_t)(struct rte_eth_dev *dev);
+/**< @internal Function used to link down a configured Ethernet device administratively. */
+
 typedef void (*eth_dev_close_t)(struct rte_eth_dev *dev);
 /**< @internal Function used to close a configured Ethernet device. */
 
@@ -1223,6 +1229,8 @@ struct eth_dev_ops {
 	eth_dev_configure_t        dev_configure; /**< Configure device. */
 	eth_dev_start_t            dev_start;     /**< Start device. */
 	eth_dev_stop_t             dev_stop;      /**< Stop device. */
+	eth_dev_admin_link_up_t    dev_admin_link_up;   /**< Device link up administratively. */
+	eth_dev_admin_link_down_t  dev_admin_link_down; /**< device link down admininstratively. */
 	eth_dev_close_t            dev_close;     /**< Close device. */
 	eth_promiscuous_enable_t   promiscuous_enable; /**< Promiscuous ON. */
 	eth_promiscuous_disable_t  promiscuous_disable;/**< Promiscuous OFF. */
@@ -1696,6 +1704,32 @@ extern int rte_eth_dev_start(uint8_t port_id);
  */
 extern void rte_eth_dev_stop(uint8_t port_id);
 
+
+/**
+ * Link up an Ethernet device administratively.
+ *
+ * The administrative device link up will re-enable the device rx/tx functionality
+ * after it is previously administrative device linked down.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @return
+ *   - 0: Success, Ethernet device linked up administratively.
+ *   - <0: Error code of the driver device link up function.
+ */
+extern int rte_eth_dev_admin_link_up(uint8_t port_id);
+
+/**
+ * Link down an Ethernet device administratively.
+ * The device rx/tx functionality will be disabled if success,
+ * and it can be re-enabled with a call to
+ * rte_eth_dev_admin_link_up()
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ */
+extern int rte_eth_dev_admin_link_down(uint8_t port_id);
+
 /**
  * Close an Ethernet device. The device cannot be restarted!
  *
-- 
1.9.0

[PATCH 2/3] ixgbe: Implement the functionality of administrative link up and down in IXGBE PMD

[Ouyang Changchun]

This patch implements the functionality of administrative link up and down in IXGBE PMD.

Signed-off-by: Ouyang Changchun <changchun.ouyang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 58 +++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
index 76f09af..b6ffad0 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
@@ -98,6 +98,8 @@ static int eth_ixgbe_dev_init(struct eth_driver *eth_drv,
 static int  ixgbe_dev_configure(struct rte_eth_dev *dev);
 static int  ixgbe_dev_start(struct rte_eth_dev *dev);
 static void ixgbe_dev_stop(struct rte_eth_dev *dev);
+static int  ixgbe_dev_admin_link_up(struct rte_eth_dev *dev);
+static int  ixgbe_dev_admin_link_down(struct rte_eth_dev *dev);
 static void ixgbe_dev_close(struct rte_eth_dev *dev);
 static void ixgbe_dev_promiscuous_enable(struct rte_eth_dev *dev);
 static void ixgbe_dev_promiscuous_disable(struct rte_eth_dev *dev);
@@ -263,6 +265,8 @@ static struct eth_dev_ops ixgbe_eth_dev_ops = {
 	.dev_configure        = ixgbe_dev_configure,
 	.dev_start            = ixgbe_dev_start,
 	.dev_stop             = ixgbe_dev_stop,
+	.dev_admin_link_up    = ixgbe_dev_admin_link_up,
+	.dev_admin_link_down  = ixgbe_dev_admin_link_down,
 	.dev_close            = ixgbe_dev_close,
 	.promiscuous_enable   = ixgbe_dev_promiscuous_enable,
 	.promiscuous_disable  = ixgbe_dev_promiscuous_disable,
@@ -1487,6 +1491,60 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
 }
 
 /*
+ * Link up device administratively: enable tx laser.
+ */
+static int
+ixgbe_dev_admin_link_up(struct rte_eth_dev *dev)
+{
+	struct ixgbe_hw *hw =
+		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	if (hw->mac.type == ixgbe_mac_82599EB) {
+#ifdef RTE_NIC_BYPASS
+		if (hw->device_id == IXGBE_DEV_ID_82599_BYPASS) {
+			/* Not suported in bypass mode */
+			PMD_INIT_LOG(ERR, "\nAdmin link up is not supported by device id 0x%x\n",
+					 hw->device_id);
+			return -ENOTSUP;
+		}
+#endif
+		/* Turn on the laser */
+		ixgbe_enable_tx_laser(hw);
+		return 0;
+	}
+
+	PMD_INIT_LOG(ERR, "\nAdmin link up is not supported by device id 0x%x\n",
+			 hw->device_id);
+	return -ENOTSUP;
+}
+
+/*
+ * Link down device administratively: disable tx laser.
+ */
+static int
+ixgbe_dev_admin_link_down(struct rte_eth_dev *dev)
+{
+	struct ixgbe_hw *hw =
+		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	if (hw->mac.type == ixgbe_mac_82599EB) {
+#ifdef RTE_NIC_BYPASS
+		if (hw->device_id == IXGBE_DEV_ID_82599_BYPASS) {
+			/* Not suported in bypass mode */
+			PMD_INIT_LOG(ERR, "\nAdmin link down is not supported by device id 0x%x\n",
+					 hw->device_id);
+			return -ENOTSUP;
+		}
+#endif
+		/* Turn off the laser */
+		ixgbe_disable_tx_laser(hw);
+		return 0;
+	}
+
+	PMD_INIT_LOG(ERR, "\nAdmin link down is not supported by device id 0x%x\n",
+			 hw->device_id);
+	return -ENOTSUP;
+}
+
+/*
  * Reest and stop device.
  */
 static void
-- 
1.9.0

[PATCH 3/3] testpmd: Add commands to test administrative link up and down of PMD

[Ouyang Changchun]

This patch adds commands to test the functionality of administrative link up and down of PMD in testpmd.

Signed-off-by: Ouyang Changchun <changchun.ouyang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 app/test-pmd/cmdline.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.c | 14 +++++++++
 app/test-pmd/testpmd.h |  2 ++
 3 files changed, 94 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6030192..9dcf475 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -3844,6 +3844,82 @@ cmdline_parse_inst_t cmd_start_tx_first = {
 	},
 };
 
+/* *** LINK UP ADMINISTRATIVELY *** */
+struct cmd_admin_link_up_result {
+	cmdline_fixed_string_t admin;
+	cmdline_fixed_string_t link_up;
+	cmdline_fixed_string_t port;
+	uint8_t port_id;
+};
+
+cmdline_parse_token_string_t cmd_admin_link_up_admin =
+	TOKEN_STRING_INITIALIZER(struct cmd_admin_link_up_result, admin, "admin");
+cmdline_parse_token_string_t cmd_admin_link_up_link_up =
+	TOKEN_STRING_INITIALIZER(struct cmd_admin_link_up_result, link_up, "link-up");
+cmdline_parse_token_string_t cmd_admin_link_up_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_admin_link_up_result, port, "port");
+cmdline_parse_token_num_t cmd_admin_link_up_port_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_admin_link_up_result, port_id, UINT8);
+
+static void cmd_admin_link_up_parsed(__attribute__((unused)) void *parsed_result,
+			     __attribute__((unused)) struct cmdline *cl,
+			     __attribute__((unused)) void *data)
+{
+	struct cmd_admin_link_up_result *res = parsed_result;
+	dev_admin_link_up(res->port_id);
+}
+
+cmdline_parse_inst_t cmd_admin_link_up = {
+	.f = cmd_admin_link_up_parsed,
+	.data = NULL,
+	.help_str = "admin link-up port (port id)",
+	.tokens = {
+		(void *)&cmd_admin_link_up_admin,
+		(void *)&cmd_admin_link_up_link_up,
+		(void *)&cmd_admin_link_up_port,
+		(void *)&cmd_admin_link_up_port_id,
+		NULL,
+	},
+};
+
+/* *** LINK DOWN ADMINISTRATIVELY *** */
+struct cmd_admin_link_down_result {
+	cmdline_fixed_string_t admin;
+	cmdline_fixed_string_t link_down;
+	cmdline_fixed_string_t port;
+	uint8_t port_id;
+};
+
+cmdline_parse_token_string_t cmd_admin_link_down_admin =
+	TOKEN_STRING_INITIALIZER(struct cmd_admin_link_down_result, admin, "admin");
+cmdline_parse_token_string_t cmd_admin_link_down_link_down =
+	TOKEN_STRING_INITIALIZER(struct cmd_admin_link_down_result, link_down, "link-down");
+cmdline_parse_token_string_t cmd_admin_link_down_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_admin_link_down_result, port, "port");
+cmdline_parse_token_num_t cmd_admin_link_down_port_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_admin_link_down_result, port_id, UINT8);
+
+static void cmd_admin_link_down_parsed(__attribute__((unused)) void *parsed_result,
+			     __attribute__((unused)) struct cmdline *cl,
+			     __attribute__((unused)) void *data)
+{
+	struct cmd_admin_link_down_result *res = parsed_result;
+	dev_admin_link_down(res->port_id);
+}
+
+cmdline_parse_inst_t cmd_admin_link_down = {
+	.f = cmd_admin_link_down_parsed,
+	.data = NULL,
+	.help_str = "admin link-down port (port id)",
+	.tokens = {
+		(void *)&cmd_admin_link_down_admin,
+		(void *)&cmd_admin_link_down_link_down,
+		(void *)&cmd_admin_link_down_port,
+		(void *)&cmd_admin_link_down_port_id,
+		NULL,
+	},
+};
+
 /* *** SHOW CFG *** */
 struct cmd_showcfg_result {
 	cmdline_fixed_string_t show;
@@ -6055,6 +6131,8 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_showcfg,
 	(cmdline_parse_inst_t *)&cmd_start,
 	(cmdline_parse_inst_t *)&cmd_start_tx_first,
+	(cmdline_parse_inst_t *)&cmd_admin_link_up,
+	(cmdline_parse_inst_t *)&cmd_admin_link_down,
 	(cmdline_parse_inst_t *)&cmd_reset,
 	(cmdline_parse_inst_t *)&cmd_set_numbers,
 	(cmdline_parse_inst_t *)&cmd_set_txpkts,
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index bc38305..9e9997f 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1208,6 +1208,20 @@ stop_packet_forwarding(void)
 	test_done = 1;
 }
 
+void
+dev_admin_link_up(portid_t pid)
+{
+	if (rte_eth_dev_admin_link_up((uint8_t)pid) < 0)
+		printf("\nAdmin link up fail.\n");
+}
+
+void
+dev_admin_link_down(portid_t pid)
+{
+	if (rte_eth_dev_admin_link_down((uint8_t)pid) < 0)
+		printf("\nAdmin link down fail.\n");
+}
+
 static int
 all_ports_started(void)
 {
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index c7998a6..90c306d 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -499,6 +499,8 @@ char *list_pkt_forwarding_modes(void);
 void set_pkt_forwarding_mode(const char *fwd_mode);
 void start_packet_forwarding(int with_tx_first);
 void stop_packet_forwarding(void);
+void dev_admin_link_up(portid_t pid);
+void dev_admin_link_down(portid_t pid);
 void init_port_config(void);
 int init_port_dcb_config(portid_t pid,struct dcb_config *dcb_conf);
 int start_port(portid_t pid);
-- 
1.9.0

Re: [PATCH 0/3] Support administrative link up and link down

[Ivan Boule]

On 05/22/2014 08:11 AM, Ouyang Changchun wrote:
> This patch series contain the following 3 items:
> 1. Add API to support administrative link up and down.
> 2. Implement the functionality of administrative link up and down in IXGBE PMD.
> 3. Add command in testpmd to test the functionality of administrative link up and down of PMD.
>
> Ouyang Changchun (3):
>    Add API for supporting administrative link up and down.
>    Implement the functionality of administrative link up and down in
>      IXGBE PMD.
>    Add command line to test the functionality of administrative link up
>      and down of PMD in testpmd.
>
>   app/test-pmd/cmdline.c              | 78 +++++++++++++++++++++++++++++++++++++
>   app/test-pmd/testpmd.c              | 14 +++++++
>   app/test-pmd/testpmd.h              |  2 +
>   lib/librte_ether/rte_ethdev.c       | 38 ++++++++++++++++++
>   lib/librte_ether/rte_ethdev.h       | 34 ++++++++++++++++
>   lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 58 +++++++++++++++++++++++++++
>   6 files changed, 224 insertions(+)
>

Hi Changchun,

The 2 functions "rte_eth_dev_admin_link_up" and 
"rte_eth_dev_admin_link_down"
don't have an equivalent in the Linux kernel, thus I am wondering what 
is their effective usage from a network application perspective.
Could you briefly explain in which use case these functions can be used for?

By the way, it's not completely evident to infer the exact semantics of 
these 2 functions from their name.
In particular, I do not see what the term "admin" brings to the 
understanding of their role. If it is to suggest that these functions 
are intended to force the link to a different state of its initial 
[self-detected] state, then the term "force" would be more appropriate.

Otherwise, if eventually these functions appear to be mandatory, I 
suggest to rename them "rte_eth_dev_link_start" and 
"rte_eth_dev_link_stop" respectively, and to apply the same naming 
conventions in the 2 other patches.

It might also be worth documenting in the comment section of the
prototype of these 2 functions whether it makes sense or not to support
a notion of link that can be dynamically started or stopped in
non-physical PMDs (vmxnet3, virtio, etc).

Regards,
Ivan



-- 
Ivan Boule
6WIND Development Engineer

Re: [PATCH 0/3] Support administrative link up and link down

[Ouyang, Changchun]

Hi Ivan
For this one, it seems long story for that...
In short, 
Some customer have such kind of requirement, 
they want to repeatedly start(rte_dev_start) and stop(rte_dev_stop) the port for RX and TX, but they find
after several times start and stop, the RX and TX can't work well even the port starts,  and the packets error number increase.

To resolve this error number increase issue, and let port work fine even after repeatedly start and stop,
We need a new API to do it, after discussing, we have these 2 API, admin link up and admin link down.

Any difference if use " dev_link_start/stop" or " dev_link_up/down"? to me, admin_link_up/down is better than dev_link_start/stop,

If most people think we need change the name, it is ok to rename it.

I don't think we need it in non-physical PMDs. So no implementation in virtio PMD.

Thanks
Changchun


-----Original Message-----
From: Ivan Boule [mailto:ivan.boule-pdR9zngts4EAvxtiuMwx3w@public.gmane.org] 
Sent: Thursday, May 22, 2014 9:17 PM
To: Ouyang, Changchun; dev-VfR2kkLFssw@public.gmane.org
Subject: Re: [dpdk-dev] [PATCH 0/3] Support administrative link up and link down

On 05/22/2014 08:11 AM, Ouyang Changchun wrote:
> This patch series contain the following 3 items:
> 1. Add API to support administrative link up and down.
> 2. Implement the functionality of administrative link up and down in IXGBE PMD.
> 3. Add command in testpmd to test the functionality of administrative link up and down of PMD.
>
> Ouyang Changchun (3):
>    Add API for supporting administrative link up and down.
>    Implement the functionality of administrative link up and down in
>      IXGBE PMD.
>    Add command line to test the functionality of administrative link up
>      and down of PMD in testpmd.
>
>   app/test-pmd/cmdline.c              | 78 +++++++++++++++++++++++++++++++++++++
>   app/test-pmd/testpmd.c              | 14 +++++++
>   app/test-pmd/testpmd.h              |  2 +
>   lib/librte_ether/rte_ethdev.c       | 38 ++++++++++++++++++
>   lib/librte_ether/rte_ethdev.h       | 34 ++++++++++++++++
>   lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 58 +++++++++++++++++++++++++++
>   6 files changed, 224 insertions(+)
>

Hi Changchun,

The 2 functions "rte_eth_dev_admin_link_up" and "rte_eth_dev_admin_link_down"
don't have an equivalent in the Linux kernel, thus I am wondering what is their effective usage from a network application perspective.
Could you briefly explain in which use case these functions can be used for?

By the way, it's not completely evident to infer the exact semantics of these 2 functions from their name.
In particular, I do not see what the term "admin" brings to the understanding of their role. If it is to suggest that these functions are intended to force the link to a different state of its initial [self-detected] state, then the term "force" would be more appropriate.

Otherwise, if eventually these functions appear to be mandatory, I suggest to rename them "rte_eth_dev_link_start" and "rte_eth_dev_link_stop" respectively, and to apply the same naming conventions in the 2 other patches.

It might also be worth documenting in the comment section of the prototype of these 2 functions whether it makes sense or not to support a notion of link that can be dynamically started or stopped in non-physical PMDs (vmxnet3, virtio, etc).

Regards,
Ivan



--
Ivan Boule
6WIND Development Engineer

Re: [PATCH 0/3] Support administrative link up and link down

[Ivan Boule]

Hi Changchun,

On 05/22/2014 04:44 PM, Ouyang, Changchun wrote:
> Hi Ivan
> For this one, it seems long story for that...
> In short,
> Some customer have such kind of requirement,
> they want to repeatedly start(rte_dev_start) and stop(rte_dev_stop) the port for RX and TX, but they find
> after several times start and stop, the RX and TX can't work well even the port starts,  and the packets error number increase.
>
> To resolve this error number increase issue, and let port work fine even after repeatedly start and stop,
> We need a new API to do it, after discussing, we have these 2 API, admin link up and admin link down.

If I understand well, this "feature" is not needed by itself, but only 
as a work-around to address issues when repeatedly invoking the 
functions ixgbe_dev_stop and ixgbe_dev_start.
Do such issues appear when performing the same operations with the Linux 
kernel driver?

Anyway, I suppose that such functions have to be automatically invoked 
by the same code of the network application that invokes the functions 
ixgbe_dev_stop and ixgbe_dev_start (said differently, there is no need 
for a manual assistance !)

In that case, would not it be possible - and highly preferable - to 
directly invoke the functions ixgbe_disable_tx_laser and, then, 
ixgbe_enable_tx_laser from the appropriate step during the execution of 
the function ixgbe_dev_start(), waiting for some appropriate delays 
between the two operations, if so needed?

Regards,
Ivan


>
> Any difference if use " dev_link_start/stop" or " dev_link_up/down"? to me, admin_link_up/down is better than dev_link_start/stop,
>
> If most people think we need change the name, it is ok to rename it.
>
> I don't think we need it in non-physical PMDs. So no implementation in virtio PMD.
>
> Thanks
> Changchun
>
>
> -----Original Message-----
> From: Ivan Boule [mailto:ivan.boule-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Thursday, May 22, 2014 9:17 PM
> To: Ouyang, Changchun; dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] [PATCH 0/3] Support administrative link up and link down
>
> On 05/22/2014 08:11 AM, Ouyang Changchun wrote:
>> This patch series contain the following 3 items:
>> 1. Add API to support administrative link up and down.
>> 2. Implement the functionality of administrative link up and down in IXGBE PMD.
>> 3. Add command in testpmd to test the functionality of administrative link up and down of PMD.
>>
...

> Hi Changchun,
>
> The 2 functions "rte_eth_dev_admin_link_up" and "rte_eth_dev_admin_link_down"
> don't have an equivalent in the Linux kernel, thus I am wondering what is their effective usage from a network application perspective.
> Could you briefly explain in which use case these functions can be used for?
>
> By the way, it's not completely evident to infer the exact semantics of these 2 functions from their name.
> In particular, I do not see what the term "admin" brings to the understanding of their role. If it is to suggest that these functions are intended to force the link to a different state of its initial [self-detected] state, then the term "force" would be more appropriate.
>
> Otherwise, if eventually these functions appear to be mandatory, I suggest to rename them "rte_eth_dev_link_start" and "rte_eth_dev_link_stop" respectively, and to apply the same naming conventions in the 2 other patches.
>
> It might also be worth documenting in the comment section of the prototype of these 2 functions whether it makes sense or not to support a notion of link that can be dynamically started or stopped in non-physical PMDs (vmxnet3, virtio, etc).


-- 
Ivan Boule
6WIND Development Engineer

Re: [PATCH 0/3] Support administrative link up and link down

[Ouyang, Changchun]

Hi Ivan

To some extent, I also agree with you.
But customer hope DPDK can provide an interface like "ifconfig up" and "ifconfig down" in linux,
They can invoke such an interface in user application to repeated stop and start dev frequently, and
Make sure RX and TX work fine after each start, I think it is not necessary to do really device start and stop at
Each time, just need start and stop RX and TX function, so the straightforward method is to enable and disable
tx lazer in ixgbe. 
But in the ether level we need a more generic api name, here is rte_eth_dev_admin_link_up/down, while enable_tx_laser is not suitable, 
Enable and disable tx laser is a way in ixgbe to fulfill the administrative link up and link down.
maybe Fortville and future generation NIC will use other ways to fulfill the admin_link_up/down.

Thanks and regards,
Changchun


-----Original Message-----
From: Ivan Boule [mailto:ivan.boule-pdR9zngts4EAvxtiuMwx3w@public.gmane.org] 
Sent: Thursday, May 22, 2014 11:31 PM
To: Ouyang, Changchun; dev-VfR2kkLFssw@public.gmane.org
Subject: Re: [dpdk-dev] [PATCH 0/3] Support administrative link up and link down

Hi Changchun,

On 05/22/2014 04:44 PM, Ouyang, Changchun wrote:
> Hi Ivan
> For this one, it seems long story for that...
> In short,
> Some customer have such kind of requirement, they want to repeatedly 
> start(rte_dev_start) and stop(rte_dev_stop) the port for RX and TX, 
> but they find after several times start and stop, the RX and TX can't work well even the port starts,  and the packets error number increase.
>
> To resolve this error number increase issue, and let port work fine 
> even after repeatedly start and stop, We need a new API to do it, after discussing, we have these 2 API, admin link up and admin link down.

If I understand well, this "feature" is not needed by itself, but only as a work-around to address issues when repeatedly invoking the functions ixgbe_dev_stop and ixgbe_dev_start.
Do such issues appear when performing the same operations with the Linux kernel driver?

Anyway, I suppose that such functions have to be automatically invoked by the same code of the network application that invokes the functions ixgbe_dev_stop and ixgbe_dev_start (said differently, there is no need for a manual assistance !)

In that case, would not it be possible - and highly preferable - to directly invoke the functions ixgbe_disable_tx_laser and, then, ixgbe_enable_tx_laser from the appropriate step during the execution of the function ixgbe_dev_start(), waiting for some appropriate delays between the two operations, if so needed?

Regards,
Ivan


>
> Any difference if use " dev_link_start/stop" or " dev_link_up/down"? 
> to me, admin_link_up/down is better than dev_link_start/stop,
>
> If most people think we need change the name, it is ok to rename it.
>
> I don't think we need it in non-physical PMDs. So no implementation in virtio PMD.
>
> Thanks
> Changchun
>
>
> -----Original Message-----
> From: Ivan Boule [mailto:ivan.boule-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
> Sent: Thursday, May 22, 2014 9:17 PM
> To: Ouyang, Changchun; dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] [PATCH 0/3] Support administrative link up and 
> link down
>
> On 05/22/2014 08:11 AM, Ouyang Changchun wrote:
>> This patch series contain the following 3 items:
>> 1. Add API to support administrative link up and down.
>> 2. Implement the functionality of administrative link up and down in IXGBE PMD.
>> 3. Add command in testpmd to test the functionality of administrative link up and down of PMD.
>>
...

> Hi Changchun,
>
> The 2 functions "rte_eth_dev_admin_link_up" and "rte_eth_dev_admin_link_down"
> don't have an equivalent in the Linux kernel, thus I am wondering what is their effective usage from a network application perspective.
> Could you briefly explain in which use case these functions can be used for?
>
> By the way, it's not completely evident to infer the exact semantics of these 2 functions from their name.
> In particular, I do not see what the term "admin" brings to the understanding of their role. If it is to suggest that these functions are intended to force the link to a different state of its initial [self-detected] state, then the term "force" would be more appropriate.
>
> Otherwise, if eventually these functions appear to be mandatory, I suggest to rename them "rte_eth_dev_link_start" and "rte_eth_dev_link_stop" respectively, and to apply the same naming conventions in the 2 other patches.
>
> It might also be worth documenting in the comment section of the prototype of these 2 functions whether it makes sense or not to support a notion of link that can be dynamically started or stopped in non-physical PMDs (vmxnet3, virtio, etc).


--
Ivan Boule
6WIND Development Engineer

Re: [PATCH 0/3] Support administrative link up and link down

[Ivan Boule]

On 05/23/2014 04:08 AM, Ouyang, Changchun wrote:
> Hi Ivan
>
> To some extent, I also agree with you.
> But customer hope DPDK can provide an interface like "ifconfig up" and "ifconfig down" in linux,
> They can invoke such an interface in user application to repeated stop and start dev frequently, and
> Make sure RX and TX work fine after each start, I think it is not necessary to do really device start and stop at
> Each time, just need start and stop RX and TX function, so the straightforward method is to enable and disable
> tx lazer in ixgbe.
> But in the ether level we need a more generic api name, here is rte_eth_dev_admin_link_up/down, while enable_tx_laser is not suitable,
> Enable and disable tx laser is a way in ixgbe to fulfill the administrative link up and link down.
> maybe Fortville and future generation NIC will use other ways to fulfill the admin_link_up/down.
>

Hi Changchun,

I do not understand what your customer effectively needs.
First of all, if I understand well, your customer's application does not 
really need to invoke the DPDK functions "eth_dev_stop" and 
"eth_dev_start" for addressing its problem, for instance to reconfigure 
RX/TX queues of the port.
When considering the implementation in the ixgbe PMD of the function
"rte_eth_dev_admin_link_down", its only visible effects from the DPDK
application perspective is that no input packet can be received anymore, 
and output packets cannot be transmitted (once having filled the TX queues).

Conversely, the only visible effect of the "rte_eth_dev_admin_link_up"
function is that input packets are received again, and that output 
packets can be successfully transmitted.

In fact, by disabling the TX laser on a ixgbe port, the only interesting
effect of the function "rte_eth_dev_admin_link_down" is that it notifies
the peer system of a hardware link DOWN event (with no physical link
unplug on the peer side).
Conversely, by enabling the TX laser on a ixgbe port, the only 
interesting effect of the function "rte_eth_dev_admin_link_up" is that 
it notifies the peer system of a hardware link UP event.

Is that the actions that your customer's application actually needs to
perform? If so, then this certainly deserves a real operational use case
that it is worth describing in the patch log.
This would help DPDK PMD implementors to understand what such functions 
can be used for, and to decide whether they actually need to be 
supported by the PMD.

Assuming that these 2 functions need to be provided to address the issue
described above, I do not think that the word "admin" brings anything 
for understanding their role. In fact, the word "admin" rather suggests 
a pure "software" down/up setting, instead of a physical one.
Naming these 2 functions "rte_eth_dev_set_link_down"
and "rte_eth_dev_set_link_up" better describes their expected effect.

Regards,
Ivan

>
> On 05/22/2014 04:44 PM, Ouyang, Changchun wrote:
>> Hi Ivan
>> For this one, it seems long story for that...
>> In short,
>> Some customer have such kind of requirement, they want to repeatedly
>> start(rte_dev_start) and stop(rte_dev_stop) the port for RX and TX,
>> but they find after several times start and stop, the RX and TX can't work well even the port starts,  and the packets error number increase.
>>
>> To resolve this error number increase issue, and let port work fine
>> even after repeatedly start and stop, We need a new API to do it, after discussing, we have these 2 API, admin link up and admin link down.
>
> If I understand well, this "feature" is not needed by itself, but only as a work-around to address issues when repeatedly invoking the functions ixgbe_dev_stop and ixgbe_dev_start.
> Do such issues appear when performing the same operations with the Linux kernel driver?
>
> Anyway, I suppose that such functions have to be automatically invoked by the same code of the network application that invokes the functions ixgbe_dev_stop and ixgbe_dev_start (said differently, there is no need for a manual assistance !)
>
> In that case, would not it be possible - and highly preferable - to directly invoke the functions ixgbe_disable_tx_laser and, then, ixgbe_enable_tx_laser from the appropriate step during the execution of the function ixgbe_dev_start(), waiting for some appropriate delays between the two operations, if so needed?
>
> Regards,
> Ivan
>
>
>>
>> Any difference if use " dev_link_start/stop" or " dev_link_up/down"?
>> to me, admin_link_up/down is better than dev_link_start/stop,
>>
>> If most people think we need change the name, it is ok to rename it.
>>
>> I don't think we need it in non-physical PMDs. So no implementation in virtio PMD.
>>
>> Thanks
>> Changchun
>>
>>
>> -----Original Message-----
>> From: Ivan Boule [mailto:ivan.boule-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
>> Sent: Thursday, May 22, 2014 9:17 PM
>> To: Ouyang, Changchun; dev-VfR2kkLFssw@public.gmane.org
>> Subject: Re: [dpdk-dev] [PATCH 0/3] Support administrative link up and
>> link down
>>
>> On 05/22/2014 08:11 AM, Ouyang Changchun wrote:
>>> This patch series contain the following 3 items:
>>> 1. Add API to support administrative link up and down.
>>> 2. Implement the functionality of administrative link up and down in IXGBE PMD.
>>> 3. Add command in testpmd to test the functionality of administrative link up and down of PMD.
>>>
> ...
>
>> Hi Changchun,
>>
>> The 2 functions "rte_eth_dev_admin_link_up" and "rte_eth_dev_admin_link_down"
>> don't have an equivalent in the Linux kernel, thus I am wondering what is their effective usage from a network application perspective.
>> Could you briefly explain in which use case these functions can be used for?
>>
>> By the way, it's not completely evident to infer the exact semantics of these 2 functions from their name.
>> In particular, I do not see what the term "admin" brings to the understanding of their role. If it is to suggest that these functions are intended to force the link to a different state of its initial [self-detected] state, then the term "force" would be more appropriate.
>>
>> Otherwise, if eventually these functions appear to be mandatory, I suggest to rename them "rte_eth_dev_link_start" and "rte_eth_dev_link_stop" respectively, and to apply the same naming conventions in the 2 other patches.
>>
>> It might also be worth documenting in the comment section of the prototype of these 2 functions whether it makes sense or not to support a notion of link that can be dynamically started or stopped in non-physical PMDs (vmxnet3, virtio, etc).
>
>
> --
> Ivan Boule
> 6WIND Development Engineer
>


-- 
Ivan Boule
6WIND Development Engineer

Re: [PATCH 0/3] Support administrative link up and link down

[Ouyang, Changchun]

Hi Ivan,
Thanks very much for your detailed response for this issue,
I think your recommendation makes sense, and I will update the naming and re-send a patch for link-up and link-down.

Best regards,
Changchun

-----Original Message-----
From: Ivan Boule [mailto:ivan.boule-pdR9zngts4EAvxtiuMwx3w@public.gmane.org] 
Sent: Friday, May 23, 2014 5:25 PM
To: Ouyang, Changchun; dev-VfR2kkLFssw@public.gmane.org
Subject: Re: [dpdk-dev] [PATCH 0/3] Support administrative link up and link down

On 05/23/2014 04:08 AM, Ouyang, Changchun wrote:
> Hi Ivan
>
> To some extent, I also agree with you.
> But customer hope DPDK can provide an interface like "ifconfig up" and 
> "ifconfig down" in linux, They can invoke such an interface in user 
> application to repeated stop and start dev frequently, and Make sure 
> RX and TX work fine after each start, I think it is not necessary to 
> do really device start and stop at Each time, just need start and stop RX and TX function, so the straightforward method is to enable and disable tx lazer in ixgbe.
> But in the ether level we need a more generic api name, here is 
> rte_eth_dev_admin_link_up/down, while enable_tx_laser is not suitable, Enable and disable tx laser is a way in ixgbe to fulfill the administrative link up and link down.
> maybe Fortville and future generation NIC will use other ways to fulfill the admin_link_up/down.
>

Hi Changchun,

I do not understand what your customer effectively needs.
First of all, if I understand well, your customer's application does not really need to invoke the DPDK functions "eth_dev_stop" and "eth_dev_start" for addressing its problem, for instance to reconfigure RX/TX queues of the port.
When considering the implementation in the ixgbe PMD of the function "rte_eth_dev_admin_link_down", its only visible effects from the DPDK application perspective is that no input packet can be received anymore, and output packets cannot be transmitted (once having filled the TX queues).

Conversely, the only visible effect of the "rte_eth_dev_admin_link_up"
function is that input packets are received again, and that output packets can be successfully transmitted.

In fact, by disabling the TX laser on a ixgbe port, the only interesting effect of the function "rte_eth_dev_admin_link_down" is that it notifies the peer system of a hardware link DOWN event (with no physical link unplug on the peer side).
Conversely, by enabling the TX laser on a ixgbe port, the only interesting effect of the function "rte_eth_dev_admin_link_up" is that it notifies the peer system of a hardware link UP event.

Is that the actions that your customer's application actually needs to perform? If so, then this certainly deserves a real operational use case that it is worth describing in the patch log.
This would help DPDK PMD implementors to understand what such functions can be used for, and to decide whether they actually need to be supported by the PMD.

Assuming that these 2 functions need to be provided to address the issue described above, I do not think that the word "admin" brings anything for understanding their role. In fact, the word "admin" rather suggests a pure "software" down/up setting, instead of a physical one.
Naming these 2 functions "rte_eth_dev_set_link_down"
and "rte_eth_dev_set_link_up" better describes their expected effect.

Regards,
Ivan

>
> On 05/22/2014 04:44 PM, Ouyang, Changchun wrote:
>> Hi Ivan
>> For this one, it seems long story for that...
>> In short,
>> Some customer have such kind of requirement, they want to repeatedly
>> start(rte_dev_start) and stop(rte_dev_stop) the port for RX and TX, 
>> but they find after several times start and stop, the RX and TX can't work well even the port starts,  and the packets error number increase.
>>
>> To resolve this error number increase issue, and let port work fine 
>> even after repeatedly start and stop, We need a new API to do it, after discussing, we have these 2 API, admin link up and admin link down.
>
> If I understand well, this "feature" is not needed by itself, but only as a work-around to address issues when repeatedly invoking the functions ixgbe_dev_stop and ixgbe_dev_start.
> Do such issues appear when performing the same operations with the Linux kernel driver?
>
> Anyway, I suppose that such functions have to be automatically invoked 
> by the same code of the network application that invokes the functions 
> ixgbe_dev_stop and ixgbe_dev_start (said differently, there is no need 
> for a manual assistance !)
>
> In that case, would not it be possible - and highly preferable - to directly invoke the functions ixgbe_disable_tx_laser and, then, ixgbe_enable_tx_laser from the appropriate step during the execution of the function ixgbe_dev_start(), waiting for some appropriate delays between the two operations, if so needed?
>
> Regards,
> Ivan
>
>
>>
>> Any difference if use " dev_link_start/stop" or " dev_link_up/down"?
>> to me, admin_link_up/down is better than dev_link_start/stop,
>>
>> If most people think we need change the name, it is ok to rename it.
>>
>> I don't think we need it in non-physical PMDs. So no implementation in virtio PMD.
>>
>> Thanks
>> Changchun
>>
>>
>> -----Original Message-----
>> From: Ivan Boule [mailto:ivan.boule-pdR9zngts4EAvxtiuMwx3w@public.gmane.org]
>> Sent: Thursday, May 22, 2014 9:17 PM
>> To: Ouyang, Changchun; dev-VfR2kkLFssw@public.gmane.org
>> Subject: Re: [dpdk-dev] [PATCH 0/3] Support administrative link up 
>> and link down
>>
>> On 05/22/2014 08:11 AM, Ouyang Changchun wrote:
>>> This patch series contain the following 3 items:
>>> 1. Add API to support administrative link up and down.
>>> 2. Implement the functionality of administrative link up and down in IXGBE PMD.
>>> 3. Add command in testpmd to test the functionality of administrative link up and down of PMD.
>>>
> ...
>
>> Hi Changchun,
>>
>> The 2 functions "rte_eth_dev_admin_link_up" and "rte_eth_dev_admin_link_down"
>> don't have an equivalent in the Linux kernel, thus I am wondering what is their effective usage from a network application perspective.
>> Could you briefly explain in which use case these functions can be used for?
>>
>> By the way, it's not completely evident to infer the exact semantics of these 2 functions from their name.
>> In particular, I do not see what the term "admin" brings to the understanding of their role. If it is to suggest that these functions are intended to force the link to a different state of its initial [self-detected] state, then the term "force" would be more appropriate.
>>
>> Otherwise, if eventually these functions appear to be mandatory, I suggest to rename them "rte_eth_dev_link_start" and "rte_eth_dev_link_stop" respectively, and to apply the same naming conventions in the 2 other patches.
>>
>> It might also be worth documenting in the comment section of the prototype of these 2 functions whether it makes sense or not to support a notion of link that can be dynamically started or stopped in non-physical PMDs (vmxnet3, virtio, etc).
>
>
> --
> Ivan Boule
> 6WIND Development Engineer
>


--
Ivan Boule
6WIND Development Engineer