RE: [PATCH v2 1/2] ice: restore DDP state during PFR recovery

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Aaron Ma <aaron.ma@xxxxxxxxxxxxx>
> Sent: Monday, September 7, 2026 1:52 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@xxxxxxxxx>; Andrew Lunn
> <andrew+netdev@xxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Eric
> Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo
> Abeni <pabeni@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Cc: Henry Tieman <henry.w.tieman@xxxxxxxxx>; moderated list:INTEL
> ETHERNET DRIVERS <intel-wired-lan@xxxxxxxxxxxxxxxx>
> Subject: [PATCH v2 1/2] ice: restore DDP state during PFR recovery
> 
> The firmware package and switch recipes are shared by all PFs of an
> adapter. However, each PF rebuilds independently after system resume.
> The resulting package downloads and recipe updates can interleave,
> causing firmware timeouts and leaving interfaces unusable.
> 
> PFR also clears package-derived firmware state on affected devices.
> The existing PFR path only rebuilds the driver's block tables, so VLAN
> recipe programming can fail after the reset.
> 
> The failure is reported as:
> 
>   ice 0000:04:00.0: Update pkg failed: err -5
>   ice 0000:04:00.0: package load failed, -12
>   ice 0000:04:00.0: Rebuild failed, unload and reload driver
> 
> This was observed on an Intel E810-XXV-2 adapter (PCI ID 8086:159b,
> revision 02) with NVM package 1.0.0.18 and ICE OS Default DDP package
> 1.3.43.0.
> 
> Serialize rebuilds across PFs of the same adapter, while allowing each
> PFR to complete independently. Reload the DDP package from its cached
> copy and restore the default DVM recipes before rebuilding the
> remaining PF state.
> Abort recovery if either operation fails.
> 
> In addition, stop the service task before tearing down resources in
> ice_remove() so that an in-flight rebuild completes and cannot
> dereference
> pf->adapter after ice_adapter_put().
> 
> Fixes: 462acf6aca85 ("ice: Enable DDP package download")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@xxxxxxxxx>
> Signed-off-by: Aaron Ma <aaron.ma@xxxxxxxxxxxxx>
> ---
> v1 -> v2:
> - Stop service task early in ice_remove() to prevent use-after-free on
>   pf->adapter if rebuild is in flight.
> 
>  drivers/net/ethernet/intel/ice/ice_adapter.c  |  2 ++
> drivers/net/ethernet/intel/ice/ice_adapter.h  |  3 ++
>  drivers/net/ethernet/intel/ice/ice_main.c     | 35 ++++++++++++++----
> -
>  .../net/ethernet/intel/ice/ice_vlan_mode.c    |  2 +-
>  .../net/ethernet/intel/ice/ice_vlan_mode.h    |  1 +
>  5 files changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c
> b/drivers/net/ethernet/intel/ice/ice_adapter.c
> index 2dc3629d6d0f6..7de9223e971a7 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.c
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
> @@ -64,6 +64,7 @@ static struct ice_adapter *ice_adapter_new(struct
> pci_dev *pdev)
>  	spin_lock_init(&adapter->txq_ctx_lock);
>  	for (int i = 0; i < ARRAY_SIZE(adapter->cpi_phy_lock); i++)
>  		mutex_init(&adapter->cpi_phy_lock[i]);
> +	mutex_init(&adapter->rebuild_lock);
>  	refcount_set(&adapter->refcount, 1);
> 
>  	mutex_init(&adapter->ports.lock);
> @@ -77,6 +78,7 @@ static void ice_adapter_free(struct ice_adapter
> *adapter)
>  	WARN_ON(!list_empty(&adapter->ports.ports));
>  	for (int i = 0; i < ARRAY_SIZE(adapter->cpi_phy_lock); i++)
>  		mutex_destroy(&adapter->cpi_phy_lock[i]);
> +	mutex_destroy(&adapter->rebuild_lock);
>  	mutex_destroy(&adapter->ports.lock);
> 
>  	kfree(adapter);
> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h
> b/drivers/net/ethernet/intel/ice/ice_adapter.h
> index 4f695f32da3d8..f9265de973efc 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
> @@ -37,6 +37,7 @@ struct ice_port_list {
>   * @cpi_phy_lock: Per-PHY mutex serializing CPI REQ/ACK transactions.
>   *               Index 0 = PHY0, index 1 = PHY1. Used on E825C
> devices.
>   * @ctrl_pf: Control PF of the adapter
> + * @rebuild_lock: serialize PFR recovery across PFs of the same
> adapter
>   * @ports: Ports list
>   * @index: 64-bit index cached for collision detection on 32bit
> systems
>   */
> @@ -48,6 +49,8 @@ struct ice_adapter {
>  	spinlock_t txq_ctx_lock;
>  	/* Serialize CPI REQ/ACK transactions per PHY (E825C only) */
>  	struct mutex cpi_phy_lock[ICE_E825_MAX_PHYS];
> +	/* Serialize PFR recovery touching shared FW global state */
> +	struct mutex rebuild_lock;
> 
>  	struct ice_pf *ctrl_pf;
>  	struct ice_port_list ports;
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
> b/drivers/net/ethernet/intel/ice/ice_main.c
> index d88835482d3aa..abb6e850ec09f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -659,7 +659,9 @@ static void ice_do_reset(struct ice_pf *pf, enum
> ice_reset_req reset_type)
>  	 */
>  	if (reset_type == ICE_RESET_PFR) {
>  		pf->pfr_count++;
> +		mutex_lock(&pf->adapter->rebuild_lock);
>  		ice_rebuild(pf, reset_type);
> +		mutex_unlock(&pf->adapter->rebuild_lock);
>  		clear_bit(ICE_PREPARED_FOR_RESET, pf->state);
>  		clear_bit(ICE_PFR_REQ, pf->state);
>  		wake_up(&pf->reset_wait_queue);
> @@ -704,7 +706,9 @@ static void ice_reset_subtask(struct ice_pf *pf)
>  		} else {
>  			/* done with reset. start rebuild */
>  			pf->hw.reset_ongoing = false;
> +			mutex_lock(&pf->adapter->rebuild_lock);
>  			ice_rebuild(pf, reset_type);
> +			mutex_unlock(&pf->adapter->rebuild_lock);
>  			/* clear bit to resume normal operations, but
>  			 * ICE_NEEDS_RESTART bit is set in case rebuild
> failed
>  			 */
> @@ -5374,6 +5378,8 @@ static void ice_remove(struct pci_dev *pdev)
>  		return;
>  	}
> 
> +	ice_service_task_stop(pf);
> +
>  	if (test_bit(ICE_FLAG_SRIOV_ENA, pf->flags)) {
>  		set_bit(ICE_VF_RESETS_DISABLED, pf->state);
>  		ice_free_vfs(pf);
> @@ -7674,14 +7680,27 @@ static void ice_rebuild(struct ice_pf *pf,
> enum ice_reset_req reset_type)
>  		goto err_init_ctrlq;
>  	}
> 
> -	/* if DDP was previously loaded successfully */
> -	if (!ice_is_safe_mode(pf)) {
> -		/* reload the SW DB of filter tables */
> -		if (reset_type == ICE_RESET_PFR)
> -			ice_fill_blk_tbls(hw);
> -		else
> -			/* Reload DDP Package after CORER/GLOBR reset */
> -			ice_load_pkg(NULL, pf);
> +	if (!ice_is_safe_mode(pf) && reset_type == ICE_RESET_PFR) {
> +		enum ice_ddp_state state;
> +
> +		state = ice_init_pkg(hw, hw->pkg_copy, hw->pkg_size);
> +		ice_log_pkg_init(hw, state);
> +		if (!ice_is_init_pkg_successful(state))
> +			goto err_init_ctrlq;
> +	} else if (!ice_is_safe_mode(pf)) {
> +		/* Reload DDP Package after CORER/GLOBR reset */
> +		ice_load_pkg(NULL, pf);
> +	}
> +
> +	/* PFR can lose DVM recipes after system suspend. */
> +	if (!ice_is_safe_mode(pf) && reset_type == ICE_RESET_PFR &&
> +	    ice_is_dvm_ena(hw)) {
> +		err = ice_dvm_update_dflt_recipes(hw);
> +		if (err) {
> +			dev_err(dev, "failed to restore default DVM
> recipes: %d\n",
> +				err);
> +			goto err_init_ctrlq;
> +		}
>  	}
> 
>  	err = ice_clear_pf_cfg(hw);
> diff --git a/drivers/net/ethernet/intel/ice/ice_vlan_mode.c
> b/drivers/net/ethernet/intel/ice/ice_vlan_mode.c
> index fb526cb847764..58fb191903a75 100644
> --- a/drivers/net/ethernet/intel/ice/ice_vlan_mode.c
> +++ b/drivers/net/ethernet/intel/ice/ice_vlan_mode.c
> @@ -240,7 +240,7 @@ static struct ice_update_recipe_lkup_idx_params
> ice_dvm_dflt_recipes[] = {
>   * ice_dvm_update_dflt_recipes - update default switch recipes in DVM
>   * @hw: hardware structure used to update the recipes
>   */
> -static int ice_dvm_update_dflt_recipes(struct ice_hw *hw)
> +int ice_dvm_update_dflt_recipes(struct ice_hw *hw)
>  {
>  	unsigned long i;
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_vlan_mode.h
> b/drivers/net/ethernet/intel/ice/ice_vlan_mode.h
> index a0fb743d08e20..5dc705435f56a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_vlan_mode.h
> +++ b/drivers/net/ethernet/intel/ice/ice_vlan_mode.h
> @@ -8,6 +8,7 @@ struct ice_hw;
> 
>  bool ice_is_dvm_ena(struct ice_hw *hw);  int ice_set_vlan_mode(struct
> ice_hw *hw);
> +int ice_dvm_update_dflt_recipes(struct ice_hw *hw);
>  void ice_post_pkg_dwnld_vlan_mode_cfg(struct ice_hw *hw);
> 
>  #endif /* _ICE_VLAN_MODE_H */
> --
> 2.43.0

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@xxxxxxxxx>