Re: [PATCH wireless v2 3/3] wifi: iwlwifi: pcie: recover a device that lost power in D3cold

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Time-of-check to time-of-use (TOCTOU) race condition when checking and setting STATUS_TRANS_DEAD in iwl_trans_pcie_reset.
--

commit 6f5f8c9b1441f8d8ad9817104244243beaa97d1d
Author: Navon John Lukose <navonjohnlukose@gmail.com>

wifi: iwlwifi: pcie: recover a device that lost power in D3cold
    
This commit recovers discrete modules that lose power in D3cold by arming a
product reset via DSM during suspend. If the device does not respond upon
resume, it falls back to a software reset via iwl_trans_pcie_reset().

Link: https://bugzilla.kernel.org/show_bug.cgi?id=221695
Link: https://lore.kernel.org/all/20260722021321.68902-1-nika@nikableh.moe/
Link: https://lore.kernel.org/all/20260829093922.37103-1-navonjohnlukose@gmail.com/

> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> @@ -1229,6 +1237,22 @@ static int _iwl_pci_resume(struct device *device, bool restore)

[ ... ]

> +	if (trans_pcie->prod_reset_set) {
> +		iwl_trans_pcie_arm_product_reset(trans, false);
> +		if (trans_pcie->prod_reset_set &&
> +		    iwl_read32(trans, CSR_HW_REV) == ~0U) {
> +			IWL_ERR(trans, "device not responding after resume\n");
> +			iwl_trans_pcie_reset(trans, IWL_RESET_MODE_PROD_RESET);

[Severity: High]
This is a pre-existing issue, but can this new call to iwl_trans_pcie_reset()
trigger a time-of-check to time-of-use (TOCTOU) race condition with concurrent
callers?

Looking at iwl_trans_pcie_reset() in
drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c, the function checks
the STATUS_TRANS_DEAD bit:

    if (test_bit(STATUS_TRANS_DEAD, &trans->status))
        return;

Then there is a wide window involving memory allocation and CSR dumps before
the bit is actually set:

    set_bit(STATUS_TRANS_DEAD, &trans->status);

If this resume path executes concurrently with another error handler (like an
interrupt or timer), both threads could pass the test_bit() check before
either sets it.

Could this result in two iwl_trans_pcie_removal_wk work items being queued
for the same device?

The first worker would remove the device, free its state, and rescan the bus,
creating a new pdev and driver instance. The second worker would then operate
on the old pdev reference, potentially calling pci_reset_function() which
issues a hardware FLR and restores the old PCI config state over the new
driver's device.

> +			return 0;
> +		}
> +		trans_pcie->prod_reset_set = false;
> +	}
> +
>  	if (!trans->op_mode)
>  		return 0;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831130332.323549-1-navonjohnlukose@gmail.com?part=3