[PATCH v2 0/2] mwifiex: Add quirks for MS Surface devices

[PATCH v2 0/2] mwifiex: Add quirks for MS Surface devices

[Jonas Dreßler]

This is version 2 of the patches proposed earlier here:
https://lore.kernel.org/linux-wireless/20210522131827.67551-1-verdre@v0yd.nl/

Compared to version 1, I left out the last commit, that one had some open
questions and I couldn't test changes to that commit since I don't own a
Surface 3. Other than that, only a few code comments were changed and commit
messages rephrased.

Jonas Dreßler (1):
  mwifiex: pcie: add DMI-based quirk implementation for Surface devices

Tsuchiya Yuto (1):
  mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

 drivers/net/wireless/marvell/mwifiex/Makefile |   1 +
 drivers/net/wireless/marvell/mwifiex/pcie.c   |  11 ++
 drivers/net/wireless/marvell/mwifiex/pcie.h   |   1 +
 .../wireless/marvell/mwifiex/pcie_quirks.c    | 155 ++++++++++++++++++
 .../wireless/marvell/mwifiex/pcie_quirks.h    |  11 ++
 5 files changed, 179 insertions(+)
 create mode 100644 drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
 create mode 100644 drivers/net/wireless/marvell/mwifiex/pcie_quirks.h

-- 
2.31.1

[PATCH v2 1/2] mwifiex: pcie: add DMI-based quirk implementation for Surface devices

[Jonas Dreßler]

This commit adds the ability to apply device-specific quirks to the
mwifiex driver. It uses DMI matching similar to the quirks brcmfmac uses
with dmi.c. We'll add identifiers to match various MS Surface devices,
which this is primarily meant for, later.

This commit is a slightly modified version of a previous patch sent in
by Tsuchiya Yuto.

Co-developed-by: Tsuchiya Yuto <kitakar@gmail.com>
Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 drivers/net/wireless/marvell/mwifiex/Makefile |  1 +
 drivers/net/wireless/marvell/mwifiex/pcie.c   |  4 +++
 drivers/net/wireless/marvell/mwifiex/pcie.h   |  1 +
 .../wireless/marvell/mwifiex/pcie_quirks.c    | 32 +++++++++++++++++++
 .../wireless/marvell/mwifiex/pcie_quirks.h    |  8 +++++
 5 files changed, 46 insertions(+)
 create mode 100644 drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
 create mode 100644 drivers/net/wireless/marvell/mwifiex/pcie_quirks.h

diff --git a/drivers/net/wireless/marvell/mwifiex/Makefile b/drivers/net/wireless/marvell/mwifiex/Makefile
index 162d557b78af..2bd00f40958e 100644
--- a/drivers/net/wireless/marvell/mwifiex/Makefile
+++ b/drivers/net/wireless/marvell/mwifiex/Makefile
@@ -49,6 +49,7 @@ mwifiex_sdio-y += sdio.o
 obj-$(CONFIG_MWIFIEX_SDIO) += mwifiex_sdio.o
 
 mwifiex_pcie-y += pcie.o
+mwifiex_pcie-y += pcie_quirks.o
 obj-$(CONFIG_MWIFIEX_PCIE) += mwifiex_pcie.o
 
 mwifiex_usb-y += usb.o
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 46517515ba72..a530832c9421 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -27,6 +27,7 @@
 #include "wmm.h"
 #include "11n.h"
 #include "pcie.h"
+#include "pcie_quirks.h"
 
 #define PCIE_VERSION	"1.0"
 #define DRV_NAME        "Marvell mwifiex PCIe"
@@ -410,6 +411,9 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
 			return ret;
 	}
 
+	/* check quirks */
+	mwifiex_initialize_quirks(card);
+
 	if (mwifiex_add_card(card, &card->fw_done, &pcie_ops,
 			     MWIFIEX_PCIE, &pdev->dev)) {
 		pr_err("%s failed\n", __func__);
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h b/drivers/net/wireless/marvell/mwifiex/pcie.h
index 5ed613d65709..981e330c77d7 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
@@ -244,6 +244,7 @@ struct pcie_service_card {
 	unsigned long work_flags;
 
 	bool pci_reset_ongoing;
+	unsigned long quirks;
 };
 
 static inline int
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
new file mode 100644
index 000000000000..4064f99b36ba
--- /dev/null
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * File for PCIe quirks.
+ */
+
+/* The low-level PCI operations will be performed in this file. Therefore,
+ * let's use dev_*() instead of mwifiex_dbg() here to avoid troubles (e.g.
+ * to avoid using mwifiex_adapter struct before init or wifi is powered
+ * down, or causes NULL ptr deref).
+ */
+
+#include <linux/dmi.h>
+
+#include "pcie_quirks.h"
+
+/* quirk table based on DMI matching */
+static const struct dmi_system_id mwifiex_quirk_table[] = {
+	{}
+};
+
+void mwifiex_initialize_quirks(struct pcie_service_card *card)
+{
+	struct pci_dev *pdev = card->dev;
+	const struct dmi_system_id *dmi_id;
+
+	dmi_id = dmi_first_match(mwifiex_quirk_table);
+	if (dmi_id)
+		card->quirks = (uintptr_t)dmi_id->driver_data;
+
+	if (!card->quirks)
+		dev_info(&pdev->dev, "no quirks enabled\n");
+}
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
new file mode 100644
index 000000000000..7a1fe3b3a61a
--- /dev/null
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for PCIe quirks.
+ */
+
+#include "pcie.h"
+
+void mwifiex_initialize_quirks(struct pcie_service_card *card);
-- 
2.31.1

Re: [PATCH v2 1/2] mwifiex: pcie: add DMI-based quirk implementation for Surface devices

[Kalle Valo]

Jonas Dreßler <verdre@v0yd.nl> writes:

> This commit adds the ability to apply device-specific quirks to the
> mwifiex driver. It uses DMI matching similar to the quirks brcmfmac uses
> with dmi.c. We'll add identifiers to match various MS Surface devices,
> which this is primarily meant for, later.
>
> This commit is a slightly modified version of a previous patch sent in
> by Tsuchiya Yuto.
>
> Co-developed-by: Tsuchiya Yuto <kitakar@gmail.com>
> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>

[...]

> --- /dev/null
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0

I prefer having a consistent license within a driver. So please either
convert mwifiex to use SPDX in every file or use the same license boiler
plate as used in the driver.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Jonas Dreßler]

From: Tsuchiya Yuto <kitakar@gmail.com>

To reset mwifiex on Surface gen4+ (Pro 4 or later gen) devices, it
seems that putting the wifi device into D3cold is required according
to errata.inf file on Windows installation (Windows/INF/errata.inf).

This patch adds a function that performs power-cycle (put into D3cold
then D0) and call the function at the end of reset_prepare().

Note: Need to also reset the parent device (bridge) of wifi on SB1;
it might be because the bridge of wifi always reports it's in D3hot.
When I tried to reset only the wifi device (not touching parent), it gave
the following error and the reset failed:

    acpi device:4b: Cannot transition to power state D0 for parent in D3hot
    mwifiex_pcie 0000:03:00.0: can't change power state from D3cold to D0 (config space inaccessible)

Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c   |   7 +
 .../wireless/marvell/mwifiex/pcie_quirks.c    | 123 ++++++++++++++++++
 .../wireless/marvell/mwifiex/pcie_quirks.h    |   3 +
 3 files changed, 133 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index a530832c9421..c6ccce426b49 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -528,6 +528,13 @@ static void mwifiex_pcie_reset_prepare(struct pci_dev *pdev)
 	mwifiex_shutdown_sw(adapter);
 	clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
 	clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
+
+	/* On MS Surface gen4+ devices FLR isn't effective to recover from
+	 * hangups, so we power-cycle the card instead.
+	 */
+	if (card->quirks & QUIRK_FW_RST_D3COLD)
+		mwifiex_pcie_reset_d3cold_quirk(pdev);
+
 	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
 
 	card->pci_reset_ongoing = true;
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
index 4064f99b36ba..b5f214fc1212 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
@@ -15,6 +15,72 @@
 
 /* quirk table based on DMI matching */
 static const struct dmi_system_id mwifiex_quirk_table[] = {
+	{
+		.ident = "Surface Pro 4",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 4"),
+		},
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
+	},
+	{
+		.ident = "Surface Pro 5",
+		.matches = {
+			/* match for SKU here due to generic product name "Surface Pro" */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1796"),
+		},
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
+	},
+	{
+		.ident = "Surface Pro 5 (LTE)",
+		.matches = {
+			/* match for SKU here due to generic product name "Surface Pro" */
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1807"),
+		},
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
+	},
+	{
+		.ident = "Surface Pro 6",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 6"),
+		},
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
+	},
+	{
+		.ident = "Surface Book 1",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book"),
+		},
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
+	},
+	{
+		.ident = "Surface Book 2",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book 2"),
+		},
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
+	},
+	{
+		.ident = "Surface Laptop 1",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop"),
+		},
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
+	},
+	{
+		.ident = "Surface Laptop 2",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop 2"),
+		},
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
+	},
 	{}
 };
 
@@ -29,4 +95,61 @@ void mwifiex_initialize_quirks(struct pcie_service_card *card)
 
 	if (!card->quirks)
 		dev_info(&pdev->dev, "no quirks enabled\n");
+	if (card->quirks & QUIRK_FW_RST_D3COLD)
+		dev_info(&pdev->dev, "quirk reset_d3cold enabled\n");
+}
+
+static void mwifiex_pcie_set_power_d3cold(struct pci_dev *pdev)
+{
+	dev_info(&pdev->dev, "putting into D3cold...\n");
+
+	pci_save_state(pdev);
+	if (pci_is_enabled(pdev))
+		pci_disable_device(pdev);
+	pci_set_power_state(pdev, PCI_D3cold);
+}
+
+static int mwifiex_pcie_set_power_d0(struct pci_dev *pdev)
+{
+	int ret;
+
+	dev_info(&pdev->dev, "putting into D0...\n");
+
+	pci_set_power_state(pdev, PCI_D0);
+	ret = pci_enable_device(pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "pci_enable_device failed\n");
+		return ret;
+	}
+	pci_restore_state(pdev);
+
+	return 0;
+}
+
+int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev)
+{
+	struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
+	int ret;
+
+	/* Power-cycle (put into D3cold then D0) */
+	dev_info(&pdev->dev, "Using reset_d3cold quirk to perform FW reset\n");
+
+	/* We need to perform power-cycle also for bridge of wifi because
+	 * on some devices (e.g. Surface Book 1), the OS for some reasons
+	 * can't know the real power state of the bridge.
+	 * When tried to power-cycle only wifi, the reset failed with the
+	 * following dmesg log:
+	 * "Cannot transition to power state D0 for parent in D3hot".
+	 */
+	mwifiex_pcie_set_power_d3cold(pdev);
+	mwifiex_pcie_set_power_d3cold(parent_pdev);
+
+	ret = mwifiex_pcie_set_power_d0(parent_pdev);
+	if (ret)
+		return ret;
+	ret = mwifiex_pcie_set_power_d0(pdev);
+	if (ret)
+		return ret;
+
+	return 0;
 }
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
index 7a1fe3b3a61a..549093067813 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
@@ -5,4 +5,7 @@
 
 #include "pcie.h"
 
+#define QUIRK_FW_RST_D3COLD	BIT(0)
+
 void mwifiex_initialize_quirks(struct pcie_service_card *card);
+int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev);
-- 
2.31.1

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Pali Rohár]

On Friday 09 July 2021 16:58:31 Jonas Dreßler wrote:
> From: Tsuchiya Yuto <kitakar@gmail.com>
> 
> To reset mwifiex on Surface gen4+ (Pro 4 or later gen) devices, it
> seems that putting the wifi device into D3cold is required according
> to errata.inf file on Windows installation (Windows/INF/errata.inf).
> 
> This patch adds a function that performs power-cycle (put into D3cold
> then D0) and call the function at the end of reset_prepare().
> 
> Note: Need to also reset the parent device (bridge) of wifi on SB1;
> it might be because the bridge of wifi always reports it's in D3hot.
> When I tried to reset only the wifi device (not touching parent), it gave
> the following error and the reset failed:
> 
>     acpi device:4b: Cannot transition to power state D0 for parent in D3hot
>     mwifiex_pcie 0000:03:00.0: can't change power state from D3cold to D0 (config space inaccessible)
> 
> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c   |   7 +
>  .../wireless/marvell/mwifiex/pcie_quirks.c    | 123 ++++++++++++++++++
>  .../wireless/marvell/mwifiex/pcie_quirks.h    |   3 +
>  3 files changed, 133 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index a530832c9421..c6ccce426b49 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -528,6 +528,13 @@ static void mwifiex_pcie_reset_prepare(struct pci_dev *pdev)
>  	mwifiex_shutdown_sw(adapter);
>  	clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
>  	clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
> +
> +	/* On MS Surface gen4+ devices FLR isn't effective to recover from
> +	 * hangups, so we power-cycle the card instead.
> +	 */
> +	if (card->quirks & QUIRK_FW_RST_D3COLD)
> +		mwifiex_pcie_reset_d3cold_quirk(pdev);
> +

Hello! Now I'm thinking loudly about this patch. Why this kind of reset
is needed only for Surface devices? AFAIK these 88W8897 chips are same
in all cards. Chip itself implements PCIe interface (and also SDIO) so
for me looks very strange if this 88W8897 PCIe device needs DMI specific
quirks. I cannot believe that Microsoft got some special version of
these chips from Marvell which are different than version uses on cards
in mPCIe form factor.

And now when I'm reading comment below about PCIe bridge to which is
this 88W8897 PCIe chip connected, is not this rather an issue in that
PCIe bridge (instead of mwifiex/88W8897) or in ACPI firmware which
controls this bridge?

Or are having other people same issues on mPCIe form factor wifi cards
with 88W8897 chips and then this quirk should not DMI dependent?

Note that I'm seeing issues with reset and other things also on chip
88W8997 when is connected to system via SDIO. These chips have both PCIe
and SDIO buses, it just depends which pins are used.

>  	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
>  
>  	card->pci_reset_ongoing = true;
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> index 4064f99b36ba..b5f214fc1212 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> @@ -15,6 +15,72 @@
>  
>  /* quirk table based on DMI matching */
>  static const struct dmi_system_id mwifiex_quirk_table[] = {
> +	{
> +		.ident = "Surface Pro 4",
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 4"),
> +		},
> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +	},
> +	{
> +		.ident = "Surface Pro 5",
> +		.matches = {
> +			/* match for SKU here due to generic product name "Surface Pro" */
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1796"),
> +		},
> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +	},
> +	{
> +		.ident = "Surface Pro 5 (LTE)",
> +		.matches = {
> +			/* match for SKU here due to generic product name "Surface Pro" */
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1807"),
> +		},
> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +	},
> +	{
> +		.ident = "Surface Pro 6",
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 6"),
> +		},
> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +	},
> +	{
> +		.ident = "Surface Book 1",
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book"),
> +		},
> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +	},
> +	{
> +		.ident = "Surface Book 2",
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book 2"),
> +		},
> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +	},
> +	{
> +		.ident = "Surface Laptop 1",
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop"),
> +		},
> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +	},
> +	{
> +		.ident = "Surface Laptop 2",
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop 2"),
> +		},
> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +	},
>  	{}
>  };
>  
> @@ -29,4 +95,61 @@ void mwifiex_initialize_quirks(struct pcie_service_card *card)
>  
>  	if (!card->quirks)
>  		dev_info(&pdev->dev, "no quirks enabled\n");
> +	if (card->quirks & QUIRK_FW_RST_D3COLD)
> +		dev_info(&pdev->dev, "quirk reset_d3cold enabled\n");
> +}
> +
> +static void mwifiex_pcie_set_power_d3cold(struct pci_dev *pdev)
> +{
> +	dev_info(&pdev->dev, "putting into D3cold...\n");
> +
> +	pci_save_state(pdev);
> +	if (pci_is_enabled(pdev))
> +		pci_disable_device(pdev);
> +	pci_set_power_state(pdev, PCI_D3cold);
> +}
> +
> +static int mwifiex_pcie_set_power_d0(struct pci_dev *pdev)
> +{
> +	int ret;
> +
> +	dev_info(&pdev->dev, "putting into D0...\n");
> +
> +	pci_set_power_state(pdev, PCI_D0);
> +	ret = pci_enable_device(pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "pci_enable_device failed\n");
> +		return ret;
> +	}
> +	pci_restore_state(pdev);
> +
> +	return 0;
> +}
> +
> +int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev)
> +{
> +	struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
> +	int ret;
> +
> +	/* Power-cycle (put into D3cold then D0) */
> +	dev_info(&pdev->dev, "Using reset_d3cold quirk to perform FW reset\n");
> +
> +	/* We need to perform power-cycle also for bridge of wifi because
> +	 * on some devices (e.g. Surface Book 1), the OS for some reasons
> +	 * can't know the real power state of the bridge.
> +	 * When tried to power-cycle only wifi, the reset failed with the
> +	 * following dmesg log:
> +	 * "Cannot transition to power state D0 for parent in D3hot".
> +	 */
> +	mwifiex_pcie_set_power_d3cold(pdev);
> +	mwifiex_pcie_set_power_d3cold(parent_pdev);
> +
> +	ret = mwifiex_pcie_set_power_d0(parent_pdev);
> +	if (ret)
> +		return ret;
> +	ret = mwifiex_pcie_set_power_d0(pdev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
>  }
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
> index 7a1fe3b3a61a..549093067813 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
> @@ -5,4 +5,7 @@
>  
>  #include "pcie.h"
>  
> +#define QUIRK_FW_RST_D3COLD	BIT(0)
> +
>  void mwifiex_initialize_quirks(struct pcie_service_card *card);
> +int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev);
> -- 
> 2.31.1
> 

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Jonas Dreßler]

On 7/9/21 5:18 PM, Pali Rohár wrote:
> On Friday 09 July 2021 16:58:31 Jonas Dreßler wrote:
>> From: Tsuchiya Yuto <kitakar@gmail.com>
>>
>> To reset mwifiex on Surface gen4+ (Pro 4 or later gen) devices, it
>> seems that putting the wifi device into D3cold is required according
>> to errata.inf file on Windows installation (Windows/INF/errata.inf).
>>
>> This patch adds a function that performs power-cycle (put into D3cold
>> then D0) and call the function at the end of reset_prepare().
>>
>> Note: Need to also reset the parent device (bridge) of wifi on SB1;
>> it might be because the bridge of wifi always reports it's in D3hot.
>> When I tried to reset only the wifi device (not touching parent), it gave
>> the following error and the reset failed:
>>
>>      acpi device:4b: Cannot transition to power state D0 for parent in D3hot
>>      mwifiex_pcie 0000:03:00.0: can't change power state from D3cold to D0 (config space inaccessible)
>>
>> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
>> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
>> ---
>>   drivers/net/wireless/marvell/mwifiex/pcie.c   |   7 +
>>   .../wireless/marvell/mwifiex/pcie_quirks.c    | 123 ++++++++++++++++++
>>   .../wireless/marvell/mwifiex/pcie_quirks.h    |   3 +
>>   3 files changed, 133 insertions(+)
>>
>> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
>> index a530832c9421..c6ccce426b49 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
>> @@ -528,6 +528,13 @@ static void mwifiex_pcie_reset_prepare(struct pci_dev *pdev)
>>   	mwifiex_shutdown_sw(adapter);
>>   	clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
>>   	clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
>> +
>> +	/* On MS Surface gen4+ devices FLR isn't effective to recover from
>> +	 * hangups, so we power-cycle the card instead.
>> +	 */
>> +	if (card->quirks & QUIRK_FW_RST_D3COLD)
>> +		mwifiex_pcie_reset_d3cold_quirk(pdev);
>> +
> 
> Hello! Now I'm thinking loudly about this patch. Why this kind of reset
> is needed only for Surface devices? AFAIK these 88W8897 chips are same
> in all cards. Chip itself implements PCIe interface (and also SDIO) so
> for me looks very strange if this 88W8897 PCIe device needs DMI specific
> quirks. I cannot believe that Microsoft got some special version of
> these chips from Marvell which are different than version uses on cards
> in mPCIe form factor.
> 
> And now when I'm reading comment below about PCIe bridge to which is
> this 88W8897 PCIe chip connected, is not this rather an issue in that
> PCIe bridge (instead of mwifiex/88W8897) or in ACPI firmware which
> controls this bridge?
> 
> Or are having other people same issues on mPCIe form factor wifi cards
> with 88W8897 chips and then this quirk should not DMI dependent?
> 
> Note that I'm seeing issues with reset and other things also on chip
> 88W8997 when is connected to system via SDIO. These chips have both PCIe
> and SDIO buses, it just depends which pins are used.
> 

Hi and thanks for the quick reply! Honestly I've no idea, this is just 
the first method we found that allows for a proper reset of the chip. 
What I know is that some Surface devices need that ACPI DSM call (the 
one that was done in the commit I dropped in this version of the 
patchset) to reset the chip instead of this method.

Afaik other devices with this chip don't need this resetting method, at 
least Marvell employees couldn't reproduce the issues on their testing 
devices.

So would you suggest we just try to match for the pci chip 88W8897 
instead? Then we'd probably have to check if there are any laptops where 
multiple devices are connected to the pci bridge as Amey suggested in a 
review before.

>>   	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
>>   
>>   	card->pci_reset_ongoing = true;
>> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
>> index 4064f99b36ba..b5f214fc1212 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
>> @@ -15,6 +15,72 @@
>>   
>>   /* quirk table based on DMI matching */
>>   static const struct dmi_system_id mwifiex_quirk_table[] = {
>> +	{
>> +		.ident = "Surface Pro 4",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 4"),
>> +		},
>> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
>> +	},
>> +	{
>> +		.ident = "Surface Pro 5",
>> +		.matches = {
>> +			/* match for SKU here due to generic product name "Surface Pro" */
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1796"),
>> +		},
>> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
>> +	},
>> +	{
>> +		.ident = "Surface Pro 5 (LTE)",
>> +		.matches = {
>> +			/* match for SKU here due to generic product name "Surface Pro" */
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1807"),
>> +		},
>> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
>> +	},
>> +	{
>> +		.ident = "Surface Pro 6",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 6"),
>> +		},
>> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
>> +	},
>> +	{
>> +		.ident = "Surface Book 1",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book"),
>> +		},
>> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
>> +	},
>> +	{
>> +		.ident = "Surface Book 2",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book 2"),
>> +		},
>> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
>> +	},
>> +	{
>> +		.ident = "Surface Laptop 1",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop"),
>> +		},
>> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
>> +	},
>> +	{
>> +		.ident = "Surface Laptop 2",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop 2"),
>> +		},
>> +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
>> +	},
>>   	{}
>>   };
>>   
>> @@ -29,4 +95,61 @@ void mwifiex_initialize_quirks(struct pcie_service_card *card)
>>   
>>   	if (!card->quirks)
>>   		dev_info(&pdev->dev, "no quirks enabled\n");
>> +	if (card->quirks & QUIRK_FW_RST_D3COLD)
>> +		dev_info(&pdev->dev, "quirk reset_d3cold enabled\n");
>> +}
>> +
>> +static void mwifiex_pcie_set_power_d3cold(struct pci_dev *pdev)
>> +{
>> +	dev_info(&pdev->dev, "putting into D3cold...\n");
>> +
>> +	pci_save_state(pdev);
>> +	if (pci_is_enabled(pdev))
>> +		pci_disable_device(pdev);
>> +	pci_set_power_state(pdev, PCI_D3cold);
>> +}
>> +
>> +static int mwifiex_pcie_set_power_d0(struct pci_dev *pdev)
>> +{
>> +	int ret;
>> +
>> +	dev_info(&pdev->dev, "putting into D0...\n");
>> +
>> +	pci_set_power_state(pdev, PCI_D0);
>> +	ret = pci_enable_device(pdev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "pci_enable_device failed\n");
>> +		return ret;
>> +	}
>> +	pci_restore_state(pdev);
>> +
>> +	return 0;
>> +}
>> +
>> +int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev)
>> +{
>> +	struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
>> +	int ret;
>> +
>> +	/* Power-cycle (put into D3cold then D0) */
>> +	dev_info(&pdev->dev, "Using reset_d3cold quirk to perform FW reset\n");
>> +
>> +	/* We need to perform power-cycle also for bridge of wifi because
>> +	 * on some devices (e.g. Surface Book 1), the OS for some reasons
>> +	 * can't know the real power state of the bridge.
>> +	 * When tried to power-cycle only wifi, the reset failed with the
>> +	 * following dmesg log:
>> +	 * "Cannot transition to power state D0 for parent in D3hot".
>> +	 */
>> +	mwifiex_pcie_set_power_d3cold(pdev);
>> +	mwifiex_pcie_set_power_d3cold(parent_pdev);
>> +
>> +	ret = mwifiex_pcie_set_power_d0(parent_pdev);
>> +	if (ret)
>> +		return ret;
>> +	ret = mwifiex_pcie_set_power_d0(pdev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>>   }
>> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
>> index 7a1fe3b3a61a..549093067813 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
>> +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
>> @@ -5,4 +5,7 @@
>>   
>>   #include "pcie.h"
>>   
>> +#define QUIRK_FW_RST_D3COLD	BIT(0)
>> +
>>   void mwifiex_initialize_quirks(struct pcie_service_card *card);
>> +int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev);
>> -- 
>> 2.31.1
>>

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Pali Rohár]

On Friday 09 July 2021 17:33:34 Jonas Dreßler wrote:
> On 7/9/21 5:18 PM, Pali Rohár wrote:
> > On Friday 09 July 2021 16:58:31 Jonas Dreßler wrote:
> > > From: Tsuchiya Yuto <kitakar@gmail.com>
> > > 
> > > To reset mwifiex on Surface gen4+ (Pro 4 or later gen) devices, it
> > > seems that putting the wifi device into D3cold is required according
> > > to errata.inf file on Windows installation (Windows/INF/errata.inf).
> > > 
> > > This patch adds a function that performs power-cycle (put into D3cold
> > > then D0) and call the function at the end of reset_prepare().
> > > 
> > > Note: Need to also reset the parent device (bridge) of wifi on SB1;
> > > it might be because the bridge of wifi always reports it's in D3hot.
> > > When I tried to reset only the wifi device (not touching parent), it gave
> > > the following error and the reset failed:
> > > 
> > >      acpi device:4b: Cannot transition to power state D0 for parent in D3hot
> > >      mwifiex_pcie 0000:03:00.0: can't change power state from D3cold to D0 (config space inaccessible)
> > > 
> > > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > > Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> > > ---
> > >   drivers/net/wireless/marvell/mwifiex/pcie.c   |   7 +
> > >   .../wireless/marvell/mwifiex/pcie_quirks.c    | 123 ++++++++++++++++++
> > >   .../wireless/marvell/mwifiex/pcie_quirks.h    |   3 +
> > >   3 files changed, 133 insertions(+)
> > > 
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > index a530832c9421..c6ccce426b49 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > @@ -528,6 +528,13 @@ static void mwifiex_pcie_reset_prepare(struct pci_dev *pdev)
> > >   	mwifiex_shutdown_sw(adapter);
> > >   	clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
> > >   	clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
> > > +
> > > +	/* On MS Surface gen4+ devices FLR isn't effective to recover from
> > > +	 * hangups, so we power-cycle the card instead.
> > > +	 */
> > > +	if (card->quirks & QUIRK_FW_RST_D3COLD)
> > > +		mwifiex_pcie_reset_d3cold_quirk(pdev);
> > > +
> > 
> > Hello! Now I'm thinking loudly about this patch. Why this kind of reset
> > is needed only for Surface devices? AFAIK these 88W8897 chips are same
> > in all cards. Chip itself implements PCIe interface (and also SDIO) so
> > for me looks very strange if this 88W8897 PCIe device needs DMI specific
> > quirks. I cannot believe that Microsoft got some special version of
> > these chips from Marvell which are different than version uses on cards
> > in mPCIe form factor.
> > 
> > And now when I'm reading comment below about PCIe bridge to which is
> > this 88W8897 PCIe chip connected, is not this rather an issue in that
> > PCIe bridge (instead of mwifiex/88W8897) or in ACPI firmware which
> > controls this bridge?
> > 
> > Or are having other people same issues on mPCIe form factor wifi cards
> > with 88W8897 chips and then this quirk should not DMI dependent?
> > 
> > Note that I'm seeing issues with reset and other things also on chip
> > 88W8997 when is connected to system via SDIO. These chips have both PCIe
> > and SDIO buses, it just depends which pins are used.
> > 
> 
> Hi and thanks for the quick reply! Honestly I've no idea, this is just the
> first method we found that allows for a proper reset of the chip. What I
> know is that some Surface devices need that ACPI DSM call (the one that was
> done in the commit I dropped in this version of the patchset) to reset the
> chip instead of this method.
> 
> Afaik other devices with this chip don't need this resetting method, at
> least Marvell employees couldn't reproduce the issues on their testing
> devices.
> 
> So would you suggest we just try to match for the pci chip 88W8897 instead?

Hello! Such suggestion makes sense when we know that it is 88W8897
issue. But if you got information that issue cannot be reproduced on
other 88W8897 cards then matching 88W8897 is not correct.

From all this information looks like that it is problem in (Microsoft?)
PCIe bridge to which is card connected. Otherwise I do not reason how it
can be 88W8897 affected. Either it is reproducible on 88W8897 cards also
in other devices or issue is not on 88W8897 card.

> Then we'd probably have to check if there are any laptops where multiple
> devices are connected to the pci bridge as Amey suggested in a review
> before.

Well, I do not know... But if this is issue with PCIe bridge then
similar issue could be observed also for other PCIe devices with this
PCIe bridge. But question is if there are other laptops with this PCIe
bridge. And also it can be a problem in ACPI firmware on those Surface
devices, which implements some PCIe bridge functionality. So it is
possible that issue is with PCIe bridge, not in HW, but in SW/firmware
part which can be Microsoft specific... So too many questions to which
we do not know answers.

Could you provide output of 'lspci -nn -vv' and 'lspci -tvnn' on
affected machines? If you have already sent it in some previous email,
just send a link. At least I'm not able to find it right now and output
may contain something useful...

> > >   	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
> > >   	card->pci_reset_ongoing = true;
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> > > index 4064f99b36ba..b5f214fc1212 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> > > @@ -15,6 +15,72 @@
> > >   /* quirk table based on DMI matching */
> > >   static const struct dmi_system_id mwifiex_quirk_table[] = {
> > > +	{
> > > +		.ident = "Surface Pro 4",
> > > +		.matches = {
> > > +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> > > +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 4"),
> > > +		},
> > > +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> > > +	},
> > > +	{
> > > +		.ident = "Surface Pro 5",
> > > +		.matches = {
> > > +			/* match for SKU here due to generic product name "Surface Pro" */
> > > +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> > > +			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1796"),
> > > +		},
> > > +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> > > +	},
> > > +	{
> > > +		.ident = "Surface Pro 5 (LTE)",
> > > +		.matches = {
> > > +			/* match for SKU here due to generic product name "Surface Pro" */
> > > +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> > > +			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1807"),
> > > +		},
> > > +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> > > +	},
> > > +	{
> > > +		.ident = "Surface Pro 6",
> > > +		.matches = {
> > > +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> > > +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 6"),
> > > +		},
> > > +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> > > +	},
> > > +	{
> > > +		.ident = "Surface Book 1",
> > > +		.matches = {
> > > +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> > > +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book"),
> > > +		},
> > > +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> > > +	},
> > > +	{
> > > +		.ident = "Surface Book 2",
> > > +		.matches = {
> > > +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> > > +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book 2"),
> > > +		},
> > > +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> > > +	},
> > > +	{
> > > +		.ident = "Surface Laptop 1",
> > > +		.matches = {
> > > +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> > > +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop"),
> > > +		},
> > > +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> > > +	},
> > > +	{
> > > +		.ident = "Surface Laptop 2",
> > > +		.matches = {
> > > +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> > > +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop 2"),
> > > +		},
> > > +		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> > > +	},
> > >   	{}
> > >   };
> > > @@ -29,4 +95,61 @@ void mwifiex_initialize_quirks(struct pcie_service_card *card)
> > >   	if (!card->quirks)
> > >   		dev_info(&pdev->dev, "no quirks enabled\n");
> > > +	if (card->quirks & QUIRK_FW_RST_D3COLD)
> > > +		dev_info(&pdev->dev, "quirk reset_d3cold enabled\n");
> > > +}
> > > +
> > > +static void mwifiex_pcie_set_power_d3cold(struct pci_dev *pdev)
> > > +{
> > > +	dev_info(&pdev->dev, "putting into D3cold...\n");
> > > +
> > > +	pci_save_state(pdev);
> > > +	if (pci_is_enabled(pdev))
> > > +		pci_disable_device(pdev);
> > > +	pci_set_power_state(pdev, PCI_D3cold);
> > > +}
> > > +
> > > +static int mwifiex_pcie_set_power_d0(struct pci_dev *pdev)
> > > +{
> > > +	int ret;
> > > +
> > > +	dev_info(&pdev->dev, "putting into D0...\n");
> > > +
> > > +	pci_set_power_state(pdev, PCI_D0);
> > > +	ret = pci_enable_device(pdev);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "pci_enable_device failed\n");
> > > +		return ret;
> > > +	}
> > > +	pci_restore_state(pdev);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev)
> > > +{
> > > +	struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
> > > +	int ret;
> > > +
> > > +	/* Power-cycle (put into D3cold then D0) */
> > > +	dev_info(&pdev->dev, "Using reset_d3cold quirk to perform FW reset\n");
> > > +
> > > +	/* We need to perform power-cycle also for bridge of wifi because
> > > +	 * on some devices (e.g. Surface Book 1), the OS for some reasons
> > > +	 * can't know the real power state of the bridge.
> > > +	 * When tried to power-cycle only wifi, the reset failed with the
> > > +	 * following dmesg log:
> > > +	 * "Cannot transition to power state D0 for parent in D3hot".
> > > +	 */
> > > +	mwifiex_pcie_set_power_d3cold(pdev);
> > > +	mwifiex_pcie_set_power_d3cold(parent_pdev);
> > > +
> > > +	ret = mwifiex_pcie_set_power_d0(parent_pdev);
> > > +	if (ret)
> > > +		return ret;
> > > +	ret = mwifiex_pcie_set_power_d0(pdev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > >   }
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
> > > index 7a1fe3b3a61a..549093067813 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
> > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
> > > @@ -5,4 +5,7 @@
> > >   #include "pcie.h"
> > > +#define QUIRK_FW_RST_D3COLD	BIT(0)
> > > +
> > >   void mwifiex_initialize_quirks(struct pcie_service_card *card);
> > > +int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev);
> > > -- 
> > > 2.31.1
> > > 
> 

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Maximilian Luz]

On 7/9/21 6:12 PM, Pali Rohár wrote:

[...]

>>> Hello! Now I'm thinking loudly about this patch. Why this kind of reset
>>> is needed only for Surface devices? AFAIK these 88W8897 chips are same
>>> in all cards. Chip itself implements PCIe interface (and also SDIO) so
>>> for me looks very strange if this 88W8897 PCIe device needs DMI specific
>>> quirks. I cannot believe that Microsoft got some special version of
>>> these chips from Marvell which are different than version uses on cards
>>> in mPCIe form factor.
>>>
>>> And now when I'm reading comment below about PCIe bridge to which is
>>> this 88W8897 PCIe chip connected, is not this rather an issue in that
>>> PCIe bridge (instead of mwifiex/88W8897) or in ACPI firmware which
>>> controls this bridge?
>>>
>>> Or are having other people same issues on mPCIe form factor wifi cards
>>> with 88W8897 chips and then this quirk should not DMI dependent?
>>>
>>> Note that I'm seeing issues with reset and other things also on chip
>>> 88W8997 when is connected to system via SDIO. These chips have both PCIe
>>> and SDIO buses, it just depends which pins are used.
>>>
>>
>> Hi and thanks for the quick reply! Honestly I've no idea, this is just the
>> first method we found that allows for a proper reset of the chip. What I
>> know is that some Surface devices need that ACPI DSM call (the one that was
>> done in the commit I dropped in this version of the patchset) to reset the
>> chip instead of this method.
>>
>> Afaik other devices with this chip don't need this resetting method, at
>> least Marvell employees couldn't reproduce the issues on their testing
>> devices.
>>
>> So would you suggest we just try to match for the pci chip 88W8897 instead?
> 
> Hello! Such suggestion makes sense when we know that it is 88W8897
> issue. But if you got information that issue cannot be reproduced on
> other 88W8897 cards then matching 88W8897 is not correct.
> 
>  From all this information looks like that it is problem in (Microsoft?)
> PCIe bridge to which is card connected. Otherwise I do not reason how it
> can be 88W8897 affected. Either it is reproducible on 88W8897 cards also
> in other devices or issue is not on 88W8897 card.

I doubt that it's an issue with the PCIe bridge (itself at least). The
same type of bridge is used for both dGPU and NVME SSD on my device (see
lspci output below) and those work fine. Also if I'm seeing that right
it's from the Intel CPU, so my guess is that a lot more people would
have issues with that then.

I don't know about the hardware side, so it might be possible that it's
an issue with integrating both bridge and wifi chip, in which case it's
still probably best handled via DMI quirks unless we know more.

Also as Tsuchiya mentioned in his original submission, on Windows the
device is reset via this D3cold method. I've only skimmed that
errata.inf file mentioned, but I think this is what he's referring to:

   Controls whether ACPIDeviceEnableD3ColdOnSurpriseRemoval rule will be
   evaluated or not on a given platform. Currently
   ACPIDeviceEnableD3ColdOnSurpriseRemoval rule only needs to be
   evaluated on Surface platforms which contain the Marvell WiFi
   controller which depends on device going through D3Cold as part of
   surprise-removal.

and

   Starting with Windows releases *after* Blue, ACPI will not put
   surprise-removed devices into D3Cold automatically. Some known
   scenarios (viz. WiFi reset/recovery) rely on the device cycling
   through D3Cold on surprise-removal. This hack allows surprise-removed
   devices to be put into D3Cold (if supported by the stack).

So, as far as I can tell, the chip doesn't like to be surprise-removed
(which seems to happen during reset) and then needs to be power-cycled,
which I think is likely due to some issue with firmware state.

So the quirk on Windows seems very Surface specific.

There also seem a bunch of revisions of these chips around, for example
my SB2 is affected by a bug that we've tied to the specific hardware
revision which causes some issues with host-sleep (IIRC chip switches
rapidly between wake and sleep states without any external influence,
which is not how it should behave and how it does behave on a later
hardware revision).

>> Then we'd probably have to check if there are any laptops where multiple
>> devices are connected to the pci bridge as Amey suggested in a review
>> before.
> 
> Well, I do not know... But if this is issue with PCIe bridge then
> similar issue could be observed also for other PCIe devices with this
> PCIe bridge. But question is if there are other laptops with this PCIe
> bridge. And also it can be a problem in ACPI firmware on those Surface
> devices, which implements some PCIe bridge functionality. So it is
> possible that issue is with PCIe bridge, not in HW, but in SW/firmware
> part which can be Microsoft specific... So too many questions to which
> we do not know answers.
> 
> Could you provide output of 'lspci -nn -vv' and 'lspci -tvnn' on
> affected machines? If you have already sent it in some previous email,
> just send a link. At least I'm not able to find it right now and output
> may contain something useful...

 From my Surface Book 2 (with the same issue):

  - lspci -tvnn: https://paste.ubuntu.com/p/mm3YpcZJ8N/
  - lspci -vv -nn: https://paste.ubuntu.com/p/dctTDP738N/

Regards,
Max

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Pali Rohár]

On Friday 09 July 2021 19:03:37 Maximilian Luz wrote:
> On 7/9/21 6:12 PM, Pali Rohár wrote:
> 
> [...]
> 
> > > > Hello! Now I'm thinking loudly about this patch. Why this kind of reset
> > > > is needed only for Surface devices? AFAIK these 88W8897 chips are same
> > > > in all cards. Chip itself implements PCIe interface (and also SDIO) so
> > > > for me looks very strange if this 88W8897 PCIe device needs DMI specific
> > > > quirks. I cannot believe that Microsoft got some special version of
> > > > these chips from Marvell which are different than version uses on cards
> > > > in mPCIe form factor.
> > > > 
> > > > And now when I'm reading comment below about PCIe bridge to which is
> > > > this 88W8897 PCIe chip connected, is not this rather an issue in that
> > > > PCIe bridge (instead of mwifiex/88W8897) or in ACPI firmware which
> > > > controls this bridge?
> > > > 
> > > > Or are having other people same issues on mPCIe form factor wifi cards
> > > > with 88W8897 chips and then this quirk should not DMI dependent?
> > > > 
> > > > Note that I'm seeing issues with reset and other things also on chip
> > > > 88W8997 when is connected to system via SDIO. These chips have both PCIe
> > > > and SDIO buses, it just depends which pins are used.
> > > > 
> > > 
> > > Hi and thanks for the quick reply! Honestly I've no idea, this is just the
> > > first method we found that allows for a proper reset of the chip. What I
> > > know is that some Surface devices need that ACPI DSM call (the one that was
> > > done in the commit I dropped in this version of the patchset) to reset the
> > > chip instead of this method.
> > > 
> > > Afaik other devices with this chip don't need this resetting method, at
> > > least Marvell employees couldn't reproduce the issues on their testing
> > > devices.
> > > 
> > > So would you suggest we just try to match for the pci chip 88W8897 instead?
> > 
> > Hello! Such suggestion makes sense when we know that it is 88W8897
> > issue. But if you got information that issue cannot be reproduced on
> > other 88W8897 cards then matching 88W8897 is not correct.
> > 
> >  From all this information looks like that it is problem in (Microsoft?)
> > PCIe bridge to which is card connected. Otherwise I do not reason how it
> > can be 88W8897 affected. Either it is reproducible on 88W8897 cards also
> > in other devices or issue is not on 88W8897 card.
> 
> I doubt that it's an issue with the PCIe bridge (itself at least). The
> same type of bridge is used for both dGPU and NVME SSD on my device (see
> lspci output below) and those work fine. Also if I'm seeing that right
> it's from the Intel CPU, so my guess is that a lot more people would
> have issues with that then.

From information below it seems to be related to surprise removal.
Therefore is surprise removal working without issue for dGPU or NVME
SSD? Not all PCIe bridges support surprise removal...

> 
> I don't know about the hardware side, so it might be possible that it's
> an issue with integrating both bridge and wifi chip, in which case it's
> still probably best handled via DMI quirks unless we know more.
> 
> Also as Tsuchiya mentioned in his original submission, on Windows the
> device is reset via this D3cold method. I've only skimmed that
> errata.inf file mentioned, but I think this is what he's referring to:
> 
>   Controls whether ACPIDeviceEnableD3ColdOnSurpriseRemoval rule will be
>   evaluated or not on a given platform. Currently
>   ACPIDeviceEnableD3ColdOnSurpriseRemoval rule only needs to be
>   evaluated on Surface platforms which contain the Marvell WiFi
>   controller which depends on device going through D3Cold as part of
>   surprise-removal.
> 
> and
> 
>   Starting with Windows releases *after* Blue, ACPI will not put
>   surprise-removed devices into D3Cold automatically. Some known
>   scenarios (viz. WiFi reset/recovery) rely on the device cycling
>   through D3Cold on surprise-removal. This hack allows surprise-removed
>   devices to be put into D3Cold (if supported by the stack).
> 
> So, as far as I can tell, the chip doesn't like to be surprise-removed
> (which seems to happen during reset) and then needs to be power-cycled,
> which I think is likely due to some issue with firmware state.

Thanks for information. This really does not look like PCIe bridge
specific if bridge itself can handle surprise-removed devices. lspci can
tell us if bridge supports it or not (see below).

> So the quirk on Windows seems very Surface specific.
> 
> There also seem a bunch of revisions of these chips around, for example
> my SB2 is affected by a bug that we've tied to the specific hardware
> revision which causes some issues with host-sleep (IIRC chip switches
> rapidly between wake and sleep states without any external influence,
> which is not how it should behave and how it does behave on a later
> hardware revision).

Interesting... This looks like the issue can be in 88W8897 chip and
needs some special conditions to trigger? And Surface is triggering it
always?

> > > Then we'd probably have to check if there are any laptops where multiple
> > > devices are connected to the pci bridge as Amey suggested in a review
> > > before.
> > 
> > Well, I do not know... But if this is issue with PCIe bridge then
> > similar issue could be observed also for other PCIe devices with this
> > PCIe bridge. But question is if there are other laptops with this PCIe
> > bridge. And also it can be a problem in ACPI firmware on those Surface
> > devices, which implements some PCIe bridge functionality. So it is
> > possible that issue is with PCIe bridge, not in HW, but in SW/firmware
> > part which can be Microsoft specific... So too many questions to which
> > we do not know answers.
> > 
> > Could you provide output of 'lspci -nn -vv' and 'lspci -tvnn' on
> > affected machines? If you have already sent it in some previous email,
> > just send a link. At least I'm not able to find it right now and output
> > may contain something useful...
> 
> From my Surface Book 2 (with the same issue):
> 
>  - lspci -tvnn: https://paste.ubuntu.com/p/mm3YpcZJ8N/
>  - lspci -vv -nn: https://paste.ubuntu.com/p/dctTDP738N/

Could you re-run lspci under root account? There are missing important
parts like "Capabilities: <access denied>" where is information if
bridge supports surprise removal or not.

> Regards,
> Max

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Maximilian Luz]

On 7/9/21 7:30 PM, Pali Rohár wrote:
> On Friday 09 July 2021 19:03:37 Maximilian Luz wrote:
>> On 7/9/21 6:12 PM, Pali Rohár wrote:
>>
>> [...]
>>
>>>>> Hello! Now I'm thinking loudly about this patch. Why this kind of reset
>>>>> is needed only for Surface devices? AFAIK these 88W8897 chips are same
>>>>> in all cards. Chip itself implements PCIe interface (and also SDIO) so
>>>>> for me looks very strange if this 88W8897 PCIe device needs DMI specific
>>>>> quirks. I cannot believe that Microsoft got some special version of
>>>>> these chips from Marvell which are different than version uses on cards
>>>>> in mPCIe form factor.
>>>>>
>>>>> And now when I'm reading comment below about PCIe bridge to which is
>>>>> this 88W8897 PCIe chip connected, is not this rather an issue in that
>>>>> PCIe bridge (instead of mwifiex/88W8897) or in ACPI firmware which
>>>>> controls this bridge?
>>>>>
>>>>> Or are having other people same issues on mPCIe form factor wifi cards
>>>>> with 88W8897 chips and then this quirk should not DMI dependent?
>>>>>
>>>>> Note that I'm seeing issues with reset and other things also on chip
>>>>> 88W8997 when is connected to system via SDIO. These chips have both PCIe
>>>>> and SDIO buses, it just depends which pins are used.
>>>>>
>>>>
>>>> Hi and thanks for the quick reply! Honestly I've no idea, this is just the
>>>> first method we found that allows for a proper reset of the chip. What I
>>>> know is that some Surface devices need that ACPI DSM call (the one that was
>>>> done in the commit I dropped in this version of the patchset) to reset the
>>>> chip instead of this method.
>>>>
>>>> Afaik other devices with this chip don't need this resetting method, at
>>>> least Marvell employees couldn't reproduce the issues on their testing
>>>> devices.
>>>>
>>>> So would you suggest we just try to match for the pci chip 88W8897 instead?
>>>
>>> Hello! Such suggestion makes sense when we know that it is 88W8897
>>> issue. But if you got information that issue cannot be reproduced on
>>> other 88W8897 cards then matching 88W8897 is not correct.
>>>
>>>   From all this information looks like that it is problem in (Microsoft?)
>>> PCIe bridge to which is card connected. Otherwise I do not reason how it
>>> can be 88W8897 affected. Either it is reproducible on 88W8897 cards also
>>> in other devices or issue is not on 88W8897 card.
>>
>> I doubt that it's an issue with the PCIe bridge (itself at least). The
>> same type of bridge is used for both dGPU and NVME SSD on my device (see
>> lspci output below) and those work fine. Also if I'm seeing that right
>> it's from the Intel CPU, so my guess is that a lot more people would
>> have issues with that then.
> 
>  From information below it seems to be related to surprise removal.
> Therefore is surprise removal working without issue for dGPU or NVME
> SSD? Not all PCIe bridges support surprise removal...

The dGPU on the Surface Book 2 is detachable (the whole base where that
is placed can be removed). As far as I can tell surprise removal works
perfectly fine for that one. The only thing that it needs is a driver for
out-of-band hot-plug signalling if the device is in D3cold while removed
as hotplug/removal notifications via PCI don't work in D3cold (this
works via ACPI, there is as far as I can tell no such mechanism for
WiFi, probably since it's not intended to be hot-unplugged).

>> I don't know about the hardware side, so it might be possible that it's
>> an issue with integrating both bridge and wifi chip, in which case it's
>> still probably best handled via DMI quirks unless we know more.
>>
>> Also as Tsuchiya mentioned in his original submission, on Windows the
>> device is reset via this D3cold method. I've only skimmed that
>> errata.inf file mentioned, but I think this is what he's referring to:
>>
>>    Controls whether ACPIDeviceEnableD3ColdOnSurpriseRemoval rule will be
>>    evaluated or not on a given platform. Currently
>>    ACPIDeviceEnableD3ColdOnSurpriseRemoval rule only needs to be
>>    evaluated on Surface platforms which contain the Marvell WiFi
>>    controller which depends on device going through D3Cold as part of
>>    surprise-removal.
>>
>> and
>>
>>    Starting with Windows releases *after* Blue, ACPI will not put
>>    surprise-removed devices into D3Cold automatically. Some known
>>    scenarios (viz. WiFi reset/recovery) rely on the device cycling
>>    through D3Cold on surprise-removal. This hack allows surprise-removed
>>    devices to be put into D3Cold (if supported by the stack).
>>
>> So, as far as I can tell, the chip doesn't like to be surprise-removed
>> (which seems to happen during reset) and then needs to be power-cycled,
>> which I think is likely due to some issue with firmware state.
> 
> Thanks for information. This really does not look like PCIe bridge
> specific if bridge itself can handle surprise-removed devices. lspci can
> tell us if bridge supports it or not (see below).
> 
>> So the quirk on Windows seems very Surface specific.
>>
>> There also seem a bunch of revisions of these chips around, for example
>> my SB2 is affected by a bug that we've tied to the specific hardware
>> revision which causes some issues with host-sleep (IIRC chip switches
>> rapidly between wake and sleep states without any external influence,
>> which is not how it should behave and how it does behave on a later
>> hardware revision).
> 
> Interesting... This looks like the issue can be in 88W8897 chip and
> needs some special conditions to trigger? And Surface is triggering it
> always?

Not always. It's been a while since I've been actively looking at this
and I'm not sure we ever had a good way to reproduce this. Also, I've
never really dealt with it as in-depth as Tsuchiya and Jonas have.

My (very) quick attempt ('echo 1 > /sys/bus/pci/.../reset) at
reproducing this didn't work, so I think at very least a network
connection needs to be active. Unfortunately I can't test that with a
network connection (and without compiling a custom kernel for which I
don't have the time right now) because there's currently another bug
deadlocking on device removal if there's an active connection during
removal (which also seems to trigger on reset). That one ill be fixed
by

   https://lore.kernel.org/linux-wireless/20210515024227.2159311-1-briannorris@chromium.org/

Jonas might know more.

>>>> Then we'd probably have to check if there are any laptops where multiple
>>>> devices are connected to the pci bridge as Amey suggested in a review
>>>> before.
>>>
>>> Well, I do not know... But if this is issue with PCIe bridge then
>>> similar issue could be observed also for other PCIe devices with this
>>> PCIe bridge. But question is if there are other laptops with this PCIe
>>> bridge. And also it can be a problem in ACPI firmware on those Surface
>>> devices, which implements some PCIe bridge functionality. So it is
>>> possible that issue is with PCIe bridge, not in HW, but in SW/firmware
>>> part which can be Microsoft specific... So too many questions to which
>>> we do not know answers.
>>>
>>> Could you provide output of 'lspci -nn -vv' and 'lspci -tvnn' on
>>> affected machines? If you have already sent it in some previous email,
>>> just send a link. At least I'm not able to find it right now and output
>>> may contain something useful...
>>
>>  From my Surface Book 2 (with the same issue):
>>
>>   - lspci -tvnn: https://paste.ubuntu.com/p/mm3YpcZJ8N/
>>   - lspci -vv -nn: https://paste.ubuntu.com/p/dctTDP738N/
> 
> Could you re-run lspci under root account? There are missing important
> parts like "Capabilities: <access denied>" where is information if
> bridge supports surprise removal or not.

Ah sorry, sure thing. Here's the updated lspci -nn -vv log:

   https://paste.ubuntu.com/p/fzsmCvm86Y/

The log for lspci -tvnn is the same.

>> Regards,
>> Max

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Pali Rohár]

On Friday 09 July 2021 20:16:49 Maximilian Luz wrote:
> On 7/9/21 7:30 PM, Pali Rohár wrote:
> > On Friday 09 July 2021 19:03:37 Maximilian Luz wrote:
> > > On 7/9/21 6:12 PM, Pali Rohár wrote:
> > > 
> > > [...]
> > > 
> > > > > > Hello! Now I'm thinking loudly about this patch. Why this kind of reset
> > > > > > is needed only for Surface devices? AFAIK these 88W8897 chips are same
> > > > > > in all cards. Chip itself implements PCIe interface (and also SDIO) so
> > > > > > for me looks very strange if this 88W8897 PCIe device needs DMI specific
> > > > > > quirks. I cannot believe that Microsoft got some special version of
> > > > > > these chips from Marvell which are different than version uses on cards
> > > > > > in mPCIe form factor.
> > > > > > 
> > > > > > And now when I'm reading comment below about PCIe bridge to which is
> > > > > > this 88W8897 PCIe chip connected, is not this rather an issue in that
> > > > > > PCIe bridge (instead of mwifiex/88W8897) or in ACPI firmware which
> > > > > > controls this bridge?
> > > > > > 
> > > > > > Or are having other people same issues on mPCIe form factor wifi cards
> > > > > > with 88W8897 chips and then this quirk should not DMI dependent?
> > > > > > 
> > > > > > Note that I'm seeing issues with reset and other things also on chip
> > > > > > 88W8997 when is connected to system via SDIO. These chips have both PCIe
> > > > > > and SDIO buses, it just depends which pins are used.
> > > > > > 
> > > > > 
> > > > > Hi and thanks for the quick reply! Honestly I've no idea, this is just the
> > > > > first method we found that allows for a proper reset of the chip. What I
> > > > > know is that some Surface devices need that ACPI DSM call (the one that was
> > > > > done in the commit I dropped in this version of the patchset) to reset the
> > > > > chip instead of this method.
> > > > > 
> > > > > Afaik other devices with this chip don't need this resetting method, at
> > > > > least Marvell employees couldn't reproduce the issues on their testing
> > > > > devices.
> > > > > 
> > > > > So would you suggest we just try to match for the pci chip 88W8897 instead?
> > > > 
> > > > Hello! Such suggestion makes sense when we know that it is 88W8897
> > > > issue. But if you got information that issue cannot be reproduced on
> > > > other 88W8897 cards then matching 88W8897 is not correct.
> > > > 
> > > >   From all this information looks like that it is problem in (Microsoft?)
> > > > PCIe bridge to which is card connected. Otherwise I do not reason how it
> > > > can be 88W8897 affected. Either it is reproducible on 88W8897 cards also
> > > > in other devices or issue is not on 88W8897 card.
> > > 
> > > I doubt that it's an issue with the PCIe bridge (itself at least). The
> > > same type of bridge is used for both dGPU and NVME SSD on my device (see
> > > lspci output below) and those work fine. Also if I'm seeing that right
> > > it's from the Intel CPU, so my guess is that a lot more people would
> > > have issues with that then.
> > 
> >  From information below it seems to be related to surprise removal.
> > Therefore is surprise removal working without issue for dGPU or NVME
> > SSD? Not all PCIe bridges support surprise removal...
> 
> The dGPU on the Surface Book 2 is detachable (the whole base where that
> is placed can be removed). As far as I can tell surprise removal works
> perfectly fine for that one. The only thing that it needs is a driver for
> out-of-band hot-plug signalling if the device is in D3cold while removed
> as hotplug/removal notifications via PCI don't work in D3cold (this
> works via ACPI, there is as far as I can tell no such mechanism for
> WiFi, probably since it's not intended to be hot-unplugged).

Ok. Thank you for confirmation.

> > > I don't know about the hardware side, so it might be possible that it's
> > > an issue with integrating both bridge and wifi chip, in which case it's
> > > still probably best handled via DMI quirks unless we know more.
> > > 
> > > Also as Tsuchiya mentioned in his original submission, on Windows the
> > > device is reset via this D3cold method. I've only skimmed that
> > > errata.inf file mentioned, but I think this is what he's referring to:
> > > 
> > >    Controls whether ACPIDeviceEnableD3ColdOnSurpriseRemoval rule will be
> > >    evaluated or not on a given platform. Currently
> > >    ACPIDeviceEnableD3ColdOnSurpriseRemoval rule only needs to be
> > >    evaluated on Surface platforms which contain the Marvell WiFi
> > >    controller which depends on device going through D3Cold as part of
> > >    surprise-removal.
> > > 
> > > and
> > > 
> > >    Starting with Windows releases *after* Blue, ACPI will not put
> > >    surprise-removed devices into D3Cold automatically. Some known
> > >    scenarios (viz. WiFi reset/recovery) rely on the device cycling
> > >    through D3Cold on surprise-removal. This hack allows surprise-removed
> > >    devices to be put into D3Cold (if supported by the stack).
> > > 
> > > So, as far as I can tell, the chip doesn't like to be surprise-removed
> > > (which seems to happen during reset) and then needs to be power-cycled,
> > > which I think is likely due to some issue with firmware state.
> > 
> > Thanks for information. This really does not look like PCIe bridge
> > specific if bridge itself can handle surprise-removed devices. lspci can
> > tell us if bridge supports it or not (see below).
> > 
> > > So the quirk on Windows seems very Surface specific.
> > > 
> > > There also seem a bunch of revisions of these chips around, for example
> > > my SB2 is affected by a bug that we've tied to the specific hardware
> > > revision which causes some issues with host-sleep (IIRC chip switches
> > > rapidly between wake and sleep states without any external influence,
> > > which is not how it should behave and how it does behave on a later
> > > hardware revision).
> > 
> > Interesting... This looks like the issue can be in 88W8897 chip and
> > needs some special conditions to trigger? And Surface is triggering it
> > always?
> 
> Not always. It's been a while since I've been actively looking at this
> and I'm not sure we ever had a good way to reproduce this. Also, I've
> never really dealt with it as in-depth as Tsuchiya and Jonas have.
> 
> My (very) quick attempt ('echo 1 > /sys/bus/pci/.../reset) at
> reproducing this didn't work, so I think at very least a network
> connection needs to be active.

This is doing PCIe function level reset. Maybe you can get more luck
with PCIe Hot Reset. See following link how to trigger PCIe Hot Reset
from userspace: https://alexforencich.com/wiki/en/pcie/hot-reset-linux

> Unfortunately I can't test that with a
> network connection (and without compiling a custom kernel for which I
> don't have the time right now) because there's currently another bug
> deadlocking on device removal if there's an active connection during
> removal (which also seems to trigger on reset). That one ill be fixed
> by
> 
>   https://lore.kernel.org/linux-wireless/20210515024227.2159311-1-briannorris@chromium.org/
> 
> Jonas might know more.
> 
> > > > > Then we'd probably have to check if there are any laptops where multiple
> > > > > devices are connected to the pci bridge as Amey suggested in a review
> > > > > before.
> > > > 
> > > > Well, I do not know... But if this is issue with PCIe bridge then
> > > > similar issue could be observed also for other PCIe devices with this
> > > > PCIe bridge. But question is if there are other laptops with this PCIe
> > > > bridge. And also it can be a problem in ACPI firmware on those Surface
> > > > devices, which implements some PCIe bridge functionality. So it is
> > > > possible that issue is with PCIe bridge, not in HW, but in SW/firmware
> > > > part which can be Microsoft specific... So too many questions to which
> > > > we do not know answers.
> > > > 
> > > > Could you provide output of 'lspci -nn -vv' and 'lspci -tvnn' on
> > > > affected machines? If you have already sent it in some previous email,
> > > > just send a link. At least I'm not able to find it right now and output
> > > > may contain something useful...
> > > 
> > >  From my Surface Book 2 (with the same issue):
> > > 
> > >   - lspci -tvnn: https://paste.ubuntu.com/p/mm3YpcZJ8N/
> > >   - lspci -vv -nn: https://paste.ubuntu.com/p/dctTDP738N/
> > 
> > Could you re-run lspci under root account? There are missing important
> > parts like "Capabilities: <access denied>" where is information if
> > bridge supports surprise removal or not.
> 
> Ah sorry, sure thing. Here's the updated lspci -nn -vv log:
> 
>   https://paste.ubuntu.com/p/fzsmCvm86Y/
> 
> The log for lspci -tvnn is the same.

Ok. So bridge for wifi card (00:1c.0) indicates:

    SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug- Surprise-
            Slot #0, PowerLimit 10.000W; Interlock- NoCompl+

No support for surprise removal, nor for hotplug interrupt.

But bridge for nvidia card (00:1c.4) indicates:

    SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+
            Slot #4, PowerLimit 25.000W; Interlock- NoCompl+

And interesting, it supports hotplug interrupt and also surprise
removal. Which matches what you wrote above about dGPU.

So another idea: maybe problem is really in 88W8897 and recovering is
working only via bridge which supports surprise removal? Just guessing.
Or kernel PCIe hotplug driver is doing something which is needed for
recovering 88W8897 and because this bridge does not support surprise
removal, it behaves differently?

> 
> > > Regards,
> > > Max

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Maximilian Luz]

On 7/9/21 8:44 PM, Pali Rohár wrote:

[...]

>> My (very) quick attempt ('echo 1 > /sys/bus/pci/.../reset) at
>> reproducing this didn't work, so I think at very least a network
>> connection needs to be active.
> 
> This is doing PCIe function level reset. Maybe you can get more luck
> with PCIe Hot Reset. See following link how to trigger PCIe Hot Reset
> from userspace: https://alexforencich.com/wiki/en/pcie/hot-reset-linux

Thanks for that link! That does indeed do something which breaks the
adapter. Running the script produces

   [  178.388414] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
   [  178.389128] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
   [  178.461365] mwifiex_pcie 0000:01:00.0: performing cancel_work_sync()...
   [  178.461373] mwifiex_pcie 0000:01:00.0: cancel_work_sync() done
   [  178.984106] pci 0000:01:00.0: [11ab:2b38] type 00 class 0x020000
   [  178.984161] pci 0000:01:00.0: reg 0x10: [mem 0x00000000-0x000fffff 64bit pref]
   [  178.984193] pci 0000:01:00.0: reg 0x18: [mem 0x00000000-0x000fffff 64bit pref]
   [  178.984430] pci 0000:01:00.0: supports D1 D2
   [  178.984434] pci 0000:01:00.0: PME# supported from D0 D1 D3hot D3cold
   [  178.984871] pcieport 0000:00:1c.0: ASPM: current common clock configuration is inconsistent, reconfiguring
   [  179.297919] pci 0000:01:00.0: BAR 0: assigned [mem 0xd4400000-0xd44fffff 64bit pref]
   [  179.297961] pci 0000:01:00.0: BAR 2: assigned [mem 0xd4500000-0xd45fffff 64bit pref]
   [  179.298316] mwifiex_pcie 0000:01:00.0: enabling device (0000 -> 0002)
   [  179.298752] mwifiex_pcie: PCI memory map Virt0: 00000000c4593df1 PCI memory map Virt2: 0000000039d67daf
   [  179.300522] mwifiex_pcie 0000:01:00.0: WLAN read winner status failed!
   [  179.300552] mwifiex_pcie 0000:01:00.0: info: _mwifiex_fw_dpc: unregister device
   [  179.300622] mwifiex_pcie 0000:01:00.0: Read register failed
   [  179.300912] mwifiex_pcie 0000:01:00.0: performing cancel_work_sync()...
   [  179.300928] mwifiex_pcie 0000:01:00.0: cancel_work_sync() done

after which the card is unusable (there is no WiFi interface availabel
any more). Reloading the driver module doesn't help and produces

   [  376.906833] mwifiex_pcie: PCI memory map Virt0: 0000000025149d28 PCI memory map Virt2: 00000000c4593df1
   [  376.907278] mwifiex_pcie 0000:01:00.0: WLAN read winner status failed!
   [  376.907281] mwifiex_pcie 0000:01:00.0: info: _mwifiex_fw_dpc: unregister device
   [  376.907293] mwifiex_pcie 0000:01:00.0: Read register failed
   [  376.907404] mwifiex_pcie 0000:01:00.0: performing cancel_work_sync()...
   [  376.907406] mwifiex_pcie 0000:01:00.0: cancel_work_sync() done

again. Performing a function level reset produces

   [  402.489572] mwifiex_pcie 0000:01:00.0: mwifiex_pcie_reset_prepare: adapter structure is not valid
   [  403.514219] mwifiex_pcie 0000:01:00.0: mwifiex_pcie_reset_done: adapter structure is not valid

and doesn't help either.

Running the same command on a kernel with (among other) this patch
unfortunately also breaks the adapter in the same way. As far as I can
tell though, it doesn't run through the reset code added by this patch
(as indicated by the log message when performing FLR), which I assume
in a non-forced scenario, e.g. firmware issues (which IIRC is why this
patch exists), it would?

>> Unfortunately I can't test that with a
>> network connection (and without compiling a custom kernel for which I
>> don't have the time right now) because there's currently another bug
>> deadlocking on device removal if there's an active connection during
>> removal (which also seems to trigger on reset). That one ill be fixed
>> by
>>
>>    https://lore.kernel.org/linux-wireless/20210515024227.2159311-1-briannorris@chromium.org/
>>
>> Jonas might know more.

[...]

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Pali Rohár]

On Friday 09 July 2021 21:27:51 Maximilian Luz wrote:
> On 7/9/21 8:44 PM, Pali Rohár wrote:
> 
> [...]
> 
> > > My (very) quick attempt ('echo 1 > /sys/bus/pci/.../reset) at
> > > reproducing this didn't work, so I think at very least a network
> > > connection needs to be active.
> > 
> > This is doing PCIe function level reset. Maybe you can get more luck
> > with PCIe Hot Reset. See following link how to trigger PCIe Hot Reset
> > from userspace: https://alexforencich.com/wiki/en/pcie/hot-reset-linux
> 
> Thanks for that link! That does indeed do something which breaks the
> adapter. Running the script produces
> 
>   [  178.388414] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
>   [  178.389128] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
>   [  178.461365] mwifiex_pcie 0000:01:00.0: performing cancel_work_sync()...
>   [  178.461373] mwifiex_pcie 0000:01:00.0: cancel_work_sync() done
>   [  178.984106] pci 0000:01:00.0: [11ab:2b38] type 00 class 0x020000
>   [  178.984161] pci 0000:01:00.0: reg 0x10: [mem 0x00000000-0x000fffff 64bit pref]
>   [  178.984193] pci 0000:01:00.0: reg 0x18: [mem 0x00000000-0x000fffff 64bit pref]
>   [  178.984430] pci 0000:01:00.0: supports D1 D2
>   [  178.984434] pci 0000:01:00.0: PME# supported from D0 D1 D3hot D3cold
>   [  178.984871] pcieport 0000:00:1c.0: ASPM: current common clock configuration is inconsistent, reconfiguring
>   [  179.297919] pci 0000:01:00.0: BAR 0: assigned [mem 0xd4400000-0xd44fffff 64bit pref]
>   [  179.297961] pci 0000:01:00.0: BAR 2: assigned [mem 0xd4500000-0xd45fffff 64bit pref]
>   [  179.298316] mwifiex_pcie 0000:01:00.0: enabling device (0000 -> 0002)
>   [  179.298752] mwifiex_pcie: PCI memory map Virt0: 00000000c4593df1 PCI memory map Virt2: 0000000039d67daf
>   [  179.300522] mwifiex_pcie 0000:01:00.0: WLAN read winner status failed!
>   [  179.300552] mwifiex_pcie 0000:01:00.0: info: _mwifiex_fw_dpc: unregister device
>   [  179.300622] mwifiex_pcie 0000:01:00.0: Read register failed
>   [  179.300912] mwifiex_pcie 0000:01:00.0: performing cancel_work_sync()...
>   [  179.300928] mwifiex_pcie 0000:01:00.0: cancel_work_sync() done
> 
> after which the card is unusable (there is no WiFi interface availabel
> any more). Reloading the driver module doesn't help and produces
> 
>   [  376.906833] mwifiex_pcie: PCI memory map Virt0: 0000000025149d28 PCI memory map Virt2: 00000000c4593df1
>   [  376.907278] mwifiex_pcie 0000:01:00.0: WLAN read winner status failed!
>   [  376.907281] mwifiex_pcie 0000:01:00.0: info: _mwifiex_fw_dpc: unregister device
>   [  376.907293] mwifiex_pcie 0000:01:00.0: Read register failed
>   [  376.907404] mwifiex_pcie 0000:01:00.0: performing cancel_work_sync()...
>   [  376.907406] mwifiex_pcie 0000:01:00.0: cancel_work_sync() done
> 
> again. Performing a function level reset produces
> 
>   [  402.489572] mwifiex_pcie 0000:01:00.0: mwifiex_pcie_reset_prepare: adapter structure is not valid
>   [  403.514219] mwifiex_pcie 0000:01:00.0: mwifiex_pcie_reset_done: adapter structure is not valid
> 
> and doesn't help either.

More Qualcomm/Atheros wifi cards are broken in a way that they stop
responding after PCIe Hot Reset and completely disappear from the PCIe
bus. It is possible that similar issue have also these Marvell cards?

As now we know that bride does not support hotplug it cannot inform
system when card disconnect from the bus. The one possible way how to
detect if PCIe card is available at specific address is trying to read
its device and vendor id. Kernel caches device/vendor id, so from
userspace is needed to call lspci with -b argument (to ignore kernel's
reported cached value). Could you provide output of following command
after you do PCIe Hot Reset?

    lspci -s 01:00.0 -b -vv

(and compare with output which you have already provided if there are
any differences)

If PCIe Hot Reset is breaking the card then the only option how to
"reset" card into working state again is PCIe Warm Reset. Unfortunately
there is no common mechanism how to do it from system. PCIe Warm Reset
is done by asserting PERST# signal on card itself, in mPCIe form factor
it is pin 22. In most cases pin 22 is connected to some GPIO so via GPIO
subsystem it could be controlled.

> Running the same command on a kernel with (among other) this patch
> unfortunately also breaks the adapter in the same way. As far as I can
> tell though, it doesn't run through the reset code added by this patch
> (as indicated by the log message when performing FLR), which I assume
> in a non-forced scenario, e.g. firmware issues (which IIRC is why this
> patch exists), it would?

Err... I have caught this part. Is proposed patch able to recover also
from state which happens after PCIe Hot Reset?

> > > Unfortunately I can't test that with a
> > > network connection (and without compiling a custom kernel for which I
> > > don't have the time right now) because there's currently another bug
> > > deadlocking on device removal if there's an active connection during
> > > removal (which also seems to trigger on reset). That one ill be fixed
> > > by
> > > 
> > >    https://lore.kernel.org/linux-wireless/20210515024227.2159311-1-briannorris@chromium.org/
> > > 
> > > Jonas might know more.
> 
> [...]

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Maximilian Luz]

On 7/9/21 9:44 PM, Pali Rohár wrote:
> On Friday 09 July 2021 21:27:51 Maximilian Luz wrote:
>> On 7/9/21 8:44 PM, Pali Rohár wrote:
>>
>> [...]
>>
>>>> My (very) quick attempt ('echo 1 > /sys/bus/pci/.../reset) at
>>>> reproducing this didn't work, so I think at very least a network
>>>> connection needs to be active.
>>>
>>> This is doing PCIe function level reset. Maybe you can get more luck
>>> with PCIe Hot Reset. See following link how to trigger PCIe Hot Reset
>>> from userspace: https://alexforencich.com/wiki/en/pcie/hot-reset-linux
>>
>> Thanks for that link! That does indeed do something which breaks the
>> adapter. Running the script produces
>>
>>    [  178.388414] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
>>    [  178.389128] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
>>    [  178.461365] mwifiex_pcie 0000:01:00.0: performing cancel_work_sync()...
>>    [  178.461373] mwifiex_pcie 0000:01:00.0: cancel_work_sync() done
>>    [  178.984106] pci 0000:01:00.0: [11ab:2b38] type 00 class 0x020000
>>    [  178.984161] pci 0000:01:00.0: reg 0x10: [mem 0x00000000-0x000fffff 64bit pref]
>>    [  178.984193] pci 0000:01:00.0: reg 0x18: [mem 0x00000000-0x000fffff 64bit pref]
>>    [  178.984430] pci 0000:01:00.0: supports D1 D2
>>    [  178.984434] pci 0000:01:00.0: PME# supported from D0 D1 D3hot D3cold
>>    [  178.984871] pcieport 0000:00:1c.0: ASPM: current common clock configuration is inconsistent, reconfiguring
>>    [  179.297919] pci 0000:01:00.0: BAR 0: assigned [mem 0xd4400000-0xd44fffff 64bit pref]
>>    [  179.297961] pci 0000:01:00.0: BAR 2: assigned [mem 0xd4500000-0xd45fffff 64bit pref]
>>    [  179.298316] mwifiex_pcie 0000:01:00.0: enabling device (0000 -> 0002)
>>    [  179.298752] mwifiex_pcie: PCI memory map Virt0: 00000000c4593df1 PCI memory map Virt2: 0000000039d67daf
>>    [  179.300522] mwifiex_pcie 0000:01:00.0: WLAN read winner status failed!
>>    [  179.300552] mwifiex_pcie 0000:01:00.0: info: _mwifiex_fw_dpc: unregister device
>>    [  179.300622] mwifiex_pcie 0000:01:00.0: Read register failed
>>    [  179.300912] mwifiex_pcie 0000:01:00.0: performing cancel_work_sync()...
>>    [  179.300928] mwifiex_pcie 0000:01:00.0: cancel_work_sync() done
>>
>> after which the card is unusable (there is no WiFi interface availabel
>> any more). Reloading the driver module doesn't help and produces
>>
>>    [  376.906833] mwifiex_pcie: PCI memory map Virt0: 0000000025149d28 PCI memory map Virt2: 00000000c4593df1
>>    [  376.907278] mwifiex_pcie 0000:01:00.0: WLAN read winner status failed!
>>    [  376.907281] mwifiex_pcie 0000:01:00.0: info: _mwifiex_fw_dpc: unregister device
>>    [  376.907293] mwifiex_pcie 0000:01:00.0: Read register failed
>>    [  376.907404] mwifiex_pcie 0000:01:00.0: performing cancel_work_sync()...
>>    [  376.907406] mwifiex_pcie 0000:01:00.0: cancel_work_sync() done
>>
>> again. Performing a function level reset produces
>>
>>    [  402.489572] mwifiex_pcie 0000:01:00.0: mwifiex_pcie_reset_prepare: adapter structure is not valid
>>    [  403.514219] mwifiex_pcie 0000:01:00.0: mwifiex_pcie_reset_done: adapter structure is not valid
>>
>> and doesn't help either.
> 
> More Qualcomm/Atheros wifi cards are broken in a way that they stop
> responding after PCIe Hot Reset and completely disappear from the PCIe
> bus. It is possible that similar issue have also these Marvell cards?
> 
> As now we know that bride does not support hotplug it cannot inform
> system when card disconnect from the bus. The one possible way how to
> detect if PCIe card is available at specific address is trying to read
> its device and vendor id. Kernel caches device/vendor id, so from
> userspace is needed to call lspci with -b argument (to ignore kernel's
> reported cached value). Could you provide output of following command
> after you do PCIe Hot Reset?
> 
>      lspci -s 01:00.0 -b -vv
> 
> (and compare with output which you have already provided if there are
> any differences)

There do seem to be some differences, specifically regarding memory.
See https://paste.ubuntu.com/p/Rz2CDjwkCv/ for the full output.

> If PCIe Hot Reset is breaking the card then the only option how to
> "reset" card into working state again is PCIe Warm Reset. Unfortunately
> there is no common mechanism how to do it from system. PCIe Warm Reset
> is done by asserting PERST# signal on card itself, in mPCIe form factor
> it is pin 22. In most cases pin 22 is connected to some GPIO so via GPIO
> subsystem it could be controlled.
> 
>> Running the same command on a kernel with (among other) this patch
>> unfortunately also breaks the adapter in the same way. As far as I can
>> tell though, it doesn't run through the reset code added by this patch
>> (as indicated by the log message when performing FLR), which I assume
>> in a non-forced scenario, e.g. firmware issues (which IIRC is why this
>> patch exists), it would?
> 
> Err... I have caught this part. Is proposed patch able to recover also
> from state which happens after PCIe Hot Reset?

I'm not sure at this point if the power-cycle through D3cold would fix
this (I think it might, but have no evidence for that). This patch alone
isn't able to recover the device, as, when triggering the hot-reset via
that script, the code never seems to run mwifiex_pcie_reset_d3cold_quirk().

If I remember correctly, the main issue was that the firmware state
doesn't get reset completely. This can be somewhat observed when doing
'echo 1 > /sys/bus/pci/devices/.../reset' via the difference in log
messages:

For an unpatched kernel:

   [   64.159509] mwifiex_pcie 0000:01:00.0: info: shutdown mwifiex...
   [   64.159546] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
   [   64.159922] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
   [   65.240272] mwifiex_pcie 0000:01:00.0: WLAN FW already running! Skip FW dnld
   [   65.240285] mwifiex_pcie 0000:01:00.0: WLAN FW is active
   [   65.327359] mwifiex_pcie 0000:01:00.0: info: MWIFIEX VERSION: mwifiex 1.0 (15.68.19.p21)
   [   65.327370] mwifiex_pcie 0000:01:00.0: driver_version = mwifiex 1.0 (15.68.19.p21)

For a patched kernel:

   [   41.966094] mwifiex_pcie 0000:01:00.0: info: shutdown mwifiex...
   [   41.966451] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
   [   41.967227] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
   [   42.063543] mwifiex_pcie 0000:01:00.0: Using reset_d3cold quirk to perform FW reset
   [   42.063558] mwifiex_pcie 0000:01:00.0: putting into D3cold...
   [   42.081010] usb 1-6: USB disconnect, device number 9
   [   42.339922] pcieport 0000:00:1c.0: putting into D3cold...
   [   42.425766] pcieport 0000:00:1c.0: putting into D0...
   [   42.695987] mwifiex_pcie 0000:01:00.0: putting into D0...
   [   42.956673] mwifiex_pcie 0000:01:00.0: enabling device (0000 -> 0002)
   [   45.012736] mwifiex_pcie 0000:01:00.0: info: FW download over, size 723540 bytes
   [   45.740882] usb 1-6: new high-speed USB device number 10 using xhci_hcd
   [   45.881294] usb 1-6: New USB device found, idVendor=1286, idProduct=204c, bcdDevice=32.01
   [   45.881308] usb 1-6: New USB device strings: Mfr=1, Product=2, SerialNumber=3
   [   45.881313] usb 1-6: Product: Bluetooth and Wireless LAN Composite Device
   [   45.881318] usb 1-6: Manufacturer: Marvell
   [   45.881322] usb 1-6: SerialNumber: 0000000000000000
   [   45.884882] Bluetooth: hci0: unexpected event for opcode 0x0000
   [   45.885128] Bluetooth: hci0: unexpected event for opcode 0x0000
   [   45.966218] mwifiex_pcie 0000:01:00.0: WLAN FW is active
   [   46.157474] mwifiex_pcie 0000:01:00.0: info: MWIFIEX VERSION: mwifiex 1.0 (15.68.19.p21)
   [   46.157485] mwifiex_pcie 0000:01:00.0: driver_version = mwifiex 1.0 (15.68.19.p21)

Note the absence of "WLAN FW already running! Skip FW dnld" on the
second log. Due to the power cycle we're essentially forcing the device
to re-download and re-initialize its firmware. On an unpatched kernel,
it looks like the firmware itself is kept and state may not be cleared
properly. So in other words it looks like the firmware, while being
prompted to do a reset, doesn't do that properly (at least when it has
crashed before).

IIRC this then allowed us to recover from firmware issues that the
"normal" firmware reset that mwifiex supposedly performs on a FLR
(or reloading driver modules) didn't help with.

I'm not so sure any more if resetting actively caused issues or if it
just showed different symptoms of some firmware issue that prompted us
to do the reset in the first place (again, been quite a while since I
last dealt with this stuff, sorry). All I know is that this patched
reset gets the card going again.

As a side note: There are also more patches by Jonas (and Tsuchiya?)
building on top of the quirk implementation introduced here which
significantly reduce the need for doing resets in the first place
(nevertheless having a reset that actually does properly reset the
device is a good thing IMHO). Those patches can for example be found
here:

   https://github.com/linux-surface/kernel/compare/eaaf96ba58a5fe5999b89fe3afaded74caa96480...989c8725a6d4e62db6370dd0fefe45498274d3ce

>>>> Unfortunately I can't test that with a
>>>> network connection (and without compiling a custom kernel for which I
>>>> don't have the time right now) because there's currently another bug
>>>> deadlocking on device removal if there's an active connection during
>>>> removal (which also seems to trigger on reset). That one ill be fixed
>>>> by
>>>>
>>>>     https://lore.kernel.org/linux-wireless/20210515024227.2159311-1-briannorris@chromium.org/
>>>>
>>>> Jonas might know more.
>>
>> [...]

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Pali Rohár]

On Friday 09 July 2021 22:54:09 Maximilian Luz wrote:
> On 7/9/21 9:44 PM, Pali Rohár wrote:
> > On Friday 09 July 2021 21:27:51 Maximilian Luz wrote:
> > > On 7/9/21 8:44 PM, Pali Rohár wrote:
> > > 
> > > [...]
> > > 
> > > > > My (very) quick attempt ('echo 1 > /sys/bus/pci/.../reset) at
> > > > > reproducing this didn't work, so I think at very least a network
> > > > > connection needs to be active.
> > > > 
> > > > This is doing PCIe function level reset. Maybe you can get more luck
> > > > with PCIe Hot Reset. See following link how to trigger PCIe Hot Reset
> > > > from userspace: https://alexforencich.com/wiki/en/pcie/hot-reset-linux
> > > 
> > > Thanks for that link! That does indeed do something which breaks the
> > > adapter. Running the script produces
> > > 
> > >    [  178.388414] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
> > >    [  178.389128] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
> > >    [  178.461365] mwifiex_pcie 0000:01:00.0: performing cancel_work_sync()...
> > >    [  178.461373] mwifiex_pcie 0000:01:00.0: cancel_work_sync() done
> > >    [  178.984106] pci 0000:01:00.0: [11ab:2b38] type 00 class 0x020000
> > >    [  178.984161] pci 0000:01:00.0: reg 0x10: [mem 0x00000000-0x000fffff 64bit pref]
> > >    [  178.984193] pci 0000:01:00.0: reg 0x18: [mem 0x00000000-0x000fffff 64bit pref]
> > >    [  178.984430] pci 0000:01:00.0: supports D1 D2
> > >    [  178.984434] pci 0000:01:00.0: PME# supported from D0 D1 D3hot D3cold
> > >    [  178.984871] pcieport 0000:00:1c.0: ASPM: current common clock configuration is inconsistent, reconfiguring
> > >    [  179.297919] pci 0000:01:00.0: BAR 0: assigned [mem 0xd4400000-0xd44fffff 64bit pref]
> > >    [  179.297961] pci 0000:01:00.0: BAR 2: assigned [mem 0xd4500000-0xd45fffff 64bit pref]
> > >    [  179.298316] mwifiex_pcie 0000:01:00.0: enabling device (0000 -> 0002)
> > >    [  179.298752] mwifiex_pcie: PCI memory map Virt0: 00000000c4593df1 PCI memory map Virt2: 0000000039d67daf
> > >    [  179.300522] mwifiex_pcie 0000:01:00.0: WLAN read winner status failed!
> > >    [  179.300552] mwifiex_pcie 0000:01:00.0: info: _mwifiex_fw_dpc: unregister device
> > >    [  179.300622] mwifiex_pcie 0000:01:00.0: Read register failed
> > >    [  179.300912] mwifiex_pcie 0000:01:00.0: performing cancel_work_sync()...
> > >    [  179.300928] mwifiex_pcie 0000:01:00.0: cancel_work_sync() done
> > > 
> > > after which the card is unusable (there is no WiFi interface availabel
> > > any more). Reloading the driver module doesn't help and produces
> > > 
> > >    [  376.906833] mwifiex_pcie: PCI memory map Virt0: 0000000025149d28 PCI memory map Virt2: 00000000c4593df1
> > >    [  376.907278] mwifiex_pcie 0000:01:00.0: WLAN read winner status failed!
> > >    [  376.907281] mwifiex_pcie 0000:01:00.0: info: _mwifiex_fw_dpc: unregister device
> > >    [  376.907293] mwifiex_pcie 0000:01:00.0: Read register failed
> > >    [  376.907404] mwifiex_pcie 0000:01:00.0: performing cancel_work_sync()...
> > >    [  376.907406] mwifiex_pcie 0000:01:00.0: cancel_work_sync() done
> > > 
> > > again. Performing a function level reset produces
> > > 
> > >    [  402.489572] mwifiex_pcie 0000:01:00.0: mwifiex_pcie_reset_prepare: adapter structure is not valid
> > >    [  403.514219] mwifiex_pcie 0000:01:00.0: mwifiex_pcie_reset_done: adapter structure is not valid
> > > 
> > > and doesn't help either.
> > 
> > More Qualcomm/Atheros wifi cards are broken in a way that they stop
> > responding after PCIe Hot Reset and completely disappear from the PCIe
> > bus. It is possible that similar issue have also these Marvell cards?
> > 
> > As now we know that bride does not support hotplug it cannot inform
> > system when card disconnect from the bus. The one possible way how to
> > detect if PCIe card is available at specific address is trying to read
> > its device and vendor id. Kernel caches device/vendor id, so from
> > userspace is needed to call lspci with -b argument (to ignore kernel's
> > reported cached value). Could you provide output of following command
> > after you do PCIe Hot Reset?
> > 
> >      lspci -s 01:00.0 -b -vv
> > 
> > (and compare with output which you have already provided if there are
> > any differences)
> 
> There do seem to be some differences, specifically regarding memory.
> See https://paste.ubuntu.com/p/Rz2CDjwkCv/ for the full output.

Output is OK, card is alive and available on bus. So at least we are not
dealing with the similar issue like are observed in Qualcomm/Atheros.
Missing memory just means that driver has not enabled memory access
(because it did not successfully initialized).

Anyway, card is reporting error via AER:

	Capabilities: [100 v2] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq+ ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		AERCap:	First Error Pointer: 14, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn-
			MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
		HeaderLog: 40000001 0000000f d4500c58 00000000

There is (UESta) marked that Unsupported Request happened and in
HeaderLog is information about error. Unfortunately I'm not skilled to
decode it, so maybe other PCI developers could say if there is something
important in HeaderLog or not.

> > If PCIe Hot Reset is breaking the card then the only option how to
> > "reset" card into working state again is PCIe Warm Reset. Unfortunately
> > there is no common mechanism how to do it from system. PCIe Warm Reset
> > is done by asserting PERST# signal on card itself, in mPCIe form factor
> > it is pin 22. In most cases pin 22 is connected to some GPIO so via GPIO
> > subsystem it could be controlled.
> > 
> > > Running the same command on a kernel with (among other) this patch
> > > unfortunately also breaks the adapter in the same way. As far as I can
> > > tell though, it doesn't run through the reset code added by this patch
> > > (as indicated by the log message when performing FLR), which I assume
> > > in a non-forced scenario, e.g. firmware issues (which IIRC is why this
> > > patch exists), it would?
> > 
> > Err... I have caught this part. Is proposed patch able to recover also
> > from state which happens after PCIe Hot Reset?
> 
> I'm not sure at this point if the power-cycle through D3cold would fix
> this (I think it might, but have no evidence for that). This patch alone
> isn't able to recover the device, as, when triggering the hot-reset via
> that script, the code never seems to run mwifiex_pcie_reset_d3cold_quirk().
> 
> If I remember correctly, the main issue was that the firmware state
> doesn't get reset completely. This can be somewhat observed when doing
> 'echo 1 > /sys/bus/pci/devices/.../reset' via the difference in log
> messages:
> 
> For an unpatched kernel:
> 
>   [   64.159509] mwifiex_pcie 0000:01:00.0: info: shutdown mwifiex...
>   [   64.159546] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
>   [   64.159922] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
>   [   65.240272] mwifiex_pcie 0000:01:00.0: WLAN FW already running! Skip FW dnld
>   [   65.240285] mwifiex_pcie 0000:01:00.0: WLAN FW is active
>   [   65.327359] mwifiex_pcie 0000:01:00.0: info: MWIFIEX VERSION: mwifiex 1.0 (15.68.19.p21)
>   [   65.327370] mwifiex_pcie 0000:01:00.0: driver_version = mwifiex 1.0 (15.68.19.p21)
> 
> For a patched kernel:
> 
>   [   41.966094] mwifiex_pcie 0000:01:00.0: info: shutdown mwifiex...
>   [   41.966451] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
>   [   41.967227] mwifiex_pcie 0000:01:00.0: PREP_CMD: card is removed
>   [   42.063543] mwifiex_pcie 0000:01:00.0: Using reset_d3cold quirk to perform FW reset
>   [   42.063558] mwifiex_pcie 0000:01:00.0: putting into D3cold...
>   [   42.081010] usb 1-6: USB disconnect, device number 9
>   [   42.339922] pcieport 0000:00:1c.0: putting into D3cold...
>   [   42.425766] pcieport 0000:00:1c.0: putting into D0...
>   [   42.695987] mwifiex_pcie 0000:01:00.0: putting into D0...
>   [   42.956673] mwifiex_pcie 0000:01:00.0: enabling device (0000 -> 0002)
>   [   45.012736] mwifiex_pcie 0000:01:00.0: info: FW download over, size 723540 bytes
>   [   45.740882] usb 1-6: new high-speed USB device number 10 using xhci_hcd
>   [   45.881294] usb 1-6: New USB device found, idVendor=1286, idProduct=204c, bcdDevice=32.01
>   [   45.881308] usb 1-6: New USB device strings: Mfr=1, Product=2, SerialNumber=3
>   [   45.881313] usb 1-6: Product: Bluetooth and Wireless LAN Composite Device
>   [   45.881318] usb 1-6: Manufacturer: Marvell
>   [   45.881322] usb 1-6: SerialNumber: 0000000000000000
>   [   45.884882] Bluetooth: hci0: unexpected event for opcode 0x0000
>   [   45.885128] Bluetooth: hci0: unexpected event for opcode 0x0000
>   [   45.966218] mwifiex_pcie 0000:01:00.0: WLAN FW is active
>   [   46.157474] mwifiex_pcie 0000:01:00.0: info: MWIFIEX VERSION: mwifiex 1.0 (15.68.19.p21)
>   [   46.157485] mwifiex_pcie 0000:01:00.0: driver_version = mwifiex 1.0 (15.68.19.p21)

Interesting... putting PCIe card into D3cold cause shutdown also of USB
bluetooth part.

> Note the absence of "WLAN FW already running! Skip FW dnld" on the
> second log. Due to the power cycle we're essentially forcing the device
> to re-download and re-initialize its firmware.

Yea, this proves that putting into D3cold and returning back to D0 cause
full reset of the card, both PCIe and USB parts. ARM CPU on this card
therefore must be reset and so new full firmware download and start it
again is needed.

> On an unpatched kernel,
> it looks like the firmware itself is kept and state may not be cleared
> properly. So in other words it looks like the firmware, while being
> prompted to do a reset, doesn't do that properly (at least when it has
> crashed before).
> 
> IIRC this then allowed us to recover from firmware issues that the
> "normal" firmware reset that mwifiex supposedly performs on a FLR
> (or reloading driver modules) didn't help with.

PCIe Function Level Reset should reset only one PCIe part of device. And
seems that this type of reset does not work properly in some situations.

Note that PCIe Function Level Reset is independent of running firmware.
It is implement in hardware and (should) work also at early stage when
firmware is not loaded yet.

I'm starting to think more and more if quirk in this patch really needs
to be behind DMI check and if rather it should not be called on other
platforms too?

> I'm not so sure any more if resetting actively caused issues or if it
> just showed different symptoms of some firmware issue that prompted us
> to do the reset in the first place (again, been quite a while since I
> last dealt with this stuff, sorry). All I know is that this patched
> reset gets the card going again.
> 
> As a side note: There are also more patches by Jonas (and Tsuchiya?)
> building on top of the quirk implementation introduced here which
> significantly reduce the need for doing resets in the first place
> (nevertheless having a reset that actually does properly reset the
> device is a good thing IMHO). Those patches can for example be found
> here:
> 
>   https://github.com/linux-surface/kernel/compare/eaaf96ba58a5fe5999b89fe3afaded74caa96480...989c8725a6d4e62db6370dd0fefe45498274d3ce
> 
> > > > > Unfortunately I can't test that with a
> > > > > network connection (and without compiling a custom kernel for which I
> > > > > don't have the time right now) because there's currently another bug
> > > > > deadlocking on device removal if there's an active connection during
> > > > > removal (which also seems to trigger on reset). That one ill be fixed
> > > > > by
> > > > > 
> > > > >     https://lore.kernel.org/linux-wireless/20210515024227.2159311-1-briannorris@chromium.org/
> > > > > 
> > > > > Jonas might know more.
> > > 
> > > [...]

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Maximilian Luz]

On 7/9/21 11:25 PM, Pali Rohár wrote:

[...]

> PCIe Function Level Reset should reset only one PCIe part of device. And
> seems that this type of reset does not work properly in some situations.
> 
> Note that PCIe Function Level Reset is independent of running firmware.
> It is implement in hardware and (should) work also at early stage when
> firmware is not loaded yet.
> 
> I'm starting to think more and more if quirk in this patch really needs
> to be behind DMI check and if rather it should not be called on other
> platforms too?

Maybe? I'm not sure how well this behaves on other devices and if there
even are any devices outside of the MS Surface line that really require
or benefit from something like this. To me it seems safer to put it
behind quirks, at least until we know more.

Also not sure if this is just my bias, but it feels like the Surface
line always had more problems with that driver (and firmware) than
others.  I'm honestly a bit surprised that MS stuck with them for this
long (they decided to go with Intel for 7th gen devices). AFAICT they
initially chose Marvell due to connected standby support, so maybe that
causes issue for us and others simply aren't using that feature? Just
guessing though.

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Pali Rohár]

On Saturday 10 July 2021 00:25:36 Maximilian Luz wrote:
> On 7/9/21 11:25 PM, Pali Rohár wrote:
> 
> [...]
> 
> > PCIe Function Level Reset should reset only one PCIe part of device. And
> > seems that this type of reset does not work properly in some situations.
> > 
> > Note that PCIe Function Level Reset is independent of running firmware.
> > It is implement in hardware and (should) work also at early stage when
> > firmware is not loaded yet.
> > 
> > I'm starting to think more and more if quirk in this patch really needs
> > to be behind DMI check and if rather it should not be called on other
> > platforms too?
> 
> Maybe? I'm not sure how well this behaves on other devices and if there
> even are any devices outside of the MS Surface line that really require
> or benefit from something like this. To me it seems safer to put it
> behind quirks, at least until we know more.

This is a good argument for DMI based quirk.

But at the end, PCI people should look at this patch and say what do
they thing about it, if it is better to enable it only for specific set
of machines (Surface) or it is better to enable it for every 88W8xxx
wifi or for none.

> Also not sure if this is just my bias, but it feels like the Surface
> line always had more problems with that driver (and firmware) than
> others.

Ehm, really? I see reports also from non-Surface users about bad quality
of these 88W[89]xxx cards and repeating firmware issues. I have bad
personal experience with 88W8997 SDIO firmware and lot of times I get
advice about ex-Marvell/NXP wifi cards "do not touch and run away
quickly".

I think that more people if they get mPCIe/M.2 wifi card in laptop which
does not work, they just replace it with some working one. And not
spending infinite time in trying to fix it... So this may explain why
there are more Surface users with these issues...

> I'm honestly a bit surprised that MS stuck with them for this
> long (they decided to go with Intel for 7th gen devices). AFAICT they
> initially chose Marvell due to connected standby support, so maybe that
> causes issue for us and others simply aren't using that feature? Just
> guessing though.

In my opinion that "Connected Standby" is just MS marketing term.

88W[89]xxx chips using full-mac firmware and drivers [*]. Full-mac lot
of times causing more issues than soft-mac solution. Moreover this
Marvell firmware implements also other "application" layers in firmware
which OS drivers can use, e.g. there is fully working "wpa-supplicant"
replacement and also AP part. Maybe windows drivers are using it and it
cause less problems? Duno. mwifiex uses only "low level" commands and
WPA state machine is implemented in userspace wpa-supplicant daemon.


[*] - Small note: There are also soft-mac firmwares and drivers but
apparently Marvell has never finished linux driver and firmware was not
released to public...

And there is also Laird Connectivity which offers their own proprietary
linux kernel drivers with their own firmware for these 88W[89]xxx chips.
Last time I checked it they released some parts of driver on github.
Maybe somebody could contact Laird or check if their driver can be
replaced by mwifiex? Or just replacing ex-Marvell/NXP firmware by their?
But I'm not sure if they have something for 88W8897.

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Maximilian Luz]

On 7/10/21 12:54 AM, Pali Rohár wrote:

[...]

>> Also not sure if this is just my bias, but it feels like the Surface
>> line always had more problems with that driver (and firmware) than
>> others.
> 
> Ehm, really? I see reports also from non-Surface users about bad quality
> of these 88W[89]xxx cards and repeating firmware issues. I have bad
> personal experience with 88W8997 SDIO firmware and lot of times I get
> advice about ex-Marvell/NXP wifi cards "do not touch and run away
> quickly".

Yeah, then I'm probably biased since I'm mostly dealing with Surface
stuff.

> I think that more people if they get mPCIe/M.2 wifi card in laptop which
> does not work, they just replace it with some working one. And not
> spending infinite time in trying to fix it... So this may explain why
> there are more Surface users with these issues...

That might be an explanation. If it wouldn't need a heat-gun to open it
up, I'd probably have done that at some point in the past (there were
times when WiFi at my Uni was pretty much unusable with this device...
and I'm still not sure what fixed that or even if it's fixed completely).

>> I'm honestly a bit surprised that MS stuck with them for this
>> long (they decided to go with Intel for 7th gen devices). AFAICT they
>> initially chose Marvell due to connected standby support, so maybe that
>> causes issue for us and others simply aren't using that feature? Just
>> guessing though.
> 
> In my opinion that "Connected Standby" is just MS marketing term.

I can only really repeat what I've been told: Apparently when they
started designing those devices, the only option with "Connected
standby" (or probably rather that feature set that MS wanted) was,
unfortunately for us, Marvell.

> 88W[89]xxx chips using full-mac firmware and drivers [*]. Full-mac lot
> of times causing more issues than soft-mac solution. Moreover this
> Marvell firmware implements also other "application" layers in firmware
> which OS drivers can use, e.g. there is fully working "wpa-supplicant"
> replacement and also AP part. Maybe windows drivers are using it and it
> cause less problems? Duno. mwifiex uses only "low level" commands and
> WPA state machine is implemented in userspace wpa-supplicant daemon.
> 
> [*] - Small note: There are also soft-mac firmwares and drivers but
> apparently Marvell has never finished linux driver and firmware was not
> released to public...
> 
> And there is also Laird Connectivity which offers their own proprietary
> linux kernel drivers with their own firmware for these 88W[89]xxx chips.
> Last time I checked it they released some parts of driver on github.
> Maybe somebody could contact Laird or check if their driver can be
> replaced by mwifiex? Or just replacing ex-Marvell/NXP firmware by their?
> But I'm not sure if they have something for 88W8897.

Interesting, I was not aware of this. IIRC we've been experimenting with
the mwlwifi driver (which that lrdmwl driver seems to be based on?), but
couldn't get that to work with the firmware we have. IIRC it also didn't
work with the Windows firmware (which seems to be significantly
different from the one we have for Linux and seems to use or be modeled
after some special Windows WiFi driver interface).

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Pali Rohár]

On Saturday 10 July 2021 02:00:08 Maximilian Luz wrote:
> On 7/10/21 12:54 AM, Pali Rohár wrote:
> 
> [...]
> 
> > > Also not sure if this is just my bias, but it feels like the Surface
> > > line always had more problems with that driver (and firmware) than
> > > others.
> > 
> > Ehm, really? I see reports also from non-Surface users about bad quality
> > of these 88W[89]xxx cards and repeating firmware issues. I have bad
> > personal experience with 88W8997 SDIO firmware and lot of times I get
> > advice about ex-Marvell/NXP wifi cards "do not touch and run away
> > quickly".
> 
> Yeah, then I'm probably biased since I'm mostly dealing with Surface
> stuff.
> 
> > I think that more people if they get mPCIe/M.2 wifi card in laptop which
> > does not work, they just replace it with some working one. And not
> > spending infinite time in trying to fix it... So this may explain why
> > there are more Surface users with these issues...
> 
> That might be an explanation. If it wouldn't need a heat-gun to open it
> up, I'd probably have done that at some point in the past (there were
> times when WiFi at my Uni was pretty much unusable with this device...
> and I'm still not sure what fixed that or even if it's fixed completely).
> 
> > > I'm honestly a bit surprised that MS stuck with them for this
> > > long (they decided to go with Intel for 7th gen devices). AFAICT they
> > > initially chose Marvell due to connected standby support, so maybe that
> > > causes issue for us and others simply aren't using that feature? Just
> > > guessing though.
> > 
> > In my opinion that "Connected Standby" is just MS marketing term.
> 
> I can only really repeat what I've been told: Apparently when they
> started designing those devices, the only option with "Connected
> standby" (or probably rather that feature set that MS wanted) was,
> unfortunately for us, Marvell.
> 
> > 88W[89]xxx chips using full-mac firmware and drivers [*]. Full-mac lot
> > of times causing more issues than soft-mac solution. Moreover this
> > Marvell firmware implements also other "application" layers in firmware
> > which OS drivers can use, e.g. there is fully working "wpa-supplicant"
> > replacement and also AP part. Maybe windows drivers are using it and it
> > cause less problems? Duno. mwifiex uses only "low level" commands and
> > WPA state machine is implemented in userspace wpa-supplicant daemon.
> > 
> > [*] - Small note: There are also soft-mac firmwares and drivers but
> > apparently Marvell has never finished linux driver and firmware was not
> > released to public...
> > 
> > And there is also Laird Connectivity which offers their own proprietary
> > linux kernel drivers with their own firmware for these 88W[89]xxx chips.
> > Last time I checked it they released some parts of driver on github.
> > Maybe somebody could contact Laird or check if their driver can be
> > replaced by mwifiex? Or just replacing ex-Marvell/NXP firmware by their?
> > But I'm not sure if they have something for 88W8897.
> 
> Interesting, I was not aware of this. IIRC we've been experimenting with
> the mwlwifi driver (which that lrdmwl driver seems to be based on?), but
> couldn't get that to work with the firmware we have.

mwlwifi is that soft-mac driver and uses completely different firmware.
For sure it would not work with current full-mac firmware.

> IIRC it also didn't
> work with the Windows firmware (which seems to be significantly
> different from the one we have for Linux and seems to use or be modeled
> after some special Windows WiFi driver interface).

So... Microsoft has different firmware for this chip? And it is working
with mwifiex driver?

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Maximilian Luz]

On 7/10/21 2:07 AM, Pali Rohár wrote:

[...]

>> Interesting, I was not aware of this. IIRC we've been experimenting with
>> the mwlwifi driver (which that lrdmwl driver seems to be based on?), but
>> couldn't get that to work with the firmware we have.
> 
> mwlwifi is that soft-mac driver and uses completely different firmware.
> For sure it would not work with current full-mac firmware.
> 
>> IIRC it also didn't
>> work with the Windows firmware (which seems to be significantly
>> different from the one we have for Linux and seems to use or be modeled
>> after some special Windows WiFi driver interface).
> 
> So... Microsoft has different firmware for this chip? And it is working
> with mwifiex driver?

I'm not sure how special that firmware really is (i.e. if it is Surface
specific or just what Marvell uses on Windows), only that it doesn't
look like the firmware included in the linux-firmware repo. The Windows
firmware doesn't work with either mwlwifi or mwifiex drivers (IIRC) and
on Linux we use the official firmware from the linux-firmware repo.

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Pali Rohár]

On Saturday 10 July 2021 02:18:12 Maximilian Luz wrote:
> On 7/10/21 2:07 AM, Pali Rohár wrote:
> 
> [...]
> 
> > > Interesting, I was not aware of this. IIRC we've been experimenting with
> > > the mwlwifi driver (which that lrdmwl driver seems to be based on?), but
> > > couldn't get that to work with the firmware we have.
> > 
> > mwlwifi is that soft-mac driver and uses completely different firmware.
> > For sure it would not work with current full-mac firmware.
> > 
> > > IIRC it also didn't
> > > work with the Windows firmware (which seems to be significantly
> > > different from the one we have for Linux and seems to use or be modeled
> > > after some special Windows WiFi driver interface).
> > 
> > So... Microsoft has different firmware for this chip? And it is working
> > with mwifiex driver?
> 
> I'm not sure how special that firmware really is (i.e. if it is Surface
> specific or just what Marvell uses on Windows), only that it doesn't
> look like the firmware included in the linux-firmware repo. The Windows
> firmware doesn't work with either mwlwifi or mwifiex drivers (IIRC) and
> on Linux we use the official firmware from the linux-firmware repo.

Version available in the linux-firmware repo is also what big companies
(like google) receive for their systems... sometimes just only older
version as Marvell/NXP is slow in updating files in linux-firmware.
Seems that it is also same what receive customers under NDA as more
companies dropped "proprietary" ex-Marvell/NXP driver on internet and it
contained this firmware with some sources of driver which looks like a
fork of mwifiex (or maybe mwifiex is "cleaned fork" of that driver :D)

There is old firmware documentation which describe RPC communication
between OS and firmware:
http://wiki.laptop.org/images/f/f3/Firmware-Spec-v5.1-MV-S103752-00.pdf

It is really old for very old wifi chips and when I checked it, it still
matches what mwifiex is doing with new chips. Just there are new and
more commands. And documentation is OS-neutral.

So if Microsoft has some "incompatible" firmware with this, it could
mean that they got something special which nobody else have? Maybe it
can explain that "connected standby" and maybe also better stability?

Or just windows distribute firmware in different format and needs to
"unpack" or "preprocess" prior downloading it to device?

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Maximilian Luz]

On 7/10/21 2:38 AM, Pali Rohár wrote:
> On Saturday 10 July 2021 02:18:12 Maximilian Luz wrote:
>> On 7/10/21 2:07 AM, Pali Rohár wrote:
>>
>> [...]
>>
>>>> Interesting, I was not aware of this. IIRC we've been experimenting with
>>>> the mwlwifi driver (which that lrdmwl driver seems to be based on?), but
>>>> couldn't get that to work with the firmware we have.
>>>
>>> mwlwifi is that soft-mac driver and uses completely different firmware.
>>> For sure it would not work with current full-mac firmware.
>>>
>>>> IIRC it also didn't
>>>> work with the Windows firmware (which seems to be significantly
>>>> different from the one we have for Linux and seems to use or be modeled
>>>> after some special Windows WiFi driver interface).
>>>
>>> So... Microsoft has different firmware for this chip? And it is working
>>> with mwifiex driver?
>>
>> I'm not sure how special that firmware really is (i.e. if it is Surface
>> specific or just what Marvell uses on Windows), only that it doesn't
>> look like the firmware included in the linux-firmware repo. The Windows
>> firmware doesn't work with either mwlwifi or mwifiex drivers (IIRC) and
>> on Linux we use the official firmware from the linux-firmware repo.
> 
> Version available in the linux-firmware repo is also what big companies
> (like google) receive for their systems... sometimes just only older
> version as Marvell/NXP is slow in updating files in linux-firmware.
> Seems that it is also same what receive customers under NDA as more
> companies dropped "proprietary" ex-Marvell/NXP driver on internet and it
> contained this firmware with some sources of driver which looks like a
> fork of mwifiex (or maybe mwifiex is "cleaned fork" of that driver :D)
> 
> There is old firmware documentation which describe RPC communication
> between OS and firmware:
> http://wiki.laptop.org/images/f/f3/Firmware-Spec-v5.1-MV-S103752-00.pdf
> 
> It is really old for very old wifi chips and when I checked it, it still
> matches what mwifiex is doing with new chips. Just there are new and
> more commands. And documentation is OS-neutral.
> 
> So if Microsoft has some "incompatible" firmware with this, it could
> mean that they got something special which nobody else have? Maybe it
> can explain that "connected standby" and maybe also better stability?
> 
> Or just windows distribute firmware in different format and needs to
> "unpack" or "preprocess" prior downloading it to device?

If memory serves me right, Jonas did some reverse engineering on the
Windows driver and found that it uses the "new" WDI Miniport API: It
seems that originally both Windows and Linux drivers (and firmware)
were pretty much the same (he mentioned there were similarities in
terminology), but then they switched to that new API on Windows and
changed the firmware with it, so that the driver now essentially only
forwards the commands from that API to the firmware and the firmware
handles the rest.

By reading the Windows docs on that API, that change might have been
forced on them as some Windows 10 features apparently only work via
that API.

He'll probably know more about that than I do.

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Jonas Dreßler]

On 7/10/21 3:07 AM, Maximilian Luz wrote:
> On 7/10/21 2:38 AM, Pali Rohár wrote:
>> On Saturday 10 July 2021 02:18:12 Maximilian Luz wrote:
>>> On 7/10/21 2:07 AM, Pali Rohár wrote:
>>>
>>> [...]
>>>
>>>>> Interesting, I was not aware of this. IIRC we've been experimenting 
>>>>> with
>>>>> the mwlwifi driver (which that lrdmwl driver seems to be based 
>>>>> on?), but
>>>>> couldn't get that to work with the firmware we have.
>>>>
>>>> mwlwifi is that soft-mac driver and uses completely different firmware.
>>>> For sure it would not work with current full-mac firmware.
>>>>
>>>>> IIRC it also didn't
>>>>> work with the Windows firmware (which seems to be significantly
>>>>> different from the one we have for Linux and seems to use or be 
>>>>> modeled
>>>>> after some special Windows WiFi driver interface).
>>>>
>>>> So... Microsoft has different firmware for this chip? And it is working
>>>> with mwifiex driver?
>>>
>>> I'm not sure how special that firmware really is (i.e. if it is Surface
>>> specific or just what Marvell uses on Windows), only that it doesn't
>>> look like the firmware included in the linux-firmware repo. The Windows
>>> firmware doesn't work with either mwlwifi or mwifiex drivers (IIRC) and
>>> on Linux we use the official firmware from the linux-firmware repo.
>>
>> Version available in the linux-firmware repo is also what big companies
>> (like google) receive for their systems... sometimes just only older
>> version as Marvell/NXP is slow in updating files in linux-firmware.
>> Seems that it is also same what receive customers under NDA as more
>> companies dropped "proprietary" ex-Marvell/NXP driver on internet and it
>> contained this firmware with some sources of driver which looks like a
>> fork of mwifiex (or maybe mwifiex is "cleaned fork" of that driver :D)
>>
>> There is old firmware documentation which describe RPC communication
>> between OS and firmware:
>> http://wiki.laptop.org/images/f/f3/Firmware-Spec-v5.1-MV-S103752-00.pdf
>>
>> It is really old for very old wifi chips and when I checked it, it still
>> matches what mwifiex is doing with new chips. Just there are new and
>> more commands. And documentation is OS-neutral.
>>
>> So if Microsoft has some "incompatible" firmware with this, it could
>> mean that they got something special which nobody else have? Maybe it
>> can explain that "connected standby" and maybe also better stability?
>>
>> Or just windows distribute firmware in different format and needs to
>> "unpack" or "preprocess" prior downloading it to device?
> 
> If memory serves me right, Jonas did some reverse engineering on the
> Windows driver and found that it uses the "new" WDI Miniport API: It
> seems that originally both Windows and Linux drivers (and firmware)
> were pretty much the same (he mentioned there were similarities in
> terminology), but then they switched to that new API on Windows and
> changed the firmware with it, so that the driver now essentially only
> forwards the commands from that API to the firmware and the firmware
> handles the rest.
> 
> By reading the Windows docs on that API, that change might have been
> forced on them as some Windows 10 features apparently only work via
> that API.
> 
> He'll probably know more about that than I do.

Not much I can add there, it seemed a lot like both mwifiex and the 
Windows 10 WDI miniport driver were both derived from the same codebase 
originally, but in order to be compatible with the WDI miniport API and 
other stuff Windows requires from wifi devices (I recall there was some 
SAR-value control/reporting stuff too), some parts of the firmware had 
to be rewritten.

In the end, the Windows firmware is updated a lot more often and likely 
includes a bunch of bugfixes the linux firmware doesn't have, but it 
can't be used on linux without a ton of work that would probably include 
rebuilding proprietary APIs from Windows.

Also, from my testing with custom APs and sniffing packets with 
Wireshark, the functionality, limitations and weird 
"semi-spec-compliant" behaviors were exactly the same with the Windows 
firmware: It doesn't support WPA3, it can't connect to fast transition 
APs (funnily enough that's opposed to what MS claims) and it also can't 
spawn an AP with WPA-PSK-SHA256 AKM ciphers. So not sure there's a lot 
of sense in spending more time trying to go down that path.

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Pali Rohár]

On Sunday 11 July 2021 18:53:32 Jonas Dreßler wrote:
> On 7/10/21 3:07 AM, Maximilian Luz wrote:
> > On 7/10/21 2:38 AM, Pali Rohár wrote:
> > > On Saturday 10 July 2021 02:18:12 Maximilian Luz wrote:
> > > > On 7/10/21 2:07 AM, Pali Rohár wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > > Interesting, I was not aware of this. IIRC we've been
> > > > > > experimenting with
> > > > > > the mwlwifi driver (which that lrdmwl driver seems to be
> > > > > > based on?), but
> > > > > > couldn't get that to work with the firmware we have.
> > > > > 
> > > > > mwlwifi is that soft-mac driver and uses completely different firmware.
> > > > > For sure it would not work with current full-mac firmware.
> > > > > 
> > > > > > IIRC it also didn't
> > > > > > work with the Windows firmware (which seems to be significantly
> > > > > > different from the one we have for Linux and seems to
> > > > > > use or be modeled
> > > > > > after some special Windows WiFi driver interface).
> > > > > 
> > > > > So... Microsoft has different firmware for this chip? And it is working
> > > > > with mwifiex driver?
> > > > 
> > > > I'm not sure how special that firmware really is (i.e. if it is Surface
> > > > specific or just what Marvell uses on Windows), only that it doesn't
> > > > look like the firmware included in the linux-firmware repo. The Windows
> > > > firmware doesn't work with either mwlwifi or mwifiex drivers (IIRC) and
> > > > on Linux we use the official firmware from the linux-firmware repo.
> > > 
> > > Version available in the linux-firmware repo is also what big companies
> > > (like google) receive for their systems... sometimes just only older
> > > version as Marvell/NXP is slow in updating files in linux-firmware.
> > > Seems that it is also same what receive customers under NDA as more
> > > companies dropped "proprietary" ex-Marvell/NXP driver on internet and it
> > > contained this firmware with some sources of driver which looks like a
> > > fork of mwifiex (or maybe mwifiex is "cleaned fork" of that driver :D)
> > > 
> > > There is old firmware documentation which describe RPC communication
> > > between OS and firmware:
> > > http://wiki.laptop.org/images/f/f3/Firmware-Spec-v5.1-MV-S103752-00.pdf
> > > 
> > > It is really old for very old wifi chips and when I checked it, it still
> > > matches what mwifiex is doing with new chips. Just there are new and
> > > more commands. And documentation is OS-neutral.
> > > 
> > > So if Microsoft has some "incompatible" firmware with this, it could
> > > mean that they got something special which nobody else have? Maybe it
> > > can explain that "connected standby" and maybe also better stability?
> > > 
> > > Or just windows distribute firmware in different format and needs to
> > > "unpack" or "preprocess" prior downloading it to device?
> > 
> > If memory serves me right, Jonas did some reverse engineering on the
> > Windows driver and found that it uses the "new" WDI Miniport API: It
> > seems that originally both Windows and Linux drivers (and firmware)
> > were pretty much the same (he mentioned there were similarities in
> > terminology), but then they switched to that new API on Windows and
> > changed the firmware with it, so that the driver now essentially only
> > forwards the commands from that API to the firmware and the firmware
> > handles the rest.
> > 
> > By reading the Windows docs on that API, that change might have been
> > forced on them as some Windows 10 features apparently only work via
> > that API.
> > 
> > He'll probably know more about that than I do.
> 
> Not much I can add there, it seemed a lot like both mwifiex and the Windows
> 10 WDI miniport driver were both derived from the same codebase originally,
> but in order to be compatible with the WDI miniport API and other stuff
> Windows requires from wifi devices (I recall there was some SAR-value
> control/reporting stuff too), some parts of the firmware had to be
> rewritten.
> 
> In the end, the Windows firmware is updated a lot more often and likely
> includes a bunch of bugfixes the linux firmware doesn't have, but it can't
> be used on linux without a ton of work that would probably include
> rebuilding proprietary APIs from Windows.
> 
> Also, from my testing with custom APs and sniffing packets with Wireshark,
> the functionality, limitations and weird "semi-spec-compliant" behaviors
> were exactly the same with the Windows firmware: It doesn't support WPA3, it
> can't connect to fast transition APs (funnily enough that's opposed to what
> MS claims) and it also can't spawn an AP with WPA-PSK-SHA256 AKM ciphers. So
> not sure there's a lot of sense in spending more time trying to go down that
> path.

New version of firmware files are available on NXP portal, where are
updated more frequently, but only for companies which have NXP accounts
and signed NDA with NXP. Not for end users.

If you want these new firmware files, you need to ask NXP developers as
only they can ask for non-NDA distribution and include new version into
linux-firmware repository. Like in this pull request where is new SDIO
firmware for 88W8897:
https://lore.kernel.org/linux-firmware/DB7PR04MB453855B0D6C41923BCB0922EFC1C9@DB7PR04MB4538.eurprd04.prod.outlook.com/

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Jonas Dreßler]

On 7/9/21 7:30 PM, Pali Rohár wrote:
> On Friday 09 July 2021 19:03:37 Maximilian Luz wrote:
>> On 7/9/21 6:12 PM, Pali Rohár wrote:
>>
>> [...]
>>
>>>>> Hello! Now I'm thinking loudly about this patch. Why this kind of reset
>>>>> is needed only for Surface devices? AFAIK these 88W8897 chips are same
>>>>> in all cards. Chip itself implements PCIe interface (and also SDIO) so
>>>>> for me looks very strange if this 88W8897 PCIe device needs DMI specific
>>>>> quirks. I cannot believe that Microsoft got some special version of
>>>>> these chips from Marvell which are different than version uses on cards
>>>>> in mPCIe form factor.
>>>>>
>>>>> And now when I'm reading comment below about PCIe bridge to which is
>>>>> this 88W8897 PCIe chip connected, is not this rather an issue in that
>>>>> PCIe bridge (instead of mwifiex/88W8897) or in ACPI firmware which
>>>>> controls this bridge?
>>>>>
>>>>> Or are having other people same issues on mPCIe form factor wifi cards
>>>>> with 88W8897 chips and then this quirk should not DMI dependent?
>>>>>
>>>>> Note that I'm seeing issues with reset and other things also on chip
>>>>> 88W8997 when is connected to system via SDIO. These chips have both PCIe
>>>>> and SDIO buses, it just depends which pins are used.
>>>>>
>>>>
>>>> Hi and thanks for the quick reply! Honestly I've no idea, this is just the
>>>> first method we found that allows for a proper reset of the chip. What I
>>>> know is that some Surface devices need that ACPI DSM call (the one that was
>>>> done in the commit I dropped in this version of the patchset) to reset the
>>>> chip instead of this method.
>>>>
>>>> Afaik other devices with this chip don't need this resetting method, at
>>>> least Marvell employees couldn't reproduce the issues on their testing
>>>> devices.
>>>>
>>>> So would you suggest we just try to match for the pci chip 88W8897 instead?
>>>
>>> Hello! Such suggestion makes sense when we know that it is 88W8897
>>> issue. But if you got information that issue cannot be reproduced on
>>> other 88W8897 cards then matching 88W8897 is not correct.
>>>
>>>   From all this information looks like that it is problem in (Microsoft?)
>>> PCIe bridge to which is card connected. Otherwise I do not reason how it
>>> can be 88W8897 affected. Either it is reproducible on 88W8897 cards also
>>> in other devices or issue is not on 88W8897 card.
>>
>> I doubt that it's an issue with the PCIe bridge (itself at least). The
>> same type of bridge is used for both dGPU and NVME SSD on my device (see
>> lspci output below) and those work fine. Also if I'm seeing that right
>> it's from the Intel CPU, so my guess is that a lot more people would
>> have issues with that then.
> 
>  From information below it seems to be related to surprise removal.
> Therefore is surprise removal working without issue for dGPU or NVME
> SSD? Not all PCIe bridges support surprise removal...
> 
>>
>> I don't know about the hardware side, so it might be possible that it's
>> an issue with integrating both bridge and wifi chip, in which case it's
>> still probably best handled via DMI quirks unless we know more.
>>
>> Also as Tsuchiya mentioned in his original submission, on Windows the
>> device is reset via this D3cold method. I've only skimmed that
>> errata.inf file mentioned, but I think this is what he's referring to:
>>
>>    Controls whether ACPIDeviceEnableD3ColdOnSurpriseRemoval rule will be
>>    evaluated or not on a given platform. Currently
>>    ACPIDeviceEnableD3ColdOnSurpriseRemoval rule only needs to be
>>    evaluated on Surface platforms which contain the Marvell WiFi
>>    controller which depends on device going through D3Cold as part of
>>    surprise-removal.
>>
>> and
>>
>>    Starting with Windows releases *after* Blue, ACPI will not put
>>    surprise-removed devices into D3Cold automatically. Some known
>>    scenarios (viz. WiFi reset/recovery) rely on the device cycling
>>    through D3Cold on surprise-removal. This hack allows surprise-removed
>>    devices to be put into D3Cold (if supported by the stack).
>>
>> So, as far as I can tell, the chip doesn't like to be surprise-removed
>> (which seems to happen during reset) and then needs to be power-cycled,
>> which I think is likely due to some issue with firmware state.
> 
> Thanks for information. This really does not look like PCIe bridge
> specific if bridge itself can handle surprise-removed devices. lspci can
> tell us if bridge supports it or not (see below).
> 
>> So the quirk on Windows seems very Surface specific.
>>
>> There also seem a bunch of revisions of these chips around, for example
>> my SB2 is affected by a bug that we've tied to the specific hardware
>> revision which causes some issues with host-sleep (IIRC chip switches
>> rapidly between wake and sleep states without any external influence,
>> which is not how it should behave and how it does behave on a later
>> hardware revision).
> 
> Interesting... This looks like the issue can be in 88W8897 chip and
> needs some special conditions to trigger? And Surface is triggering it
> always?

The specific issue was that the card wakes up very quickly after going 
into deep sleep (deep sleep is a state the firmware enters when idle and 
not connected to an AP). Now this in turn messed with the host 
suspending, because deep sleep is involved there and the card is not 
expected to wake up that quickly again (I'm oversimplifying here, it's 
also been some time since we looked into it).

Anyway, in the end those wakeups from deep sleep only happened with one 
hardware revision of the card (I guess it's caused by a hardware design 
issue like a floating gpio or something), and we managed to fix the 
problems by disabling deep sleep on that hardware revision, but as Max 
mentioned that problem is completely unrelated from this patch.

> 
>>>> Then we'd probably have to check if there are any laptops where multiple
>>>> devices are connected to the pci bridge as Amey suggested in a review
>>>> before.
>>>
>>> Well, I do not know... But if this is issue with PCIe bridge then
>>> similar issue could be observed also for other PCIe devices with this
>>> PCIe bridge. But question is if there are other laptops with this PCIe
>>> bridge. And also it can be a problem in ACPI firmware on those Surface
>>> devices, which implements some PCIe bridge functionality. So it is
>>> possible that issue is with PCIe bridge, not in HW, but in SW/firmware
>>> part which can be Microsoft specific... So too many questions to which
>>> we do not know answers.
>>>
>>> Could you provide output of 'lspci -nn -vv' and 'lspci -tvnn' on
>>> affected machines? If you have already sent it in some previous email,
>>> just send a link. At least I'm not able to find it right now and output
>>> may contain something useful...
>>
>>  From my Surface Book 2 (with the same issue):
>>
>>   - lspci -tvnn: https://paste.ubuntu.com/p/mm3YpcZJ8N/
>>   - lspci -vv -nn: https://paste.ubuntu.com/p/dctTDP738N/
> 
> Could you re-run lspci under root account? There are missing important
> parts like "Capabilities: <access denied>" where is information if
> bridge supports surprise removal or not.
> 
>> Regards,
>> Max

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Andy Shevchenko]

On Fri, Jul 9, 2021 at 6:18 PM Pali Rohár <pali@kernel.org> wrote:
> On Friday 09 July 2021 16:58:31 Jonas Dreßler wrote:


> Hello! Now I'm thinking loudly about this patch. Why this kind of reset
> is needed only for Surface devices? AFAIK these 88W8897 chips are same
> in all cards. Chip itself implements PCIe interface (and also SDIO) so
> for me looks very strange if this 88W8897 PCIe device needs DMI specific
> quirks. I cannot believe that Microsoft got some special version of
> these chips from Marvell which are different than version uses on cards
> in mPCIe form factor.
>
> And now when I'm reading comment below about PCIe bridge to which is
> this 88W8897 PCIe chip connected, is not this rather an issue in that
> PCIe bridge (instead of mwifiex/88W8897) or in ACPI firmware which
> controls this bridge?
>
> Or are having other people same issues on mPCIe form factor wifi cards
> with 88W8897 chips and then this quirk should not DMI dependent?
>
> Note that I'm seeing issues with reset and other things also on chip
> 88W8997 when is connected to system via SDIO. These chips have both PCIe
> and SDIO buses, it just depends which pins are used.

I'm replying loudly :-)

You know that depending on the interface the firmware even for the
same chip may be way different. And if you have had any experience
working in product companies you should know well that bug in product
X is not gonna be fixed if it was not reported, but gets fixed on
product Y due to that. Besides that, how do you know that MS has not
been given the special edition of the FW?

As icing on the cake, the Marvell has been bought and I believe they
abandoned their products quite a while ago. You may read kernel
bugzilla for the details (last Marvell developer who answered to the
reports seems has no clue about the driver).

All that said, I believe that we have to follow whatever fixes we
would have and be thankful to contributors and testers.

For the record, I've been suffering from the Linux driver of this
hardware for a while. And I'm fully in support of any quirks that will
help UX.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Pali Rohár]

On Friday 09 July 2021 19:01:44 Andy Shevchenko wrote:
> On Fri, Jul 9, 2021 at 6:18 PM Pali Rohár <pali@kernel.org> wrote:
> > On Friday 09 July 2021 16:58:31 Jonas Dreßler wrote:
> 
> 
> > Hello! Now I'm thinking loudly about this patch. Why this kind of reset
> > is needed only for Surface devices? AFAIK these 88W8897 chips are same
> > in all cards. Chip itself implements PCIe interface (and also SDIO) so
> > for me looks very strange if this 88W8897 PCIe device needs DMI specific
> > quirks. I cannot believe that Microsoft got some special version of
> > these chips from Marvell which are different than version uses on cards
> > in mPCIe form factor.
> >
> > And now when I'm reading comment below about PCIe bridge to which is
> > this 88W8897 PCIe chip connected, is not this rather an issue in that
> > PCIe bridge (instead of mwifiex/88W8897) or in ACPI firmware which
> > controls this bridge?
> >
> > Or are having other people same issues on mPCIe form factor wifi cards
> > with 88W8897 chips and then this quirk should not DMI dependent?
> >
> > Note that I'm seeing issues with reset and other things also on chip
> > 88W8997 when is connected to system via SDIO. These chips have both PCIe
> > and SDIO buses, it just depends which pins are used.
> 
> I'm replying loudly :-)
> 
> You know that depending on the interface the firmware even for the
> same chip may be way different. And if you have had any experience
> working in product companies you should know well that bug in product
> X is not gonna be fixed if it was not reported, but gets fixed on
> product Y due to that. Besides that, how do you know that MS has not
> been given the special edition of the FW?

Yes! But I know something about these chips/cards (I have also one
development kit) and it is quite different. It is possible that
Microsoft may have its special version, because I know that e.g. Google
got "fixed version" for some 88W8xxx chips (it is/was available on
internet). But firmware is loading by mwifiex driver and we
(linux-firmware) have just one version of firmware for these cards.
These 88W8xxx cards lost state and running firmware after reset/power so
after linux is booted, it loads "linux-firmware" version into 88W8897
card and then card not run "possibly MS special edition FW".

What can be possible is that we are dealing with ACPI firmware (which is
same for both Windows and Linux OS) and then it is related to PCIe
bridge where are some PCIe parts implemented...

> As icing on the cake, the Marvell has been bought and I believe they
> abandoned their products quite a while ago. You may read kernel
> bugzilla for the details (last Marvell developer who answered to the
> reports seems has no clue about the driver).

Marvell 88W[89]xxx wifi cards were sold to NXP together with developers.
Old @marvell addresses do not work so it is required to find new @nxp
addresses for developers.

There are recent firmware upgrades from NXP for linux-firmware, see:
https://lore.kernel.org/linux-firmware/DB7PR04MB453855B0D6C41923BCB0922EFC1C9@DB7PR04MB4538.eurprd04.prod.outlook.com/
(just only SDIO firmware for 88W8897, not PCIe)

But I know that response from NXP about these cards is slow... And more
people are complaining about firmware/driver issues for these cards.

> All that said, I believe that we have to follow whatever fixes we
> would have and be thankful to contributors and testers.

I agree. If this is really fixing these issues and we do not have better
fix yet, go ahead with it (if PCIe/WiFi maintainers are fine with it).

Once we have better fix, we can replace it.

Currently I'm just trying to understand this issue more deeply (e.g. how
it can relate with similar issue which I see on SDIO and if I cannot fix
also SDIO in better way) but seems that nobody knows more than "this
hack/quirk somehow works for PCIe".

> For the record, I've been suffering from the Linux driver of this
> hardware for a while. And I'm fully in support of any quirks that will
> help UX.
> 
> -- 
> With Best Regards,
> Andy Shevchenko