* [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
* 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 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
[parent not found: <1a844abf-2259-ff4f-d49d-de95870345dc@mailbox.org>]
* 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-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
* 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
* [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 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 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
* [RFC PATCH 0/3] mwifiex: add fw reset quirks for Microsoft Surface @ 2020-10-28 14:27 Tsuchiya Yuto 2020-10-28 14:27 ` [RFC PATCH 2/3] mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices Tsuchiya Yuto 0 siblings, 1 reply; 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 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 ^ 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.