Re: [PATCH iwl-next] ice: do not carry link status over to link event data

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ice: do not carry link status over to link event data

This patch removes code that updated the link_up parameter with the
current firmware state after calling ice_update_link_info().

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index e7308e381e2f..348c86b7328d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -1127,12 +1127,6 @@ 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;
> -

Could removing this check create a race condition with stale link events?

Consider this scenario: firmware generates a link DOWN event, but before
the driver processes it, the link comes back UP. When the driver later
processes the event and calls ice_update_link_info(), the firmware query
returns the current state (UP).

Previously, the removed code would check phy_info->link_info.link_info &
ICE_AQ_LINK_UP and override link_up to true, reflecting the current
reality. Now link_up stays false (the stale event data).

Later in ice_link_event(), the comparison checks:

    if (link_up == old_link && link_speed == old_link_speed)

If the link was previously UP (old_link=true), the comparison fails
(false != true), and the driver proceeds to process a link DOWN event
even though the firmware reports the link is UP. This leads to
ice_vsi_link_event() calling netif_carrier_off() and
netif_tx_stop_all_queues() inappropriately, marking the network interface
DOWN and causing packet loss until the next link event.

The removed comment explicitly documented this protection: "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."

The commit message states "changes that occurred from update link
info...will be handled appropriately by itself" but doesn't explain what
mechanism handles this. The ice_update_link_info() function only queries
firmware state; it doesn't generate events or trigger link state change
handling.

How does the new code prevent stale link DOWN events from incorrectly
bringing down an interface when the link is actually UP?

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