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

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 3/20/2026 12:33 PM, Simon Horman wrote:
> 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?

This was a part of other changes that allows this to happen. By itself, 
as evidenced here, it does not.

Thanks,
Tony

> 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;