[RFC PATCH 0/3] mwifiex: add fw reset quirks for Microsoft Surface

[RFC PATCH 0/3] mwifiex: add fw reset quirks for Microsoft Surface

[Tsuchiya Yuto]

Hello all,

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.

Thanks,
Tsuchiya Yuto

Tsuchiya Yuto (3):
  mwifiex: pcie: add DMI-based quirk impl for Surface devices
  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   |   2 +
 .../wireless/marvell/mwifiex/pcie_quirks.c    | 246 ++++++++++++++++++
 .../wireless/marvell/mwifiex/pcie_quirks.h    |  17 ++
 5 files changed, 287 insertions(+)
 create mode 100644 drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
 create mode 100644 drivers/net/wireless/marvell/mwifiex/pcie_quirks.h

-- 
2.29.1

[RFC PATCH 1/3] mwifiex: pcie: add DMI-based quirk impl for Surface devices

[Tsuchiya Yuto]

This commit adds quirk implementation based on DMI matching with DMI
table for Microsoft Surface devices that uses mwifiex chip (currently,
all the devices that use mwifiex equip PCIe-88W8897 chip).

This implementation can be used for quirks later.

Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
 drivers/net/wireless/marvell/mwifiex/Makefile |   1 +
 drivers/net/wireless/marvell/mwifiex/pcie.c   |   4 +
 drivers/net/wireless/marvell/mwifiex/pcie.h   |   2 +
 .../wireless/marvell/mwifiex/pcie_quirks.c    | 114 ++++++++++++++++++
 .../wireless/marvell/mwifiex/pcie_quirks.h    |  11 ++
 5 files changed, 132 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 fdfd9bf15ed46..8a1e7c5b9c6e2 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 6a10ff0377a24..362cf10debfa0 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 843d57eda8201..09839a3bd1753 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
@@ -242,6 +242,8 @@ struct pcie_service_card {
 	struct mwifiex_msix_context share_irq_ctx;
 	struct work_struct work;
 	unsigned long work_flags;
+
+	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 0000000000000..929aee2b0a60a
--- /dev/null
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
@@ -0,0 +1,114 @@
+// 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[] = {
+	{
+		.ident = "Surface Pro 4",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 4"),
+		},
+		.driver_data = 0,
+	},
+	{
+		.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 = 0,
+	},
+	{
+		.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 = 0,
+	},
+	{
+		.ident = "Surface Pro 6",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 6"),
+		},
+		.driver_data = 0,
+	},
+	{
+		.ident = "Surface Book 1",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book"),
+		},
+		.driver_data = 0,
+	},
+	{
+		.ident = "Surface Book 2",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book 2"),
+		},
+		.driver_data = 0,
+	},
+	{
+		.ident = "Surface Laptop 1",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop"),
+		},
+		.driver_data = 0,
+	},
+	{
+		.ident = "Surface Laptop 2",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop 2"),
+		},
+		.driver_data = 0,
+	},
+	{
+		.ident = "Surface 3",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
+		},
+		.driver_data = 0,
+	},
+	{
+		.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,
+	},
+	{}
+};
+
+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 0000000000000..5326ae7e56713
--- /dev/null
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for PCIe quirks.
+ */
+
+#include "pcie.h"
+
+/* quirks */
+// quirk flags can be added here
+
+void mwifiex_initialize_quirks(struct pcie_service_card *card);
-- 
2.29.1

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

[Tsuchiya Yuto]

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>
---
Current issue:
* After reset with this quirk, ASPM settings don't get restored.

Below is the "sudo lspci -nnvvv" diff before/after fw reset on Surface Book 1:

    #
    # 03:00.0 Ethernet controller [0200]: Marvell Technology Group Ltd. 88W8897 [AVASTAR] 802.11ac Wireless [11ab:2b38]
    #
    @@ -574,9 +574,9 @@
            Capabilities: [168 v1] L1 PM Substates
                    L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+
                              PortCommonModeRestoreTime=70us PortTPowerOnTime=10us
    -               L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
    -                          T_CommonMode=0us LTR1.2_Threshold=163840ns
    -               L1SubCtl2: T_PwrOn=44us
    +               L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
    +                          T_CommonMode=0us LTR1.2_Threshold=0ns
    +               L1SubCtl2: T_PwrOn=10us
            Kernel driver in use: mwifiex_pcie
            Kernel modules: mwifiex_pcie
    
    #
    # no changes on root port of wifi regarding ASPM
    #

As you see, all of the L1 substates are disabled after fw reset. LTR
value is also changed.

 drivers/net/wireless/marvell/mwifiex/pcie.c   |  7 ++
 .../wireless/marvell/mwifiex/pcie_quirks.c    | 73 +++++++++++++++++--
 .../wireless/marvell/mwifiex/pcie_quirks.h    |  3 +-
 3 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 362cf10debfa0..c0c4b5a9149ab 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -529,6 +529,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__);
 }
 
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
index 929aee2b0a60a..edc739c542fea 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
@@ -21,7 +21,7 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
 			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
 			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 4"),
 		},
-		.driver_data = 0,
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
 	},
 	{
 		.ident = "Surface Pro 5",
@@ -30,7 +30,7 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
 			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
 			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1796"),
 		},
-		.driver_data = 0,
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
 	},
 	{
 		.ident = "Surface Pro 5 (LTE)",
@@ -39,7 +39,7 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
 			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
 			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1807"),
 		},
-		.driver_data = 0,
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
 	},
 	{
 		.ident = "Surface Pro 6",
@@ -47,7 +47,7 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
 			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
 			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 6"),
 		},
-		.driver_data = 0,
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
 	},
 	{
 		.ident = "Surface Book 1",
@@ -55,7 +55,7 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
 			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
 			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book"),
 		},
-		.driver_data = 0,
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
 	},
 	{
 		.ident = "Surface Book 2",
@@ -63,7 +63,7 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
 			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
 			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book 2"),
 		},
-		.driver_data = 0,
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
 	},
 	{
 		.ident = "Surface Laptop 1",
@@ -71,7 +71,7 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
 			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
 			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop"),
 		},
-		.driver_data = 0,
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
 	},
 	{
 		.ident = "Surface Laptop 2",
@@ -79,7 +79,7 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
 			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
 			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop 2"),
 		},
-		.driver_data = 0,
+		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
 	},
 	{
 		.ident = "Surface 3",
@@ -111,4 +111,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 5326ae7e56713..8b9dcb5070d87 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
@@ -6,6 +6,7 @@
 #include "pcie.h"
 
 /* quirks */
-// quirk flags can be added here
+#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.29.1

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

[Tsuchiya Yuto]

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>
---
Current issues:
1) After reset with this quirk, ASPM settings don't get restored.

Below is the "sudo lspci -nnvvv" diff before/after fw reset on Surface 3:

    #
    # root port of wifi:
    # 00:1c.0 PCI bridge [0604]: Intel Corporation Atom/Celeron/Pentium Processor x5-E8000/J3xxx/N3xxx Series PCI Express Port #1 [8086:22c8] (rev 20) (prog-if 00 [Normal decode])
    #
    @@ -103,7 +103,7 @@
                    DevSta: CorrErr+ NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend-
                    LnkCap: Port #1, Speed 5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <512ns, L1 <16us
                            ClockPM- Surprise- LLActRep+ BwNot+ ASPMOptComp-
    -               LnkCtl: ASPM L0s L1 Enabled; RCB 64 bytes, Disabled- CommClk+
    +               LnkCtl: ASPM Disabled; RCB 64 bytes, Disabled- CommClk+
                            ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
                    LnkSta: Speed 2.5GT/s (downgraded), Width x1 (ok)
                            TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt-
    
    #
    # no changes on wifi regarding ASPM
    #

As you see, the ASPM of wifi root port is disabled after fw reset (thus
breaks S0ix).

2) Creating a new device driver for the WSID device is better?

then export the reset function, and call it from mwifiex. As the comments
in code states, on S3, calling the _DSM methods immediately
remove/re-probe mwifiex. So, resetting the wifi externally is a better
idea? (Also in this way, hopefully supporting SP3 may become easier.)

 drivers/net/wireless/marvell/mwifiex/pcie.c   | 10 +++
 .../wireless/marvell/mwifiex/pcie_quirks.c    | 77 ++++++++++++++++++-
 .../wireless/marvell/mwifiex/pcie_quirks.h    |  5 ++
 3 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index c0c4b5a9149ab..b3aacd19cc48f 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2966,6 +2966,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 edc739c542fea..f0a6fa0a7ae5f 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[] = {
 	{
@@ -87,7 +98,7 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
 			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
 			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
 		},
-		.driver_data = 0,
+		.driver_data = (void *)QUIRK_FW_RST_WSID_S3,
 	},
 	{
 		.ident = "Surface Pro 3",
@@ -113,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)
@@ -169,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 8b9dcb5070d87..3ef7440418e32 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
@@ -7,6 +7,11 @@
 
 /* quirks */
 #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.29.1

Re: [RFC PATCH 1/3] mwifiex: pcie: add DMI-based quirk impl for Surface devices

[Tsuchiya Yuto]

On Wed, 2020-10-28 at 17:27 +0200, Andy Shevchenko wrote:
> On Wed, Oct 28, 2020 at 11:27:51PM +0900, Tsuchiya Yuto wrote:
>> This commit adds quirk implementation based on DMI matching with DMI
>> table for Microsoft Surface devices that uses mwifiex chip (currently,
>> all the devices that use mwifiex equip PCIe-88W8897 chip).
>>
>> This implementation can be used for quirks later.
>
> I guess you might need to resend this (and possible other PCI pm related)
> patches to linux-pci@ and Bjorn in Cc.
>
> They may advise possible other (better) solutions.

Thanks for the advice! I also feel it's better. And if I resend this to
the linux-pci mailing list, I feel that I should try to use
drivers/pci/quirks.c instead if possible. But if I do so with this quirk
implementation, it might be possible that it'll be too big change.

So, I'll just resend this series (with changes so that it can be used
for powr_save quirk as well like I said in the reply to another series)
to the linux-pci mailing list and Bjorn (and linux-wireless mailing list
as well) and see what they think.

>> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
>> ---
>>  drivers/net/wireless/marvell/mwifiex/Makefile |   1 +
>>  drivers/net/wireless/marvell/mwifiex/pcie.c   |   4 +
>>  drivers/net/wireless/marvell/mwifiex/pcie.h   |   2 +
>>  .../wireless/marvell/mwifiex/pcie_quirks.c    | 114 ++++++++++++++++++
>>  .../wireless/marvell/mwifiex/pcie_quirks.h    |  11 ++
>>  5 files changed, 132 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 fdfd9bf15ed46..8a1e7c5b9c6e2 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 6a10ff0377a24..362cf10debfa0 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 843d57eda8201..09839a3bd1753 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/pcie.h
>> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
>> @@ -242,6 +242,8 @@ struct pcie_service_card {
>>  	struct mwifiex_msix_context share_irq_ctx;
>>  	struct work_struct work;
>>  	unsigned long work_flags;
>> +
>> +	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 0000000000000..929aee2b0a60a
>> --- /dev/null
>> +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
>> @@ -0,0 +1,114 @@
>> +// 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[] = {
>> +	{
>> +		.ident = "Surface Pro 4",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 4"),
>> +		},
>> +		.driver_data = 0,
>> +	},
>> +	{
>> +		.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 = 0,
>> +	},
>> +	{
>> +		.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 = 0,
>> +	},
>> +	{
>> +		.ident = "Surface Pro 6",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 6"),
>> +		},
>> +		.driver_data = 0,
>> +	},
>> +	{
>> +		.ident = "Surface Book 1",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book"),
>> +		},
>> +		.driver_data = 0,
>> +	},
>> +	{
>> +		.ident = "Surface Book 2",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book 2"),
>> +		},
>> +		.driver_data = 0,
>> +	},
>> +	{
>> +		.ident = "Surface Laptop 1",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop"),
>> +		},
>> +		.driver_data = 0,
>> +	},
>> +	{
>> +		.ident = "Surface Laptop 2",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop 2"),
>> +		},
>> +		.driver_data = 0,
>> +	},
>> +	{
>> +		.ident = "Surface 3",
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
>> +		},
>> +		.driver_data = 0,
>> +	},
>> +	{
>> +		.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,
>> +	},
>> +	{}
>> +};
>> +
>> +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 0000000000000..5326ae7e56713
>> --- /dev/null
>> +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
>> @@ -0,0 +1,11 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Header file for PCIe quirks.
>> + */
>> +
>> +#include "pcie.h"
>> +
>> +/* quirks */
>> +// quirk flags can be added here
>> +
>> +void mwifiex_initialize_quirks(struct pcie_service_card *card);
>> -- 
>> 2.29.1
>>
>