[PATCH iwl-net v3] ice: Fix "Unknown bps" during link events

[PATCH iwl-net v3] ice: Fix "Unknown bps" during link events

[Lukasz Czapnik]

From: Jakub Kaminski <jakub.kaminski@intel.com>

The driver may print "NIC Link is up Unknown bps" during link state
changes, and ethtool may report an unknown speed while the carrier is
on.

ice_link_event() receives link_up and link_speed from the ARQ link
status event, then refreshes phy.link_info with a synchronous AQ query.
link_up was forced to true if the refreshed info reported the link up
and otherwise kept the event value, link_speed always kept the event
value, and ice_print_link_msg() read the speed from the refreshed info.
Link state and speed could come from different reads, and the carrier
could stay on while the refreshed info reported the link down.

Take link_up and link_speed from the refreshed phy.link_info when the
link status read succeeds. Check the result of the link status read
itself, as ice_update_link_info() can also fail in the following PHY
caps query; split that query out into ice_update_media_caps(). If the
read fails, keep the event values and store them in phy.link_info, so
carrier, VF link state and the baseline for the next event agree. FEC,
autoneg and flow control in the link message then still come from the
last successful read.

Do not drop an event that differs from the cached state even if the
refreshed info does not, e.g. a down event handled after the link came
back at the same speed.

Add a link_speed parameter to ice_print_link_msg() so the printed speed
is the one read together with the link state the caller acted on,
instead of being re-read from phy.link_info later. Updates of
phy.link_info from other contexts are not serialized against the
service task, which also applies to the event state stored on a failed
read. The missing serialization predates this change and is not
addressed here.

Fixes: c2a23e00613b ("ice: Refactor link event flow")
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Tomasz Lichwala <tomasz.lichwala@linux.intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Jakub Kaminski <jakub.kaminski@intel.com>
Co-developed-by: Lukasz Czapnik <lukasz.czapnik@intel.com>
Signed-off-by: Lukasz Czapnik <lukasz.czapnik@intel.com>
---
v3:
- Use the refreshed link state only when the link status read itself
  succeeds; split the PHY caps query out into ice_update_media_caps().
- Store the event state in phy.link_info when the read fails.
- Don't drop events that differ from the cached state.
- Rename status to err in ice_link_event().

v2: https://lore.kernel.org/netdev/20260806162458.2384818-1-lukasz.czapnik@intel.com/
- Take link_up and link_speed from the refreshed cache for every event,
  not only for up events.
---
 drivers/net/ethernet/intel/ice/ice.h         |  2 +-
 drivers/net/ethernet/intel/ice/ice_common.c  | 37 +++++++++-------
 drivers/net/ethernet/intel/ice/ice_common.h  |  1 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c |  2 +-
 drivers/net/ethernet/intel/ice/ice_main.c    | 45 +++++++++++++-------
 5 files changed, 54 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 009765074a01..4afd9d166d08 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -1012,7 +1012,7 @@ int ice_get_rss_key(struct ice_vsi *vsi, u8 *seed);
 int ice_set_rss_hfunc(struct ice_vsi *vsi, u8 hfunc);
 void ice_fill_rss_lut(u8 *lut, u16 rss_table_size, u16 rss_size);
 int ice_schedule_reset(struct ice_pf *pf, enum ice_reset_req reset);
-void ice_print_link_msg(struct ice_vsi *vsi, bool isup);
+void ice_print_link_msg(struct ice_vsi *vsi, bool isup, u16 link_speed);
 int ice_plug_aux_dev(struct ice_pf *pf);
 void ice_unplug_aux_dev(struct ice_pf *pf);
 void ice_rdma_finalize_setup(struct ice_pf *pf);
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 81187214a630..e60ce0ed2693 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -3639,36 +3639,43 @@ ice_aq_set_phy_cfg(struct ice_hw *hw, struct ice_port_info *pi,
 	return status;
 }
 
+/**
+ * ice_update_media_caps - refresh the PHY capabilities of the present media
+ * @pi: port info structure of the interested logical port
+ *
+ * Return: 0 on success or if no media is available, negative on failure.
+ */
+int ice_update_media_caps(struct ice_port_info *pi)
+{
+	struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
+
+	if (!(pi->phy.link_info.link_info & ICE_AQ_MEDIA_AVAILABLE))
+		return 0;
+
+	pcaps = kzalloc_obj(*pcaps);
+	if (!pcaps)
+		return -ENOMEM;
+
+	return ice_aq_get_phy_caps(pi, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA,
+				   pcaps, NULL);
+}
+
 /**
  * ice_update_link_info - update status of the HW network link
  * @pi: port info structure of the interested logical port
  */
 int ice_update_link_info(struct ice_port_info *pi)
 {
-	struct ice_link_status *li;
 	int status;
 
 	if (!pi)
 		return -EINVAL;
 
-	li = &pi->phy.link_info;
-
 	status = ice_aq_get_link_info(pi, true, NULL, NULL);
 	if (status)
 		return status;
 
-	if (li->link_info & ICE_AQ_MEDIA_AVAILABLE) {
-		struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
-
-		pcaps = kzalloc_obj(*pcaps);
-		if (!pcaps)
-			return -ENOMEM;
-
-		status = ice_aq_get_phy_caps(pi, false, ICE_AQC_REPORT_TOPO_CAP_MEDIA,
-					     pcaps, NULL);
-	}
-
-	return status;
+	return ice_update_media_caps(pi);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 4ca3f5edcb92..bb36bdd34270 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -93,6 +93,7 @@ ice_clean_rq_elem(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 		  struct ice_rq_event_info *e, u16 *pending);
 int
 ice_get_link_status(struct ice_port_info *pi, bool *link_up);
+int ice_update_media_caps(struct ice_port_info *pi);
 int ice_update_link_info(struct ice_port_info *pi);
 int
 ice_acquire_res(struct ice_hw *hw, enum ice_aq_res_ids res,
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 90fa11b22820..abc835613a31 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -2775,7 +2775,7 @@ ice_set_link_ksettings(struct net_device *netdev,
 		/* Tell the OS link is going down, the link will go
 		 * back up when fw says it is ready asynchronously
 		 */
-		ice_print_link_msg(np->vsi, false);
+		ice_print_link_msg(np->vsi, false, 0);
 		netif_carrier_off(netdev);
 		netif_tx_stop_all_queues(netdev);
 	}
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 730d48adbaaf..f9f12916d03b 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -845,8 +845,9 @@ static void ice_print_topo_conflict(struct ice_vsi *vsi)
  * ice_print_link_msg - print link up or down message
  * @vsi: the VSI whose link status is being queried
  * @isup: boolean for if the link is now up or down
+ * @link_speed: current link speed value to display
  */
-void ice_print_link_msg(struct ice_vsi *vsi, bool isup)
+void ice_print_link_msg(struct ice_vsi *vsi, bool isup, u16 link_speed)
 {
 	struct ice_aqc_get_phy_caps_data *caps;
 	const char *an_advertised;
@@ -870,7 +871,7 @@ void ice_print_link_msg(struct ice_vsi *vsi, bool isup)
 		return;
 	}
 
-	switch (vsi->port_info->phy.link_info.link_speed) {
+	switch (link_speed) {
 	case ICE_AQ_LINK_SPEED_200GB:
 		speed = "200 G";
 		break;
@@ -1181,7 +1182,8 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up,
 	struct ice_vsi *vsi;
 	u16 old_link_speed;
 	bool old_link;
-	int status;
+	bool changed;
+	int err;
 
 	phy_info = &pi->phy;
 	phy_info->link_info_old = phy_info->link_info;
@@ -1189,23 +1191,32 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up,
 	old_link = !!(phy_info->link_info_old.link_info & ICE_AQ_LINK_UP);
 	old_link_speed = phy_info->link_info_old.link_speed;
 
+	/* don't drop an event that the refreshed link info no longer shows */
+	changed = link_up != old_link || link_speed != old_link_speed;
+
 	/* update the link info structures and re-enable link events,
 	 * don't bail on failure due to other book keeping needed
 	 */
-	status = ice_update_link_info(pi);
-	if (status)
+	err = ice_aq_get_link_info(pi, true, NULL, NULL);
+	if (!err) {
+		link_up = phy_info->link_info.link_info & ICE_AQ_LINK_UP;
+		link_speed = phy_info->link_info.link_speed;
+		err = ice_update_media_caps(pi);
+	} else {
+		/* read failed, keep the cache in sync with the applied state */
+		if (link_up)
+			phy_info->link_info.link_info |= ICE_AQ_LINK_UP;
+		else
+			phy_info->link_info.link_info &= ~ICE_AQ_LINK_UP;
+		phy_info->link_info.link_speed = link_speed;
+	}
+	if (err)
 		dev_dbg(dev, "Failed to update link status on port %d, err %d aq_err %s\n",
-			pi->lport, status,
+			pi->lport, err,
 			libie_aq_str(pi->hw->adminq.sq_last_status));
 
 	ice_check_link_cfg_err(pf, pi->phy.link_info.link_cfg_err);
 
-	/* Check if the link state is up after updating link info, and treat
-	 * this event as an UP event since the link is actually UP now.
-	 */
-	if (phy_info->link_info.link_info & ICE_AQ_LINK_UP)
-		link_up = true;
-
 	vsi = ice_get_main_vsi(pf);
 	if (!vsi || !vsi->port_info)
 		return -EINVAL;
@@ -1217,8 +1228,9 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up,
 		ice_set_link(vsi, false);
 	}
 
-	/* if the old link up/down and speed is the same as the new */
-	if (link_up == old_link && link_speed == old_link_speed)
+	/* nothing to do if neither the event nor the refreshed state changed */
+	changed |= link_up != old_link || link_speed != old_link_speed;
+	if (!changed)
 		return 0;
 
 	if (!link_up && old_link)
@@ -1234,7 +1246,7 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up,
 			ice_set_dflt_mib(pf);
 	}
 	ice_vsi_link_event(vsi, link_up);
-	ice_print_link_msg(vsi, link_up);
+	ice_print_link_msg(vsi, link_up, link_speed);
 
 	ice_vc_notify_link_state(pf);
 
@@ -6831,7 +6843,8 @@ static int ice_up_complete(struct ice_vsi *vsi)
 	    (vsi->port_info->phy.link_info.link_info & ICE_AQ_LINK_UP) &&
 	    ((vsi->netdev && (vsi->type == ICE_VSI_PF ||
 			      vsi->type == ICE_VSI_SF)))) {
-		ice_print_link_msg(vsi, true);
+		ice_print_link_msg(vsi, true,
+				   vsi->port_info->phy.link_info.link_speed);
 		netif_tx_start_all_queues(vsi->netdev);
 		netif_carrier_on(vsi->netdev);
 	}
-- 
2.50.1

Re: [PATCH iwl-net v3] ice: Fix "Unknown bps" during link events

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ice: data race on bitfields in struct ice_vsi causing state corruption
- [Medium] ice: unsynchronized RMW of phy_info->link_info in ice_link_event()
--

commit 35fcfc2f1fd4d9de2d01f00551ed70ac2dd02bbf
Author: Jakub Kaminski <jakub.kaminski@intel.com>
Subject: ice: Fix "Unknown bps" during link events

The commit refactors the link event flow in the ice driver to use refreshed
link state rather than event values when the status read succeeds. It also
safely caches event values on read failures, introduces ice_update_media_caps(),
and updates ice_print_link_msg() to take the link speed as a parameter.

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index c26cef0214552..ac0867ef812a0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -844,8 +844,9 @@ static void ice_print_topo_conflict(struct ice_vsi *vsi)
>   * ice_print_link_msg - print link up or down message
>   * @vsi: the VSI whose link status is being queried
>   * @isup: boolean for if the link is now up or down
> + * @link_speed: current link speed value to display
>   */
> -void ice_print_link_msg(struct ice_vsi *vsi, bool isup)
> +void ice_print_link_msg(struct ice_vsi *vsi, bool isup, u16 link_speed)
>  {

[Severity: High]
Does this function trigger a data race on the bitfields in struct ice_vsi
that can cause state corruption?

The current_isup, irqs_ready, and stat_offsets_loaded bitfields share a
single byte in struct ice_vsi:

	u8 irqs_ready:1;
	u8 current_isup:1;		 /* Sync 'link up' logging */
	u8 stat_offsets_loaded:1;

Compilers implement assignments to these bitfields as non-atomic
read-modify-write (RMW) operations on the shared byte. This function
modifies current_isup locklessly from the ice_service_task workqueue.

Concurrently, ice_vsi_req_irq_msix(), ice_vsi_free_irq(), and
ice_up_complete() modify irqs_ready and stat_offsets_loaded from process
context (e.g., ndo_open/ndo_stop) protected by the RTNL lock. 

A concurrent RMW by the service task can silently overwrite and revert
changes to irqs_ready or stat_offsets_loaded. Reverting irqs_ready to true
after it was set to false during interface teardown can lead to a double-free
of IRQs on the next down operation, or missed interrupts if reverted to false.

[ ... ]

> @@ -1188,23 +1190,32 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up,
>  	old_link = !!(phy_info->link_info_old.link_info & ICE_AQ_LINK_UP);
>  	old_link_speed = phy_info->link_info_old.link_speed;
>  
> +	/* don't drop an event that the refreshed link info no longer shows */
> +	changed = link_up != old_link || link_speed != old_link_speed;
> +
>  	/* update the link info structures and re-enable link events,
>  	 * don't bail on failure due to other book keeping needed
>  	 */
> -	status = ice_update_link_info(pi);
> -	if (status)
> +	err = ice_aq_get_link_info(pi, true, NULL, NULL);
> +	if (!err) {
> +		link_up = phy_info->link_info.link_info & ICE_AQ_LINK_UP;
> +		link_speed = phy_info->link_info.link_speed;
> +		err = ice_update_media_caps(pi);
> +	} else {
> +		/* read failed, keep the cache in sync with the applied state */
> +		if (link_up)
> +			phy_info->link_info.link_info |= ICE_AQ_LINK_UP;
> +		else
> +			phy_info->link_info.link_info &= ~ICE_AQ_LINK_UP;

[Severity: Medium]
Does this code introduce an unprotected read-modify-write operation on the
shared link_info member?

The driver's service task handles link events via ice_link_event(). On this
newly added failure path for ice_aq_get_link_info(), the code performs a
read-modify-write (RMW) operation on a shared structure field:
phy_info->link_info.link_info |= ICE_AQ_LINK_UP;

Concurrently, other execution paths (e.g., ethtool callbacks like
ice_set_link_ksettings() running under the RTNL lock) can trigger
ice_aq_get_link_info(), which unconditionally updates the same
pi->phy.link_info cache without holding any locks.

While the commit message acknowledges that accesses to phy.link_info are not
serialized between the service task and other contexts, adding a new bitwise
RMW operation on the error path increases the risk of tearing the link_info
byte or silently clobbering concurrent state updates.

> +		phy_info->link_info.link_speed = link_speed;
> +	}
> +	if (err)
>  		dev_dbg(dev, "Failed to update link status on port %d, err %d aq_err %s\n",
> -			pi->lport, status,
> +			pi->lport, err,
>  			libie_aq_str(pi->hw->adminq.sq_last_status));
>  
>  	ice_check_link_cfg_err(pf, pi->phy.link_info.link_cfg_err);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016111653.1196653-1-lukasz.czapnik@intel.com?part=1