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/[email protected]/
>> @@ -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.
