Re: [PATCH wireless 1/2] wifi: iwlwifi: pcie: arm the product reset at probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Silent disarm failure unintentionally upgrades a function reset to a product reset, causing an unhandled surprise removal of the Bluetooth device and bypassing ME protections.
- [Medium] Unconditional error log emitted during probe on platforms lacking the product reset _DSM.
--

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

wifi: iwlwifi: pcie: arm the product reset at probe

This patch arms the platform-level product reset via ACPI _DSM unconditionally
during probe instead of device teardown. This is intended to fix recovery
failures on laptops where the device loses power and cannot respond to the
_DSM when the reset is requested later.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=221695

> Arm it during probe instead, while the device still answers. The mode is
> sticky in the platform's namespace, so it is still set when the device
> later disappears. _RST is only evaluated when the driver actually asks
> for a reset, and a subsequent non-product reset re-runs the existing
> disarm path as before. On platforms with no such _DSM this is a no-op.

[Severity: Medium]
Will this actually be a no-op on platforms without the _DSM?

Looking at iwl_trans_pcie_set_product_reset(), if the _DSM fails and enable is
true, it prints an error:

	if (IS_ERR(res)) {
		if (enable)
			IWL_ERR_DEV(&pdev->dev,
				    "ACPI _DSM not available (%d), cannot do product reset\n",
				    (int)PTR_ERR(res));
		return;
	}

Because this is now called unconditionally during probe with enable=true, won't
this spam dmesg with an error on every boot for unsupported platforms?

> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
> index 28b276c659526..a932e068cd0a5 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
> @@ -4252,6 +4252,11 @@ int iwl_pci_gen1_2_probe(struct pci_dev *pdev,
>  	iwl_trans_pcie_check_product_reset_status(pdev);
>  	iwl_trans_pcie_check_product_reset_mode(pdev);
>  
> +	/* must be armed while the device still answers: the _DSM is gated on
> +	 * reading its PCI ID out of config space
> +	 */
> +	iwl_trans_pcie_set_product_reset(pdev, true, mac_cfg->integrated);

[Severity: High]
Does arming the sticky reset here unintentionally upgrade future function resets
to product resets if the device is lost?

If the device falls off the bus (e.g. D3cold), the driver requests a reset.
If Intel ME is present, iwl_trans_pcie_reset() forcibly downgrades a PROD_RESET
to a FUNC_RESET.

When iwl_trans_pcie_removal_wk() later tries to disarm the reset:

	iwl_trans_pcie_set_product_reset(pdev,
					 removal->mode ==
						IWL_RESET_MODE_PROD_RESET,
					 removal->integrated);

The _DSM call will fail because the device is inaccessible.
iwl_trans_pcie_set_product_reset() ignores this failure silently when
enable=false.

Then the worker proceeds to call _RST:

	if (removal->mode >= IWL_RESET_MODE_FUNC_RESET)
		iwl_trans_pcie_call_reset(pdev);

Because the disarm failed, the sticky product reset armed here at probe is
still active. Won't this cause the platform to execute an unexpected product
reset, skipping the companion Bluetooth device teardown and bypassing ME
protections?

> +
>  	/* set the things we know so far for the grab NIC access */
>  	iwl_trans_set_info(iwl_trans, &info);
>

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