[PATCH iwl-net 0/2] ice: fix issues with loading certain older DDP packages

[PATCH iwl-net 0/2] ice: fix issues with loading certain older DDP packages

[Jacob Keller]

This series contains two fixes to improve the software handling for certain
error conditions when loading an older DDP package on a device. In
particular, some combinations of DDP and firmware version can result in the
driver accidentally locking the device up due to blocking the global
configuration lock used for DDP programming.

Also, fix an error in the cleanup logic for a failed probe could result in
memory corruption due to a use-after-free from double-calling
ice_deinit_hw().

It is not clear if any publicly released DDP versions suffer from the exact
issues that caused the ice_cfg_tx_topo() failure. However, it is entirely
plausible that the error could be triggered in the future. Thus, it is
important to ensure the error flow is safe and won't make the device
inaccessible for such tasks as updating the firmware. Additionally,
degrading functionality simply because a user has not updated a DDP package
is wrong.

I settled on -ENODEV as the error code for handling ice_init_hw() failures,
to ensure probe stops in the rare cases where this fails. I am open to
alternative suggestions for how to handle errors from ice_cfg_tx_topo()
through into ice_init_tx_topology().

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Jacob Keller (2):
      ice: fix double-call to ice_deinit_hw() during probe failure
      ice: don't leave device non-functional if Tx scheduler config fails

 drivers/net/ethernet/intel/ice/devlink/devlink.c | 10 +-----
 drivers/net/ethernet/intel/ice/ice_ddp.c         | 44 +++++++++++++++++-------
 drivers/net/ethernet/intel/ice/ice_main.c        | 38 ++++++++++----------
 3 files changed, 53 insertions(+), 39 deletions(-)
---
base-commit: dae7f9cbd1909de2b0bccc30afef95c23f93e477
change-id: 20250716-jk-ddp-safe-mode-issue-d6130b8c8205

Best regards,
--  
Jacob Keller <jacob.e.keller@intel.com>

[PATCH iwl-net 1/2] ice: fix double-call to ice_deinit_hw() during probe failure

[Jacob Keller]

The following (and similar) KFENCE bugs have recently been found occurring
during certain error flows of the ice_probe() function:

kernel: ==================================================================
kernel: BUG: KFENCE: use-after-free read in ice_cleanup_fltr_mgmt_struct+0x1d
kernel: Use-after-free read at 0x00000000e72fe5ed (in kfence-#223):
kernel:  ice_cleanup_fltr_mgmt_struct+0x1d/0x200 [ice]
kernel:  ice_deinit_hw+0x1e/0x60 [ice]
kernel:  ice_probe+0x245/0x2e0 [ice]
kernel:
kernel: kfence-#223: <..snip..>
kernel: allocated by task 7553 on cpu 0 at 2243.527621s (198.108303s ago):
kernel:  devm_kmalloc+0x57/0x120
kernel:  ice_init_hw+0x491/0x8e0 [ice]
kernel:  ice_probe+0x203/0x2e0 [ice]
kernel:
kernel: freed by task 7553 on cpu 0 at 2441.509158s (0.175707s ago):
kernel:  ice_deinit_hw+0x1e/0x60 [ice]
kernel:  ice_init+0x1ad/0x570 [ice]
kernel:  ice_probe+0x22b/0x2e0 [ice]
kernel:
kernel: ==================================================================

These occur as the result of a double-call to ice_deinit_hw(). This double
call happens if ice_init() fails at any point after calling
ice_init_dev().

Upon errors, ice_init() calls ice_deinit_dev(), which is supposed to be the
inverse of ice_init_dev(). However, currently ice_init_dev() does not call
ice_init_hw(). Instead, ice_init_hw() is called by ice_probe(). Thus,
ice_probe() itself calls ice_deinit_hw() as part of its error cleanup
logic.

This results in two calls to ice_deinit_hw() which results in straight
forward use-after-free violations due to double calling kfree and other
cleanup functions.

To avoid this double call, move the call to ice_init_hw() into
ice_init_dev(), and remove the now logically unnecessary cleanup from
ice_probe(). This is simpler than the alternative of moving ice_deinit_hw()
*out* of ice_deinit_dev().

Moving the calls to ice_deinit_hw() requires validating all cleanup paths,
and changing significantly more code. Moving the calls of ice_init_hw()
requires only validating that the new placement is still prior to all HW
structure accesses.

For ice_probe(), this now delays ice_init_hw() from before
ice_adapter_get() to just after it. This is safe, as ice_adapter_get() does
not rely on the HW structure.

For ice_devlink_reinit_up(), the ice_init_hw() is now called after
ice_set_min_max_msix(). This is also safe as that function does not access
the HW structure either.

This flow makes more logical sense, as ice_init_dev() is mirrored by
ice_deinit_dev(), so it reasonably should be the caller of ice_init_hw().
It also reduces one extra call to ice_init_hw() since both ice_probe() and
ice_devlink_reinit_up() call ice_init_dev().

This resolves the double-free and avoids memory corruption and other
invalid memory accesses in the event of a failed probe.

Fixes: 5b246e533d01 ("ice: split probe into smaller functions")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/devlink/devlink.c | 10 +---------
 drivers/net/ethernet/intel/ice/ice_main.c        | 24 +++++++++++-------------
 2 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c
index 4af60e2f37df..ef49da0590b3 100644
--- a/drivers/net/ethernet/intel/ice/devlink/devlink.c
+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c
@@ -1233,18 +1233,12 @@ static int ice_devlink_reinit_up(struct ice_pf *pf)
 	struct ice_vsi *vsi = ice_get_main_vsi(pf);
 	int err;
 
-	err = ice_init_hw(&pf->hw);
-	if (err) {
-		dev_err(ice_pf_to_dev(pf), "ice_init_hw failed: %d\n", err);
-		return err;
-	}
-
 	/* load MSI-X values */
 	ice_set_min_max_msix(pf);
 
 	err = ice_init_dev(pf);
 	if (err)
-		goto unroll_hw_init;
+		return err;
 
 	vsi->flags = ICE_VSI_FLAG_INIT;
 
@@ -1267,8 +1261,6 @@ static int ice_devlink_reinit_up(struct ice_pf *pf)
 	rtnl_unlock();
 err_vsi_cfg:
 	ice_deinit_dev(pf);
-unroll_hw_init:
-	ice_deinit_hw(&pf->hw);
 	return err;
 }
 
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 0a11b4281092..c44bb8153ad0 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4739,6 +4739,12 @@ int ice_init_dev(struct ice_pf *pf)
 	struct ice_hw *hw = &pf->hw;
 	int err;
 
+	err = ice_init_hw(hw);
+	if (err) {
+		dev_err(dev, "ice_init_hw failed: %d\n", err);
+		return err;
+	}
+
 	ice_init_feature_support(pf);
 
 	err = ice_init_ddp_config(hw, pf);
@@ -4759,7 +4765,7 @@ int ice_init_dev(struct ice_pf *pf)
 	err = ice_init_pf(pf);
 	if (err) {
 		dev_err(dev, "ice_init_pf failed: %d\n", err);
-		return err;
+		goto unroll_hw_init;
 	}
 
 	pf->hw.udp_tunnel_nic.set_port = ice_udp_tunnel_set_port;
@@ -4803,6 +4809,8 @@ int ice_init_dev(struct ice_pf *pf)
 	ice_clear_interrupt_scheme(pf);
 unroll_pf_init:
 	ice_deinit_pf(pf);
+unroll_hw_init:
+	ice_deinit_hw(hw);
 	return err;
 }
 
@@ -5330,17 +5338,9 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 	if (ice_is_recovery_mode(hw))
 		return ice_probe_recovery_mode(pf);
 
-	err = ice_init_hw(hw);
-	if (err) {
-		dev_err(dev, "ice_init_hw failed: %d\n", err);
-		return err;
-	}
-
 	adapter = ice_adapter_get(pdev);
-	if (IS_ERR(adapter)) {
-		err = PTR_ERR(adapter);
-		goto unroll_hw_init;
-	}
+	if (IS_ERR(adapter))
+		return PTR_ERR(adapter);
 	pf->adapter = adapter;
 
 	err = ice_init(pf);
@@ -5366,8 +5366,6 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 	ice_deinit(pf);
 unroll_adapter:
 	ice_adapter_put(pdev);
-unroll_hw_init:
-	ice_deinit_hw(hw);
 	return err;
 }
 

-- 
2.50.0.349.ga842a77808e9

Re: [PATCH iwl-net 1/2] ice: fix double-call to ice_deinit_hw() during probe failure

[Simon Horman]

On Thu, Jul 17, 2025 at 09:57:08AM -0700, Jacob Keller wrote:
> The following (and similar) KFENCE bugs have recently been found occurring
> during certain error flows of the ice_probe() function:
> 
> kernel: ==================================================================
> kernel: BUG: KFENCE: use-after-free read in ice_cleanup_fltr_mgmt_struct+0x1d
> kernel: Use-after-free read at 0x00000000e72fe5ed (in kfence-#223):
> kernel:  ice_cleanup_fltr_mgmt_struct+0x1d/0x200 [ice]
> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
> kernel:  ice_probe+0x245/0x2e0 [ice]
> kernel:
> kernel: kfence-#223: <..snip..>
> kernel: allocated by task 7553 on cpu 0 at 2243.527621s (198.108303s ago):
> kernel:  devm_kmalloc+0x57/0x120
> kernel:  ice_init_hw+0x491/0x8e0 [ice]
> kernel:  ice_probe+0x203/0x2e0 [ice]
> kernel:
> kernel: freed by task 7553 on cpu 0 at 2441.509158s (0.175707s ago):
> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
> kernel:  ice_init+0x1ad/0x570 [ice]
> kernel:  ice_probe+0x22b/0x2e0 [ice]
> kernel:
> kernel: ==================================================================
> 
> These occur as the result of a double-call to ice_deinit_hw(). This double
> call happens if ice_init() fails at any point after calling
> ice_init_dev().
> 
> Upon errors, ice_init() calls ice_deinit_dev(), which is supposed to be the
> inverse of ice_init_dev(). However, currently ice_init_dev() does not call
> ice_init_hw(). Instead, ice_init_hw() is called by ice_probe(). Thus,
> ice_probe() itself calls ice_deinit_hw() as part of its error cleanup
> logic.
> 
> This results in two calls to ice_deinit_hw() which results in straight
> forward use-after-free violations due to double calling kfree and other
> cleanup functions.
> 
> To avoid this double call, move the call to ice_init_hw() into
> ice_init_dev(), and remove the now logically unnecessary cleanup from
> ice_probe(). This is simpler than the alternative of moving ice_deinit_hw()
> *out* of ice_deinit_dev().
> 
> Moving the calls to ice_deinit_hw() requires validating all cleanup paths,
> and changing significantly more code. Moving the calls of ice_init_hw()
> requires only validating that the new placement is still prior to all HW
> structure accesses.
> 
> For ice_probe(), this now delays ice_init_hw() from before
> ice_adapter_get() to just after it. This is safe, as ice_adapter_get() does
> not rely on the HW structure.
> 
> For ice_devlink_reinit_up(), the ice_init_hw() is now called after
> ice_set_min_max_msix(). This is also safe as that function does not access
> the HW structure either.
> 
> This flow makes more logical sense, as ice_init_dev() is mirrored by
> ice_deinit_dev(), so it reasonably should be the caller of ice_init_hw().
> It also reduces one extra call to ice_init_hw() since both ice_probe() and
> ice_devlink_reinit_up() call ice_init_dev().
> 
> This resolves the double-free and avoids memory corruption and other
> invalid memory accesses in the event of a failed probe.
> 
> Fixes: 5b246e533d01 ("ice: split probe into smaller functions")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Thanks for the detailed explanation.

Reviewed-by: Simon Horman <horms@kernel.org>

RE: [PATCH iwl-net 1/2] ice: fix double-call to ice_deinit_hw() during probe failure

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@intel.com>
> Sent: Thursday, July 17, 2025 6:57 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Intel Wired LAN
> <intel-wired-lan@lists.osuosl.org>; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Grinberg, Vitaly
> <vgrinber@redhat.com>; netdev@vger.kernel.org
> Subject: [PATCH iwl-net 1/2] ice: fix double-call to ice_deinit_hw()
> during probe failure
> 
> The following (and similar) KFENCE bugs have recently been found
> occurring during certain error flows of the ice_probe() function:
> 
> kernel:
> ==================================================================
> kernel: BUG: KFENCE: use-after-free read in
> ice_cleanup_fltr_mgmt_struct+0x1d
> kernel: Use-after-free read at 0x00000000e72fe5ed (in kfence-#223):
> kernel:  ice_cleanup_fltr_mgmt_struct+0x1d/0x200 [ice]
> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
> kernel:  ice_probe+0x245/0x2e0 [ice]
> kernel:
> kernel: kfence-#223: <..snip..>
> kernel: allocated by task 7553 on cpu 0 at 2243.527621s (198.108303s
> ago):
> kernel:  devm_kmalloc+0x57/0x120
> kernel:  ice_init_hw+0x491/0x8e0 [ice]
> kernel:  ice_probe+0x203/0x2e0 [ice]
> kernel:
> kernel: freed by task 7553 on cpu 0 at 2441.509158s (0.175707s ago):
> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
> kernel:  ice_init+0x1ad/0x570 [ice]
> kernel:  ice_probe+0x22b/0x2e0 [ice]
> kernel:
> kernel:
> ==================================================================
> 
> These occur as the result of a double-call to ice_deinit_hw(). This
> double call happens if ice_init() fails at any point after calling
> ice_init_dev().
> 
> Upon errors, ice_init() calls ice_deinit_dev(), which is supposed to
> be the inverse of ice_init_dev(). However, currently ice_init_dev()
> does not call ice_init_hw(). Instead, ice_init_hw() is called by
> ice_probe(). Thus,
> ice_probe() itself calls ice_deinit_hw() as part of its error cleanup
> logic.
> 
> This results in two calls to ice_deinit_hw() which results in straight
> forward use-after-free violations due to double calling kfree and
> other cleanup functions.
> 
> To avoid this double call, move the call to ice_init_hw() into
> ice_init_dev(), and remove the now logically unnecessary cleanup from
> ice_probe(). This is simpler than the alternative of moving
> ice_deinit_hw()
> *out* of ice_deinit_dev().
> 
> Moving the calls to ice_deinit_hw() requires validating all cleanup
> paths, and changing significantly more code. Moving the calls of
> ice_init_hw() requires only validating that the new placement is still
> prior to all HW structure accesses.
> 
> For ice_probe(), this now delays ice_init_hw() from before
> ice_adapter_get() to just after it. This is safe, as ice_adapter_get()
> does not rely on the HW structure.
> 
> For ice_devlink_reinit_up(), the ice_init_hw() is now called after
> ice_set_min_max_msix(). This is also safe as that function does not
> access the HW structure either.
> 
> This flow makes more logical sense, as ice_init_dev() is mirrored by
> ice_deinit_dev(), so it reasonably should be the caller of
> ice_init_hw().
> It also reduces one extra call to ice_init_hw() since both ice_probe()
> and
> ice_devlink_reinit_up() call ice_init_dev().
> 
> This resolves the double-free and avoids memory corruption and other
> invalid memory accesses in the event of a failed probe.
> 
> Fixes: 5b246e533d01 ("ice: split probe into smaller functions")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Re: [PATCH iwl-net 1/2] ice: fix double-call to ice_deinit_hw() during probe failure

[Przemek Kitszel]

On 7/17/25 18:57, Jacob Keller wrote:
> The following (and similar) KFENCE bugs have recently been found occurring
> during certain error flows of the ice_probe() function:
> 
> kernel: ==================================================================
> kernel: BUG: KFENCE: use-after-free read in ice_cleanup_fltr_mgmt_struct+0x1d
> kernel: Use-after-free read at 0x00000000e72fe5ed (in kfence-#223):
> kernel:  ice_cleanup_fltr_mgmt_struct+0x1d/0x200 [ice]
> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
> kernel:  ice_probe+0x245/0x2e0 [ice]
> kernel:
> kernel: kfence-#223: <..snip..>
> kernel: allocated by task 7553 on cpu 0 at 2243.527621s (198.108303s ago):
> kernel:  devm_kmalloc+0x57/0x120
> kernel:  ice_init_hw+0x491/0x8e0 [ice]
> kernel:  ice_probe+0x203/0x2e0 [ice]
> kernel:
> kernel: freed by task 7553 on cpu 0 at 2441.509158s (0.175707s ago):
> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
> kernel:  ice_init+0x1ad/0x570 [ice]
> kernel:  ice_probe+0x22b/0x2e0 [ice]
> kernel:
> kernel: ==================================================================
> 
> These occur as the result of a double-call to ice_deinit_hw(). This double
> call happens if ice_init() fails at any point after calling
> ice_init_dev().
> 
> Upon errors, ice_init() calls ice_deinit_dev(), which is supposed to be the
> inverse of ice_init_dev(). However, currently ice_init_dev() does not call
> ice_init_hw(). Instead, ice_init_hw() is called by ice_probe(). Thus,
> ice_probe() itself calls ice_deinit_hw() as part of its error cleanup
> logic.
> 
> This results in two calls to ice_deinit_hw() which results in straight
> forward use-after-free violations due to double calling kfree and other
> cleanup functions.
> 
> To avoid this double call, move the call to ice_init_hw() into
> ice_init_dev(), and remove the now logically unnecessary cleanup from
> ice_probe(). This is simpler than the alternative of moving ice_deinit_hw()
> *out* of ice_deinit_dev().

[1]

> 
> Moving the calls to ice_deinit_hw() requires validating all cleanup paths,
> and changing significantly more code. Moving the calls of ice_init_hw()
> requires only validating that the new placement is still prior to all HW
> structure accesses.
> 
> For ice_probe(), this now delays ice_init_hw() from before
> ice_adapter_get() to just after it. This is safe, as ice_adapter_get() does
> not rely on the HW structure.

I introduced the order change by
commit fb59a520bbb1 ("ice: ice_probe: init ice_adapter after HW init").
You are right that current driver does not yet utilizes that, but it
will in the future.

> 
> For ice_devlink_reinit_up(), the ice_init_hw() is now called after
> ice_set_min_max_msix(). This is also safe as that function does not access
> the HW structure either.
> 
> This flow makes more logical sense, as ice_init_dev() is mirrored by
> ice_deinit_dev(), so it reasonably should be the caller of ice_init_hw().
> It also reduces one extra call to ice_init_hw() since both ice_probe() and
> ice_devlink_reinit_up() call ice_init_dev().
> 
> This resolves the double-free and avoids memory corruption and other
> invalid memory accesses in the event of a failed probe.
> 
> Fixes: 5b246e533d01 ("ice: split probe into smaller functions")

The blame should be on me here. But instead of fixing current bug in
a way that I would need to invert later, it would be better to fix w/o
order change. (If unwilling to wait, simple revert would be also better)

I would like to do [1] above, either by my or Jake hands (will sync).

> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/devlink/devlink.c | 10 +---------
>   drivers/net/ethernet/intel/ice/ice_main.c        | 24 +++++++++++-------------
>   2 files changed, 12 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c
> index 4af60e2f37df..ef49da0590b3 100644
> --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c
> +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c
> @@ -1233,18 +1233,12 @@ static int ice_devlink_reinit_up(struct ice_pf *pf)
>   	struct ice_vsi *vsi = ice_get_main_vsi(pf);
>   	int err;
>   
> -	err = ice_init_hw(&pf->hw);
> -	if (err) {
> -		dev_err(ice_pf_to_dev(pf), "ice_init_hw failed: %d\n", err);
> -		return err;
> -	}
> -
>   	/* load MSI-X values */
>   	ice_set_min_max_msix(pf);
>   
>   	err = ice_init_dev(pf);
>   	if (err)
> -		goto unroll_hw_init;
> +		return err;
>   
>   	vsi->flags = ICE_VSI_FLAG_INIT;
>   
> @@ -1267,8 +1261,6 @@ static int ice_devlink_reinit_up(struct ice_pf *pf)
>   	rtnl_unlock();
>   err_vsi_cfg:
>   	ice_deinit_dev(pf);
> -unroll_hw_init:
> -	ice_deinit_hw(&pf->hw);
>   	return err;
>   }
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 0a11b4281092..c44bb8153ad0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -4739,6 +4739,12 @@ int ice_init_dev(struct ice_pf *pf)
>   	struct ice_hw *hw = &pf->hw;
>   	int err;
>   
> +	err = ice_init_hw(hw);
> +	if (err) {
> +		dev_err(dev, "ice_init_hw failed: %d\n", err);
> +		return err;
> +	}
> +
>   	ice_init_feature_support(pf);
>   
>   	err = ice_init_ddp_config(hw, pf);
> @@ -4759,7 +4765,7 @@ int ice_init_dev(struct ice_pf *pf)
>   	err = ice_init_pf(pf);
>   	if (err) {
>   		dev_err(dev, "ice_init_pf failed: %d\n", err);
> -		return err;
> +		goto unroll_hw_init;
>   	}
>   
>   	pf->hw.udp_tunnel_nic.set_port = ice_udp_tunnel_set_port;
> @@ -4803,6 +4809,8 @@ int ice_init_dev(struct ice_pf *pf)
>   	ice_clear_interrupt_scheme(pf);
>   unroll_pf_init:
>   	ice_deinit_pf(pf);
> +unroll_hw_init:
> +	ice_deinit_hw(hw);
>   	return err;
>   }
>   
> @@ -5330,17 +5338,9 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>   	if (ice_is_recovery_mode(hw))
>   		return ice_probe_recovery_mode(pf);
>   
> -	err = ice_init_hw(hw);
> -	if (err) {
> -		dev_err(dev, "ice_init_hw failed: %d\n", err);
> -		return err;
> -	}
> -
>   	adapter = ice_adapter_get(pdev);
> -	if (IS_ERR(adapter)) {
> -		err = PTR_ERR(adapter);
> -		goto unroll_hw_init;
> -	}
> +	if (IS_ERR(adapter))
> +		return PTR_ERR(adapter);
>   	pf->adapter = adapter;
>   
>   	err = ice_init(pf);
> @@ -5366,8 +5366,6 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>   	ice_deinit(pf);
>   unroll_adapter:
>   	ice_adapter_put(pdev);
> -unroll_hw_init:
> -	ice_deinit_hw(hw);
>   	return err;
>   }
>   
> 

Re: [PATCH iwl-net 1/2] ice: fix double-call to ice_deinit_hw() during probe failure

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 7525 bytes --]



On 8/18/2025 3:27 AM, Przemek Kitszel wrote:
> On 7/17/25 18:57, Jacob Keller wrote:
>> The following (and similar) KFENCE bugs have recently been found occurring
>> during certain error flows of the ice_probe() function:
>>
>> kernel: ==================================================================
>> kernel: BUG: KFENCE: use-after-free read in ice_cleanup_fltr_mgmt_struct+0x1d
>> kernel: Use-after-free read at 0x00000000e72fe5ed (in kfence-#223):
>> kernel:  ice_cleanup_fltr_mgmt_struct+0x1d/0x200 [ice]
>> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
>> kernel:  ice_probe+0x245/0x2e0 [ice]
>> kernel:
>> kernel: kfence-#223: <..snip..>
>> kernel: allocated by task 7553 on cpu 0 at 2243.527621s (198.108303s ago):
>> kernel:  devm_kmalloc+0x57/0x120
>> kernel:  ice_init_hw+0x491/0x8e0 [ice]
>> kernel:  ice_probe+0x203/0x2e0 [ice]
>> kernel:
>> kernel: freed by task 7553 on cpu 0 at 2441.509158s (0.175707s ago):
>> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
>> kernel:  ice_init+0x1ad/0x570 [ice]
>> kernel:  ice_probe+0x22b/0x2e0 [ice]
>> kernel:
>> kernel: ==================================================================
>>
>> These occur as the result of a double-call to ice_deinit_hw(). This double
>> call happens if ice_init() fails at any point after calling
>> ice_init_dev().
>>
>> Upon errors, ice_init() calls ice_deinit_dev(), which is supposed to be the
>> inverse of ice_init_dev(). However, currently ice_init_dev() does not call
>> ice_init_hw(). Instead, ice_init_hw() is called by ice_probe(). Thus,
>> ice_probe() itself calls ice_deinit_hw() as part of its error cleanup
>> logic.
>>
>> This results in two calls to ice_deinit_hw() which results in straight
>> forward use-after-free violations due to double calling kfree and other
>> cleanup functions.
>>
>> To avoid this double call, move the call to ice_init_hw() into
>> ice_init_dev(), and remove the now logically unnecessary cleanup from
>> ice_probe(). This is simpler than the alternative of moving ice_deinit_hw()
>> *out* of ice_deinit_dev().
> 
> [1]
> 
>>
>> Moving the calls to ice_deinit_hw() requires validating all cleanup paths,
>> and changing significantly more code. Moving the calls of ice_init_hw()
>> requires only validating that the new placement is still prior to all HW
>> structure accesses.
>>
>> For ice_probe(), this now delays ice_init_hw() from before
>> ice_adapter_get() to just after it. This is safe, as ice_adapter_get() does
>> not rely on the HW structure.
> 
> I introduced the order change by
> commit fb59a520bbb1 ("ice: ice_probe: init ice_adapter after HW init").
> You are right that current driver does not yet utilizes that, but it
> will in the future.
> 

If you have a better fix, I'm all for it. The current driver code is
buggy at least in the error handling phase.

>>
>> For ice_devlink_reinit_up(), the ice_init_hw() is now called after
>> ice_set_min_max_msix(). This is also safe as that function does not access
>> the HW structure either.
>>
>> This flow makes more logical sense, as ice_init_dev() is mirrored by
>> ice_deinit_dev(), so it reasonably should be the caller of ice_init_hw().
>> It also reduces one extra call to ice_init_hw() since both ice_probe() and
>> ice_devlink_reinit_up() call ice_init_dev().
>>
>> This resolves the double-free and avoids memory corruption and other
>> invalid memory accesses in the event of a failed probe.
>>
>> Fixes: 5b246e533d01 ("ice: split probe into smaller functions")
> 
> The blame should be on me here. But instead of fixing current bug in
> a way that I would need to invert later, it would be better to fix w/o
> order change. (If unwilling to wait, simple revert would be also better)
> 
> I would like to do [1] above, either by my or Jake hands (will sync).
> 

I hadn't noticed your change when looking through history. I am fine
with either a revert, or dropping this fix from the DDP fix and
submitting that separately. This issue only occurs if probe fails at
certain points and cleanup logic is triggered. That should be rare.

Alternatively I'm fine with reverting the change if we want to get this
potential issue fixed now.

>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/devlink/devlink.c | 10 +---------
>>   drivers/net/ethernet/intel/ice/ice_main.c        | 24 +++++++++++-------------
>>   2 files changed, 12 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c
>> index 4af60e2f37df..ef49da0590b3 100644
>> --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c
>> +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c
>> @@ -1233,18 +1233,12 @@ static int ice_devlink_reinit_up(struct ice_pf *pf)
>>   	struct ice_vsi *vsi = ice_get_main_vsi(pf);
>>   	int err;
>>   
>> -	err = ice_init_hw(&pf->hw);
>> -	if (err) {
>> -		dev_err(ice_pf_to_dev(pf), "ice_init_hw failed: %d\n", err);
>> -		return err;
>> -	}
>> -
>>   	/* load MSI-X values */
>>   	ice_set_min_max_msix(pf);
>>   
>>   	err = ice_init_dev(pf);
>>   	if (err)
>> -		goto unroll_hw_init;
>> +		return err;
>>   
>>   	vsi->flags = ICE_VSI_FLAG_INIT;
>>   
>> @@ -1267,8 +1261,6 @@ static int ice_devlink_reinit_up(struct ice_pf *pf)
>>   	rtnl_unlock();
>>   err_vsi_cfg:
>>   	ice_deinit_dev(pf);
>> -unroll_hw_init:
>> -	ice_deinit_hw(&pf->hw);
>>   	return err;
>>   }
>>   
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index 0a11b4281092..c44bb8153ad0 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -4739,6 +4739,12 @@ int ice_init_dev(struct ice_pf *pf)
>>   	struct ice_hw *hw = &pf->hw;
>>   	int err;
>>   
>> +	err = ice_init_hw(hw);
>> +	if (err) {
>> +		dev_err(dev, "ice_init_hw failed: %d\n", err);
>> +		return err;
>> +	}
>> +
>>   	ice_init_feature_support(pf);
>>   
>>   	err = ice_init_ddp_config(hw, pf);
>> @@ -4759,7 +4765,7 @@ int ice_init_dev(struct ice_pf *pf)
>>   	err = ice_init_pf(pf);
>>   	if (err) {
>>   		dev_err(dev, "ice_init_pf failed: %d\n", err);
>> -		return err;
>> +		goto unroll_hw_init;
>>   	}
>>   
>>   	pf->hw.udp_tunnel_nic.set_port = ice_udp_tunnel_set_port;
>> @@ -4803,6 +4809,8 @@ int ice_init_dev(struct ice_pf *pf)
>>   	ice_clear_interrupt_scheme(pf);
>>   unroll_pf_init:
>>   	ice_deinit_pf(pf);
>> +unroll_hw_init:
>> +	ice_deinit_hw(hw);
>>   	return err;
>>   }
>>   
>> @@ -5330,17 +5338,9 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>>   	if (ice_is_recovery_mode(hw))
>>   		return ice_probe_recovery_mode(pf);
>>   
>> -	err = ice_init_hw(hw);
>> -	if (err) {
>> -		dev_err(dev, "ice_init_hw failed: %d\n", err);
>> -		return err;
>> -	}
>> -
>>   	adapter = ice_adapter_get(pdev);
>> -	if (IS_ERR(adapter)) {
>> -		err = PTR_ERR(adapter);
>> -		goto unroll_hw_init;
>> -	}
>> +	if (IS_ERR(adapter))
>> +		return PTR_ERR(adapter);
>>   	pf->adapter = adapter;
>>   
>>   	err = ice_init(pf);
>> @@ -5366,8 +5366,6 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>>   	ice_deinit(pf);
>>   unroll_adapter:
>>   	ice_adapter_put(pdev);
>> -unroll_hw_init:
>> -	ice_deinit_hw(hw);
>>   	return err;
>>   }
>>   
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

RE: [Intel-wired-lan] [PATCH iwl-net 1/2] ice: fix double-call to ice_deinit_hw() during probe failure

[Rinitha, SX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Jacob Keller
> Sent: 17 July 2025 22:27
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Intel Wired LAN <intel-wired-lan@lists.osuosl.org>; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Grinberg, Vitaly <vgrinber@redhat.com>; netdev@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH iwl-net 1/2] ice: fix double-call to ice_deinit_hw() during probe failure
>
> The following (and similar) KFENCE bugs have recently been found occurring during certain error flows of the ice_probe() function:
>
> kernel: ==================================================================
> kernel: BUG: KFENCE: use-after-free read in ice_cleanup_fltr_mgmt_struct+0x1d
> kernel: Use-after-free read at 0x00000000e72fe5ed (in kfence-#223):
> kernel:  ice_cleanup_fltr_mgmt_struct+0x1d/0x200 [ice]
> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
> kernel:  ice_probe+0x245/0x2e0 [ice]
> kernel:
> kernel: kfence-#223: <..snip..>
> kernel: allocated by task 7553 on cpu 0 at 2243.527621s (198.108303s ago):
> kernel:  devm_kmalloc+0x57/0x120
> kernel:  ice_init_hw+0x491/0x8e0 [ice]
> kernel:  ice_probe+0x203/0x2e0 [ice]
> kernel:
> kernel: freed by task 7553 on cpu 0 at 2441.509158s (0.175707s ago):
> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
> kernel:  ice_init+0x1ad/0x570 [ice]
> kernel:  ice_probe+0x22b/0x2e0 [ice]
> kernel:
> kernel: ==================================================================
>
> These occur as the result of a double-call to ice_deinit_hw(). This double call happens if ice_init() fails at any point after calling ice_init_dev().
>
> Upon errors, ice_init() calls ice_deinit_dev(), which is supposed to be the inverse of ice_init_dev(). However, currently ice_init_dev() does not call ice_init_hw(). Instead, ice_init_hw() is called by ice_probe(). Thus,
ice_probe() itself calls ice_deinit_hw() as part of its error cleanup logic.
>
> This results in two calls to ice_deinit_hw() which results in straight forward use-after-free violations due to double calling kfree and other cleanup functions.
>
> To avoid this double call, move the call to ice_init_hw() into ice_init_dev(), and remove the now logically unnecessary cleanup from ice_probe(). This is simpler than the alternative of moving ice_deinit_hw()
*out* of ice_deinit_dev().
>
> Moving the calls to ice_deinit_hw() requires validating all cleanup paths, and changing significantly more code. Moving the calls of ice_init_hw() requires only validating that the new placement is still prior to all HW structure accesses.
>
> For ice_probe(), this now delays ice_init_hw() from before
ice_adapter_get() to just after it. This is safe, as ice_adapter_get() does not rely on the HW structure.
>
> For ice_devlink_reinit_up(), the ice_init_hw() is now called after ice_set_min_max_msix(). This is also safe as that function does not access the HW structure either.
>
> This flow makes more logical sense, as ice_init_dev() is mirrored by ice_deinit_dev(), so it reasonably should be the caller of ice_init_hw().
> It also reduces one extra call to ice_init_hw() since both ice_probe() and
ice_devlink_reinit_up() call ice_init_dev().
>
> This resolves the double-free and avoids memory corruption and other invalid memory accesses in the event of a failed probe.
>
> Fixes: 5b246e533d01 ("ice: split probe into smaller functions")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> drivers/net/ethernet/intel/ice/devlink/devlink.c | 10 +---------
> drivers/net/ethernet/intel/ice/ice_main.c        | 24 +++++++++++-------------
> 2 files changed, 12 insertions(+), 22 deletions(-)
>

Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)

[PATCH iwl-net 2/2] ice: don't leave device non-functional if Tx scheduler config fails

[Jacob Keller]

The ice_cfg_tx_topo function attempts to apply Tx scheduler topology
configuration based on NVM parameters, selecting either a 5 or 9 layer
topology.

As part of this flow, the driver acquires the "Global Configuration Lock",
which is a hardware resource associated with programming the DDP package
to the device. This "lock" is implemented by firmware as a way to
guarantee that only one PF can program the DDP for a device. Unlike a
traditional lock, once a PF has acquired this lock, no other PF will be
able to acquire it again (including that PF) until a CORER of the device.
Future requests to acquire the lock report that global configuration has
already completed.

The following flow is used to program the Tx topology:

 * Read the DDP package for scheduler configuration data
 * Acquire the global configuration lock
 * Program Tx scheduler topology according to DDP package data
 * Trigger a CORER which clears the global configuration lock

This is followed by the flow for programming the DDP package:

 * Acquire the global configuration lock (again)
 * Download the DDP package to the device
 * Release the global configuration lock.

However, if configuration of the Tx topology fails, (i.e.
ice_get_set_tx_topo returns an error code), the driver exits
ice_cfg_tx_topo() immediately, and fails to trigger CORER.

While the global configuration lock is held, the firmware rejects most
AdminQ commands, as it is waiting for the DDP package download (or Tx
scheduler topology programming) to occur.

The current driver flows assume that the global configuration lock has been
reset by CORER after programming the Tx topology. Thus, the same PF
attempts to acquire the global lock again, and fails. This results in the
driver reporting "an unknown error occurred when loading the DDP package".
It then attempts to enter safe mode, but ultimately fails to finish
ice_probe() since nearly all AdminQ command report error codes, and the
driver stops loading the device at some point during its initialization.

The only currently known way that ice_get_set_tx_topo() can fail is with
certain older DDP packages which contain invalid topology configuration, on
firmware versions which strictly validate this data. The most recent
releases of the DDP have resolved the invalid data. However, it is still
poor practice to essentially brick the device, and prevent access to the
device even through safe mode or recovery mode. It is also plausible that
this command could fail for some other reason in the future.

We cannot simply release the global lock after a failed call to
ice_get_set_tx_topo(). Releasing the lock indicates to firmware that global
configuration (downloading of the DDP) has completed. Future attempts by
this or other PFs to load the DDP will fail with a report that the DDP
package has already been downloaded. Then, PFs will enter safe mode as they
realize that the package on the device does not meet the minimum version
requirement to load. The reported error messages are confusing, as they
indicate the version of the default "safe mode" package in the NVM, rather
than the version of the file loaded from /lib/firmware.

Instead, we need to trigger CORER to clear global configuration. This is
the lowest level of hardware reset which clears the global configuration
lock and related state. It also clears any already downloaded DDP.
Crucially, it does *not* clear the Tx scheduler topology configuration.

Refactor ice_cfg_tx_topo() to always trigger a CORER after acquiring the
global lock, regardless of success or failure of the topology
configuration.

We need to re-initialize the HW structure when we trigger the CORER. Thus,
it makes sense for this to be the responsibility of ice_cfg_tx_topo()
rather than its caller, ice_init_tx_topology(). This avoids needless
re-initialization in cases where we don't attempt to update the Tx
scheduler topology, such as if it has already been programmed.

There is one catch: failure to re-initialize the HW struct should stop
ice_probe(). If this function fails, we won't have a valid HW structure and
cannot ensure the device is functioning properly. To handle this, ensure
ice_cfg_tx_topo() returns a limited set of error codes. Set aside one
specifically, -ENODEV, to indicate that the ice_init_tx_topology() should
fail and stop probe.

Other error codes indicate failure to apply the Tx scheduler topology. This
is treated as a non-fatal error, with an informational message informing
the system administrator that the updated Tx topology did not apply. This
allows the device to load and function with the default Tx scheduler
topology, rather than failing to load entirely.

Note that this use of CORER will not result in loops with future PFs
attempting to also load the invalid Tx topology configuration. The first PF
will acquire the global configuration lock as part of programming the DDP.
Each PF after this will attempt to acquire the global lock as part of
programming the Tx topology, and will fail with the indication from
firmware that global configuration is already complete. Tx scheduler
topology configuration is only performed during driver init (probe or
devlink reload) and not during cleanup for a CORER that happens after probe
completes.

Fixes: 91427e6d9030 ("ice: Support 5 layer topology")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ddp.c  | 44 ++++++++++++++++++++++---------
 drivers/net/ethernet/intel/ice/ice_main.c | 14 ++++++----
 2 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
index 59323c019544..bc525de019de 100644
--- a/drivers/net/ethernet/intel/ice/ice_ddp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
@@ -2374,7 +2374,13 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size,
  * The function will apply the new Tx topology from the package buffer
  * if available.
  *
- * Return: zero when update was successful, negative values otherwise.
+ * Return:
+ * * 0 - Successfully applied topology configuration.
+ * * -EBUSY - Failed to acquire global configuration lock.
+ * * -EEXIST - Topology configuration has already been applied.
+ * * -EIO - Unable to apply topology configuration.
+ * * -ENODEV - Failed to re-initialize device after applying configuration.
+ * * Other negative error codes indicate unexpected failures.
  */
 int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len)
 {
@@ -2407,7 +2413,7 @@ int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len)
 
 	if (status) {
 		ice_debug(hw, ICE_DBG_INIT, "Get current topology is failed\n");
-		return status;
+		return -EIO;
 	}
 
 	/* Is default topology already applied ? */
@@ -2494,31 +2500,45 @@ int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len)
 				 ICE_GLOBAL_CFG_LOCK_TIMEOUT);
 	if (status) {
 		ice_debug(hw, ICE_DBG_INIT, "Failed to acquire global lock\n");
-		return status;
+		return -EBUSY;
 	}
 
 	/* Check if reset was triggered already. */
 	reg = rd32(hw, GLGEN_RSTAT);
 	if (reg & GLGEN_RSTAT_DEVSTATE_M) {
-		/* Reset is in progress, re-init the HW again */
 		ice_debug(hw, ICE_DBG_INIT, "Reset is in progress. Layer topology might be applied already\n");
 		ice_check_reset(hw);
-		return 0;
+		/* Reset is in progress, re-init the HW again */
+		goto reinit_hw;
 	}
 
 	/* Set new topology */
 	status = ice_get_set_tx_topo(hw, new_topo, size, NULL, NULL, true);
 	if (status) {
-		ice_debug(hw, ICE_DBG_INIT, "Failed setting Tx topology\n");
-		return status;
+		ice_debug(hw, ICE_DBG_INIT, "Failed to set Tx topology, status %pe\n",
+			  ERR_PTR(status));
+		/* only report -EIO here as the caller checks the error value
+		 * and reports an informational error message informing that
+		 * the driver failed to program Tx topology.
+		 */
+		status = -EIO;
 	}
 
-	/* New topology is updated, delay 1 second before issuing the CORER */
+	/* Even if Tx topology config failed, we need to CORE reset here to
+	 * clear the global configuration lock. Delay 1 second to allow
+	 * hardware to settle then issue a CORER
+	 */
 	msleep(1000);
 	ice_reset(hw, ICE_RESET_CORER);
-	/* CORER will clear the global lock, so no explicit call
-	 * required for release.
-	 */
+	ice_check_reset(hw);
 
-	return 0;
+reinit_hw:
+	/* Since we triggered a CORER, re-initialize hardware */
+	ice_deinit_hw(hw);
+	if (ice_init_hw(hw)) {
+		ice_debug(hw, ICE_DBG_INIT, "Failed to re-init hardware after setting Tx topology\n");
+		return -ENODEV;
+	}
+
+	return status;
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index c44bb8153ad0..b31b3aa2b662 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4532,17 +4532,21 @@ ice_init_tx_topology(struct ice_hw *hw, const struct firmware *firmware)
 			dev_info(dev, "Tx scheduling layers switching feature disabled\n");
 		else
 			dev_info(dev, "Tx scheduling layers switching feature enabled\n");
-		/* if there was a change in topology ice_cfg_tx_topo triggered
-		 * a CORER and we need to re-init hw
+		return 0;
+	} else if (err == -ENODEV) {
+		/* If we failed to re-initialize the device, we can no longer
+		 * continue loading.
 		 */
-		ice_deinit_hw(hw);
-		err = ice_init_hw(hw);
-
+		dev_warn(dev, "Failed to initialize hardware after applying Tx scheduling configuration.\n");
 		return err;
 	} else if (err == -EIO) {
 		dev_info(dev, "DDP package does not support Tx scheduling layers switching feature - please update to the latest DDP package and try again\n");
+		return 0;
 	}
 
+	/* Do not treat this as a fatal error. */
+	dev_info(dev, "Failed to apply Tx scheduling configuration, err %pe\n",
+		 ERR_PTR(err));
 	return 0;
 }
 

-- 
2.50.0.349.ga842a77808e9

Re: [PATCH iwl-net 2/2] ice: don't leave device non-functional if Tx scheduler config fails

[Simon Horman]

On Thu, Jul 17, 2025 at 09:57:09AM -0700, Jacob Keller wrote:
> The ice_cfg_tx_topo function attempts to apply Tx scheduler topology
> configuration based on NVM parameters, selecting either a 5 or 9 layer
> topology.
> 
> As part of this flow, the driver acquires the "Global Configuration Lock",
> which is a hardware resource associated with programming the DDP package
> to the device. This "lock" is implemented by firmware as a way to
> guarantee that only one PF can program the DDP for a device. Unlike a
> traditional lock, once a PF has acquired this lock, no other PF will be
> able to acquire it again (including that PF) until a CORER of the device.
> Future requests to acquire the lock report that global configuration has
> already completed.
> 
> The following flow is used to program the Tx topology:
> 
>  * Read the DDP package for scheduler configuration data
>  * Acquire the global configuration lock
>  * Program Tx scheduler topology according to DDP package data
>  * Trigger a CORER which clears the global configuration lock
> 
> This is followed by the flow for programming the DDP package:
> 
>  * Acquire the global configuration lock (again)
>  * Download the DDP package to the device
>  * Release the global configuration lock.
> 
> However, if configuration of the Tx topology fails, (i.e.
> ice_get_set_tx_topo returns an error code), the driver exits
> ice_cfg_tx_topo() immediately, and fails to trigger CORER.
> 
> While the global configuration lock is held, the firmware rejects most
> AdminQ commands, as it is waiting for the DDP package download (or Tx
> scheduler topology programming) to occur.
> 
> The current driver flows assume that the global configuration lock has been
> reset by CORER after programming the Tx topology. Thus, the same PF
> attempts to acquire the global lock again, and fails. This results in the
> driver reporting "an unknown error occurred when loading the DDP package".
> It then attempts to enter safe mode, but ultimately fails to finish
> ice_probe() since nearly all AdminQ command report error codes, and the
> driver stops loading the device at some point during its initialization.
> 
> The only currently known way that ice_get_set_tx_topo() can fail is with
> certain older DDP packages which contain invalid topology configuration, on
> firmware versions which strictly validate this data. The most recent
> releases of the DDP have resolved the invalid data. However, it is still
> poor practice to essentially brick the device, and prevent access to the
> device even through safe mode or recovery mode. It is also plausible that
> this command could fail for some other reason in the future.
> 
> We cannot simply release the global lock after a failed call to
> ice_get_set_tx_topo(). Releasing the lock indicates to firmware that global
> configuration (downloading of the DDP) has completed. Future attempts by
> this or other PFs to load the DDP will fail with a report that the DDP
> package has already been downloaded. Then, PFs will enter safe mode as they
> realize that the package on the device does not meet the minimum version
> requirement to load. The reported error messages are confusing, as they
> indicate the version of the default "safe mode" package in the NVM, rather
> than the version of the file loaded from /lib/firmware.
> 
> Instead, we need to trigger CORER to clear global configuration. This is
> the lowest level of hardware reset which clears the global configuration
> lock and related state. It also clears any already downloaded DDP.
> Crucially, it does *not* clear the Tx scheduler topology configuration.
> 
> Refactor ice_cfg_tx_topo() to always trigger a CORER after acquiring the
> global lock, regardless of success or failure of the topology
> configuration.
> 
> We need to re-initialize the HW structure when we trigger the CORER. Thus,
> it makes sense for this to be the responsibility of ice_cfg_tx_topo()
> rather than its caller, ice_init_tx_topology(). This avoids needless
> re-initialization in cases where we don't attempt to update the Tx
> scheduler topology, such as if it has already been programmed.
> 
> There is one catch: failure to re-initialize the HW struct should stop
> ice_probe(). If this function fails, we won't have a valid HW structure and
> cannot ensure the device is functioning properly. To handle this, ensure
> ice_cfg_tx_topo() returns a limited set of error codes. Set aside one
> specifically, -ENODEV, to indicate that the ice_init_tx_topology() should
> fail and stop probe.
> 
> Other error codes indicate failure to apply the Tx scheduler topology. This
> is treated as a non-fatal error, with an informational message informing
> the system administrator that the updated Tx topology did not apply. This
> allows the device to load and function with the default Tx scheduler
> topology, rather than failing to load entirely.
> 
> Note that this use of CORER will not result in loops with future PFs
> attempting to also load the invalid Tx topology configuration. The first PF
> will acquire the global configuration lock as part of programming the DDP.
> Each PF after this will attempt to acquire the global lock as part of
> programming the Tx topology, and will fail with the indication from
> firmware that global configuration is already complete. Tx scheduler
> topology configuration is only performed during driver init (probe or
> devlink reload) and not during cleanup for a CORER that happens after probe
> completes.
> 
> Fixes: 91427e6d9030 ("ice: Support 5 layer topology")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Thanks for the extensive explanation.

I have a minor nit below, but that notwithstanding this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>


> ---
>  drivers/net/ethernet/intel/ice/ice_ddp.c  | 44 ++++++++++++++++++++++---------
>  drivers/net/ethernet/intel/ice/ice_main.c | 14 ++++++----
>  2 files changed, 41 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
> index 59323c019544..bc525de019de 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
> @@ -2374,7 +2374,13 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size,
>   * The function will apply the new Tx topology from the package buffer
>   * if available.
>   *
> - * Return: zero when update was successful, negative values otherwise.
> + * Return:
> + * * 0 - Successfully applied topology configuration.
> + * * -EBUSY - Failed to acquire global configuration lock.
> + * * -EEXIST - Topology configuration has already been applied.
> + * * -EIO - Unable to apply topology configuration.
> + * * -ENODEV - Failed to re-initialize device after applying configuration.
> + * * Other negative error codes indicate unexpected failures.
>   */
>  int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len)
>  {
> @@ -2407,7 +2413,7 @@ int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len)
>  
>  	if (status) {
>  		ice_debug(hw, ICE_DBG_INIT, "Get current topology is failed\n");
> -		return status;
> +		return -EIO;
>  	}
>  
>  	/* Is default topology already applied ? */
> @@ -2494,31 +2500,45 @@ int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len)
>  				 ICE_GLOBAL_CFG_LOCK_TIMEOUT);
>  	if (status) {
>  		ice_debug(hw, ICE_DBG_INIT, "Failed to acquire global lock\n");
> -		return status;
> +		return -EBUSY;
>  	}
>  
>  	/* Check if reset was triggered already. */
>  	reg = rd32(hw, GLGEN_RSTAT);
>  	if (reg & GLGEN_RSTAT_DEVSTATE_M) {
> -		/* Reset is in progress, re-init the HW again */
>  		ice_debug(hw, ICE_DBG_INIT, "Reset is in progress. Layer topology might be applied already\n");
>  		ice_check_reset(hw);
> -		return 0;
> +		/* Reset is in progress, re-init the HW again */
> +		goto reinit_hw;
>  	}
>  
>  	/* Set new topology */
>  	status = ice_get_set_tx_topo(hw, new_topo, size, NULL, NULL, true);
>  	if (status) {
> -		ice_debug(hw, ICE_DBG_INIT, "Failed setting Tx topology\n");
> -		return status;
> +		ice_debug(hw, ICE_DBG_INIT, "Failed to set Tx topology, status %pe\n",
> +			  ERR_PTR(status));
> +		/* only report -EIO here as the caller checks the error value
> +		 * and reports an informational error message informing that
> +		 * the driver failed to program Tx topology.
> +		 */
> +		status = -EIO;
>  	}
>  
> -	/* New topology is updated, delay 1 second before issuing the CORER */
> +	/* Even if Tx topology config failed, we need to CORE reset here to
> +	 * clear the global configuration lock. Delay 1 second to allow
> +	 * hardware to settle then issue a CORER
> +	 */
>  	msleep(1000);
>  	ice_reset(hw, ICE_RESET_CORER);
> -	/* CORER will clear the global lock, so no explicit call
> -	 * required for release.
> -	 */
> +	ice_check_reset(hw);
>  
> -	return 0;
> +reinit_hw:

nit: I think you can move this label above ice_check_reset().
     As the only place that jumps to this label calls ice_check_reset()
     immediately before doing so. If so, renaming the label might
     also be appropriate (up to you on all fronts:)

> +	/* Since we triggered a CORER, re-initialize hardware */
> +	ice_deinit_hw(hw);
> +	if (ice_init_hw(hw)) {
> +		ice_debug(hw, ICE_DBG_INIT, "Failed to re-init hardware after setting Tx topology\n");
> +		return -ENODEV;
> +	}
> +
> +	return status;
>  }

...

Re: [PATCH iwl-net 2/2] ice: don't leave device non-functional if Tx scheduler config fails

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 4617 bytes --]



On 7/18/2025 9:50 AM, Simon Horman wrote:
> On Thu, Jul 17, 2025 at 09:57:09AM -0700, Jacob Keller wrote:
>>
>> Fixes: 91427e6d9030 ("ice: Support 5 layer topology")
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> Thanks for the extensive explanation.
> 

Thanks. This took me forever to track down exactly what went wrong,
enough that I had to have the customer send me the card back because we
thought the firmware was unrecoverable and bricked.

> I have a minor nit below, but that notwithstanding this looks good to me.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> 
>> ---
>>  drivers/net/ethernet/intel/ice/ice_ddp.c  | 44 ++++++++++++++++++++++---------
>>  drivers/net/ethernet/intel/ice/ice_main.c | 14 ++++++----
>>  2 files changed, 41 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
>> index 59323c019544..bc525de019de 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
>> @@ -2374,7 +2374,13 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size,
>>   * The function will apply the new Tx topology from the package buffer
>>   * if available.
>>   *
>> - * Return: zero when update was successful, negative values otherwise.
>> + * Return:
>> + * * 0 - Successfully applied topology configuration.
>> + * * -EBUSY - Failed to acquire global configuration lock.
>> + * * -EEXIST - Topology configuration has already been applied.
>> + * * -EIO - Unable to apply topology configuration.
>> + * * -ENODEV - Failed to re-initialize device after applying configuration.
>> + * * Other negative error codes indicate unexpected failures.
>>   */
>>  int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len)
>>  {
>> @@ -2407,7 +2413,7 @@ int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len)
>>  
>>  	if (status) {
>>  		ice_debug(hw, ICE_DBG_INIT, "Get current topology is failed\n");
>> -		return status;
>> +		return -EIO;
>>  	}
>>  
>>  	/* Is default topology already applied ? */
>> @@ -2494,31 +2500,45 @@ int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len)
>>  				 ICE_GLOBAL_CFG_LOCK_TIMEOUT);
>>  	if (status) {
>>  		ice_debug(hw, ICE_DBG_INIT, "Failed to acquire global lock\n");
>> -		return status;
>> +		return -EBUSY;
>>  	}
>>  
>>  	/* Check if reset was triggered already. */
>>  	reg = rd32(hw, GLGEN_RSTAT);
>>  	if (reg & GLGEN_RSTAT_DEVSTATE_M) {
>> -		/* Reset is in progress, re-init the HW again */
>>  		ice_debug(hw, ICE_DBG_INIT, "Reset is in progress. Layer topology might be applied already\n");
>>  		ice_check_reset(hw);
>> -		return 0;
>> +		/* Reset is in progress, re-init the HW again */
>> +		goto reinit_hw;
>>  	}
>>  
>>  	/* Set new topology */
>>  	status = ice_get_set_tx_topo(hw, new_topo, size, NULL, NULL, true);
>>  	if (status) {
>> -		ice_debug(hw, ICE_DBG_INIT, "Failed setting Tx topology\n");
>> -		return status;
>> +		ice_debug(hw, ICE_DBG_INIT, "Failed to set Tx topology, status %pe\n",
>> +			  ERR_PTR(status));
>> +		/* only report -EIO here as the caller checks the error value
>> +		 * and reports an informational error message informing that
>> +		 * the driver failed to program Tx topology.
>> +		 */
>> +		status = -EIO;
>>  	}
>>  
>> -	/* New topology is updated, delay 1 second before issuing the CORER */
>> +	/* Even if Tx topology config failed, we need to CORE reset here to
>> +	 * clear the global configuration lock. Delay 1 second to allow
>> +	 * hardware to settle then issue a CORER
>> +	 */
>>  	msleep(1000);
>>  	ice_reset(hw, ICE_RESET_CORER);
>> -	/* CORER will clear the global lock, so no explicit call
>> -	 * required for release.
>> -	 */
>> +	ice_check_reset(hw);
>>  
>> -	return 0;
>> +reinit_hw:
> 
> nit: I think you can move this label above ice_check_reset().
>      As the only place that jumps to this label calls ice_check_reset()
>      immediately before doing so. If so, renaming the label might
>      also be appropriate (up to you on all fronts:)
> 

You're right thats probably slightly better. I'm not sure its worth a
re-roll vs getting this fix out since its a pretty minor difference.

>> +	/* Since we triggered a CORER, re-initialize hardware */
>> +	ice_deinit_hw(hw);
>> +	if (ice_init_hw(hw)) {
>> +		ice_debug(hw, ICE_DBG_INIT, "Failed to re-init hardware after setting Tx topology\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	return status;
>>  }
> 
> ...


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH iwl-net 2/2] ice: don't leave device non-functional if Tx scheduler config fails

[Simon Horman]

On Fri, Jul 18, 2025 at 12:56:29PM -0700, Jacob Keller wrote:
> 
> 
> On 7/18/2025 9:50 AM, Simon Horman wrote:
> > On Thu, Jul 17, 2025 at 09:57:09AM -0700, Jacob Keller wrote:
> >>
> >> Fixes: 91427e6d9030 ("ice: Support 5 layer topology")
> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > 
> > Thanks for the extensive explanation.
> > 
> 
> Thanks. This took me forever to track down exactly what went wrong,
> enough that I had to have the customer send me the card back because we
> thought the firmware was unrecoverable and bricked.

Ouch!

...

> >>  	msleep(1000);
> >>  	ice_reset(hw, ICE_RESET_CORER);
> >> -	/* CORER will clear the global lock, so no explicit call
> >> -	 * required for release.
> >> -	 */
> >> +	ice_check_reset(hw);
> >>  
> >> -	return 0;
> >> +reinit_hw:
> > 
> > nit: I think you can move this label above ice_check_reset().
> >      As the only place that jumps to this label calls ice_check_reset()
> >      immediately before doing so. If so, renaming the label might
> >      also be appropriate (up to you on all fronts:)
> > 
> 
> You're right thats probably slightly better. I'm not sure its worth a
> re-roll vs getting this fix out since its a pretty minor difference.

Yes, agreed. I'm happy to let this lie if you prefer.

...

RE: [Intel-wired-lan] [PATCH iwl-net 2/2] ice: don't leave device non-functional if Tx scheduler config fails

[Rinitha, SX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Jacob Keller
> Sent: 17 July 2025 22:27
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Intel Wired LAN <intel-wired-lan@lists.osuosl.org>; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Grinberg, Vitaly <vgrinber@redhat.com>; netdev@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH iwl-net 2/2] ice: don't leave device non-functional if Tx scheduler config fails
>
> The ice_cfg_tx_topo function attempts to apply Tx scheduler topology configuration based on NVM parameters, selecting either a 5 or 9 layer topology.
>
> As part of this flow, the driver acquires the "Global Configuration Lock", which is a hardware resource associated with programming the DDP package to the device. This "lock" is implemented by firmware as a way to guarantee that only one PF can program the DDP for a device. Unlike a traditional lock, once a PF has acquired this lock, no other PF will be able to acquire it again (including that PF) until a CORER of the device.
> Future requests to acquire the lock report that global configuration has already completed.
>
> The following flow is used to program the Tx topology:
>
> * Read the DDP package for scheduler configuration data
> * Acquire the global configuration lock
> * Program Tx scheduler topology according to DDP package data
> * Trigger a CORER which clears the global configuration lock
>
> This is followed by the flow for programming the DDP package:
>
> * Acquire the global configuration lock (again)
> * Download the DDP package to the device
> * Release the global configuration lock.
>
> However, if configuration of the Tx topology fails, (i.e.
> ice_get_set_tx_topo returns an error code), the driver exits
> ice_cfg_tx_topo() immediately, and fails to trigger CORER.
>
> While the global configuration lock is held, the firmware rejects most AdminQ commands, as it is waiting for the DDP package download (or Tx scheduler topology programming) to occur.
>
> The current driver flows assume that the global configuration lock has been reset by CORER after programming the Tx topology. Thus, the same PF attempts to acquire the global lock again, and fails. This results in the driver reporting "an unknown error occurred when loading the DDP package".
> It then attempts to enter safe mode, but ultimately fails to finish
> ice_probe() since nearly all AdminQ command report error codes, and the driver stops loading the device at some point during its initialization.
>
> The only currently known way that ice_get_set_tx_topo() can fail is with certain older DDP packages which contain invalid topology configuration, on firmware versions which strictly validate this data. The most recent releases of the DDP have resolved the invalid data. However, it is still poor practice to essentially brick the device, and prevent access to the device even through safe mode or recovery mode. It is also plausible that this command could fail for some other reason in the future.
>
> We cannot simply release the global lock after a failed call to ice_get_set_tx_topo(). Releasing the lock indicates to firmware that global configuration (downloading of the DDP) has completed. Future attempts by this or other PFs to load the DDP will fail with a report that the DDP package has already been downloaded. Then, PFs will enter safe mode as they realize that the package on the device does not meet the minimum version requirement to load. The reported error messages are confusing, as they indicate the version of the default "safe mode" package in the NVM, rather than the version of the file loaded from /lib/firmware.
>
> Instead, we need to trigger CORER to clear global configuration. This is the lowest level of hardware reset which clears the global configuration lock and related state. It also clears any already downloaded DDP.
> Crucially, it does *not* clear the Tx scheduler topology configuration.
>
> Refactor ice_cfg_tx_topo() to always trigger a CORER after acquiring the global lock, regardless of success or failure of the topology configuration.
>
> We need to re-initialize the HW structure when we trigger the CORER. Thus, it makes sense for this to be the responsibility of ice_cfg_tx_topo() rather than its caller, ice_init_tx_topology(). This avoids needless re-initialization in cases where we don't attempt to update the Tx scheduler topology, such as if it has already been programmed.
>
> There is one catch: failure to re-initialize the HW struct should stop ice_probe(). If this function fails, we won't have a valid HW structure and cannot ensure the device is functioning properly. To handle this, ensure
Ice_cfg_tx_topo() returns a limited set of error codes. Set aside one specifically, -ENODEV, to indicate that the ice_init_tx_topology() should fail and stop probe.
>
>
> Other error codes indicate failure to apply the Tx scheduler topology. This is treated as a non-fatal error, with an informational message informing the system administrator that the updated Tx topology did not apply. > This allows the device to load and function with the default Tx scheduler topology, rather than failing to load entirely.
>
> Note that this use of CORER will not result in loops with future PFs attempting to also load the invalid Tx topology configuration. The first PF will acquire the global configuration lock as part of programming the DDP.
> Each PF after this will attempt to acquire the global lock as part of programming the Tx topology, and will fail with the indication from firmware that global configuration is already complete. Tx scheduler topology configuration is only performed during driver init (probe or devlink reload) and not during cleanup for a CORER that happens after probe completes.
>
> Fixes: 91427e6d9030 ("ice: Support 5 layer topology")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice_ddp.c  | 44 ++++++++++++++++++++++---------  drivers/net/ethernet/intel/ice/ice_main.c | 14 ++++++----
> 2 files changed, 41 insertions(+), 17 deletions(-)
>

Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)