wifi: iwlwifi: recover the BE200 after D3cold removes its power

On a Lenovo Yoga Pro 7 14IAH10 the Intel Wi-Fi 7 BE200 (8086:272b) does not survive D3cold. _PR3 removes the M.2 module's rail, and when the rail and PERST# come back the card never restarts. The link does not train and config space reads 0xffffffff until reboot.

Nothing here is BE200-specific. Patch 4's arming is gated on !integrated, and the AML gate on this board also accepts 0x2526/0x271b (9000 series), 0x2723 (AX200) and 0x2725 (AX210). BE200 is the only one I have and the only one I tested.

  Patch 1  cancel the ME recheck work on the probe error path.
  Patch 2  stop inferring CSME presence from a read that never landed.
  Patch 3  deselect the product reset mode at probe.
  Patch 4  arm the mode in .suspend, disarm in .resume, and reset when the

disarm fails and the device reads all ones.

+60/-10. Patches 1 to 3 carry Fixes: and Cc: stable. Patch 4 has no Fixes:
and is Cc: stable+noautosel, because the device dying in D3cold is not a regression from any commit. The driver simply never handled it.

The DMI quirk patch 4 Links to disables D3cold on the affected machines, which keeps the module's rail up for every runtime idle from then on. This keeps D3cold and pays ~7 s on the resumes that fail. Patches 1 to 3 stand alone if you would rather take the quirk.

Tested on one machine, one BIOS, discrete only, with s2idle and hibernate. s2idle was broken before this and is fixed now. Hibernate was never broken and still works with the arming in .freeze. Methodology and the untested surface are under the --- of patch 4.

Since v2:
https://lore.kernel.org/all/[email protected]/

- The ME recheck cancel, sent on its own before, is folded in as patch 1 so patch 2 does not widen the use-after-free it closes.
- Bjorn: the new tests use PCI_POSSIBLE_ERROR().
- Ilpo: str_enable_disable(). The same line now prints the error with %pe.

v1: https://lore.kernel.org/all/[email protected]/

Navon John Lukose (4):
wifi: iwlwifi: pcie: cancel the ME recheck work on probe failure
wifi: iwlwifi: pcie: don't infer CSME presence from a failed read
wifi: iwlwifi: pcie: deselect the product reset mode at probe
wifi: iwlwifi: pcie: recover a device that lost power in D3cold

 drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 24 +++++++++++
 .../intel/iwlwifi/pcie/gen1_2/internal.h      |  4 ++
 .../intel/iwlwifi/pcie/gen1_2/trans.c         | 42 ++++++++++++++-----
 3 files changed, 60 insertions(+), 10 deletions(-)

Interdiff against v2:

index 5d01a4d..80d6c25 100644
@@ -1245,7 +1245,7 @@ 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) {
+		    PCI_POSSIBLE_ERROR(iwl_read32(trans, CSR_HW_REV))) {
 			IWL_ERR(trans, "device not responding after resume\n");
 			iwl_trans_pcie_reset(trans, IWL_RESET_MODE_PROD_RESET);
 			return 0;

index 56eb35d..a764c6d 100644
@@ -5,6 +5,7 @@
  * Copyright (C) 2016-2017 Intel Deutschland GmbH
  */
 #include <linux/pci.h>
+#include <linux/string_choices.h>
 #include <linux/interrupt.h>
 #include <linux/debugfs.h>
 #include <linux/sched.h>

@@ -2090,8 +2091,8 @@ static bool iwl_trans_pcie_set_product_reset(struct pci_dev *pdev, bool enable,
 						 mode);
 	if (IS_ERR(res)) {
 		IWL_DEBUG_DEV_POWER(&pdev->dev,
-				    "can't %sable product reset via DSM (%d)\n",
-				    enable ? "en" : "dis", (int)PTR_ERR(res));
+				    "can't %s product reset via DSM (%pe)\n",
+				    str_enable_disable(enable), res);
 		return false;
 	}
 

@@ -4206,7 +4207,7 @@ static void iwl_pcie_recheck_me_status(struct work_struct *wk)
 	u32 val;
 
 	val = iwl_read32(trans_pcie->trans, CSR_HW_IF_CONFIG_REG);
-	if (val != ~0U)
+	if (!PCI_POSSIBLE_ERROR(val))
 		trans_pcie->me_present = !!(val & CSR_HW_IF_CONFIG_REG_IAMT_UP);
 }
 

@@ -4228,7 +4229,7 @@ static void iwl_pcie_check_me_status(struct iwl_trans *trans)
 	/* iwl_read_prph() returns 0x5a5a5a5a if it never reached the NIC, and
 	 * that value has WIAMT_KNOWN set and WIAMT_PRESENT clear
 	 */
-	if (val != ~0U && !iwl_trans_is_hw_error_value(val) &&
+	if (!PCI_POSSIBLE_ERROR(val) && !iwl_trans_is_hw_error_value(val) &&
 	    (val & CNVI_SCU_REG_FOR_ECO_1_WIAMT_KNOWN)) {
 		trans_pcie->me_present =
 			!!(val & CNVI_SCU_REG_FOR_ECO_1_WIAMT_PRESENT);

@@ -4236,8 +4237,8 @@ static void iwl_pcie_check_me_status(struct iwl_trans *trans)
 	}
 
 	val = iwl_read32(trans, CSR_HW_IF_CONFIG_REG);
-	if (val != ~0U && (val & (CSR_HW_IF_CONFIG_REG_ME_OWN |
-				  CSR_HW_IF_CONFIG_REG_IAMT_UP))) {
+	if (!PCI_POSSIBLE_ERROR(val) && (val & (CSR_HW_IF_CONFIG_REG_ME_OWN |
+					       CSR_HW_IF_CONFIG_REG_IAMT_UP))) {
 		trans_pcie->me_present = 1;
 		return;
 	}

@@ -4388,6 +4389,8 @@ int iwl_pci_gen1_2_probe(struct pci_dev *pdev,
 
 	if (IS_ERR(iwl_trans->drv)) {
 		ret = PTR_ERR(iwl_trans->drv);
+		/* iwl_pcie_check_me_status() may have armed this */
+		cancel_delayed_work_sync(&trans_pcie->me_recheck_wk);
 		goto out_free_trans;
 	}
 

iwl_pcie_check_me_status() schedules me_recheck_wk a second later. If iwl_drv_start() then fails, the probe error path frees the trans without cancelling it and the callback runs on freed memory. iwl_pcie_gen1_2_remove() already cancels it; do the same here.

Sashiko's review of v2 1/3 pointed this out.

---
 drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c | 2 ++
 1 file changed, 2 insertions(+)

index 28b276c..a30854d 100644
@@ -4369,6 +4369,8 @@ int iwl_pci_gen1_2_probe(struct pci_dev *pdev,
 
 	if (IS_ERR(iwl_trans->drv)) {
 		ret = PTR_ERR(iwl_trans->drv);
+		/* iwl_pcie_check_me_status() may have armed this */
+		cancel_delayed_work_sync(&trans_pcie->me_recheck_wk);
 		goto out_free_trans;
 	}
 

iwl_read_prph() returns 0x5a5a5a5a when it cannot grab NIC access, which reads as "no CSME"; an all-ones CSR_HW_IF_CONFIG_REG reads as "CSME present". me_present is never recomputed and gates the reset ladder, so one bad read skews it for good. Reject both values. This does downgrade a product reset that a poisoned read used to permit.

---
Applies as-is to 6.18.y and 7.2.y.

 .../net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c  | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

index a30854d..e5edcc2 100644
@@ -4194,7 +4194,8 @@ static void iwl_pcie_recheck_me_status(struct work_struct *wk)
 	u32 val;
 
 	val = iwl_read32(trans_pcie->trans, CSR_HW_IF_CONFIG_REG);
-	trans_pcie->me_present = !!(val & CSR_HW_IF_CONFIG_REG_IAMT_UP);
+	if (!PCI_POSSIBLE_ERROR(val))
+		trans_pcie->me_present = !!(val & CSR_HW_IF_CONFIG_REG_IAMT_UP);
 }
 
 static void iwl_pcie_check_me_status(struct iwl_trans *trans)

@@ -4212,15 +4213,19 @@ static void iwl_pcie_check_me_status(struct iwl_trans *trans)
 		return;
 
 	val = iwl_read_prph(trans, CNVI_SCU_REG_FOR_ECO_1);
-	if (val & CNVI_SCU_REG_FOR_ECO_1_WIAMT_KNOWN) {
+	/* iwl_read_prph() returns 0x5a5a5a5a if it never reached the NIC, and
+	 * that value has WIAMT_KNOWN set and WIAMT_PRESENT clear
+	 */
+	if (!PCI_POSSIBLE_ERROR(val) && !iwl_trans_is_hw_error_value(val) &&
+	    (val & CNVI_SCU_REG_FOR_ECO_1_WIAMT_KNOWN)) {
 		trans_pcie->me_present =
 			!!(val & CNVI_SCU_REG_FOR_ECO_1_WIAMT_PRESENT);
 		return;
 	}
 
 	val = iwl_read32(trans, CSR_HW_IF_CONFIG_REG);
-	if (val & (CSR_HW_IF_CONFIG_REG_ME_OWN |
-		   CSR_HW_IF_CONFIG_REG_IAMT_UP)) {
+	if (!PCI_POSSIBLE_ERROR(val) && (val & (CSR_HW_IF_CONFIG_REG_ME_OWN |
+					       CSR_HW_IF_CONFIG_REG_IAMT_UP))) {
 		trans_pcie->me_present = 1;
 		return;
 	}

The product reset mode lives in the platform's ACPI namespace, so it outlives the trans that set it. _RST branches on it and does not clear it, and the deselect in iwl_trans_pcie_removal_wk() is a DSM that fails once the device is off the bus, so a later request for a function level reset can get a full product reset instead, with Bluetooth still bound. Deselect it at probe.

---
Patch 4 depends on this, not the other way round. By the time a recovery reset runs, the disarm in .resume has already failed, so the probe deselect after the rescan is what clears the mode. This one therefore stands alone as a fix and is tagged for stable while patch 4 is not.

 drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c | 2 ++
 1 file changed, 2 insertions(+)

index e5edcc2..d93d8f9 100644
@@ -4256,6 +4256,8 @@ 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);
+	/* a previous trans may have left the mode selected */
+	iwl_trans_pcie_set_product_reset(pdev, false, mac_cfg->integrated);
 
 	/* set the things we know so far for the grab NIC access */
 	iwl_trans_set_info(iwl_trans, &info);

On some boards D3cold cuts a discrete module's power rail and the device never comes back. The link does not train and config space reads all ones until reboot.

The product reset the driver already implements recovers it, but the vendor DSM that selects the product reset mode reads the device's PCI ID out of config space first, so it can only be armed while the device still answers. iwl_trans_pcie_removal_wk(), the only place that arms it, is too late.

Arm it in .suspend and disarm in .resume. If the disarm fails and CSR_HW_REV reads all ones, run the product reset. Skip integrated CNVi, whose DSM takes an AML path I cannot test. The DSM failure log drops to debug, since .suspend would otherwise hit it on every suspend on boards without the DSM.

---
The bugzilla and the first lore Link: are other BE200/GL reports of the same 0xffffffff-until-reboot, on machines I do not have. The second is my own analysis of this machine's AML.

Lenovo Yoga Pro 7 14IAH10 (Arrow Lake-H, Core Ultra 9 285H), BIOS QGCN35WW, discrete BE200 SUBSYS_00F48086, Bluetooth on USB, no CSME, stock ACPI tables.

Before the patch s2idle left the device dead until reboot. With it the device comes back, both with wifi connected at suspend and with the radio down, the case where .suspend runs with no op_mode. Skipping just the _RST call and leaving the remove and rescan in place left it absent.

With debug=0x100 one cycle logs the intended path end to end:

iwl_trans_pcie_set_product_reset Enabled product reset via DSM iwl_trans_pcie_check_product_reset_mode product reset mode is 0x1 iwl_trans_pcie_set_product_reset can't disable product reset via DSM (-ENODEV) device not responding after resume
scheduling reset (mode=6)
iwl_trans_pcie_set_product_reset can't enable product reset via DSM (-ENODEV) iwl_trans_pcie_call_reset called _RST on _PRR object iwl_trans_pcie_set_product_reset Disabled product reset via DSM

mode=6 is IWL_RESET_MODE_PROD_RESET, so the request was not downgraded, and the last line is the probe deselect patch 3 adds. Wifi is usable about 5 s after .resume returns, 7 s end to end: 4.4 s of that is the platform's _RST and ~2 s is the PCI core retrying the link before .resume runs.

What is untested or untestable with one machine:

- .suspend and .resume are untouched on integrated/CNVi, where the arming helper returns early and prod_reset_set is never set. Getting the CNVi case working needs someone with the hardware.
- This box has no CSME, so the ME path is untested. Where me_present is non-zero the request is downgraded to IWL_RESET_MODE_FUNC_RESET, but the device should still come back, since the mode is already armed and _RST does the product reset regardless. Bluetooth is not torn down first, which is the pre-existing hazard the previous patch describes.
- The failure looks specific to s2idle. One real hibernate cycle armed the mode in .freeze and cleared it again on restore, both confirmed by the DSM readback, and the device came back without needing the reset at all. Hibernate cold-boots to resume, so the part gets a genuine power-on reset, where s2idle cuts the rail and leaves it wedged.
- A device that dies at runtime is still unrecoverable. That needs the same thing hooked to runtime PM, which iwlwifi does not implement.

 drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 24 ++++++++++++++++++
 .../intel/iwlwifi/pcie/gen1_2/internal.h      |  4 +++
 .../intel/iwlwifi/pcie/gen1_2/trans.c         | 25 ++++++++++++++-----
 3 files changed, 47 insertions(+), 6 deletions(-)

index a3e6c9e..96fb7fc 100644
@@ -1204,11 +1204,19 @@ static void iwl_pci_remove(struct pci_dev *pdev)
 
 static int iwl_pci_suspend(struct device *device)
 {
+	struct iwl_trans *trans = pci_get_drvdata(to_pci_dev(device));
+
 	/* Before you put code here, think about WoWLAN. You cannot check here
 	 * whether WoWLAN is enabled or not, and your code will run even if
 	 * WoWLAN is enabled - don't kill the NIC, someone may need it in Sx.
 	 */
 
+	/* Arming has to happen while the device still answers, because the
+	 * AML gates this DSM on reading the device's PCI ID out of config
+	 * space. It does not touch the NIC.
+	 */
+	iwl_trans_pcie_arm_product_reset(trans, true);
+
 	return 0;
 }
 

@@ -1230,6 +1238,22 @@ static int _iwl_pci_resume(struct device *device, bool restore)
 	 */
 	pci_write_config_byte(pdev, PCI_CFG_RETRY_TIMEOUT, 0x00);
 
+	/* Two signals that the device didn't come back from D3cold: the
+	 * platform can't deselect the mode armed in .suspend (so it can't see
+	 * the device either), and the device doesn't answer. This runs before
+	 * the op_mode test because the firmware may never have been loaded.
+	 */
+	if (trans_pcie->prod_reset_set) {
+		iwl_trans_pcie_arm_product_reset(trans, false);
+		if (trans_pcie->prod_reset_set &&
+		    PCI_POSSIBLE_ERROR(iwl_read32(trans, CSR_HW_REV))) {
+			IWL_ERR(trans, "device not responding after resume\n");
+			iwl_trans_pcie_reset(trans, IWL_RESET_MODE_PROD_RESET);
+			return 0;
+		}
+		trans_pcie->prod_reset_set = false;
+	}
+
 	if (!trans->op_mode)
 		return 0;
 

index d84c7c1..1caaff9 100644
@@ -495,6 +495,8 @@ struct iwl_pcie_txqs {
  * @isr_stats: interrupt statistics
  * @napi_dev: (fake) netdev for NAPI registration
  * @txqs: transport tx queues data.
+ * @prod_reset_set: the product reset mode is selected in the platform;
+ *	system suspend/resume only, so process context only
  * @me_present: WiAMT/CSME is detected as present (1), not present (0)
  *	or unknown (-1, so can still use it as a boolean safely)
  * @me_recheck_wk: worker to recheck WiAMT/CSME presence

@@ -605,6 +607,7 @@ struct iwl_trans_pcie {
 
 	struct iwl_pcie_txqs txqs;
 
+	bool prod_reset_set;
 	s8 me_present;
 	struct delayed_work me_recheck_wk;
 

@@ -657,6 +660,7 @@ bool _iwl_trans_pcie_grab_nic_access(struct iwl_trans *trans, bool silent);
 
 void iwl_trans_pcie_check_product_reset_status(struct pci_dev *pdev);
 void iwl_trans_pcie_check_product_reset_mode(struct pci_dev *pdev);
+void iwl_trans_pcie_arm_product_reset(struct iwl_trans *trans, bool arm);
 
 /*****************************************************
 * RX

index d93d8f9..a764c6d 100644
@@ -5,6 +5,7 @@
  * Copyright (C) 2016-2017 Intel Deutschland GmbH
  */
 #include <linux/pci.h>
+#include <linux/string_choices.h>
 #include <linux/interrupt.h>
 #include <linux/debugfs.h>
 #include <linux/sched.h>

@@ -2075,7 +2076,7 @@ void iwl_trans_pcie_check_product_reset_mode(struct pci_dev *pdev)
 	ACPI_FREE(res);
 }
 
-static void iwl_trans_pcie_set_product_reset(struct pci_dev *pdev, bool enable,
+static bool iwl_trans_pcie_set_product_reset(struct pci_dev *pdev, bool enable,
 					     bool integrated)
 {
 	union acpi_object *res;

@@ -2089,17 +2090,29 @@ static void iwl_trans_pcie_set_product_reset(struct pci_dev *pdev, bool enable,
 						 DSM_INTERNAL_PLDR_CMD_SET_MODE,
 						 mode);
 	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;
+		IWL_DEBUG_DEV_POWER(&pdev->dev,
+				    "can't %s product reset via DSM (%pe)\n",
+				    str_enable_disable(enable), res);
+		return false;
 	}
 
 	ACPI_FREE(res);
 	IWL_DEBUG_DEV_POWER(&pdev->dev, "%sabled product reset via DSM\n",
 			    enable ? "En" : "Dis");
 	iwl_trans_pcie_check_product_reset_mode(pdev);
+	return true;
+}
+
+void iwl_trans_pcie_arm_product_reset(struct iwl_trans *trans, bool arm)
+{
+	struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans);
+
+	/* discrete only: the integrated arming mask is untested */
+	if (trans->mac_cfg->integrated)
+		return;
+
+	if (iwl_trans_pcie_set_product_reset(trans_pcie->pci_dev, arm, false))
+		trans_pcie->prod_reset_set = arm;
 }
 
 void iwl_trans_pcie_check_product_reset_status(struct pci_dev *pdev)

Hi Miri,

Ping on this series. It is still needed on 7.2.8: when _PR3 removes the card's rail in s2idle, BE200 (and others) comes back reading all ones and Wi-Fi stays down until reboot.

Hi, I've been testing this patchset for almost two days, with good results. I applied it on top of mainline (7.3-rc6).

I was having issues with my new company laptop for a few weeks. A Dell Pro 14 Plus PB14250, with BE200 Series Wi-Fi 7 (00:14.3)

More details below:

Before, this would loop for usually long times, or until I reloaded the driver:

```
[  349.794606] wlp0s20f3: disconnect from AP $AP_MAC for new auth to $MAC
[  349.899518] wlp0s20f3: authenticate with $MAC (local address=$LOCAL_MAC)
[  349.900258] wlp0s20f3: send auth to $MAC (try 1/3)
[  351.906522] wlp0s20f3: send auth to $MAC (try 2/3)
[  353.922497] wlp0s20f3: send auth to $MAC (try 3/3)
[  354.900727] wlp0s20f3: aborting authentication with $MAC by local choice (Reason: 3=DEAUTH_LEAVING)
```

With the patches applied, I still got a few disconnects, but they were imperceptible. I only know abour them because I was keeping an eye on my logs (AKA dmesg -w | rg 'wlp|iwl|auth|reason' ).

Example disconnection log with the patchset applied:

```
[ 341.720214] wlp0s20f3: disconnect from AP $AP_MAC for new auth to $MAC
[ 341.802793] wlp0s20f3: authenticate with $MAC (local address=$LOCAL_MAC)
[ 341.803277] wlp0s20f3: send auth to $MAC (try 1/3)
[ 343.820077] wlp0s20f3: send auth to $MAC (try 2/3)
[ 345.832125] wlp0s20f3: send auth to $MAC (try 3/3)
[ 346.803865] wlp0s20f3: aborting authentication with $MAC by local choice (Reason: 3=DEAUTH_LEAVING)
[ 350.389895] wlp0s20f3: authenticate with $AP_MAC (local address=$LOCAL_MAC)
[ 350.390412] wlp0s20f3: send auth to $AP_MAC (try 1/3)
[ 350.396872] wlp0s20f3: authenticated
[ 350.400017] wlp0s20f3: associate with $AP_MAC (try 1/3)
[ 350.413168] wlp0s20f3: RX AssocResp from $AP_MAC (capab=0x1111 status=0 aid=5)
[ 350.414412] wlp0s20f3: associated
[ 350.441020] wlp0s20f3: Limiting TX power to 36 (36 - 0) dBm as advertised by $AP_MAC
```

Before finding this patchset, I also tried a few options from forums:
Running plain mainline 7.3.0-rc6,

disabling d3cold with udev rules,

# disabling WiFi 7
options iwlwifi disable_11be=1
# Disabling Power Saving on BE200 wifi
options iwlmld power_scheme=1
options iwlwifi power_save=0 uapsd_disable=1

Also tried connecting to a WiFi 2 only ssid (with "IOT friendly" options).

Thanks,
Rafael Passos