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

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

[Aleksandr Loktionov]

From: Tony Nguyen <anthony.l.nguyen@intel.com>

Since we now report changes from the link event and changes that
occurred from update link info, there is no need to carry the latter
info over as it will be handled appropriately by itself.

Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index e7308e3..348c86b 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;
-
 	vsi = ice_get_main_vsi(pf);
 	if (!vsi || !vsi->port_info)
 		return -EINVAL;
-- 
2.52.0

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

[Simon Horman]

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;

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

[Tony Nguyen]

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;

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

[Simon Horman]

On Fri, Mar 20, 2026 at 03:05:24PM -0700, Tony Nguyen wrote:
> 
> 
> 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 for the clarification, much appreciated.