On 8/6/2026 9:24 AM, Lukasz Czapnik wrote:
From: Jakub Kaminski <[email protected]>

...

@@ -1190,11 +1191,8 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info 
*pi, bool link_up,
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;
+       link_up = phy_info->link_info.link_info & ICE_AQ_LINK_UP;
+       link_speed = phy_info->link_info.link_speed;

Sashiko says:
Does this unconditionally overwrite the ARQ-provided link_up and link_speed
arguments with the cached AQ state?
If the synchronous AQ query (ice_update_link_info()) called earlier in
ice_link_event() fails, for example due to an AQ timeout, the
phy_info->link_info structure is not updated.
By unconditionally reading from this potentially stale structure, link_up
and link_speed are guaranteed to match old_link and old_link_speed. This
would cause the following early return block to trigger erroneously:
drivers/net/ethernet/intel/ice/ice_main.c:ice_link_event() {
        ...
        if (link_up == old_link && link_speed == old_link_speed)
                return 0;
        ...
}
Could this silently drop a valid hardware link state change, leaving the
OS and the hardware out of sync and causing network hangs?

Tony says:
I think the right/wrong combination could cause this to occur. I, also, think there's an issue with accounting for the link event and updated link info that could get lost if it changes.

Thanks,
Tony

        vsi = ice_get_main_vsi(pf);
        if (!vsi || !vsi->port_info)

Reply via email to