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

[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

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