[PATCH] net/ice: make link state configurable on device stop

[PATCH] net/ice: make link state configurable on device stop

[Ciara Loftus]

Introduce the boolean restore_link_state devarg for the ice driver. When
set, when the ice port is stopped, the physical link will be restored to
the state it was in when the device was started.

Prior to this patch the default behaviour was to always restore the link
to its original state. The new default behaiour is to always bring the
link down, which is in keeping with most other drivers.

Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
---
 doc/guides/nics/ice.rst            | 12 ++++++++++++
 drivers/net/intel/ice/ice_ethdev.c | 10 ++++++++--
 drivers/net/intel/ice/ice_ethdev.h |  1 +
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index 2c0f41c56c..9007b79471 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -322,6 +322,18 @@ Runtime Configuration
 
     -a af:00.0,pps_out='[pin:0]'
 
+- ``Restore initial link state`` (default ``0``)
+
+  The user can request that the link be restored to its original state when
+  the device is stopped.
+
+    -a af:00.0,restore_link_state=1
+
+  The default behaviour is to bring the link down when the device is stopped,
+  however if restore_link_state is set and if the link state was up when the
+  device was started, then it will be restored to the up state when the
+  device is stopped.
+
 - ``Low Rx latency`` (default ``0``)
 
   vRAN workloads require low latency DPDK interface for the front haul
diff --git a/drivers/net/intel/ice/ice_ethdev.c b/drivers/net/intel/ice/ice_ethdev.c
index 513777e372..0c1dc9db1a 100644
--- a/drivers/net/intel/ice/ice_ethdev.c
+++ b/drivers/net/intel/ice/ice_ethdev.c
@@ -41,6 +41,7 @@
 #define ICE_DDP_FILENAME_ARG      "ddp_pkg_file"
 #define ICE_DDP_LOAD_SCHED_ARG    "ddp_load_sched_topo"
 #define ICE_TM_LEVELS_ARG         "tm_sched_levels"
+#define ICE_RESTORE_LINK_STATE    "restore_link_state"
 
 #define ICE_CYCLECOUNTER_MASK  0xffffffffffffffffULL
 
@@ -57,6 +58,7 @@ static const char * const ice_valid_args[] = {
 	ICE_DDP_FILENAME_ARG,
 	ICE_DDP_LOAD_SCHED_ARG,
 	ICE_TM_LEVELS_ARG,
+	ICE_RESTORE_LINK_STATE,
 	NULL
 };
 
@@ -2368,6 +2370,9 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
 	if (ret)
 		goto bail;
 
+	ret = rte_kvargs_process(kvlist, ICE_RESTORE_LINK_STATE,
+				 &parse_bool, &ad->devargs.restore_link_state);
+
 bail:
 	rte_kvargs_free(kvlist);
 	return ret;
@@ -2811,7 +2816,7 @@ ice_dev_stop(struct rte_eth_dev *dev)
 	/* disable all queue interrupts */
 	ice_vsi_disable_queues_intr(main_vsi);
 
-	if (pf->init_link_up)
+	if (pf->adapter->devargs.restore_link_state && pf->init_link_up)
 		ice_dev_set_link_up(dev);
 	else
 		ice_dev_set_link_down(dev);
@@ -7320,7 +7325,8 @@ RTE_PMD_REGISTER_PARAM_STRING(net_ice,
 			      ICE_DDP_FILENAME_ARG "=</path/to/file>"
 			      ICE_DDP_LOAD_SCHED_ARG "=<0|1>"
 			      ICE_TM_LEVELS_ARG "=<N>"
-			      ICE_RX_LOW_LATENCY_ARG "=<0|1>");
+			      ICE_RX_LOW_LATENCY_ARG "=<0|1>"
+			      ICE_RESTORE_LINK_STATE "=<0|1>");
 
 RTE_LOG_REGISTER_SUFFIX(ice_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(ice_logtype_driver, driver, NOTICE);
diff --git a/drivers/net/intel/ice/ice_ethdev.h b/drivers/net/intel/ice/ice_ethdev.h
index 8e5799f8b4..4605fee6c5 100644
--- a/drivers/net/intel/ice/ice_ethdev.h
+++ b/drivers/net/intel/ice/ice_ethdev.h
@@ -597,6 +597,7 @@ struct ice_devargs {
 	uint8_t pps_out_ena;
 	uint8_t ddp_load_sched;
 	uint8_t tm_exposed_levels;
+	uint8_t restore_link_state;
 	int xtr_field_offs;
 	uint8_t xtr_flag_offs[PROTO_XTR_MAX];
 	/* Name of the field. */
-- 
2.34.1

Re: [PATCH] net/ice: make link state configurable on device stop

[Bruce Richardson]

On Thu, Aug 28, 2025 at 10:23:05AM +0000, Ciara Loftus wrote:
> Introduce the boolean restore_link_state devarg for the ice driver. When
> set, when the ice port is stopped, the physical link will be restored to
> the state it was in when the device was started.
> 
> Prior to this patch the default behaviour was to always restore the link
> to its original state. The new default behaiour is to always bring the
> link down, which is in keeping with most other drivers.
> 
> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> ---
>  doc/guides/nics/ice.rst            | 12 ++++++++++++
>  drivers/net/intel/ice/ice_ethdev.c | 10 ++++++++--
>  drivers/net/intel/ice/ice_ethdev.h |  1 +
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> index 2c0f41c56c..9007b79471 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -322,6 +322,18 @@ Runtime Configuration
>  
>      -a af:00.0,pps_out='[pin:0]'
>  
> +- ``Restore initial link state`` (default ``0``)
> +
> +  The user can request that the link be restored to its original state when
> +  the device is stopped.
> +
> +    -a af:00.0,restore_link_state=1
> +
> +  The default behaviour is to bring the link down when the device is stopped,
> +  however if restore_link_state is set and if the link state was up when the
> +  device was started, then it will be restored to the up state when the
> +  device is stopped.
> +
>  - ``Low Rx latency`` (default ``0``)
>  
I think that control over this is a good thing to have. However, I'm also
wondering if we also need a way to force the link to always be up on close,
therefore requiring a tri-state option, rather than just two as here
(previous or down). How about renaming the option to maybe:
"link_state_on_close" with 3 options of "up", "down", "initial"?

/Bruce

Re: [PATCH] net/ice: make link state configurable on device stop

[Stephen Hemminger]

On Thu, 28 Aug 2025 10:23:05 +0000
Ciara Loftus <ciara.loftus@intel.com> wrote:

> Introduce the boolean restore_link_state devarg for the ice driver. When
> set, when the ice port is stopped, the physical link will be restored to
> the state it was in when the device was started.
> 
> Prior to this patch the default behaviour was to always restore the link
> to its original state. The new default behaiour is to always bring the
> link down, which is in keeping with most other drivers.
> 
> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> ---

Device specific devargs should be discouraged. Seems like the same
issue occurs across multiple drivers.

[PATCH v2] net/ice: make link state configurable on device close

[Ciara Loftus]

Introduce the device argument link_state_on_close for the ice driver.
When the device is stopped or closed, the link state will be configured
as follows for each of the three options:
1. down: bring (or keep) the link down
2. up: bring (or keep) the link up
3. initial: restore the link to the state it was in when the device
started.

Prior to this commit the default behaviour was to always restore the
link to its initial state. The new default behaiour is to always bring
the link down, which is in keeping with most other drivers.

Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
---
v2:
* Changed devarg name and made it tri-state instead of boolean
---
 doc/guides/nics/ice.rst            | 13 ++++++++++
 drivers/net/intel/ice/ice_ethdev.c | 41 ++++++++++++++++++++++++++++--
 drivers/net/intel/ice/ice_ethdev.h |  1 +
 3 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index 2c0f41c56c..3802db8120 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -322,6 +322,19 @@ Runtime Configuration
 
     -a af:00.0,pps_out='[pin:0]'
 
+- ``Link state on close`` (default ``down``)
+
+  The user can request that the link be set to up or down or restored to its
+  original state when the device is closed.
+
+    -a af:00.0,link_state_on_close=<state>
+
+  Supported values for the ``<state>`` parameter:
+
+  * ``down``: Leave the link in the down state.
+  * ``up``: Leave the link in the up state.
+  * ``initial``: Restore the link to the state it was in when the device started.
+
 - ``Low Rx latency`` (default ``0``)
 
   vRAN workloads require low latency DPDK interface for the front haul
diff --git a/drivers/net/intel/ice/ice_ethdev.c b/drivers/net/intel/ice/ice_ethdev.c
index 513777e372..a93b0fe202 100644
--- a/drivers/net/intel/ice/ice_ethdev.c
+++ b/drivers/net/intel/ice/ice_ethdev.c
@@ -41,6 +41,7 @@
 #define ICE_DDP_FILENAME_ARG      "ddp_pkg_file"
 #define ICE_DDP_LOAD_SCHED_ARG    "ddp_load_sched_topo"
 #define ICE_TM_LEVELS_ARG         "tm_sched_levels"
+#define ICE_LINK_STATE_ON_CLOSE   "link_state_on_close"
 
 #define ICE_CYCLECOUNTER_MASK  0xffffffffffffffffULL
 
@@ -57,6 +58,7 @@ static const char * const ice_valid_args[] = {
 	ICE_DDP_FILENAME_ARG,
 	ICE_DDP_LOAD_SCHED_ARG,
 	ICE_TM_LEVELS_ARG,
+	ICE_LINK_STATE_ON_CLOSE,
 	NULL
 };
 
@@ -95,6 +97,12 @@ static struct proto_xtr_ol_flag ice_proto_xtr_ol_flag_params[] = {
 		.param = { .name = "intel_pmd_dynflag_proto_xtr_ip_offset" }}
 };
 
+enum ice_link_state_on_close {
+	ICE_LINK_DOWN,
+	ICE_LINK_UP,
+	ICE_LINK_INITIAL,
+};
+
 #define ICE_OS_DEFAULT_PKG_NAME		"ICE OS Default Package"
 #define ICE_COMMS_PKG_NAME			"ICE COMMS Package"
 #define ICE_MAX_RES_DESC_NUM        1024
@@ -2116,6 +2124,29 @@ parse_tx_sched_levels(const char *key, const char *value, void *args)
 	return 0;
 }
 
+static int
+parse_link_state_on_close(const char *key, const char *value, void *args)
+{
+	int *state = args, ret = 0;
+
+	if (value == NULL || state == NULL)
+		return -EINVAL;
+
+	if (strcmp(value, "down") == 0) {
+		*state = ICE_LINK_DOWN;
+	} else if (strcmp(value, "up") == 0) {
+		*state = ICE_LINK_UP;
+	} else if (strcmp(value, "initial") == 0) {
+		*state = ICE_LINK_INITIAL;
+	} else {
+		ret = -EINVAL;
+		PMD_DRV_LOG(WARNING, "%s: Invalid value \"%s\", "
+				"should be \"down\" \"up\" or \"initial\"", key, value);
+	}
+
+	return ret;
+}
+
 static int
 lookup_pps_type(const char *pps_name)
 {
@@ -2368,6 +2399,9 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
 	if (ret)
 		goto bail;
 
+	ret = rte_kvargs_process(kvlist, ICE_LINK_STATE_ON_CLOSE,
+				 &parse_link_state_on_close, &ad->devargs.link_state_on_close);
+
 bail:
 	rte_kvargs_free(kvlist);
 	return ret;
@@ -2811,7 +2845,9 @@ ice_dev_stop(struct rte_eth_dev *dev)
 	/* disable all queue interrupts */
 	ice_vsi_disable_queues_intr(main_vsi);
 
-	if (pf->init_link_up)
+	if (pf->adapter->devargs.link_state_on_close == ICE_LINK_UP ||
+			(pf->adapter->devargs.link_state_on_close == ICE_LINK_INITIAL &&
+				pf->init_link_up))
 		ice_dev_set_link_up(dev);
 	else
 		ice_dev_set_link_down(dev);
@@ -7320,7 +7356,8 @@ RTE_PMD_REGISTER_PARAM_STRING(net_ice,
 			      ICE_DDP_FILENAME_ARG "=</path/to/file>"
 			      ICE_DDP_LOAD_SCHED_ARG "=<0|1>"
 			      ICE_TM_LEVELS_ARG "=<N>"
-			      ICE_RX_LOW_LATENCY_ARG "=<0|1>");
+			      ICE_RX_LOW_LATENCY_ARG "=<0|1>"
+			      ICE_LINK_STATE_ON_CLOSE "=<down|up|initial>");
 
 RTE_LOG_REGISTER_SUFFIX(ice_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(ice_logtype_driver, driver, NOTICE);
diff --git a/drivers/net/intel/ice/ice_ethdev.h b/drivers/net/intel/ice/ice_ethdev.h
index 8e5799f8b4..23014f4f72 100644
--- a/drivers/net/intel/ice/ice_ethdev.h
+++ b/drivers/net/intel/ice/ice_ethdev.h
@@ -597,6 +597,7 @@ struct ice_devargs {
 	uint8_t pps_out_ena;
 	uint8_t ddp_load_sched;
 	uint8_t tm_exposed_levels;
+	int link_state_on_close;
 	int xtr_field_offs;
 	uint8_t xtr_flag_offs[PROTO_XTR_MAX];
 	/* Name of the field. */
-- 
2.34.1

RE: [PATCH] net/ice: make link state configurable on device stop

[Loftus, Ciara]

> 
> On Thu, 28 Aug 2025 10:23:05 +0000
> Ciara Loftus <ciara.loftus@intel.com> wrote:
> 
> > Introduce the boolean restore_link_state devarg for the ice driver. When
> > set, when the ice port is stopped, the physical link will be restored to
> > the state it was in when the device was started.
> >
> > Prior to this patch the default behaviour was to always restore the link
> > to its original state. The new default behaiour is to always bring the
> > link down, which is in keeping with most other drivers.
> >
> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> > ---
> 
> Device specific devargs should be discouraged. Seems like the same
> issue occurs across multiple drivers.

I had considered creating a generic ethdev API for this rather than a
device specific devarg. However when I looked into it I wasn't sure
there would be much appetite for it, as most drivers bring the link
down on close as standard behaviour.
I'll submit that patch as RFC and if it gets traction and acceptance
we can drop this devarg.