[Intel-wired-lan] [PATCH iwl-net v2 0/2] ixgbe: fix issues around ixgbe_recovery_probe()

[Intel-wired-lan] [PATCH iwl-net v2 0/2] ixgbe: fix issues around ixgbe_recovery_probe()

[Kohei Enju]

ixgbe_recovery_probe() and codes around this function have two bugs:
1. resource freeing up is not complete, resulting in memory leaks
2. mutex lock (hw->aci.lock) is initialized twice

Fix these issues.

Changes:
  v2:
    - let ixgbe_probe() to clean up, instead of ixgbe_recovery_probe()
    - don't initialize aci lock twice
  v1: https://lore.kernel.org/intel-wired-lan/20251206155146.95857-1-enjuk@amazon.com/

Kohei Enju (2):
  ixgbe: fix memory leaks in the ixgbe_recovery_probe() path
  ixgbe: don't initialize aci lock in ixgbe_recovery_probe()

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 26 +++++++------------
 1 file changed, 10 insertions(+), 16 deletions(-)

-- 
2.52.0

[PATCH iwl-net v2 0/2] ixgbe: fix issues around ixgbe_recovery_probe()

[Kohei Enju]

ixgbe_recovery_probe() and codes around this function have two bugs:
1. resource freeing up is not complete, resulting in memory leaks
2. mutex lock (hw->aci.lock) is initialized twice

Fix these issues.

Changes:
  v2:
    - let ixgbe_probe() to clean up, instead of ixgbe_recovery_probe()
    - don't initialize aci lock twice
  v1: https://lore.kernel.org/intel-wired-lan/20251206155146.95857-1-enjuk@amazon.com/

Kohei Enju (2):
  ixgbe: fix memory leaks in the ixgbe_recovery_probe() path
  ixgbe: don't initialize aci lock in ixgbe_recovery_probe()

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 26 +++++++------------
 1 file changed, 10 insertions(+), 16 deletions(-)

-- 
2.52.0

[Intel-wired-lan] [PATCH iwl-net v2 1/2] ixgbe: fix memory leaks in the ixgbe_recovery_probe() path

[Kohei Enju]

When ixgbe_recovery_probe() is invoked and this function fails,
allocated resources in advance are not completely freed, because
ixgbe_probe() returns ixgbe_recovery_probe() directly and
ixgbe_recovery_probe() only frees partial resources, resulting in memory
leaks including:
- adapter->io_addr
- adapter->jump_tables[0]
- adapter->mac_table
- adapter->rss_key
- adapter->af_xdp_zc_qps

The leaked MMIO region can be observed in /proc/vmallocinfo, and the
remaining leaks are reported by kmemleak.

Don't return ixgbe_recovery_probe() directly, and instead let
ixgbe_probe() to clean up resources on failures.

Fixes: 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery mode")
Signed-off-by: Kohei Enju <enjuk@amazon.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 20 ++++++++-----------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 4af3b3e71ff1..85023bb4e5a5 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -11468,14 +11468,12 @@ static void ixgbe_set_fw_version(struct ixgbe_adapter *adapter)
  */
 static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)
 {
-	struct net_device *netdev = adapter->netdev;
 	struct pci_dev *pdev = adapter->pdev;
 	struct ixgbe_hw *hw = &adapter->hw;
-	bool disable_dev;
 	int err = -EIO;
 
 	if (hw->mac.type != ixgbe_mac_e610)
-		goto clean_up_probe;
+		return err;
 
 	ixgbe_get_hw_control(adapter);
 	mutex_init(&hw->aci.lock);
@@ -11507,13 +11505,6 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)
 shutdown_aci:
 	mutex_destroy(&adapter->hw.aci.lock);
 	ixgbe_release_hw_control(adapter);
-clean_up_probe:
-	disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter->state);
-	free_netdev(netdev);
-	devlink_free(adapter->devlink);
-	pci_release_mem_regions(pdev);
-	if (disable_dev)
-		pci_disable_device(pdev);
 	return err;
 }
 
@@ -11655,8 +11646,13 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (err)
 		goto err_sw_init;
 
-	if (ixgbe_check_fw_error(adapter))
-		return ixgbe_recovery_probe(adapter);
+	if (ixgbe_check_fw_error(adapter)) {
+		err = ixgbe_recovery_probe(adapter);
+		if (err)
+			goto err_sw_init;
+
+		return 0;
+	}
 
 	if (adapter->hw.mac.type == ixgbe_mac_e610) {
 		err = ixgbe_get_caps(&adapter->hw);
-- 
2.52.0

[PATCH iwl-net v2 1/2] ixgbe: fix memory leaks in the ixgbe_recovery_probe() path

[Kohei Enju]

When ixgbe_recovery_probe() is invoked and this function fails,
allocated resources in advance are not completely freed, because
ixgbe_probe() returns ixgbe_recovery_probe() directly and
ixgbe_recovery_probe() only frees partial resources, resulting in memory
leaks including:
- adapter->io_addr
- adapter->jump_tables[0]
- adapter->mac_table
- adapter->rss_key
- adapter->af_xdp_zc_qps

The leaked MMIO region can be observed in /proc/vmallocinfo, and the
remaining leaks are reported by kmemleak.

Don't return ixgbe_recovery_probe() directly, and instead let
ixgbe_probe() to clean up resources on failures.

Fixes: 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery mode")
Signed-off-by: Kohei Enju <enjuk@amazon.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 20 ++++++++-----------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 4af3b3e71ff1..85023bb4e5a5 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -11468,14 +11468,12 @@ static void ixgbe_set_fw_version(struct ixgbe_adapter *adapter)
  */
 static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)
 {
-	struct net_device *netdev = adapter->netdev;
 	struct pci_dev *pdev = adapter->pdev;
 	struct ixgbe_hw *hw = &adapter->hw;
-	bool disable_dev;
 	int err = -EIO;
 
 	if (hw->mac.type != ixgbe_mac_e610)
-		goto clean_up_probe;
+		return err;
 
 	ixgbe_get_hw_control(adapter);
 	mutex_init(&hw->aci.lock);
@@ -11507,13 +11505,6 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)
 shutdown_aci:
 	mutex_destroy(&adapter->hw.aci.lock);
 	ixgbe_release_hw_control(adapter);
-clean_up_probe:
-	disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter->state);
-	free_netdev(netdev);
-	devlink_free(adapter->devlink);
-	pci_release_mem_regions(pdev);
-	if (disable_dev)
-		pci_disable_device(pdev);
 	return err;
 }
 
@@ -11655,8 +11646,13 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (err)
 		goto err_sw_init;
 
-	if (ixgbe_check_fw_error(adapter))
-		return ixgbe_recovery_probe(adapter);
+	if (ixgbe_check_fw_error(adapter)) {
+		err = ixgbe_recovery_probe(adapter);
+		if (err)
+			goto err_sw_init;
+
+		return 0;
+	}
 
 	if (adapter->hw.mac.type == ixgbe_mac_e610) {
 		err = ixgbe_get_caps(&adapter->hw);
-- 
2.52.0

Re: [Intel-wired-lan] [PATCH iwl-net v2 1/2] ixgbe: fix memory leaks in the ixgbe_recovery_probe() path

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Kohei Enju
> Sent: Thursday, December 11, 2025 10:16 AM
> To: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Jagielski, Jedrzej
> <jedrzej.jagielski@intel.com>; Wegrzyn, Stefan
> <stefan.wegrzyn@intel.com>; Simon Horman <horms@kernel.org>; Keller,
> Jacob E <jacob.e.keller@intel.com>; kohei@enjuk.org; Kohei Enju
> <enjuk@amazon.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net v2 1/2] ixgbe: fix memory
> leaks in the ixgbe_recovery_probe() path
> 
> When ixgbe_recovery_probe() is invoked and this function fails,
> allocated resources in advance are not completely freed, because
> ixgbe_probe() returns ixgbe_recovery_probe() directly and
> ixgbe_recovery_probe() only frees partial resources, resulting in
> memory leaks including:
> - adapter->io_addr
> - adapter->jump_tables[0]
> - adapter->mac_table
> - adapter->rss_key
> - adapter->af_xdp_zc_qps
> 
> The leaked MMIO region can be observed in /proc/vmallocinfo, and the
> remaining leaks are reported by kmemleak.
> 
> Don't return ixgbe_recovery_probe() directly, and instead let
> ixgbe_probe() to clean up resources on failures.
> 
> Fixes: 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery
> mode")
> Signed-off-by: Kohei Enju <enjuk@amazon.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 20 ++++++++----------
> -
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 4af3b3e71ff1..85023bb4e5a5 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -11468,14 +11468,12 @@ static void ixgbe_set_fw_version(struct
> ixgbe_adapter *adapter)
>   */
>  static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)  {
> -	struct net_device *netdev = adapter->netdev;
>  	struct pci_dev *pdev = adapter->pdev;
>  	struct ixgbe_hw *hw = &adapter->hw;
> -	bool disable_dev;
>  	int err = -EIO;
> 
>  	if (hw->mac.type != ixgbe_mac_e610)
> -		goto clean_up_probe;
> +		return err;
You've removed the clean_up_probe: label and its cleanup code (free_netdev, devlink_free, pci_release_mem_regions, pci_disable_device) without providing a proof that ixgbe_probe()'s error path correctly handles all these resources.
I'm afraid this patch may trade one leak for another, or cause double-free issues.


> 
>  	ixgbe_get_hw_control(adapter);
>  	mutex_init(&hw->aci.lock);
> @@ -11507,13 +11505,6 @@ static int ixgbe_recovery_probe(struct
> ixgbe_adapter *adapter)
>  shutdown_aci:
>  	mutex_destroy(&adapter->hw.aci.lock);
>  	ixgbe_release_hw_control(adapter);
> -clean_up_probe:
> -	disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter-
> >state);
> -	free_netdev(netdev);
> -	devlink_free(adapter->devlink);
> -	pci_release_mem_regions(pdev);
> -	if (disable_dev)
> -		pci_disable_device(pdev);
>  	return err;
>  }
> 
> @@ -11655,8 +11646,13 @@ static int ixgbe_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>  	if (err)
>  		goto err_sw_init;
> 
> -	if (ixgbe_check_fw_error(adapter))
> -		return ixgbe_recovery_probe(adapter);
> +	if (ixgbe_check_fw_error(adapter)) {
> +		err = ixgbe_recovery_probe(adapter);
> +		if (err)
> +			goto err_sw_init;
The early return 0; on successful ixgbe_recovery_probe() bypasses the remainder of ixgbe_probe().
The commit message doesn't explain what subsequent initialization steps (if any) are intentionally skipped in recovery mode, or whether this is correct behavior.

> +
> +		return 0;
> +	}
> 
>  	if (adapter->hw.mac.type == ixgbe_mac_e610) {
>  		err = ixgbe_get_caps(&adapter->hw);
> --
> 2.52.0

RE: [Intel-wired-lan] [PATCH iwl-net v2 1/2] ixgbe: fix memory leaks in the ixgbe_recovery_probe() path

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Kohei Enju
> Sent: Thursday, December 11, 2025 10:16 AM
> To: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Jagielski, Jedrzej
> <jedrzej.jagielski@intel.com>; Wegrzyn, Stefan
> <stefan.wegrzyn@intel.com>; Simon Horman <horms@kernel.org>; Keller,
> Jacob E <jacob.e.keller@intel.com>; kohei@enjuk.org; Kohei Enju
> <enjuk@amazon.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net v2 1/2] ixgbe: fix memory
> leaks in the ixgbe_recovery_probe() path
> 
> When ixgbe_recovery_probe() is invoked and this function fails,
> allocated resources in advance are not completely freed, because
> ixgbe_probe() returns ixgbe_recovery_probe() directly and
> ixgbe_recovery_probe() only frees partial resources, resulting in
> memory leaks including:
> - adapter->io_addr
> - adapter->jump_tables[0]
> - adapter->mac_table
> - adapter->rss_key
> - adapter->af_xdp_zc_qps
> 
> The leaked MMIO region can be observed in /proc/vmallocinfo, and the
> remaining leaks are reported by kmemleak.
> 
> Don't return ixgbe_recovery_probe() directly, and instead let
> ixgbe_probe() to clean up resources on failures.
> 
> Fixes: 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery
> mode")
> Signed-off-by: Kohei Enju <enjuk@amazon.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 20 ++++++++----------
> -
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 4af3b3e71ff1..85023bb4e5a5 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -11468,14 +11468,12 @@ static void ixgbe_set_fw_version(struct
> ixgbe_adapter *adapter)
>   */
>  static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)  {
> -	struct net_device *netdev = adapter->netdev;
>  	struct pci_dev *pdev = adapter->pdev;
>  	struct ixgbe_hw *hw = &adapter->hw;
> -	bool disable_dev;
>  	int err = -EIO;
> 
>  	if (hw->mac.type != ixgbe_mac_e610)
> -		goto clean_up_probe;
> +		return err;
You've removed the clean_up_probe: label and its cleanup code (free_netdev, devlink_free, pci_release_mem_regions, pci_disable_device) without providing a proof that ixgbe_probe()'s error path correctly handles all these resources.
I'm afraid this patch may trade one leak for another, or cause double-free issues.


> 
>  	ixgbe_get_hw_control(adapter);
>  	mutex_init(&hw->aci.lock);
> @@ -11507,13 +11505,6 @@ static int ixgbe_recovery_probe(struct
> ixgbe_adapter *adapter)
>  shutdown_aci:
>  	mutex_destroy(&adapter->hw.aci.lock);
>  	ixgbe_release_hw_control(adapter);
> -clean_up_probe:
> -	disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter-
> >state);
> -	free_netdev(netdev);
> -	devlink_free(adapter->devlink);
> -	pci_release_mem_regions(pdev);
> -	if (disable_dev)
> -		pci_disable_device(pdev);
>  	return err;
>  }
> 
> @@ -11655,8 +11646,13 @@ static int ixgbe_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>  	if (err)
>  		goto err_sw_init;
> 
> -	if (ixgbe_check_fw_error(adapter))
> -		return ixgbe_recovery_probe(adapter);
> +	if (ixgbe_check_fw_error(adapter)) {
> +		err = ixgbe_recovery_probe(adapter);
> +		if (err)
> +			goto err_sw_init;
The early return 0; on successful ixgbe_recovery_probe() bypasses the remainder of ixgbe_probe().
The commit message doesn't explain what subsequent initialization steps (if any) are intentionally skipped in recovery mode, or whether this is correct behavior.

> +
> +		return 0;
> +	}
> 
>  	if (adapter->hw.mac.type == ixgbe_mac_e610) {
>  		err = ixgbe_get_caps(&adapter->hw);
> --
> 2.52.0

Re: [Intel-wired-lan] [PATCH iwl-net v2 1/2] ixgbe: fix memory leaks in the ixgbe_recovery_probe() path

[Kohei Enju]

On Thu, 11 Dec 2025 10:13:09 +0000, Loktionov, Aleksandr wrote:

>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 4af3b3e71ff1..85023bb4e5a5 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -11468,14 +11468,12 @@ static void ixgbe_set_fw_version(struct
>> ixgbe_adapter *adapter)
>>   */
>>  static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)  {
>> -	struct net_device *netdev = adapter->netdev;
>>  	struct pci_dev *pdev = adapter->pdev;
>>  	struct ixgbe_hw *hw = &adapter->hw;
>> -	bool disable_dev;
>>  	int err = -EIO;
>> 
>>  	if (hw->mac.type != ixgbe_mac_e610)
>> -		goto clean_up_probe;
>> +		return err;
>You've removed the clean_up_probe: label and its cleanup code (free_netdev, devlink_free, pci_release_mem_regions, pci_disable_device) without providing a proof that ixgbe_probe()'s error path correctly handles all these resources.
>I'm afraid this patch may trade one leak for another, or cause double-free issues.

Hi Alex, thank you for taking a look.

First, ixgbe_recovery_probe() is a static function and is only called
from ixgbe_probe(), so potential affected scope is limited to the
ixgbe_probe() function (at least for now). Also I don't think
ixgbe_recovery_probe() is a common util function which would be used in
other functions than ixgbe_probe().

Also I changed ixgbe_probe() to cleanup those resources when
ixgbe_recovery_probe() fails to keep consistency, just because those
resources are allocated by ixgbe_probe(), not by ixgbe_recovery_probe().

Considering the conversation I had in v1 patch [1], I think this change
would be acceptable, and rather preferable to reduce the possibility of
future regression.

[1] https://lore.kernel.org/all/b5787c94-2ad0-4519-9cdb-5e82acfebe05@intel.com/

>> @@ -11655,8 +11646,13 @@ static int ixgbe_probe(struct pci_dev *pdev,
>> const struct pci_device_id *ent)
>>  	if (err)
>>  		goto err_sw_init;
>> 
>> -	if (ixgbe_check_fw_error(adapter))
>> -		return ixgbe_recovery_probe(adapter);
>> +	if (ixgbe_check_fw_error(adapter)) {
>> +		err = ixgbe_recovery_probe(adapter);
>> +		if (err)
>> +			goto err_sw_init;
>The early return 0; on successful ixgbe_recovery_probe() bypasses the remainder of ixgbe_probe().
>The commit message doesn't explain what subsequent initialization steps (if any) are intentionally skipped in recovery mode, or whether this is correct behavior.

On successful path, ixgbe_probe() just returns ixgbe_recovery_probe()
and don't execute following codes, so there's no change of behavior.

Re: RE: [Intel-wired-lan] [PATCH iwl-net v2 1/2] ixgbe: fix memory leaks in the ixgbe_recovery_probe() path

[Kohei Enju]

On Thu, 11 Dec 2025 10:13:09 +0000, Loktionov, Aleksandr wrote:

>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 4af3b3e71ff1..85023bb4e5a5 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -11468,14 +11468,12 @@ static void ixgbe_set_fw_version(struct
>> ixgbe_adapter *adapter)
>>   */
>>  static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)  {
>> -	struct net_device *netdev = adapter->netdev;
>>  	struct pci_dev *pdev = adapter->pdev;
>>  	struct ixgbe_hw *hw = &adapter->hw;
>> -	bool disable_dev;
>>  	int err = -EIO;
>> 
>>  	if (hw->mac.type != ixgbe_mac_e610)
>> -		goto clean_up_probe;
>> +		return err;
>You've removed the clean_up_probe: label and its cleanup code (free_netdev, devlink_free, pci_release_mem_regions, pci_disable_device) without providing a proof that ixgbe_probe()'s error path correctly handles all these resources.
>I'm afraid this patch may trade one leak for another, or cause double-free issues.

Hi Alex, thank you for taking a look.

First, ixgbe_recovery_probe() is a static function and is only called
from ixgbe_probe(), so potential affected scope is limited to the
ixgbe_probe() function (at least for now). Also I don't think
ixgbe_recovery_probe() is a common util function which would be used in
other functions than ixgbe_probe().

Also I changed ixgbe_probe() to cleanup those resources when
ixgbe_recovery_probe() fails to keep consistency, just because those
resources are allocated by ixgbe_probe(), not by ixgbe_recovery_probe().

Considering the conversation I had in v1 patch [1], I think this change
would be acceptable, and rather preferable to reduce the possibility of
future regression.

[1] https://lore.kernel.org/all/b5787c94-2ad0-4519-9cdb-5e82acfebe05@intel.com/

>> @@ -11655,8 +11646,13 @@ static int ixgbe_probe(struct pci_dev *pdev,
>> const struct pci_device_id *ent)
>>  	if (err)
>>  		goto err_sw_init;
>> 
>> -	if (ixgbe_check_fw_error(adapter))
>> -		return ixgbe_recovery_probe(adapter);
>> +	if (ixgbe_check_fw_error(adapter)) {
>> +		err = ixgbe_recovery_probe(adapter);
>> +		if (err)
>> +			goto err_sw_init;
>The early return 0; on successful ixgbe_recovery_probe() bypasses the remainder of ixgbe_probe().
>The commit message doesn't explain what subsequent initialization steps (if any) are intentionally skipped in recovery mode, or whether this is correct behavior.

On successful path, ixgbe_probe() just returns ixgbe_recovery_probe()
and don't execute following codes, so there's no change of behavior.

[Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't initialize aci lock in ixgbe_recovery_probe()

[Kohei Enju]

hw->aci.lock is already initialized in ixgbe_sw_init(), so
ixgbe_recovery_probe() doesn't need to initialize the lock. This
function is also not responsible for destroying the lock on failures.

Additionally, change the name of label in accordance with this change.

Fixes: 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery mode")
Reported-by: Simon Horman <horms@kernel.org>
Closes: https://lore.kernel.org/intel-wired-lan/aTcFhoH-z2btEKT-@horms.kernel.org/
Signed-off-by: Kohei Enju <enjuk@amazon.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 85023bb4e5a5..b5de8a218424 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -11476,10 +11476,9 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)
 		return err;
 
 	ixgbe_get_hw_control(adapter);
-	mutex_init(&hw->aci.lock);
 	err = ixgbe_get_flash_data(&adapter->hw);
 	if (err)
-		goto shutdown_aci;
+		goto err_release_hw_control;
 
 	timer_setup(&adapter->service_timer, ixgbe_service_timer, 0);
 	INIT_WORK(&adapter->service_task, ixgbe_recovery_service_task);
@@ -11502,8 +11501,7 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)
 	devl_unlock(adapter->devlink);
 
 	return 0;
-shutdown_aci:
-	mutex_destroy(&adapter->hw.aci.lock);
+err_release_hw_control:
 	ixgbe_release_hw_control(adapter);
 	return err;
 }
-- 
2.52.0

[PATCH iwl-net v2 2/2] ixgbe: don't initialize aci lock in ixgbe_recovery_probe()

[Kohei Enju]

hw->aci.lock is already initialized in ixgbe_sw_init(), so
ixgbe_recovery_probe() doesn't need to initialize the lock. This
function is also not responsible for destroying the lock on failures.

Additionally, change the name of label in accordance with this change.

Fixes: 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery mode")
Reported-by: Simon Horman <horms@kernel.org>
Closes: https://lore.kernel.org/intel-wired-lan/aTcFhoH-z2btEKT-@horms.kernel.org/
Signed-off-by: Kohei Enju <enjuk@amazon.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 85023bb4e5a5..b5de8a218424 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -11476,10 +11476,9 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)
 		return err;
 
 	ixgbe_get_hw_control(adapter);
-	mutex_init(&hw->aci.lock);
 	err = ixgbe_get_flash_data(&adapter->hw);
 	if (err)
-		goto shutdown_aci;
+		goto err_release_hw_control;
 
 	timer_setup(&adapter->service_timer, ixgbe_service_timer, 0);
 	INIT_WORK(&adapter->service_task, ixgbe_recovery_service_task);
@@ -11502,8 +11501,7 @@ static int ixgbe_recovery_probe(struct ixgbe_adapter *adapter)
 	devl_unlock(adapter->devlink);
 
 	return 0;
-shutdown_aci:
-	mutex_destroy(&adapter->hw.aci.lock);
+err_release_hw_control:
 	ixgbe_release_hw_control(adapter);
 	return err;
 }
-- 
2.52.0

Re: [Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't initialize aci lock in ixgbe_recovery_probe()

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Kohei Enju
> Sent: Thursday, December 11, 2025 10:16 AM
> To: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Jagielski, Jedrzej
> <jedrzej.jagielski@intel.com>; Wegrzyn, Stefan
> <stefan.wegrzyn@intel.com>; Simon Horman <horms@kernel.org>; Keller,
> Jacob E <jacob.e.keller@intel.com>; kohei@enjuk.org; Kohei Enju
> <enjuk@amazon.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't
> initialize aci lock in ixgbe_recovery_probe()
> 
> hw->aci.lock is already initialized in ixgbe_sw_init(), so
> ixgbe_recovery_probe() doesn't need to initialize the lock. This
You claim that ixgbe_sw_init() initializes hw->aci.lock but don't provide evidence(s).
Can you?


> function is also not responsible for destroying the lock on failures.
> 
> Additionally, change the name of label in accordance with this change.
> 
> Fixes: 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery
> mode")
> Reported-by: Simon Horman <horms@kernel.org>
> Closes: https://lore.kernel.org/intel-wired-lan/aTcFhoH-z2btEKT-
> @horms.kernel.org/
> Signed-off-by: Kohei Enju <enjuk@amazon.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 85023bb4e5a5..b5de8a218424 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -11476,10 +11476,9 @@ static int ixgbe_recovery_probe(struct
> ixgbe_adapter *adapter)
>  		return err;
> 
>  	ixgbe_get_hw_control(adapter);
> -	mutex_init(&hw->aci.lock);
>  	err = ixgbe_get_flash_data(&adapter->hw);
>  	if (err)
> -		goto shutdown_aci;
> +		goto err_release_hw_control;
> 
>  	timer_setup(&adapter->service_timer, ixgbe_service_timer, 0);
>  	INIT_WORK(&adapter->service_task, ixgbe_recovery_service_task);
> @@ -11502,8 +11501,7 @@ static int ixgbe_recovery_probe(struct
> ixgbe_adapter *adapter)
>  	devl_unlock(adapter->devlink);
> 
>  	return 0;
> -shutdown_aci:
> -	mutex_destroy(&adapter->hw.aci.lock);
> +err_release_hw_control:
>  	ixgbe_release_hw_control(adapter);
>  	return err;
>  }
> --
> 2.52.0

RE: [Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't initialize aci lock in ixgbe_recovery_probe()

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Kohei Enju
> Sent: Thursday, December 11, 2025 10:16 AM
> To: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Jagielski, Jedrzej
> <jedrzej.jagielski@intel.com>; Wegrzyn, Stefan
> <stefan.wegrzyn@intel.com>; Simon Horman <horms@kernel.org>; Keller,
> Jacob E <jacob.e.keller@intel.com>; kohei@enjuk.org; Kohei Enju
> <enjuk@amazon.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't
> initialize aci lock in ixgbe_recovery_probe()
> 
> hw->aci.lock is already initialized in ixgbe_sw_init(), so
> ixgbe_recovery_probe() doesn't need to initialize the lock. This
You claim that ixgbe_sw_init() initializes hw->aci.lock but don't provide evidence(s).
Can you?


> function is also not responsible for destroying the lock on failures.
> 
> Additionally, change the name of label in accordance with this change.
> 
> Fixes: 29cb3b8d95c7 ("ixgbe: add E610 implementation of FW recovery
> mode")
> Reported-by: Simon Horman <horms@kernel.org>
> Closes: https://lore.kernel.org/intel-wired-lan/aTcFhoH-z2btEKT-
> @horms.kernel.org/
> Signed-off-by: Kohei Enju <enjuk@amazon.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 85023bb4e5a5..b5de8a218424 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -11476,10 +11476,9 @@ static int ixgbe_recovery_probe(struct
> ixgbe_adapter *adapter)
>  		return err;
> 
>  	ixgbe_get_hw_control(adapter);
> -	mutex_init(&hw->aci.lock);
>  	err = ixgbe_get_flash_data(&adapter->hw);
>  	if (err)
> -		goto shutdown_aci;
> +		goto err_release_hw_control;
> 
>  	timer_setup(&adapter->service_timer, ixgbe_service_timer, 0);
>  	INIT_WORK(&adapter->service_task, ixgbe_recovery_service_task);
> @@ -11502,8 +11501,7 @@ static int ixgbe_recovery_probe(struct
> ixgbe_adapter *adapter)
>  	devl_unlock(adapter->devlink);
> 
>  	return 0;
> -shutdown_aci:
> -	mutex_destroy(&adapter->hw.aci.lock);
> +err_release_hw_control:
>  	ixgbe_release_hw_control(adapter);
>  	return err;
>  }
> --
> 2.52.0

Re: [Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't initialize aci lock in ixgbe_recovery_probe()

[Kohei Enju]

On Thu, 11 Dec 2025 10:12:19 +0000, Loktionov, Aleksandr wrote:

>> Subject: [Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't
>> initialize aci lock in ixgbe_recovery_probe()
>> 
>> hw->aci.lock is already initialized in ixgbe_sw_init(), so
>> ixgbe_recovery_probe() doesn't need to initialize the lock. This
>You claim that ixgbe_sw_init() initializes hw->aci.lock but don't provide evidence(s).
>Can you?

Hi Alex, thank you for reviewing!

Yeah, I claim that because currently ixgbe_recovery_probe() is only
called from ixgbe_probe(), and this is called after ixgbe_sw_init().
Also I don't expect ixgbe_recovery_probe() would be called from other
contexts in the future.

We confirmed the that double initialization would occur in the
context[1], but are there any recommended solutions we can adopt?

I understand that double initialization doesn't always introduce
realistic issue because it would be problematic only when reinialization
is done while the lock is held, but it's a fact that actually
unnecessary initialization is done in ixgbe_recovery_probe().

I believe this change would be right, but maybe we should ask Jedrzej
for the intention of mutex_init() in ixgbe_recovery_probe(), and
possibility that ixgbe_recovery_probe() would be called from any other
contexts.

[1] https://lore.kernel.org/all/b5787c94-2ad0-4519-9cdb-5e82acfebe05@intel.com/

Re: RE: [Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't initialize aci lock in ixgbe_recovery_probe()

[Kohei Enju]

On Thu, 11 Dec 2025 10:12:19 +0000, Loktionov, Aleksandr wrote:

>> Subject: [Intel-wired-lan] [PATCH iwl-net v2 2/2] ixgbe: don't
>> initialize aci lock in ixgbe_recovery_probe()
>> 
>> hw->aci.lock is already initialized in ixgbe_sw_init(), so
>> ixgbe_recovery_probe() doesn't need to initialize the lock. This
>You claim that ixgbe_sw_init() initializes hw->aci.lock but don't provide evidence(s).
>Can you?

Hi Alex, thank you for reviewing!

Yeah, I claim that because currently ixgbe_recovery_probe() is only
called from ixgbe_probe(), and this is called after ixgbe_sw_init().
Also I don't expect ixgbe_recovery_probe() would be called from other
contexts in the future.

We confirmed the that double initialization would occur in the
context[1], but are there any recommended solutions we can adopt?

I understand that double initialization doesn't always introduce
realistic issue because it would be problematic only when reinialization
is done while the lock is held, but it's a fact that actually
unnecessary initialization is done in ixgbe_recovery_probe().

I believe this change would be right, but maybe we should ask Jedrzej
for the intention of mutex_init() in ixgbe_recovery_probe(), and
possibility that ixgbe_recovery_probe() would be called from any other
contexts.

[1] https://lore.kernel.org/all/b5787c94-2ad0-4519-9cdb-5e82acfebe05@intel.com/