net/intel: make link state configurable on device start

Starting a port always brings its link up, so the link partner sees the port as soon as the application starts it. An application that enables ports only after its own configuration is applied, such as a firewall or a switch, cannot keep a disabled port dark: it can only bring the link down right after rte_eth_dev_start(), and the link partner may still see the link flap.

The ice driver already has the link_state_on_close device argument to choose the link state when the port is stopped. This series adds its counterpart, link_state_on_start, to the ice, i40e and ixgbe drivers:

-a <pci>,link_state_on_start=down

With "down", the port is started with the link down, and the application brings it up later with rte_eth_dev_set_link_up(). The default "up" keeps the current behavior.

ixgbe has no firmware command to bring the link down, so "down" leaves the port in the same state as rte_eth_dev_set_link_down(): the laser is kept off on fiber links, the copper PHY is powered off right after the link setup.

Tested with testpmd on E810-C (ice, firmware 4.60) connected to a network packet broker: with link_state_on_start=down the link stays down after the port start and the link partner does not see it until "set link-up"; without the device argument the link comes up on start as before. i40e and ixgbe are build tested only.

Roman Khromenok (3):
net/ice: make link state configurable on device start net/i40e: make link state configurable on device start net/ixgbe: make link state configurable on device start

 doc/guides/nics/i40e.rst               | 13 ++++++
 doc/guides/nics/ice.rst                | 13 ++++++
 doc/guides/nics/ixgbe.rst              | 13 ++++++
 doc/guides/rel_notes/release_26_11.rst | 13 +++++-
 drivers/net/intel/i40e/i40e_ethdev.c   | 58 +++++++++++++++++++++++++-
 drivers/net/intel/i40e/i40e_ethdev.h   |  2 +
 drivers/net/intel/ice/ice_ethdev.c     | 36 +++++++++++++++-
 drivers/net/intel/ice/ice_ethdev.h     |  1 +
 drivers/net/intel/ixgbe/ixgbe_ethdev.c | 44 ++++++++++++++++++-
 drivers/net/intel/ixgbe/ixgbe_ethdev.h |  2 +
 10 files changed, 187 insertions(+), 8 deletions(-)

2.47.3

The device start always brings the link up, so the port shows up on the link partner as soon as the application starts it, even if the application wants to enable it later, for example only after its configuration is applied.

Introduce the device argument link_state_on_start, the counterpart of link_state_on_close:
1. up: bring the link up (default, unchanged behavior)
2. down: bring (or keep) the link down; the application brings it up later with rte_eth_dev_set_link_up().

---
 doc/guides/nics/ice.rst                | 13 ++++++++++
 doc/guides/rel_notes/release_26_11.rst |  2 ++
 drivers/net/intel/ice/ice_ethdev.c     | 36 ++++++++++++++++++++++++--
 drivers/net/intel/ice/ice_ethdev.h     |  1 +
 4 files changed, 50 insertions(+), 2 deletions(-)

index ba598742c5..1a78760ff1 100644
@@ -324,6 +324,19 @@ Runtime Configuration
   * ``up``: Leave the link in the up state.
   * ``initial``: Restore the link to the state it was in when the device started.
 
+- ``Link state on start`` (default ``up``)
+
+  The user can request that the link be kept down when the device is started,
+  so that the port does not show up on the link partner until the application
+  calls ``rte_eth_dev_set_link_up()``::
+
+    -a af:00.0,link_state_on_start=<state>
+
+  Supported values for the ``<state>`` parameter:
+
+  * ``up``: Bring the link up.
+  * ``down``: Bring (or keep) the link down.
+
 - ``Low Rx latency`` (default ``0``)
 
   vRAN workloads require low latency DPDK interface for the front haul

index e8905f5533..a7fda78c5b 100644
@@ -90,6 +90,8 @@ New Features
     enabling QinQ tag insertion and outer IPv4/UDP checksum offloads on those paths.
   * Added support for getting and setting link (802.3x) flow control.
   * Added support for Tx rate limiting per queue.
+  * Added ``link_state_on_start`` devarg to keep the link down
+    when the device is started.
 
 * **Updated Intel ixgbe driver.**
 

index 163304ba1e..51a82830af 100644
@@ -45,6 +45,7 @@
 #define ICE_RL_BURST_SIZE_ARG     "rl_burst_size"
 #define ICE_SOURCE_PRUNE_ARG      "source-prune"
 #define ICE_LINK_STATE_ON_CLOSE   "link_state_on_close"
+#define ICE_LINK_STATE_ON_START   "link_state_on_start"
 
 #define ICE_CYCLECOUNTER_MASK  0xffffffffffffffffULL
 

@@ -64,6 +65,7 @@ static const char * const ice_valid_args[] = {
 	ICE_RL_BURST_SIZE_ARG,
 	ICE_SOURCE_PRUNE_ARG,
 	ICE_LINK_STATE_ON_CLOSE,
+	ICE_LINK_STATE_ON_START,
 	NULL
 };
 

@@ -2265,6 +2267,27 @@ parse_link_state_on_close(const char *key, const char *value, void *args)
 	return ret;
 }
 
+static int
+parse_link_state_on_start(const char *key, const char *value, void *args)
+{
+	bool *link_down = args;
+
+	if (value == NULL || link_down == NULL)
+		return -EINVAL;
+
+	if (strcmp(value, "down") == 0) {
+		*link_down = true;
+	} else if (strcmp(value, "up") == 0) {
+		*link_down = false;
+	} else {
+		PMD_DRV_LOG(WARNING, "%s: Invalid value \"%s\", "
+				"should be \"down\" or \"up\"", key, value);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int
 lookup_pps_type(const char *pps_name)
 {

@@ -2529,6 +2552,11 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
 
 	ret = rte_kvargs_process(kvlist, ICE_LINK_STATE_ON_CLOSE,
 				 &parse_link_state_on_close, &ad->devargs.link_state_on_close);
+	if (ret)
+		goto bail;
+
+	ret = rte_kvargs_process(kvlist, ICE_LINK_STATE_ON_START,
+				 &parse_link_state_on_start, &ad->devargs.link_down_on_start);
 
 bail:
 	rte_kvargs_free(kvlist);

@@ -4856,7 +4884,10 @@ ice_dev_start(struct rte_eth_dev *dev)
 
 	ice_get_init_link_status(dev);
 
-	ice_dev_set_link_up(dev);
+	if (pf->adapter->devargs.link_down_on_start)
+		ice_dev_set_link_down(dev);
+	else
+		ice_dev_set_link_up(dev);
 
 	/* Call get_link_info aq command to enable/disable LSE */
 	ice_link_update(dev, 0);

@@ -8118,7 +8149,8 @@ RTE_PMD_REGISTER_PARAM_STRING(net_ice,
 			      ICE_RL_BURST_SIZE_ARG "=<N>"
 			      ICE_SOURCE_PRUNE_ARG "=<0|1>"
 			      ICE_RX_LOW_LATENCY_ARG "=<0|1>"
-			      ICE_LINK_STATE_ON_CLOSE "=<down|up|initial>");
+			      ICE_LINK_STATE_ON_CLOSE "=<down|up|initial>"
+			      ICE_LINK_STATE_ON_START "=<down|up>");
 
 RTE_LOG_REGISTER_SUFFIX(ice_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(ice_logtype_driver, driver, NOTICE);

index 8d5e17e2df..8794870c34 100644
@@ -640,6 +640,7 @@ struct ice_devargs {
 	bool source_prune;
 	uint32_t rl_burst_size;
 	int link_state_on_close;
+	bool link_down_on_start;
 	int xtr_field_offs;
 	uint8_t xtr_flag_offs[PROTO_XTR_MAX];
 	/* Name of the field. */

2.47.3

The device start always applies the link configuration, which brings the link up, so the port shows up on the link partner as soon as the application starts it, even if the application wants to enable it later, for example only after its configuration is applied.

Introduce the device argument link_state_on_start, as for the ice driver:
1. up: bring the link up (default, unchanged behavior)
2. down: bring (or keep) the link down; the application brings it up later with rte_eth_dev_set_link_up(), which applies the link configuration.

---
 doc/guides/nics/i40e.rst               | 13 ++++++
 doc/guides/rel_notes/release_26_11.rst |  5 +++
 drivers/net/intel/i40e/i40e_ethdev.c   | 58 +++++++++++++++++++++++++-
 drivers/net/intel/i40e/i40e_ethdev.h   |  2 +
 4 files changed, 76 insertions(+), 2 deletions(-)

index 29234dbf0f..1242737611 100644
@@ -193,6 +193,19 @@ Runtime Configuration
   * ``segment``: Check number of mbuf segments not exceed hw limitation.
   * ``offload``: Check any unsupported offload flag.
 
+- ``Link state on start`` (default ``up``)
+
+  The user can request that the link be kept down when the device is started,
+  so that the port does not show up on the link partner until the application
+  calls ``rte_eth_dev_set_link_up()``::
+
+    -a 84:00.0,link_state_on_start=<state>
+
+  Supported values for the ``<state>`` parameter:
+
+  * ``up``: Bring the link up.
+  * ``down``: Bring (or keep) the link down.
+
 QinQ Configuration
 ~~~~~~~~~~~~~~~~~~
 

index a7fda78c5b..f5831a7e4e 100644
@@ -74,6 +74,11 @@ New Features
     ``xdp_meta_rx_ts_valid_mask``.
   * Added ``read_clock`` operation to query the PTP hardware clock.
 
+* **Updated Intel i40e driver.**
+
+  * Added ``link_state_on_start`` devarg to keep the link down
+    when the device is started.
+
 * **Updated Intel iavf driver.**
 
   * Runtime Rx/Tx queue setup is now automatically disabled

index 1e64a2d280..c4ddfab0be 100644
@@ -1203,6 +1203,8 @@ struct i40e_pf {
 	bool fw8_3gt;
 	/* MAC config needs re-applying when link first comes up */
 	bool mac_config_on_link_up;
+	/* Keep the link down on device start */
+	bool link_down_on_start;
 	/* true when interrupt path unavailable */
 	bool use_aq_polling;
 

2.47.3

The device start always turns on the laser or the copper PHY and sets up the link, so the port shows up on the link partner as soon as the application starts it, even if the application wants to enable it later, for example only after its configuration is applied.

Introduce the device argument link_state_on_start, as for the ice and i40e drivers:
1. up: bring the link up (default, unchanged behavior)
2. down: keep the laser off on fiber links and leave the port in the same state as after rte_eth_dev_set_link_down(); the application brings it up later with rte_eth_dev_set_link_up().

On copper links the PHY is still powered on to set up the link and powered off right after it.

---
 doc/guides/nics/ixgbe.rst              | 13 ++++++++
 doc/guides/rel_notes/release_26_11.rst |  6 ++--
 drivers/net/intel/ixgbe/ixgbe_ethdev.c | 44 ++++++++++++++++++++++++--
 drivers/net/intel/ixgbe/ixgbe_ethdev.h |  2 ++
 4 files changed, 61 insertions(+), 4 deletions(-)

index 1d14e292e2..dbf0c2c5e4 100644
@@ -187,6 +187,19 @@ It must be passed as part of EAL arguments. For example,
   This value is read only at probe time.
   Changing it requires restarting the process with a new EAL argument.
 
+- ``link_state_on_start`` (default **up**)
+
+  The user can request that the link be kept down when the device is started,
+  so that the port does not show up on the link partner until the application
+  calls ``rte_eth_dev_set_link_up()``::
+
+    -a 81:00.0,link_state_on_start=<state>
+
+  Supported values for the ``<state>`` parameter:
+
+  * ``up``: Bring the link up.
+  * ``down``: Turn off the laser on fiber links or the PHY on copper links.
+
 .. note::
 
    The driver rejects Flow Director when DCB is enabled.

index f5831a7e4e..3aadcf94a1 100644
@@ -100,8 +100,10 @@ New Features
 
 * **Updated Intel ixgbe driver.**
 
-  Added ``fdir_buffer_size`` devarg to select the Flow Director table size
-  (``64k``, ``128k`` or ``256k``) at probe time. The default remains ``64k``.
+  * Added ``fdir_buffer_size`` devarg to select the Flow Director table size
+    (``64k``, ``128k`` or ``256k``) at probe time. The default remains ``64k``.
+  * Added ``link_state_on_start`` devarg to keep the link down
+    when the device is started.
 
 * **Updated NXP DPAA2 ethernet driver.**
 

index 2e9d166e29..60206d00d5 100644
@@ -492,6 +492,8 @@ struct ixgbe_adapter {
 
 	/* Used for limiting SDP3 TX_DISABLE checks */
 	uint8_t sdp3_no_tx_disable;
+	/* Keep the link down on device start */
+	bool link_down_on_start;
 	uint16_t max_vfs;
 
 	/* Used for VF link sync with PF's physical and logical (by checking

2.47.3

NAK.

DPDK devices should not be like snowflakes, everyone unique.

When I see patches like this I think one or more of the following:
- a user does not understand how DPDK devices work and does not want to change their application
- developer refuses to say no to the user.
- DPDK devices semantics are poorly documented.
- DPDK devices do not implement the link states in RFC2863 but should. RFC 2863 separates ifAdminStatus from ifOperStatus; ethdev should as well.

If you want something done better it must done at ethdev layer and you must coordinate across all existing devices. No more specials.

Take user input as problem description, not as directive on how to solve it.

The link_state_on_close argument in ice is not a precedent. It was a workaround for hotplug problems and should not have been a devarg either. Adding a matching on_start option to three drivers repeats the mistake. I should have objected stronger then.

Thanks, understood. I withdraw this series.

The problem behind it: a firewall starts its ports at boot but must keep the ports that are disabled in its configuration down, so that the link partner never sees them. Today rte_eth_dev_start() brings the link up on most drivers, and the application can only bring it down afterwards.

Would an RFC along these lines be acceptable?
- ethdev keeps the administrative link state (RFC 2863 ifAdminStatus) set by rte_eth_dev_set_link_down()/up(), including before the port is started;
- rte_eth_dev_start() honors it: drivers that support it keep the link down on start, and for the others ethdev calls dev_set_link_down() right after dev_start(), so the semantics are the same for all devices;
- a new feature in the features matrix to track driver support.

Or would you prefer a separate API for the administrative state instead of changing the behavior of rte_eth_dev_set_link_down()?

Roman

No. Application should not call device start until it expects to bring link up.

The RFC 2863 stuff is for layered devices where lower layers are separate from the upper ones (ie. tunnels, bonding, etc).

The point of start is to signal that the application is ready to send and receive packets. I would expect the normal startup sequence is:

1. EAL init
2. Detect devices
3. Configure internal state
4. Configure devices.
5. Start devices

A device should be dark from 2 to 5. No packets coming in or out and link is down.