All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/3] mwifiex: Add quirks for MS Surface devices
@ 2021-05-22 13:18 Jonas Dreßler
  2021-05-22 13:18 ` [RFC PATCH 1/3] mwifiex: pcie: add DMI-based quirk implementation for " Jonas Dreßler
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Jonas Dreßler @ 2021-05-22 13:18 UTC (permalink / raw)
  To: Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Kalle Valo, David S. Miller, Jakub Kicinski
  Cc: Jonas Dreßler, Tsuchiya Yuto, linux-wireless, netdev,
	linux-kernel, linux-pci, Maximilian Luz, Andy Shevchenko,
	Bjorn Helgaas, Pali Rohár


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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 1/3] mwifiex: pcie: add DMI-based quirk implementation for Surface devices
  2021-05-22 13:18 [RFC PATCH 0/3] mwifiex: Add quirks for MS Surface devices Jonas Dreßler
@ 2021-05-22 13:18 ` Jonas Dreßler
  2021-05-22 13:18 ` [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices Jonas Dreßler
  2021-05-22 13:18 ` [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3 Jonas Dreßler
  2 siblings, 0 replies; 14+ messages in thread
From: Jonas Dreßler @ 2021-05-22 13:18 UTC (permalink / raw)
  To: Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Kalle Valo, David S. Miller, Jakub Kicinski
  Cc: Jonas Dreßler, Tsuchiya Yuto, linux-wireless, netdev,
	linux-kernel, linux-pci, Maximilian Luz, Andy Shevchenko,
	Bjorn Helgaas, Pali Rohár

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices
  2021-05-22 13:18 [RFC PATCH 0/3] mwifiex: Add quirks for MS Surface devices Jonas Dreßler
  2021-05-22 13:18 ` [RFC PATCH 1/3] mwifiex: pcie: add DMI-based quirk implementation for " Jonas Dreßler
@ 2021-05-22 13:18 ` Jonas Dreßler
  2021-05-22 18:44   ` Amey Narkhede
  2021-06-04 21:08   ` Bjorn Helgaas
  2021-05-22 13:18 ` [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3 Jonas Dreßler
  2 siblings, 2 replies; 14+ messages in thread
From: Jonas Dreßler @ 2021-05-22 13:18 UTC (permalink / raw)
  To: Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Kalle Valo, David S. Miller, Jakub Kicinski
  Cc: Jonas Dreßler, Tsuchiya Yuto, linux-wireless, netdev,
	linux-kernel, linux-pci, Maximilian Luz, Andy Shevchenko,
	Bjorn Helgaas, Pali Rohár

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3
  2021-05-22 13:18 [RFC PATCH 0/3] mwifiex: Add quirks for MS Surface devices Jonas Dreßler
  2021-05-22 13:18 ` [RFC PATCH 1/3] mwifiex: pcie: add DMI-based quirk implementation for " Jonas Dreßler
  2021-05-22 13:18 ` [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices Jonas Dreßler
@ 2021-05-22 13:18 ` Jonas Dreßler
  2021-05-22 22:01   ` kernel test robot
                     ` (2 more replies)
  2 siblings, 3 replies; 14+ messages in thread
From: Jonas Dreßler @ 2021-05-22 13:18 UTC (permalink / raw)
  To: Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Kalle Valo, David S. Miller, Jakub Kicinski
  Cc: Jonas Dreßler, Tsuchiya Yuto, linux-wireless, netdev,
	linux-kernel, linux-pci, Maximilian Luz, Andy Shevchenko,
	Bjorn Helgaas, Pali Rohár

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices
  2021-05-22 13:18 ` [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices Jonas Dreßler
@ 2021-05-22 18:44   ` Amey Narkhede
  2021-05-23 10:30     ` Jonas Dreßler
       [not found]     ` <1a844abf-2259-ff4f-d49d-de95870345dc@mailbox.org>
  2021-06-04 21:08   ` Bjorn Helgaas
  1 sibling, 2 replies; 14+ messages in thread
From: Amey Narkhede @ 2021-05-22 18:44 UTC (permalink / raw)
  To: Jonas Dreßler
  Cc: Alex Williamson, Tsuchiya Yuto, linux-wireless, netdev,
	linux-kernel, linux-pci, Maximilian Luz, Andy Shevchenko,
	Bjorn Helgaas, Pali Rohár, Krzysztof Wilczyński,
	Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Kalle Valo, David S. Miller, Jakub Kicinski

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
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3
  2021-05-22 13:18 ` [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3 Jonas Dreßler
@ 2021-05-22 22:01   ` kernel test robot
  2021-05-23  2:32   ` kernel test robot
  2021-06-04 21:14   ` Bjorn Helgaas
  2 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2021-05-22 22:01 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3203 bytes --]

Hi "Jonas,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on wireless-drivers/master v5.13-rc2 next-20210521]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jonas-Dre-ler/mwifiex-Add-quirks-for-MS-Surface-devices/20210522-222428
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: riscv-allyesconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/664dc875005de6def700fef775192ea019ebc8ab
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jonas-Dre-ler/mwifiex-Add-quirks-for-MS-Surface-devices/20210522-222428
        git checkout 664dc875005de6def700fef775192ea019ebc8ab
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/wireless/marvell/mwifiex/pcie_quirks.c: In function 'mwifiex_pcie_reset_wsid_quirk':
>> drivers/net/wireless/marvell/mwifiex/pcie_quirks.c:202:7: error: implicit declaration of function 'acpi_has_method'; did you mean 'acpi_has_watchdog'? [-Werror=implicit-function-declaration]
     202 |  if (!acpi_has_method(handle, "_DSM")) {
         |       ^~~~~~~~~~~~~~~
         |       acpi_has_watchdog
>> drivers/net/wireless/marvell/mwifiex/pcie_quirks.c:207:7: error: implicit declaration of function 'acpi_check_dsm'; did you mean 'acpi_check_region'? [-Werror=implicit-function-declaration]
     207 |  if (!acpi_check_dsm(handle, &wsid_dsm_guid,
         |       ^~~~~~~~~~~~~~
         |       acpi_check_region
   cc1: some warnings being treated as errors


vim +202 drivers/net/wireless/marvell/mwifiex/pcie_quirks.c

   186	
   187	int mwifiex_pcie_reset_wsid_quirk(struct pci_dev *pdev)
   188	{
   189		acpi_handle handle;
   190		union acpi_object *obj;
   191		acpi_status status;
   192	
   193		dev_info(&pdev->dev, "Using reset_wsid quirk to perform FW reset\n");
   194	
   195		status = acpi_get_handle(NULL, ACPI_WSID_PATH, &handle);
   196		if (ACPI_FAILURE(status)) {
   197			dev_err(&pdev->dev, "No ACPI handle for path %s\n",
   198				ACPI_WSID_PATH);
   199			return -ENODEV;
   200		}
   201	
 > 202		if (!acpi_has_method(handle, "_DSM")) {
   203			dev_err(&pdev->dev, "_DSM method not found\n");
   204			return -ENODEV;
   205		}
   206	
 > 207		if (!acpi_check_dsm(handle, &wsid_dsm_guid,

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 69115 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3
  2021-05-22 13:18 ` [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3 Jonas Dreßler
  2021-05-22 22:01   ` kernel test robot
@ 2021-05-23  2:32   ` kernel test robot
  2021-06-04 21:14   ` Bjorn Helgaas
  2 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2021-05-23  2:32 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3389 bytes --]

Hi "Jonas,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on wireless-drivers/master v5.13-rc2 next-20210521]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jonas-Dre-ler/mwifiex-Add-quirks-for-MS-Surface-devices/20210522-222428
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: riscv-randconfig-r004-20210523 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project e84a9b9bb3051c35dea993cdad7b3d2575638f85)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/664dc875005de6def700fef775192ea019ebc8ab
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jonas-Dre-ler/mwifiex-Add-quirks-for-MS-Surface-devices/20210522-222428
        git checkout 664dc875005de6def700fef775192ea019ebc8ab
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/wireless/marvell/mwifiex/pcie_quirks.c:202:7: error: implicit declaration of function 'acpi_has_method' [-Werror,-Wimplicit-function-declaration]
           if (!acpi_has_method(handle, "_DSM")) {
                ^
   drivers/net/wireless/marvell/mwifiex/pcie_quirks.c:202:7: note: did you mean 'acpi_has_watchdog'?
   include/linux/acpi.h:1288:20: note: 'acpi_has_watchdog' declared here
   static inline bool acpi_has_watchdog(void) { return false; }
                      ^
>> drivers/net/wireless/marvell/mwifiex/pcie_quirks.c:207:7: error: implicit declaration of function 'acpi_check_dsm' [-Werror,-Wimplicit-function-declaration]
           if (!acpi_check_dsm(handle, &wsid_dsm_guid,
                ^
   2 errors generated.


vim +/acpi_has_method +202 drivers/net/wireless/marvell/mwifiex/pcie_quirks.c

   186	
   187	int mwifiex_pcie_reset_wsid_quirk(struct pci_dev *pdev)
   188	{
   189		acpi_handle handle;
   190		union acpi_object *obj;
   191		acpi_status status;
   192	
   193		dev_info(&pdev->dev, "Using reset_wsid quirk to perform FW reset\n");
   194	
   195		status = acpi_get_handle(NULL, ACPI_WSID_PATH, &handle);
   196		if (ACPI_FAILURE(status)) {
   197			dev_err(&pdev->dev, "No ACPI handle for path %s\n",
   198				ACPI_WSID_PATH);
   199			return -ENODEV;
   200		}
   201	
 > 202		if (!acpi_has_method(handle, "_DSM")) {
   203			dev_err(&pdev->dev, "_DSM method not found\n");
   204			return -ENODEV;
   205		}
   206	
 > 207		if (!acpi_check_dsm(handle, &wsid_dsm_guid,

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 34104 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices
  2021-05-22 18:44   ` Amey Narkhede
@ 2021-05-23 10:30     ` Jonas Dreßler
       [not found]     ` <1a844abf-2259-ff4f-d49d-de95870345dc@mailbox.org>
  1 sibling, 0 replies; 14+ messages in thread
From: Jonas Dreßler @ 2021-05-23 10:30 UTC (permalink / raw)
  To: Amey Narkhede, Jonas Dreßler
  Cc: Alex Williamson, Tsuchiya Yuto, linux-wireless, netdev,
	linux-kernel, linux-pci, Maximilian Luz, Andy Shevchenko,
	Bjorn Helgaas, Pali Rohár, Krzysztof Wilczyński,
	Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Kalle Valo, David S. Miller, Jakub Kicinski

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices
       [not found]     ` <1a844abf-2259-ff4f-d49d-de95870345dc@mailbox.org>
@ 2021-05-24 20:27       ` Amey Narkhede
  2021-07-09 14:10         ` Jonas Dreßler
  0 siblings, 1 reply; 14+ messages in thread
From: Amey Narkhede @ 2021-05-24 20:27 UTC (permalink / raw)
  To: Jonas Dreßler
  Cc: Alex Williamson, Tsuchiya Yuto, linux-wireless, netdev,
	linux-kernel, linux-pci, Maximilian Luz, Andy Shevchenko,
	Bjorn Helgaas, Pali Rohár, Krzysztof Wilczyński,
	Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Kalle Valo, David S. Miller, Jakub Kicinski

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices
  2021-05-22 13:18 ` [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices Jonas Dreßler
  2021-05-22 18:44   ` Amey Narkhede
@ 2021-06-04 21:08   ` Bjorn Helgaas
  1 sibling, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2021-06-04 21:08 UTC (permalink / raw)
  To: Jonas Dreßler
  Cc: Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Kalle Valo, David S. Miller, Jakub Kicinski,
	Tsuchiya Yuto, linux-wireless, netdev, linux-kernel, linux-pci,
	Maximilian Luz, Andy Shevchenko, Bjorn Helgaas, Pali Rohár

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3
  2021-05-22 13:18 ` [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3 Jonas Dreßler
  2021-05-22 22:01   ` kernel test robot
  2021-05-23  2:32   ` kernel test robot
@ 2021-06-04 21:14   ` Bjorn Helgaas
  2021-06-04 21:26     ` Andy Shevchenko
  2 siblings, 1 reply; 14+ messages in thread
From: Bjorn Helgaas @ 2021-06-04 21:14 UTC (permalink / raw)
  To: Jonas Dreßler
  Cc: Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Kalle Valo, David S. Miller, Jakub Kicinski,
	Tsuchiya Yuto, linux-wireless, netdev, linux-kernel, linux-pci,
	Maximilian Luz, Andy Shevchenko, Bjorn Helgaas, Pali Rohár

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
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3
  2021-06-04 21:14   ` Bjorn Helgaas
@ 2021-06-04 21:26     ` Andy Shevchenko
  0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2021-06-04 21:26 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jonas Dreßler, Amitkumar Karwar, Ganapathi Bhat,
	Sharvari Harisangam, Xinming Hu, Kalle Valo, David S. Miller,
	Jakub Kicinski, Tsuchiya Yuto, linux-wireless, netdev,
	linux-kernel, linux-pci, Maximilian Luz, Andy Shevchenko,
	Bjorn Helgaas, Pali Rohár

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices
  2021-05-24 20:27       ` Amey Narkhede
@ 2021-07-09 14:10         ` Jonas Dreßler
  0 siblings, 0 replies; 14+ messages in thread
From: Jonas Dreßler @ 2021-07-09 14:10 UTC (permalink / raw)
  To: Amey Narkhede
  Cc: Alex Williamson, Tsuchiya Yuto, linux-wireless, netdev,
	linux-kernel, linux-pci, Maximilian Luz, Andy Shevchenko,
	Bjorn Helgaas, Pali Rohár, Krzysztof Wilczyński,
	Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
	Xinming Hu, Kalle Valo, David S. Miller, Jakub Kicinski

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices
  2020-10-28 14:27 [RFC PATCH 0/3] mwifiex: add fw reset quirks for Microsoft Surface Tsuchiya Yuto
@ 2020-10-28 14:27 ` Tsuchiya Yuto
  0 siblings, 0 replies; 14+ messages in thread
From: Tsuchiya Yuto @ 2020-10-28 14:27 UTC (permalink / raw)
  To: Amitkumar Karwar, Ganapathi Bhat, Xinming Hu, Kalle Valo,
	David S. Miller, Jakub Kicinski
  Cc: linux-wireless, netdev, linux-kernel, Maximilian Luz,
	Andy Shevchenko, verdre, 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-07-09 14:11 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-22 13:18 [RFC PATCH 0/3] mwifiex: Add quirks for MS Surface devices Jonas Dreßler
2021-05-22 13:18 ` [RFC PATCH 1/3] mwifiex: pcie: add DMI-based quirk implementation for " Jonas Dreßler
2021-05-22 13:18 ` [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices Jonas Dreßler
2021-05-22 18:44   ` Amey Narkhede
2021-05-23 10:30     ` Jonas Dreßler
     [not found]     ` <1a844abf-2259-ff4f-d49d-de95870345dc@mailbox.org>
2021-05-24 20:27       ` Amey Narkhede
2021-07-09 14:10         ` Jonas Dreßler
2021-06-04 21:08   ` Bjorn Helgaas
2021-05-22 13:18 ` [RFC PATCH 3/3] mwifiex: pcie: add reset_wsid quirk for Surface 3 Jonas Dreßler
2021-05-22 22:01   ` kernel test robot
2021-05-23  2:32   ` kernel test robot
2021-06-04 21:14   ` Bjorn Helgaas
2021-06-04 21:26     ` Andy Shevchenko
  -- strict thread matches above, loose matches on Subject: below --
2020-10-28 14:27 [RFC PATCH 0/3] mwifiex: add fw reset quirks for Microsoft Surface Tsuchiya Yuto
2020-10-28 14:27 ` [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices Tsuchiya Yuto

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.