[RFC PATCH 0/3] mwifiex: Add quirks for MS Surface devices

[RFC PATCH 0/3] mwifiex: Add quirks for MS Surface devices

[Jonas Dreßler]

This series is based on the patches from Tsuchiya Yuto which have been
submitted previously already, where it was suggested to cc linux-pci and
Bjorn to ask if there's a better way of doing those quirks.

Original series sent in by Tsuchiya: https://lore.kernel.org/linux-wireless/20201028142753.18855-1-kitakar@gmail.com/

Here's the summary written by Tsuchiya:

This series adds firmware reset quirks for Microsoft Surface devices
(PCIe-88W8897). Surface devices somehow requires quirks to reset the
firmware. Otherwise, current mwifiex driver can reset only software level.
This is not enough to recover from a bad state.

To do so, in the first patch, I added a DMI-based quirk implementation
for Surface devices that use mwifiex chip.

The required quirk is different by generation. Surface gen3 devices
(Surface 3 and Surface Pro 3) require a quirk that calls _DSM method
(the third patch).
Note that Surface Pro 3 is not yet supported because of the difference
between Surface 3. On Surface 3, the wifi card will be immediately
removed/reprobed after the _DSM call. On the other hand, Surface Pro 3
doesn't. Need to remove/reprobe wifi card ourselves. This behavior makes
the support difficult.

Surface gen4+ devices (Surface Pro 4 and later) require a quirk that
puts wifi into D3cold before FLR.

While here, created new files for quirks (mwifiex/pcie_quirks.c and
mwifiex/pcie_quirks.h) because the changes are a little bit too big to
add into pcie.c.

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

Tsuchiya Yuto (2):
  mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices
  mwifiex: pcie: add reset_wsid quirk for Surface 3

 drivers/net/wireless/marvell/mwifiex/Makefile |   1 +
 drivers/net/wireless/marvell/mwifiex/pcie.c   |  21 ++
 drivers/net/wireless/marvell/mwifiex/pcie.h   |   1 +
 .../wireless/marvell/mwifiex/pcie_quirks.c    | 246 ++++++++++++++++++
 .../wireless/marvell/mwifiex/pcie_quirks.h    |  17 ++
 5 files changed, 286 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

[RFC PATCH 1/3] 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 94228b316df1..02fdce926de5 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

[RFC PATCH 2/3] 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 02fdce926de5..d9acfea395ad 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);
+
+	/* For Surface gen4+ devices, we need to put wifi into D3cold right
+	 * before performing FLR
+	 */
+	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: [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Amey Narkhede]

On 21/05/22 03:18PM, 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)
>
May I know how did you reset only the wifi device when you encountered
this error?

> 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 02fdce926de5..d9acfea395ad 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);
> +
> +	/* For Surface gen4+ devices, we need to put wifi into D3cold right
> +	 * before performing FLR
> +	 */
> +	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);
> +}
pci_set_power_state with PCI_D3cold state calls
pci_bus_set_current_state(dev->subordinate, PCI_D3cold).
Maybe this was the reason for the earlier problem you had?
Not 100% sure about this though CCing: Alex

> +
> +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);
On the side note just save and restore is enough in this case?
What would be the device <-> driver state after the reset as you
are calling this on parent_pdev below so that affects other
devices on bus?

Thanks,
Amey

> +
> +	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: [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

[Jonas Dreßler]

On 5/22/21 8:44 PM, Amey Narkhede wrote:
 > On 21/05/22 03:18PM, 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)
 >>
 > May I know how did you reset only the wifi device when you encountered
 > this error?

Not exactly sure what you mean by that, the trick was to put the parent
bridge into D3cold and then into D0 before transitioning the card into
D0.

That "Cannot transition to power state" warning is just the kernel
enforcing ACPI specs afaik, and that prevents us from putting the device
into ACPI state D0. This in turn means the device still has no power and
we can't set the PCI power state to D0, which is the second error.

 >
 >> 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 02fdce926de5..d9acfea395ad 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);
 >> +
 >> +	/* For Surface gen4+ devices, we need to put wifi into D3cold right
 >> +	 * before performing FLR
 >> +	 */
 >> +	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);
 >> +}
 > pci_set_power_state with PCI_D3cold state calls
 > pci_bus_set_current_state(dev->subordinate, PCI_D3cold).
 > Maybe this was the reason for the earlier problem you had?
 > Not 100% sure about this though CCing: Alex

Hmm, so we'd only have to put the bridge into D3cold and that takes care
of the device going to D3cold automatically?

 >
 >> +
 >> +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);
 > On the side note just save and restore is enough in this case?
 > What would be the device <-> driver state after the reset as you
 > are calling this on parent_pdev below so that affects other
 > devices on bus?

Not sure we can do anything more than save and restore, can we? I don't
think it will affect other devices on the bus, the parent bridge is only
connected to the wifi card, nothing else.

 >
 > Thanks,
 > Amey
 >
 >> +
 >> +	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
 >>

Thanks for the review,
Jonas

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

[Amey Narkhede]

On 21/05/23 12:28PM, Jonas Dreßler wrote:
> On 5/22/21 8:44 PM, Amey Narkhede wrote:
> > On 21/05/22 03:18PM, 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)
> > >
> > May I know how did you reset only the wifi device when you encountered
> > this error?
>
> Not exactly sure what you mean by that, the trick was to put the parent
> bridge into D3cold and then into D0 before transitioning the card into
> D0.
>
If the parent bridge has multiple devices attached to it, this can
have some side effects on other devices after the reset but as you
mentioned below that parent bridge is only connected to wifi card it
should be fine in that case.

> That "Cannot transition to power state" warning is just the kernel
> enforcing ACPI specs afaik, and that prevents us from putting the device
> into ACPI state D0. This in turn means the device still has no power and
> we can't set the PCI power state to D0, which is the second error.
>
> >
> > > 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 02fdce926de5..d9acfea395ad 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);
> > > +
> > > +	/* For Surface gen4+ devices, we need to put wifi into D3cold right
> > > +	 * before performing FLR
> > > +	 */
> > > +	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);
> > > +}
> > pci_set_power_state with PCI_D3cold state calls
> > pci_bus_set_current_state(dev->subordinate, PCI_D3cold).
> > Maybe this was the reason for the earlier problem you had?
> > Not 100% sure about this though CCing: Alex
>
> Hmm, so we'd only have to put the bridge into D3cold and that takes care
> of the device going to D3cold automatically?
>
Yeah I think it should do it. Have you tried this?
> >
> > > +
> > > +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);
> > On the side note just save and restore is enough in this case?
> > What would be the device <-> driver state after the reset as you
> > are calling this on parent_pdev below so that affects other
> > devices on bus?
>
> Not sure we can do anything more than save and restore, can we? I don't
> think it will affect other devices on the bus, the parent bridge is only
> connected to the wifi card, nothing else.
>
I was thinking of doing remove-reset-rescan but I think save-restore
should be ok if there is a single device connected to the parent bridge.

Thanks,
Amey
[...]
> > > 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
> > >
>
> Thanks for the review,
> Jonas

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

[Jonas Dreßler]

On 5/24/21 10:27 PM, Amey Narkhede wrote:
 > On 21/05/23 12:28PM, Jonas Dreßler wrote:
 >> On 5/22/21 8:44 PM, Amey Narkhede wrote:
 >>> On 21/05/22 03:18PM, 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)
 >>>>
 >>> May I know how did you reset only the wifi device when you encountered
 >>> this error?
 >>
 >> Not exactly sure what you mean by that, the trick was to put the parent
 >> bridge into D3cold and then into D0 before transitioning the card into
 >> D0.
 >>
 > If the parent bridge has multiple devices attached to it, this can
 > have some side effects on other devices after the reset but as you
 > mentioned below that parent bridge is only connected to wifi card it
 > should be fine in that case.
 >
 >> That "Cannot transition to power state" warning is just the kernel
 >> enforcing ACPI specs afaik, and that prevents us from putting the device
 >> into ACPI state D0. This in turn means the device still has no power and
 >> we can't set the PCI power state to D0, which is the second error.
 >>
 >>>
 >>>> 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 02fdce926de5..d9acfea395ad 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);
 >>>> +
 >>>> +	/* For Surface gen4+ devices, we need to put wifi into D3cold right
 >>>> +	 * before performing FLR
 >>>> +	 */
 >>>> +	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);
 >>>> +}
 >>> pci_set_power_state with PCI_D3cold state calls
 >>> pci_bus_set_current_state(dev->subordinate, PCI_D3cold).
 >>> Maybe this was the reason for the earlier problem you had?
 >>> Not 100% sure about this though CCing: Alex
 >>
 >> Hmm, so we'd only have to put the bridge into D3cold and that takes care
 >> of the device going to D3cold automatically?
 >>
 > Yeah I think it should do it. Have you tried this?

Finally found some time to try that now and looks like it doesn't work. 
First reset works fine, but on the second one the device can't switch 
from D3cold to D0:

mwifiex_pcie 0000:01:00.0: can't change power state from D3cold to D0 
(config space inaccessible)

Thanks,
Jonas

 >>>
 >>>> +
 >>>> +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);
 >>> On the side note just save and restore is enough in this case?
 >>> What would be the device <-> driver state after the reset as you
 >>> are calling this on parent_pdev below so that affects other
 >>> devices on bus?
 >>
 >> Not sure we can do anything more than save and restore, can we? I don't
 >> think it will affect other devices on the bus, the parent bridge is only
 >> connected to the wifi card, nothing else.
 >>
 > I was thinking of doing remove-reset-rescan but I think save-restore
 > should be ok if there is a single device connected to the parent bridge.
 >
 > Thanks,
 > Amey
 > [...]
 >>>> 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
 >>>>
 >>
 >> Thanks for the review,
 >> Jonas

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

[Bjorn Helgaas]

On Sat, May 22, 2021 at 03:18:26PM +0200, 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 02fdce926de5..d9acfea395ad 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);
> +
> +	/* For Surface gen4+ devices, we need to put wifi into D3cold right
> +	 * before performing FLR

This comment seems incorrect or at least incomplete.  When the device
is in D3cold, it isn't powered at all, so you can't do anything with
it, including FLR.  But maybe you meant that you need to put it in
D3cold and back to D0 before doing an FLR.  That would work.  But in
that case, there's no point in an FLR because the power cycle has
already reset more than the FLR will.

Bjorn

[RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3

[Jonas Dreßler]

From: Tsuchiya Yuto <kitakar@gmail.com>

This commit adds reset_wsid quirk and uses this quirk for Surface 3 on
card reset.

To reset mwifiex on Surface 3, it seems that calling the _DSM method
exists in \_SB.WSID [1] device is required.

On Surface 3, calling the _DSM method removes/re-probes the card by
itself. So, need to place the reset function before performing FLR and
skip performing any other reset-related works.

Note that Surface Pro 3 also has the WSID device [2], but it seems to need
more work. This commit only supports Surface 3 yet.

[1] https://github.com/linux-surface/acpidumps/blob/05cba925f3a515f222acb5b3551a032ddde958fe/surface_3/dsdt.dsl#L11947-L12011
[2] https://github.com/linux-surface/acpidumps/blob/05cba925f3a515f222acb5b3551a032ddde958fe/surface_pro_3/dsdt.dsl#L12164-L12216

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

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index d9acfea395ad..6e049236ae1a 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2969,6 +2969,16 @@ static void mwifiex_pcie_card_reset_work(struct mwifiex_adapter *adapter)
 {
 	struct pcie_service_card *card = adapter->card;
 
+	/* On Surface 3, reset_wsid method removes then re-probes card by
+	 * itself. So, need to place it here and skip performing any other
+	 * reset-related works.
+	 */
+	if (card->quirks & QUIRK_FW_RST_WSID_S3) {
+		mwifiex_pcie_reset_wsid_quirk(card->dev);
+		/* skip performing any other reset-related works */
+		return;
+	}
+
 	/* We can't afford to wait here; remove() might be waiting on us. If we
 	 * can't grab the device lock, maybe we'll get another chance later.
 	 */
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
index b5f214fc1212..f0a6fa0a7ae5 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
@@ -9,10 +9,21 @@
  * down, or causes NULL ptr deref).
  */
 
+#include <linux/acpi.h>
 #include <linux/dmi.h>
 
 #include "pcie_quirks.h"
 
+/* For reset_wsid quirk */
+#define ACPI_WSID_PATH		"\\_SB.WSID"
+#define WSID_REV		0x0
+#define WSID_FUNC_WIFI_PWR_OFF	0x1
+#define WSID_FUNC_WIFI_PWR_ON	0x2
+/* WSID _DSM UUID: "534ea3bf-fcc2-4e7a-908f-a13978f0c7ef" */
+static const guid_t wsid_dsm_guid =
+	GUID_INIT(0x534ea3bf, 0xfcc2, 0x4e7a,
+		  0x90, 0x8f, 0xa1, 0x39, 0x78, 0xf0, 0xc7, 0xef);
+
 /* quirk table based on DMI matching */
 static const struct dmi_system_id mwifiex_quirk_table[] = {
 	{
@@ -81,6 +92,22 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
 		},
 		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
 	},
+	{
+		.ident = "Surface 3",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
+		},
+		.driver_data = (void *)QUIRK_FW_RST_WSID_S3,
+	},
+	{
+		.ident = "Surface Pro 3",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 3"),
+		},
+		.driver_data = 0,
+	},
 	{}
 };
 
@@ -97,6 +124,9 @@ void mwifiex_initialize_quirks(struct pcie_service_card *card)
 		dev_info(&pdev->dev, "no quirks enabled\n");
 	if (card->quirks & QUIRK_FW_RST_D3COLD)
 		dev_info(&pdev->dev, "quirk reset_d3cold enabled\n");
+	if (card->quirks & QUIRK_FW_RST_WSID_S3)
+		dev_info(&pdev->dev,
+			 "quirk reset_wsid for Surface 3 enabled\n");
 }
 
 static void mwifiex_pcie_set_power_d3cold(struct pci_dev *pdev)
@@ -153,3 +183,64 @@ int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev)
 
 	return 0;
 }
+
+int mwifiex_pcie_reset_wsid_quirk(struct pci_dev *pdev)
+{
+	acpi_handle handle;
+	union acpi_object *obj;
+	acpi_status status;
+
+	dev_info(&pdev->dev, "Using reset_wsid quirk to perform FW reset\n");
+
+	status = acpi_get_handle(NULL, ACPI_WSID_PATH, &handle);
+	if (ACPI_FAILURE(status)) {
+		dev_err(&pdev->dev, "No ACPI handle for path %s\n",
+			ACPI_WSID_PATH);
+		return -ENODEV;
+	}
+
+	if (!acpi_has_method(handle, "_DSM")) {
+		dev_err(&pdev->dev, "_DSM method not found\n");
+		return -ENODEV;
+	}
+
+	if (!acpi_check_dsm(handle, &wsid_dsm_guid,
+			    WSID_REV, WSID_FUNC_WIFI_PWR_OFF)) {
+		dev_err(&pdev->dev,
+			"_DSM method doesn't support wifi power off func\n");
+		return -ENODEV;
+	}
+
+	if (!acpi_check_dsm(handle, &wsid_dsm_guid,
+			    WSID_REV, WSID_FUNC_WIFI_PWR_ON)) {
+		dev_err(&pdev->dev,
+			"_DSM method doesn't support wifi power on func\n");
+		return -ENODEV;
+	}
+
+	/* card will be removed immediately after this call on Surface 3 */
+	dev_info(&pdev->dev, "turning wifi off...\n");
+	obj = acpi_evaluate_dsm(handle, &wsid_dsm_guid,
+				WSID_REV, WSID_FUNC_WIFI_PWR_OFF,
+				NULL);
+	if (!obj) {
+		dev_err(&pdev->dev,
+			"device _DSM execution failed for turning wifi off\n");
+		return -EIO;
+	}
+	ACPI_FREE(obj);
+
+	/* card will be re-probed immediately after this call on Surface 3 */
+	dev_info(&pdev->dev, "turning wifi on...\n");
+	obj = acpi_evaluate_dsm(handle, &wsid_dsm_guid,
+				WSID_REV, WSID_FUNC_WIFI_PWR_ON,
+				NULL);
+	if (!obj) {
+		dev_err(&pdev->dev,
+			"device _DSM execution failed for turning wifi on\n");
+		return -EIO;
+	}
+	ACPI_FREE(obj);
+
+	return 0;
+}
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
index 549093067813..d90ada3f2daa 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
@@ -7,5 +7,11 @@
 
 #define QUIRK_FW_RST_D3COLD	BIT(0)
 
+/* Surface 3 and Surface Pro 3 have the same _DSM method but need to
+ * be handled differently. Currently, only S3 is supported.
+ */
+#define QUIRK_FW_RST_WSID_S3	BIT(1)
+
 void mwifiex_initialize_quirks(struct pcie_service_card *card);
 int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev);
+int mwifiex_pcie_reset_wsid_quirk(struct pci_dev *pdev);
-- 
2.31.1

Re: [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3

[Bjorn Helgaas]

On Sat, May 22, 2021 at 03:18:27PM +0200, Jonas Dreßler wrote:
> From: Tsuchiya Yuto <kitakar@gmail.com>
> 
> This commit adds reset_wsid quirk and uses this quirk for Surface 3 on
> card reset.
> 
> To reset mwifiex on Surface 3, it seems that calling the _DSM method
> exists in \_SB.WSID [1] device is required.
> 
> On Surface 3, calling the _DSM method removes/re-probes the card by
> itself. So, need to place the reset function before performing FLR and
> skip performing any other reset-related works.

Maybe this is a nit-pick, but I understand "probing" to be something
the OS does, namely what we normally call "enumeration," i.e.,
discovering a device.

So it sounds like the _DSM causes a logical hot-removal of the card,
which the PCI hotplug driver should notice and it should remove the
driver and remove the pci_dev.

And the _DSM also causes a hot-add (reading the code below, it looks
like this is actually a second _DSM), which the PCI hotplug driver
should also notice and enumerate the bus (i.e., it reads config space
looking for a device).  This all would cause a new pci_dev to be
allocated, resources assigned for its BARs, and the driver .probe()
method to be called again?

That seems like a lot, so maybe I didn't understand what's actually
happening.

> Note that Surface Pro 3 also has the WSID device [2], but it seems to need
> more work. This commit only supports Surface 3 yet.
> 
> [1] https://github.com/linux-surface/acpidumps/blob/05cba925f3a515f222acb5b3551a032ddde958fe/surface_3/dsdt.dsl#L11947-L12011
> [2] https://github.com/linux-surface/acpidumps/blob/05cba925f3a515f222acb5b3551a032ddde958fe/surface_pro_3/dsdt.dsl#L12164-L12216
> 
> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c   | 10 ++
>  .../wireless/marvell/mwifiex/pcie_quirks.c    | 91 +++++++++++++++++++
>  .../wireless/marvell/mwifiex/pcie_quirks.h    |  6 ++
>  3 files changed, 107 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index d9acfea395ad..6e049236ae1a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -2969,6 +2969,16 @@ static void mwifiex_pcie_card_reset_work(struct mwifiex_adapter *adapter)
>  {
>  	struct pcie_service_card *card = adapter->card;
>  
> +	/* On Surface 3, reset_wsid method removes then re-probes card by
> +	 * itself. So, need to place it here and skip performing any other
> +	 * reset-related works.
> +	 */
> +	if (card->quirks & QUIRK_FW_RST_WSID_S3) {
> +		mwifiex_pcie_reset_wsid_quirk(card->dev);
> +		/* skip performing any other reset-related works */
> +		return;
> +	}
> +
>  	/* We can't afford to wait here; remove() might be waiting on us. If we
>  	 * can't grab the device lock, maybe we'll get another chance later.
>  	 */
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> index b5f214fc1212..f0a6fa0a7ae5 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> @@ -9,10 +9,21 @@
>   * down, or causes NULL ptr deref).
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/dmi.h>
>  
>  #include "pcie_quirks.h"
>  
> +/* For reset_wsid quirk */
> +#define ACPI_WSID_PATH		"\\_SB.WSID"
> +#define WSID_REV		0x0
> +#define WSID_FUNC_WIFI_PWR_OFF	0x1
> +#define WSID_FUNC_WIFI_PWR_ON	0x2
> +/* WSID _DSM UUID: "534ea3bf-fcc2-4e7a-908f-a13978f0c7ef" */
> +static const guid_t wsid_dsm_guid =
> +	GUID_INIT(0x534ea3bf, 0xfcc2, 0x4e7a,
> +		  0x90, 0x8f, 0xa1, 0x39, 0x78, 0xf0, 0xc7, 0xef);
> +
>  /* quirk table based on DMI matching */
>  static const struct dmi_system_id mwifiex_quirk_table[] = {
>  	{
> @@ -81,6 +92,22 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
>  		},
>  		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
>  	},
> +	{
> +		.ident = "Surface 3",
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
> +		},
> +		.driver_data = (void *)QUIRK_FW_RST_WSID_S3,
> +	},
> +	{
> +		.ident = "Surface Pro 3",
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 3"),
> +		},
> +		.driver_data = 0,
> +	},
>  	{}
>  };
>  
> @@ -97,6 +124,9 @@ void mwifiex_initialize_quirks(struct pcie_service_card *card)
>  		dev_info(&pdev->dev, "no quirks enabled\n");
>  	if (card->quirks & QUIRK_FW_RST_D3COLD)
>  		dev_info(&pdev->dev, "quirk reset_d3cold enabled\n");
> +	if (card->quirks & QUIRK_FW_RST_WSID_S3)
> +		dev_info(&pdev->dev,
> +			 "quirk reset_wsid for Surface 3 enabled\n");
>  }
>  
>  static void mwifiex_pcie_set_power_d3cold(struct pci_dev *pdev)
> @@ -153,3 +183,64 @@ int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev)
>  
>  	return 0;
>  }
> +
> +int mwifiex_pcie_reset_wsid_quirk(struct pci_dev *pdev)
> +{
> +	acpi_handle handle;
> +	union acpi_object *obj;
> +	acpi_status status;
> +
> +	dev_info(&pdev->dev, "Using reset_wsid quirk to perform FW reset\n");
> +
> +	status = acpi_get_handle(NULL, ACPI_WSID_PATH, &handle);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(&pdev->dev, "No ACPI handle for path %s\n",
> +			ACPI_WSID_PATH);
> +		return -ENODEV;
> +	}
> +
> +	if (!acpi_has_method(handle, "_DSM")) {
> +		dev_err(&pdev->dev, "_DSM method not found\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!acpi_check_dsm(handle, &wsid_dsm_guid,
> +			    WSID_REV, WSID_FUNC_WIFI_PWR_OFF)) {
> +		dev_err(&pdev->dev,
> +			"_DSM method doesn't support wifi power off func\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!acpi_check_dsm(handle, &wsid_dsm_guid,
> +			    WSID_REV, WSID_FUNC_WIFI_PWR_ON)) {
> +		dev_err(&pdev->dev,
> +			"_DSM method doesn't support wifi power on func\n");
> +		return -ENODEV;
> +	}
> +
> +	/* card will be removed immediately after this call on Surface 3 */
> +	dev_info(&pdev->dev, "turning wifi off...\n");
> +	obj = acpi_evaluate_dsm(handle, &wsid_dsm_guid,
> +				WSID_REV, WSID_FUNC_WIFI_PWR_OFF,
> +				NULL);
> +	if (!obj) {
> +		dev_err(&pdev->dev,
> +			"device _DSM execution failed for turning wifi off\n");
> +		return -EIO;
> +	}
> +	ACPI_FREE(obj);
> +
> +	/* card will be re-probed immediately after this call on Surface 3 */
> +	dev_info(&pdev->dev, "turning wifi on...\n");
> +	obj = acpi_evaluate_dsm(handle, &wsid_dsm_guid,
> +				WSID_REV, WSID_FUNC_WIFI_PWR_ON,
> +				NULL);
> +	if (!obj) {
> +		dev_err(&pdev->dev,
> +			"device _DSM execution failed for turning wifi on\n");
> +		return -EIO;
> +	}
> +	ACPI_FREE(obj);
> +
> +	return 0;
> +}
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
> index 549093067813..d90ada3f2daa 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
> @@ -7,5 +7,11 @@
>  
>  #define QUIRK_FW_RST_D3COLD	BIT(0)
>  
> +/* Surface 3 and Surface Pro 3 have the same _DSM method but need to
> + * be handled differently. Currently, only S3 is supported.
> + */
> +#define QUIRK_FW_RST_WSID_S3	BIT(1)
> +
>  void mwifiex_initialize_quirks(struct pcie_service_card *card);
>  int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev);
> +int mwifiex_pcie_reset_wsid_quirk(struct pci_dev *pdev);
> -- 
> 2.31.1
> 

Re: [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3

[Andy Shevchenko]

On Sat, Jun 5, 2021 at 12:15 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Sat, May 22, 2021 at 03:18:27PM +0200, Jonas Dreßler wrote:
> > From: Tsuchiya Yuto <kitakar@gmail.com>
> >
> > This commit adds reset_wsid quirk and uses this quirk for Surface 3 on
> > card reset.
> >
> > To reset mwifiex on Surface 3, it seems that calling the _DSM method
> > exists in \_SB.WSID [1] device is required.
> >
> > On Surface 3, calling the _DSM method removes/re-probes the card by
> > itself. So, need to place the reset function before performing FLR and
> > skip performing any other reset-related works.
>
> Maybe this is a nit-pick, but I understand "probing" to be something
> the OS does, namely what we normally call "enumeration," i.e.,
> discovering a device.
>
> So it sounds like the _DSM causes a logical hot-removal of the card,
> which the PCI hotplug driver should notice and it should remove the
> driver and remove the pci_dev.
>
> And the _DSM also causes a hot-add (reading the code below, it looks
> like this is actually a second _DSM),

_DSM can be only one (single) per device node in ACPI.
But _DSM may have "functions", that's what we see here.

> which the PCI hotplug driver
> should also notice and enumerate the bus (i.e., it reads config space
> looking for a device).  This all would cause a new pci_dev to be
> allocated, resources assigned for its BARs, and the driver .probe()
> method to be called again?
>
> That seems like a lot, so maybe I didn't understand what's actually
> happening.


-- 
With Best Regards,
Andy Shevchenko