Re: [PATCH iwl-net v2 2/5] idpf: detach and close netdevs while handling a reset

[Simon Horman <horms@kernel.org>]

On Thu, Nov 20, 2025 at 04:12:15PM -0800, Emil Tantilov wrote:

...

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> index 2a53f3d504d2..5c81f52db266 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> @@ -752,6 +752,65 @@ static int idpf_init_mac_addr(struct idpf_vport *vport,
>  	return 0;
>  }
>  
> +static void idpf_detach_and_close(struct idpf_adapter *adapter)
> +{
> +	int max_vports = adapter->max_vports;
> +
> +	for (int i = 0; i < max_vports; i++) {
> +		struct net_device *netdev = adapter->netdevs[i];
> +
> +		/* If the interface is in detached state, that means the
> +		 * previous reset was not handled successfully for this
> +		 * vport.
> +		 */
> +		if (!netif_device_present(netdev))
> +			continue;
> +
> +		/* Hold RTNL to protect racing with callbacks */
> +		rtnl_lock();
> +		netif_device_detach(netdev);
> +		if (netif_running(netdev)) {
> +			set_bit(IDPF_VPORT_UP_REQUESTED,
> +				adapter->vport_config[i]->flags);
> +			dev_close(netdev);
> +		}
> +		rtnl_unlock();
> +	}
> +}
> +
> +static void idpf_attach_and_open(struct idpf_adapter *adapter)
> +{
> +	int max_vports = adapter->max_vports;
> +
> +	for (int i = 0; i < max_vports; i++) {
> +		struct idpf_vport *vport = adapter->vports[i];
> +		struct idpf_vport_config *vport_config;
> +		struct net_device *netdev;
> +
> +		/* In case of a critical error in the init task, the vport
> +		 * will be freed. Only continue to restore the netdevs
> +		 * if the vport is allocated.
> +		 */
> +		if (!vport)
> +			continue;
> +
> +		/* No need for RTNL on attach as this function is called
> +		 * following detach and dev_close(). We do take RTNL for
> +		 * dev_open() below as it can race with external callbacks
> +		 * following the call to netif_device_attach().
> +		 */
> +		netdev = adapter->netdevs[i];
> +		netif_device_attach(netdev);
> +		vport_config = adapter->vport_config[vport->idx];
> +		if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED,
> +				       vport_config->flags)) {
> +			rtnl_lock();
> +			dev_open(netdev, NULL);
> +			rtnl_unlock();
> +		}
> +	}
> +}
> +
>  /**
>   * idpf_cfg_netdev - Allocate, configure and register a netdev
>   * @vport: main vport structure

...

> @@ -1807,27 +1860,6 @@ static int idpf_check_reset_complete(struct idpf_hw *hw,
>  	return -EBUSY;
>  }
>  
> -/**
> - * idpf_set_vport_state - Set the vport state to be after the reset
> - * @adapter: Driver specific private structure
> - */
> -static void idpf_set_vport_state(struct idpf_adapter *adapter)
> -{
> -	u16 i;
> -
> -	for (i = 0; i < adapter->max_vports; i++) {
> -		struct idpf_netdev_priv *np;
> -
> -		if (!adapter->netdevs[i])
> -			continue;
> -
> -		np = netdev_priv(adapter->netdevs[i]);
> -		if (np->state == __IDPF_VPORT_UP)
> -			set_bit(IDPF_VPORT_UP_REQUESTED,
> -				adapter->vport_config[i]->flags);
> -	}
> -}
> -
>  /**
>   * idpf_init_hard_reset - Initiate a hardware reset
>   * @adapter: Driver specific private structure

> @@ -1836,28 +1868,17 @@ static void idpf_set_vport_state(struct idpf_adapter *adapter)
>   * reallocate. Also reinitialize the mailbox. Return 0 on success,
>   * negative on failure.
>   */
> -static int idpf_init_hard_reset(struct idpf_adapter *adapter)
> +static void idpf_init_hard_reset(struct idpf_adapter *adapter)
>  {
>  	struct idpf_reg_ops *reg_ops = &adapter->dev_ops.reg_ops;
>  	struct device *dev = &adapter->pdev->dev;
> -	struct net_device *netdev;
>  	int err;
> -	u16 i;
>  
> +	idpf_detach_and_close(adapter);
>  	mutex_lock(&adapter->vport_ctrl_lock);
>  
>  	dev_info(dev, "Device HW Reset initiated\n");
>  
> -	/* Avoid TX hangs on reset */
> -	for (i = 0; i < adapter->max_vports; i++) {
> -		netdev = adapter->netdevs[i];
> -		if (!netdev)
> -			continue;

Hi Emil,

In this code that is removed there is a check for !netdev.
And also there is a similar check in idpf_set_vport_state().
But there is no such check in idpf_detach_and_close().
Is this intentional?

Likewise, is such a check needed in idpf_attach_and_open()?

My understanding is that netdevs[i] is populated asynchronously
via idpf_init_task. But I'm unsure if that is known to have
completed or not.

Flagged by Claude Code with https://github.com/masoncl/review-prompts/


> -
> -		netif_carrier_off(netdev);
> -		netif_tx_disable(netdev);
> -	}
> -
>  	/* Prepare for reset */
>  	if (test_and_clear_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
>  		reg_ops->trigger_reset(adapter, IDPF_HR_DRV_LOAD);
> @@ -1866,7 +1887,6 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
>  
>  		idpf_idc_issue_reset_event(adapter->cdev_info);
>  
> -		idpf_set_vport_state(adapter);
>  		idpf_vc_core_deinit(adapter);
>  		if (!is_reset)
>  			reg_ops->trigger_reset(adapter, IDPF_HR_FUNC_RESET);
> @@ -1913,11 +1933,14 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
>  unlock_mutex:
>  	mutex_unlock(&adapter->vport_ctrl_lock);
>  
> -	/* Wait until all vports are created to init RDMA CORE AUX */
> -	if (!err)
> -		err = idpf_idc_init(adapter);
> -
> -	return err;
> +	/* Attempt to restore netdevs and initialize RDMA CORE AUX device,
> +	 * provided vc_core_init succeeded. It is still possible that
> +	 * vports are not allocated at this point if the init task failed.
> +	 */
> +	if (!err) {
> +		idpf_attach_and_open(adapter);
> +		idpf_idc_init(adapter);
> +	}
>  }
>  
>  /**
> -- 
> 2.37.3
> 
>