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

[Navon John Lukose]

Both findings are correct. Please drop this series; a v2 is coming.

On the first: arming at probe does leave the mode selected for the
lifetime of the driver, and the disarm in iwl_trans_pcie_removal_wk()
cannot be relied on to undo it, because it fails in exactly the case
that matters - the DSM is gated on the platform reading the device's
PCI ID out of config space, so it is unavailable once the device is off
the bus, and set_product_reset() ignores that failure when disarming.
_RST is not gated the same way: it reads the mode variable directly, so
a stale selection does execute a full product reset, Bluetooth off/on
included, with no BT teardown and with the ME downgrade bypassed.

I also need to retract something. The cover letter argued for stable on
the grounds that "every _RST evaluation is already preceded by
set_product_reset() setting the mode that reset wants, so arming at
probe cannot alter the behaviour of any later reset". That is wrong. It
holds only when the disarm succeeds, and the disarm cannot succeed on a
device that is gone. The backport rationale as written does not stand.

On the second: yes, it logs IWL_ERR on every probe on any platform
without this DSM, and the commit message's claim that it is a no-op
there is wrong. The two neighbouring functions,
iwl_trans_pcie_check_product_reset_mode() and _status(), already return
silently in the same situation, so the asymmetry looks unintended - and
it is what hid the failed disarm above.

v2 will instead select the mode from the suspend callback and clear it
on resume, so it is only selected across the suspend window; guard the
_RST call with pci_device_is_present(), which reads the same config
register the platform's own gate does; and demote the log.

Unrelated, but found while checking this: iwl_pcie_recheck_me_status()
reads CSR_HW_IF_CONFIG_REG without a liveness check, so on a device that
is off the bus it sees 0xffffffff, concludes IAMT_UP is set, and marks
ME present on a machine that has none - which then downgrades every
later product reset. I will send that separately.

Patch 2/2 is substantively unchanged in v2.

Thanks for the review.

Navon