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

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

[Navon John Lukose]

On a Lenovo Yoga Pro 7 14IAH10 (Arrow Lake-H) the Intel BE200 does not
survive D3cold: _PR3 genuinely removes the M.2 module's rail and the card
does not restart when the rail and PERST# come back. After _ON both the
power-enable and PERST# GPIO pad registers read correct and the link still
never trains; config space reads 0xffffffff until reboot.

iwlwifi already has everything needed to recover it: the platform-level
device reset walks _PRR and evaluates _RST on what it returns, and the
driver already speaks the vendor _DSM that selects the product reset mode.
The problem is ordering. The mode is only ever selected from the removal
path, by which point the device no longer answers, and AML gates that _DSM
on reading the device's PCI ID back out of config space - so the selection
can never succeed at the one moment it matters.

  Patch 1  stop concluding "no CSME" (or "CSME present") from a register
           read that never reached the device. An independent bug, with a
           behaviour change of its own; see the patch.
  Patch 2  deselect the mode at probe. The driver selects it and never
           clears it on the product-reset path, and _RST does not clear it
           either.
  Patch 3  arm the mode in .suspend, disarm it in .resume, and if the
           disarm fails *and* the device reads all ones, ask for a product
           reset.

+57/-10 over three files. Patch 3's new arming is confined to discrete
parts, but its demotion of a failure log to debug level applies to
integrated parts too; patch 1 changes me_present handling on all BZ and
later hardware, and patch 2 adds a DSM evaluation to every probe.

Since v1:
https://lore.kernel.org/all/20260829095437.44716-1-navonjohnlukose@gmail.com/

- v1 selected the mode at probe and left it selected forever, which is a
  permanent change to platform state for a reset that may never happen.
  v2 arms in .suspend and disarms in .resume, and patch 2 clears what an
  earlier driver instance left behind.
- v2 requires two independent signals before it removes and resets
  anything: the disarm failing and CSR_HW_REV reading ~0. A failed DSM
  alone is not evidence of a dead device.
- Patch 1 is new. It is the me_present bug I said I would send separately;
  it is here because it is the reset ladder patch 3 reasons about, and it
  is wider than I described then. It applies alone.
- Corrections to v1 and to my analysis mail: this is Arrow Lake-H, not
  "several Meteor Lake laptops", and it is one machine tested by me. I
  also said twice that the test would key on config space
  (pci_device_is_present(), "not answering config cycles"); it does not.
  v2 makes no config access of its own, because a device in D3hot answers
  config cycles while its BARs are dark. Config space is still in the loop
  one layer down, in AML's own VDID read.
- iwl_trans_pcie_reset() is left alone. A draft suppressed the CSME
  downgrade while the mode was armed; that premise only holds for a device
  already off the bus. The consequence is under the --- of patch 3.

Notes for review:

- .suspend does not reset anything and does not touch the NIC or the
  firmware: it reads the PCI ID through AML and writes an integer into the
  ACPI namespace. The reset only happens from .resume, through the existing
  iwl_trans_pcie_reset() path, and only from the work item it queues.
- This is recovery, not avoidance. The alternative on the list is the DMI
  quirk patch 3 Links to, which disables D3cold for the affected machine
  and costs nothing per resume, where this keeps D3cold and pays ~7 s on
  the resumes that fail. I am not claiming this is the better trade; it is
  the one that does not need a DMI entry per machine, and it shows the
  hardware can recover. If you would rather take the quirk, patches 1 and
  2 still stand on their own.
- Arguably the better fix is in the PCI core: for a device with _PRR that
  comes back from D3cold not answering, evaluate _RST in place of the
  speculative retrain pcie_wait_for_link_delay() attempts. That would
  leave the device alive before any driver .resume runs, taking the ~7 s
  to ~4.5 s. But arming is a vendor DSM that has to be issued while the
  device is still alive, so the driver is involved either way. I would
  like to do the core half as a follow-up.
- This is not the missing D3cold->D0 recovery delay from "PCI: Apply
  mandatory recovery delay on return from D3cold":
  https://lore.kernel.org/all/20260708152650.536604-2-mario.limonciello@amd.com/
  That delay is real and this device is subject to it, but I left the card
  in the failed state and polled it for 60 s across ten PCI rescans:
  0xffffffff throughout, link never trained.

Testing: one machine, one BIOS, discrete only. Five s2idle cycles, four
with wifi connected at suspend and one with the radio down, and the device
recovered on all five; with the reset skipped and nothing else changed it
stayed absent until reboot. Methodology and the untested surface are under
the --- of patch 3.

Patches 1 and 2 carry Fixes: tags and Cc: stable. Patch 3 carries neither:
the device dying in D3cold is not a regression from any commit, the driver
simply never handled it, and I would rather not invent a SHA. It is still
a fix in the sense that matters, since the machine loses its WiFi until
reboot. Whether that belongs in wireless or wireless-next is your call.

Signed-off-by: Navon John Lukose <navonjohnlukose@gmail.com>

Navon John Lukose (3):
  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         | 39 ++++++++++++++-----
 3 files changed, 57 insertions(+), 10 deletions(-)

-- 
2.55.0

[PATCH wireless v2 1/3] wifi: iwlwifi: pcie: don't infer CSME presence from a failed read

[Navon John Lukose]

iwl_pcie_check_me_status() decides whether WiAMT/CSME is present from two
register reads, without checking that either read reached the device.

iwl_read_prph() returns 0x5a5a5a5a when it cannot grab NIC access, and
that value has CNVI_SCU_REG_FOR_ECO_1_WIAMT_KNOWN set and
CNVI_SCU_REG_FOR_ECO_1_WIAMT_PRESENT clear. A read that never reached
the hardware is therefore taken as a positive statement that there is no
CSME, and the function returns without scheduling the recheck. That is
reachable at probe: iwl_pci_gen1_2_probe() carries on when
iwl_pcie_prepare_card_hw() fails, and iwl_pcie_check_me_status() then
runs against a card it cannot talk to.

The second read has the mirror-image problem: an all-ones
CSR_HW_IF_CONFIG_REG has both ME_OWN and IAMT_UP set, so a device that
has fallen off the bus latches me_present to 1. So does the one in
iwl_pcie_recheck_me_status(), which runs a second after probe with no
guarantee that the device is still answering.

me_present is never recomputed after that, and any non-zero value makes
iwl_trans_pcie_reset() downgrade IWL_RESET_MODE_PROD_RESET to
IWL_RESET_MODE_FUNC_RESET, so one bad read permanently weakens the
recovery. In the 0x5a5a5a5a case it goes the other way and permits a
product reset on a machine that may well have CSME.

Don't take those values as data. iwl_trans_is_hw_error_value() matches
0x5a5a5a5[0-f] and 0xa5a5a5a[0-f] but not ~0, so the prph read in
iwl_pcie_check_me_status() needs both tests, the way
iwl_pcie_irq_handler() does; the two CSR reads only need the ~0 one. At
probe that leaves me_present at -1 (unknown) and still schedules the
recheck; in the recheck it keeps the previous value.

This does change the reset ladder in the poisoned-read case, and -1 is
truthy: a product reset that iwl_trans_pcie_reset() used to allow -
because 0x5a5a5a5a had been read as me_present = 0 - is now downgraded
to a function level reset. That is the conservative direction, and the 0
was never a reading, but it is a behaviour change and not a no-op.

Cc: stable@vger.kernel.org
Fixes: 41fff83fe6cd ("wifi: iwlwifi: pcie: check for WiAMT/CSME presence")
Signed-off-by: Navon John Lukose <navonjohnlukose@gmail.com>
---
Backport note: the bug arrived in v6.14, so the affected branches that are
still supported are 6.18.y, 7.1.y and 7.2.y. All three have the code in
pcie/gen1_2/trans.c as trans_pcie->me_present, so this applies as posted
with no rewrite.

Only if you care about anything older that has the bug - v6.14 through
v6.17, all EOL now: v6.16 and below have both functions in pcie/drv.c
(377edee91b89 "wifi: iwlwifi: pcie move gen1_2 probe to gen1_2/trans.c"
moved them), and v6.15 and below spell the field trans->me_present
(cd6d6de694e2 "wifi: iwlwifi: pcie: move ME check data to pcie" renamed
it). The hunks are otherwise identical; iwl_trans_is_hw_error_value()
exists in every affected release.

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

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 28b276c..c6a771e 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
@@ -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 (val != ~0U)
+		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 (val != ~0U && !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 (val != ~0U && (val & (CSR_HW_IF_CONFIG_REG_ME_OWN |
+				  CSR_HW_IF_CONFIG_REG_IAMT_UP))) {
 		trans_pcie->me_present = 1;
 		return;
 	}
-- 
2.55.0

[PATCH wireless v2 2/3] wifi: iwlwifi: pcie: deselect the product reset mode at probe

[Navon John Lukose]

The mode that iwl_trans_pcie_set_product_reset() selects lives in the
platform's ACPI namespace, not in the device, and nothing deselects it on
the product-reset path. iwl_trans_pcie_removal_wk() selects it, evaluates
_RST via _PRR and removes the device; the rescan re-probes, and probe only
reads the mode back for the log rather than clearing it, so it is still
selected. (A later removal with a lesser mode does pass enable=false, but
that is the path that does not need it.) It is plain namespace state - on
the platform I have it is a named integer written by the vendor DSM and
read back by the reset method - so it survives S3 and s2idle. Neither the
driver nor _RST clears it.

That has a consequence. _RST branches on the mode variable, does not
clear it, and iwl_trans_pcie_reset() takes the caller's word for which
reset to run. So after any product reset the next escalation can do the
wrong thing: iwl_trans_determine_restart_mode() asks for
IWL_RESET_MODE_FUNC_RESET on rung four of the ladder, no CSME involved,
iwl_trans_pcie_removal_wk() skips the Bluetooth teardown because the
mode it was passed is not IWL_RESET_MODE_PROD_RESET, tries to deselect,
and if the device has stopped answering by then that deselect fails
silently - the DSM is gated on AML reading the device's PCI ID out of
config space. iwl_trans_pcie_call_reset() then runs a full product reset,
Bluetooth kill GPIO and all, with the Bluetooth function still bound.

Deselect at probe, after the two calls that already read the mode and
the previous reset's status back for the log - so the inherited mode is
still what gets logged. That bounds the window to a single driver
lifetime.

Note that on discrete devices this is not literally a write of zero:
iwl_trans_pcie_set_product_reset() also sets EN_WIFI_FLR and EN_BT_OFF_ON
unconditionally for !integrated, so the write is 0x6. EN_PROD_RESET is the
bit the platform's reset method branches on, and that is the one being
cleared.

Cc: stable@vger.kernel.org
Fixes: 9673c35486d4 ("wifi: iwlwifi: implement product reset for TOP errors")
Signed-off-by: Navon John Lukose <navonjohnlukose@gmail.com>
---
Patch 3 also depends on this: it is the only thing that clears the mode if
the rescan after a recovery reset does not bring the device back. That
dependency runs patch 3 -> patch 2, not the other way about, so this one
stands alone as a fix and is tagged for stable while patch 3 is not.

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

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 c6a771e..df89fb3 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
@@ -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);
-- 
2.55.0

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

[Navon John Lukose]

On a Lenovo Yoga Pro 7 14IAH10 (Arrow Lake-H) with a discrete BE200
(8086:272b), D3cold removes the module's power rail and the device does
not restart when the rail and PERST# are restored. After _ON the
power-enable and PERST# GPIO pad registers read correct and the link
still never trains; config space reads all ones until reboot.

The platform can recover it, with a WLAN-specific reset line driven by
the object _PRR returns - which is exactly the product reset the driver
already implements. The problem is ordering: AML only dispatches the
vendor DSM that selects that mode after reading the device's PCI ID back
out of config space,

    Method (WIST) { Switch (ToInteger (VDID)) { Case (0x272B8086) {...} } }
    Method (_DSM) { ... If (WIST ()) { ... Return (IFUN (...)) } ... }

so once the device is off the bus acpi_check_dsm() fails and the mode
stays deselected. iwl_trans_pcie_removal_wk() is the only place that
selects it today, and by then it is too late: _RST takes its other
branch and issues a function level reset to a device that is not there.

So arm the mode in .suspend, while the device still answers, and disarm
it again in .resume. Treat the device as gone only when two independent
signals agree: the mode we armed can no longer be disarmed (so the
platform cannot see the device either) and CSR_HW_REV reads all ones (so
neither can we). Either alone is not enough - a DSM can fail for
transient ACPI reasons on a healthy adapter, and a false positive costs a
remove, a platform reset and a rescan on every resume. The order of the
terms is load bearing: a device in D3hot answers config cycles but does
not decode its BARs, so the DSM would still work while CSR_HW_REV read
all ones. The disarm has to short-circuit.

Recovery goes through the existing iwl_trans_pcie_reset() path, which
only queues a work item, so the remove, the _RST and the rescan happen
after .resume has returned and the PM core has dropped the device lock.
The op_mode is not notified beyond the STATUS_TRANS_DEAD that
iwl_trans_pcie_reset() sets; as today, it finds out by having its own
resume fail against the dead device. Taking this path also skips the
handshake timeouts and the bogus ADVANCED_SYSASSERT dump the driver
otherwise produces against absent hardware, which on this machine cost
about two seconds on every failed resume.

Arming is confined to discrete modules: on integrated CNVi parts
iwl_trans_pcie_set_product_reset() sends EN_PROD_RESET on its own, which
lands in \_SB.PC00.CNVW.RSTT and is what the CNVi _RST branches on before
killing Bluetooth and issuing the PLDR. Arming that from .suspend on
hardware I cannot test is not worth it, so the integrated mask stays as
unexercised as it is today.

iwl_trans_pcie_set_product_reset() now reports whether the DSM took, and
its error on failure becomes a debug message: .suspend would otherwise log
an error on every suspend on every discrete machine without this DSM. So
iwl_trans_pcie_removal_wk() no longer logs at error level when it cannot
arm, which on the recovery path is every time, since the device is off the
bus by then. The cost is that a genuine product reset on a live device
with no DSM support is now silent at error level too.

One known limitation: where me_present is not 0, iwl_trans_pcie_reset()
downgrades the request to IWL_RESET_MODE_FUNC_RESET. The device still
comes back, because the mode is already armed and _RST does the product
reset regardless, but the Bluetooth function is not torn down first.

Cc: stable+noautosel@kernel.org # new suspend/resume behaviour, one machine
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/
Signed-off-by: Navon John Lukose <navonjohnlukose@gmail.com>
---
The bugzilla and the first lore Link: are other BE200/GL reports of the
same 0xffffffff-until-reboot, on machines I do not have; neither is
claimed as fixed, hence Link: and not Closes:. The assert in the bugzilla
report is the dump against absent hardware this patch skips, not a
firmware bug it fixes. The second lore Link: 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.

This machine ships a udev rule forcing d3cold_allowed to 0, which would
have made the test vacuous. It was moved aside for the whole run and
d3cold_allowed written back to 1 before every cycle, so the device really
did reach D3cold: detection fired on all five cycles, which cannot happen
otherwise. Before the patch every cycle left the device dead until reboot;
with it the device recovered on all five, four with wifi connected at
suspend and one with the radio down, the case where .suspend runs with no
op_mode. With the reset skipped and nothing else changed it stayed absent,
so it is the reset that recovers it and not the remove/rescan.

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 (-19)
    device not responding after resume
    scheduling reset (mode=6)
    iwl_trans_pcie_set_product_reset can't enable product reset via DSM (-19)
    iwl_trans_pcie_call_reset called _RST on _PRR object

mode=6 is IWL_RESET_MODE_PROD_RESET, so the request was not downgraded.
The link trained in 64-76 ms and the interface was usable 5.00-5.05 s after
.resume returned; end to end it is closer to 7 s, because the PCI core
spends ~2 s retraining a link that cannot train before .resume is called.
4.365 s of the rest is one _RST evaluation against a 4.320 s floor computed
from the Sleep() operators in the AML, so essentially all of it is platform
AML, and asking for a product reset is not what costs it: both arms of _RST
fall through to the same two 2000 ms sleeps and the product arm adds only
2 x RDLY (160 ms each here).

What is untested or untestable with one machine:

- .suspend and .resume are untouched on integrated/CNVi: the arming helper
  returns early there, so prod_reset_set is never set. (The error-level
  demotion does apply to integrated parts on the removal_wk() path.)
  Getting the CNVi case working needs someone with the hardware.
- Where me_present is non-zero - including the permanent -1 that
  iwl_pcie_check_me_status() leaves on everything below
  IWL_DEVICE_FAMILY_BZ, which is four of the five Intel IDs this AML
  accepts - the request is downgraded to IWL_RESET_MODE_FUNC_RESET. The
  device still comes back, because the mode is armed and _RST does the
  product reset anyway, but Bluetooth is not torn down first. That is the
  pre-existing hazard the previous patch describes, now reachable from
  resume. me_present is a real 0 here, so this is reasoned, not observed.
- If the disarm fails transiently on a live device, the code clears
  prod_reset_set and carries on while the platform's mode stays selected
  until the next probe, which re-opens that same hazard. Retrying the
  disarm would narrow it; I did not, because a retry loop around an AML
  method on the resume path needs a bound I cannot justify from one
  machine.
- Only s2idle was tested. The same callback is .freeze and .poweroff, so
  hibernate arms too and the image is snapshotted while armed, meaning a
  restore kernel disarms a mode a previous boot selected. Harmless as far
  as I can reason it, but unexercised. A device that dies at runtime is
  still unrecoverable; that needs the same thing on runtime PM, which
  iwlwifi does not implement.
- Only one BIOS. On mine the reset method branches solely on the mode
  variable, never on WIST()/VDID, which is what makes the downgrade above
  survivable. I cannot claim that for every implementation.
- On a platform with the arming DSM but no usable _PRR,
  iwl_trans_pcie_call_reset() falls back to pci_reset_function() against a
  device that is gone, and pci_dev_wait() polls config space for up to
  ~65 s per reset method with pci_lock_rescan_remove() held. That is the
  cost of a true positive, not a false one: a false positive still answers
  config cycles, so pci_dev_wait() returns on its first read.
- iwl_trans_pcie_removal_wk() holds pci_lock_rescan_remove() across the
  whole reset, so it is now held for ~4.3 s during system resume on a
  machine that also has Thunderbolt wanting it. Pre-existing, but this
  patch is what puts it on the resume path.

The arming mask is heavier than the recovery needs - only EN_PROD_RESET
drives the GPIO - but I kept it so the reset armed from .suspend is bit for
bit the one iwl_trans_pcie_removal_wk() already arms. Narrowing it is an
easy follow-up.

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

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
index 7a7b101..5d01a4d 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
@@ -1203,11 +1203,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.
 	 */
 
+	/* Has to be here, while the device still answers: AML gates this DSM
+	 * on reading the device's PCI ID out of config space. It doesn't touch
+	 * the NIC.
+	 */
+	iwl_trans_pcie_arm_product_reset(trans, true);
+
 	return 0;
 }
 
@@ -1229,6 +1237,22 @@ static int _iwl_pci_resume(struct device *device, bool restore)
 	 */
 	pci_write_config_byte(pdev, PCI_CFG_RETRY_TIMEOUT, 0x00);
 
+	/* Two signals that it 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. Before the op_mode test: 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 &&
+		    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);
+			return 0;
+		}
+		trans_pcie->prod_reset_set = false;
+	}
+
 	if (!trans->op_mode)
 		return 0;
 
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/internal.h b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/internal.h
index d84c7c1..1caaff9 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/internal.h
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/internal.h
@@ -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
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 df89fb3..56eb35d 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
@@ -2075,7 +2075,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 +2089,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 %sable product reset via DSM (%d)\n",
+				    enable ? "en" : "dis", (int)PTR_ERR(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)
-- 
2.55.0

Re: [PATCH wireless v2 1/3] wifi: iwlwifi: pcie: don't infer CSME presence from a failed read

[Navon John Lukose]

Fair catch on me_recheck_wk. It is pre-existing, but this patch widens it:
a poisoned read used to take the WIAMT_KNOWN branch and return without ever
arming the work. Sent separately since it stands alone:

https://lore.kernel.org/all/20260831134140.480276-1-navonjohnlukose@gmail.com/

Worth taking that one first. The TOCTOU on STATUS_TRANS_DEAD is also
pre-existing and unchanged by this series.

Thanks,
Navon

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

[Ilpo Järvinen]

On Mon, 31 Aug 2026, Navon John Lukose wrote:

> On a Lenovo Yoga Pro 7 14IAH10 (Arrow Lake-H) with a discrete BE200
> (8086:272b), D3cold removes the module's power rail and the device does
> not restart when the rail and PERST# are restored. After _ON the
> power-enable and PERST# GPIO pad registers read correct and the link
> still never trains; config space reads all ones until reboot.
> 
> The platform can recover it, with a WLAN-specific reset line driven by
> the object _PRR returns - which is exactly the product reset the driver
> already implements. The problem is ordering: AML only dispatches the
> vendor DSM that selects that mode after reading the device's PCI ID back
> out of config space,
> 
>     Method (WIST) { Switch (ToInteger (VDID)) { Case (0x272B8086) {...} } }
>     Method (_DSM) { ... If (WIST ()) { ... Return (IFUN (...)) } ... }
> 
> so once the device is off the bus acpi_check_dsm() fails and the mode
> stays deselected. iwl_trans_pcie_removal_wk() is the only place that
> selects it today, and by then it is too late: _RST takes its other
> branch and issues a function level reset to a device that is not there.
> 
> So arm the mode in .suspend, while the device still answers, and disarm
> it again in .resume. Treat the device as gone only when two independent
> signals agree: the mode we armed can no longer be disarmed (so the
> platform cannot see the device either) and CSR_HW_REV reads all ones (so
> neither can we). Either alone is not enough - a DSM can fail for
> transient ACPI reasons on a healthy adapter, and a false positive costs a
> remove, a platform reset and a rescan on every resume. The order of the
> terms is load bearing: a device in D3hot answers config cycles but does
> not decode its BARs, so the DSM would still work while CSR_HW_REV read
> all ones. The disarm has to short-circuit.
> 
> Recovery goes through the existing iwl_trans_pcie_reset() path, which
> only queues a work item, so the remove, the _RST and the rescan happen
> after .resume has returned and the PM core has dropped the device lock.
> The op_mode is not notified beyond the STATUS_TRANS_DEAD that
> iwl_trans_pcie_reset() sets; as today, it finds out by having its own
> resume fail against the dead device. Taking this path also skips the
> handshake timeouts and the bogus ADVANCED_SYSASSERT dump the driver
> otherwise produces against absent hardware, which on this machine cost
> about two seconds on every failed resume.
> 
> Arming is confined to discrete modules: on integrated CNVi parts
> iwl_trans_pcie_set_product_reset() sends EN_PROD_RESET on its own, which
> lands in \_SB.PC00.CNVW.RSTT and is what the CNVi _RST branches on before
> killing Bluetooth and issuing the PLDR. Arming that from .suspend on
> hardware I cannot test is not worth it, so the integrated mask stays as
> unexercised as it is today.
> 
> iwl_trans_pcie_set_product_reset() now reports whether the DSM took, and
> its error on failure becomes a debug message: .suspend would otherwise log
> an error on every suspend on every discrete machine without this DSM. So
> iwl_trans_pcie_removal_wk() no longer logs at error level when it cannot
> arm, which on the recovery path is every time, since the device is off the
> bus by then. The cost is that a genuine product reset on a live device
> with no DSM support is now silent at error level too.
> 
> One known limitation: where me_present is not 0, iwl_trans_pcie_reset()
> downgrades the request to IWL_RESET_MODE_FUNC_RESET. The device still
> comes back, because the mode is already armed and _RST does the product
> reset regardless, but the Bluetooth function is not torn down first.
> 
> Cc: stable+noautosel@kernel.org # new suspend/resume behaviour, one machine
> 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/
> Signed-off-by: Navon John Lukose <navonjohnlukose@gmail.com>
> ---
> The bugzilla and the first lore Link: are other BE200/GL reports of the
> same 0xffffffff-until-reboot, on machines I do not have; neither is
> claimed as fixed, hence Link: and not Closes:. The assert in the bugzilla
> report is the dump against absent hardware this patch skips, not a
> firmware bug it fixes. The second lore Link: 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.
> 
> This machine ships a udev rule forcing d3cold_allowed to 0, which would
> have made the test vacuous. It was moved aside for the whole run and
> d3cold_allowed written back to 1 before every cycle, so the device really
> did reach D3cold: detection fired on all five cycles, which cannot happen
> otherwise. Before the patch every cycle left the device dead until reboot;
> with it the device recovered on all five, four with wifi connected at
> suspend and one with the radio down, the case where .suspend runs with no
> op_mode. With the reset skipped and nothing else changed it stayed absent,
> so it is the reset that recovers it and not the remove/rescan.
> 
> 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 (-19)
>     device not responding after resume
>     scheduling reset (mode=6)
>     iwl_trans_pcie_set_product_reset can't enable product reset via DSM (-19)
>     iwl_trans_pcie_call_reset called _RST on _PRR object
> 
> mode=6 is IWL_RESET_MODE_PROD_RESET, so the request was not downgraded.
> The link trained in 64-76 ms and the interface was usable 5.00-5.05 s after
> .resume returned; end to end it is closer to 7 s, because the PCI core
> spends ~2 s retraining a link that cannot train before .resume is called.
> 4.365 s of the rest is one _RST evaluation against a 4.320 s floor computed
> from the Sleep() operators in the AML, so essentially all of it is platform
> AML, and asking for a product reset is not what costs it: both arms of _RST
> fall through to the same two 2000 ms sleeps and the product arm adds only
> 2 x RDLY (160 ms each here).
> 
> What is untested or untestable with one machine:
> 
> - .suspend and .resume are untouched on integrated/CNVi: the arming helper
>   returns early there, so prod_reset_set is never set. (The error-level
>   demotion does apply to integrated parts on the removal_wk() path.)
>   Getting the CNVi case working needs someone with the hardware.
> - Where me_present is non-zero - including the permanent -1 that
>   iwl_pcie_check_me_status() leaves on everything below
>   IWL_DEVICE_FAMILY_BZ, which is four of the five Intel IDs this AML
>   accepts - the request is downgraded to IWL_RESET_MODE_FUNC_RESET. The
>   device still comes back, because the mode is armed and _RST does the
>   product reset anyway, but Bluetooth is not torn down first. That is the
>   pre-existing hazard the previous patch describes, now reachable from
>   resume. me_present is a real 0 here, so this is reasoned, not observed.
> - If the disarm fails transiently on a live device, the code clears
>   prod_reset_set and carries on while the platform's mode stays selected
>   until the next probe, which re-opens that same hazard. Retrying the
>   disarm would narrow it; I did not, because a retry loop around an AML
>   method on the resume path needs a bound I cannot justify from one
>   machine.
> - Only s2idle was tested. The same callback is .freeze and .poweroff, so
>   hibernate arms too and the image is snapshotted while armed, meaning a
>   restore kernel disarms a mode a previous boot selected. Harmless as far
>   as I can reason it, but unexercised. A device that dies at runtime is
>   still unrecoverable; that needs the same thing on runtime PM, which
>   iwlwifi does not implement.
> - Only one BIOS. On mine the reset method branches solely on the mode
>   variable, never on WIST()/VDID, which is what makes the downgrade above
>   survivable. I cannot claim that for every implementation.
> - On a platform with the arming DSM but no usable _PRR,
>   iwl_trans_pcie_call_reset() falls back to pci_reset_function() against a
>   device that is gone, and pci_dev_wait() polls config space for up to
>   ~65 s per reset method with pci_lock_rescan_remove() held. That is the
>   cost of a true positive, not a false one: a false positive still answers
>   config cycles, so pci_dev_wait() returns on its first read.
> - iwl_trans_pcie_removal_wk() holds pci_lock_rescan_remove() across the
>   whole reset, so it is now held for ~4.3 s during system resume on a
>   machine that also has Thunderbolt wanting it. Pre-existing, but this
>   patch is what puts it on the resume path.
> 
> The arming mask is heavier than the recovery needs - only EN_PROD_RESET
> drives the GPIO - but I kept it so the reset armed from .suspend is bit for
> bit the one iwl_trans_pcie_removal_wk() already arms. Narrowing it is an
> easy follow-up.
> 
>  drivers/net/wireless/intel/iwlwifi/pcie/drv.c | 24 +++++++++++++++++++
>  .../intel/iwlwifi/pcie/gen1_2/internal.h      |  4 ++++
>  .../intel/iwlwifi/pcie/gen1_2/trans.c         | 24 ++++++++++++++-----
>  3 files changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> index 7a7b101..5d01a4d 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/drv.c
> @@ -1203,11 +1203,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.
>  	 */
>  
> +	/* Has to be here, while the device still answers: AML gates this DSM
> +	 * on reading the device's PCI ID out of config space. It doesn't touch
> +	 * the NIC.
> +	 */
> +	iwl_trans_pcie_arm_product_reset(trans, true);
> +
>  	return 0;
>  }
>  
> @@ -1229,6 +1237,22 @@ static int _iwl_pci_resume(struct device *device, bool restore)
>  	 */
>  	pci_write_config_byte(pdev, PCI_CFG_RETRY_TIMEOUT, 0x00);
>  
> +	/* Two signals that it 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. Before the op_mode test: 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 &&
> +		    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);
> +			return 0;
> +		}
> +		trans_pcie->prod_reset_set = false;
> +	}
> +
>  	if (!trans->op_mode)
>  		return 0;
>  
> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/internal.h b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/internal.h
> index d84c7c1..1caaff9 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/internal.h
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/internal.h
> @@ -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
> 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 df89fb3..56eb35d 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
> @@ -2075,7 +2075,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 +2089,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 %sable product reset via DSM (%d)\n",
> +				    enable ? "en" : "dis", (int)PTR_ERR(res));

str_enable_disable() + make sure you've the necessary include for it.

-- 
 i.

> +		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)
> 

Re: [PATCH wireless v2 1/3] wifi: iwlwifi: pcie: don't infer CSME presence from a failed read

[Bjorn Helgaas]

On Mon, Aug 31, 2026 at 06:33:30PM +0530, Navon John Lukose wrote:
> iwl_pcie_check_me_status() decides whether WiAMT/CSME is present from two
> register reads, without checking that either read reached the device.
> 
> iwl_read_prph() returns 0x5a5a5a5a when it cannot grab NIC access, and
> that value has CNVI_SCU_REG_FOR_ECO_1_WIAMT_KNOWN set and
> CNVI_SCU_REG_FOR_ECO_1_WIAMT_PRESENT clear. A read that never reached
> the hardware is therefore taken as a positive statement that there is no
> CSME, and the function returns without scheduling the recheck. That is
> reachable at probe: iwl_pci_gen1_2_probe() carries on when
> iwl_pcie_prepare_card_hw() fails, and iwl_pcie_check_me_status() then
> runs against a card it cannot talk to.
> 
> The second read has the mirror-image problem: an all-ones
> CSR_HW_IF_CONFIG_REG has both ME_OWN and IAMT_UP set, so a device that
> has fallen off the bus latches me_present to 1. So does the one in
> iwl_pcie_recheck_me_status(), which runs a second after probe with no
> guarantee that the device is still answering.
> 
> me_present is never recomputed after that, and any non-zero value makes
> iwl_trans_pcie_reset() downgrade IWL_RESET_MODE_PROD_RESET to
> IWL_RESET_MODE_FUNC_RESET, so one bad read permanently weakens the
> recovery. In the 0x5a5a5a5a case it goes the other way and permits a
> product reset on a machine that may well have CSME.
> 
> Don't take those values as data. iwl_trans_is_hw_error_value() matches
> 0x5a5a5a5[0-f] and 0xa5a5a5a[0-f] but not ~0, so the prph read in
> iwl_pcie_check_me_status() needs both tests, the way
> iwl_pcie_irq_handler() does; the two CSR reads only need the ~0 one. At
> probe that leaves me_present at -1 (unknown) and still schedules the
> recheck; in the recheck it keeps the previous value.
> 
> This does change the reset ladder in the poisoned-read case, and -1 is
> truthy: a product reset that iwl_trans_pcie_reset() used to allow -
> because 0x5a5a5a5a had been read as me_present = 0 - is now downgraded
> to a function level reset. That is the conservative direction, and the 0
> was never a reading, but it is a behaviour change and not a no-op.
> 
> Cc: stable@vger.kernel.org
> Fixes: 41fff83fe6cd ("wifi: iwlwifi: pcie: check for WiAMT/CSME presence")
> Signed-off-by: Navon John Lukose <navonjohnlukose@gmail.com>
> ---
> Backport note: the bug arrived in v6.14, so the affected branches that are
> still supported are 6.18.y, 7.1.y and 7.2.y. All three have the code in
> pcie/gen1_2/trans.c as trans_pcie->me_present, so this applies as posted
> with no rewrite.
> 
> Only if you care about anything older that has the bug - v6.14 through
> v6.17, all EOL now: v6.16 and below have both functions in pcie/drv.c
> (377edee91b89 "wifi: iwlwifi: pcie move gen1_2 probe to gen1_2/trans.c"
> moved them), and v6.15 and below spell the field trans->me_present
> (cd6d6de694e2 "wifi: iwlwifi: pcie: move ME check data to pcie" renamed
> it). The hunks are otherwise identical; iwl_trans_is_hw_error_value()
> exists in every affected release.
> 
>  .../net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c  | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> 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 28b276c..c6a771e 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
> @@ -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 (val != ~0U)

Consider using PCI_POSSIBLE_ERROR() for these tests.

> +		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 (val != ~0U && !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 (val != ~0U && (val & (CSR_HW_IF_CONFIG_REG_ME_OWN |
> +				  CSR_HW_IF_CONFIG_REG_IAMT_UP))) {
>  		trans_pcie->me_present = 1;
>  		return;
>  	}
> -- 
> 2.55.0
> 

Re: [PATCH wireless v2 1/3] wifi: iwlwifi: pcie: don't infer CSME presence from a failed read

[Bjorn Helgaas]

On Tue, Sep 01, 2026 at 10:48:28AM -0500, Bjorn Helgaas wrote:
> On Mon, Aug 31, 2026 at 06:33:30PM +0530, Navon John Lukose wrote:
> > iwl_pcie_check_me_status() decides whether WiAMT/CSME is present from two
> > register reads, without checking that either read reached the device.
> > 
> > iwl_read_prph() returns 0x5a5a5a5a when it cannot grab NIC access, and
> > that value has CNVI_SCU_REG_FOR_ECO_1_WIAMT_KNOWN set and
> > CNVI_SCU_REG_FOR_ECO_1_WIAMT_PRESENT clear. A read that never reached
> > the hardware is therefore taken as a positive statement that there is no
> > CSME, and the function returns without scheduling the recheck. That is
> > reachable at probe: iwl_pci_gen1_2_probe() carries on when
> > iwl_pcie_prepare_card_hw() fails, and iwl_pcie_check_me_status() then
> > runs against a card it cannot talk to.
> > 
> > The second read has the mirror-image problem: an all-ones
> > CSR_HW_IF_CONFIG_REG has both ME_OWN and IAMT_UP set, so a device that
> > has fallen off the bus latches me_present to 1. So does the one in
> > iwl_pcie_recheck_me_status(), which runs a second after probe with no
> > guarantee that the device is still answering.
> > 
> > me_present is never recomputed after that, and any non-zero value makes
> > iwl_trans_pcie_reset() downgrade IWL_RESET_MODE_PROD_RESET to
> > IWL_RESET_MODE_FUNC_RESET, so one bad read permanently weakens the
> > recovery. In the 0x5a5a5a5a case it goes the other way and permits a
> > product reset on a machine that may well have CSME.
> > 
> > Don't take those values as data. iwl_trans_is_hw_error_value() matches
> > 0x5a5a5a5[0-f] and 0xa5a5a5a[0-f] but not ~0, so the prph read in
> > iwl_pcie_check_me_status() needs both tests, the way
> > iwl_pcie_irq_handler() does; the two CSR reads only need the ~0 one. At
> > probe that leaves me_present at -1 (unknown) and still schedules the
> > recheck; in the recheck it keeps the previous value.
> > 
> > This does change the reset ladder in the poisoned-read case, and -1 is
> > truthy: a product reset that iwl_trans_pcie_reset() used to allow -
> > because 0x5a5a5a5a had been read as me_present = 0 - is now downgraded
> > to a function level reset. That is the conservative direction, and the 0
> > was never a reading, but it is a behaviour change and not a no-op.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 41fff83fe6cd ("wifi: iwlwifi: pcie: check for WiAMT/CSME presence")
> > Signed-off-by: Navon John Lukose <navonjohnlukose@gmail.com>
> > ---
> > Backport note: the bug arrived in v6.14, so the affected branches that are
> > still supported are 6.18.y, 7.1.y and 7.2.y. All three have the code in
> > pcie/gen1_2/trans.c as trans_pcie->me_present, so this applies as posted
> > with no rewrite.
> > 
> > Only if you care about anything older that has the bug - v6.14 through
> > v6.17, all EOL now: v6.16 and below have both functions in pcie/drv.c
> > (377edee91b89 "wifi: iwlwifi: pcie move gen1_2 probe to gen1_2/trans.c"
> > moved them), and v6.15 and below spell the field trans->me_present
> > (cd6d6de694e2 "wifi: iwlwifi: pcie: move ME check data to pcie" renamed
> > it). The hunks are otherwise identical; iwl_trans_is_hw_error_value()
> > exists in every affected release.
> > 
> >  .../net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c  | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> > 
> > 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 28b276c..c6a771e 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
> > @@ -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 (val != ~0U)
> 
> Consider using PCI_POSSIBLE_ERROR() for these tests.
> 
> > +		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 (val != ~0U && !iwl_trans_is_hw_error_value(val) &&

I wonder whether iwl_trans_is_hw_error_value() should itself check for
PCI_POSSIBLE_ERROR() internally.  Or other callers should also check.

iwlwifi has a mix of checking for 0xffffffff and ~0.  I don't know if
it can use non-PCI transports, but some of those places look like they
could use PCI_POSSIBLE_ERROR().

> > +	    (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 (val != ~0U && (val & (CSR_HW_IF_CONFIG_REG_ME_OWN |
> > +				  CSR_HW_IF_CONFIG_REG_IAMT_UP))) {
> >  		trans_pcie->me_present = 1;
> >  		return;
> >  	}
> > -- 
> > 2.55.0
> > 

Re: [PATCH wireless v2 1/3] wifi: iwlwifi: pcie: don't infer CSME presence from a failed read

[Johannes Berg]

On Tue, 2026-09-01 at 11:17 -0500, Bjorn Helgaas wrote:
> 
> iwlwifi has a mix of checking for 0xffffffff and ~0.  I don't know if
> it can use non-PCI transports, but some of those places look like they
> could use PCI_POSSIBLE_ERROR().

I believe the hardware itself never generates ~0, so yes, it probably
should use that.

johannes