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
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 <[email protected]>
---
Current issues:
1) After reset with this quirk, ASPM settings don't get restored.
Below is the "sudo lspci -nnvvv" diff before/after fw reset on Surface 3:
#
# root port of wifi:
# 00:1c.0 PCI bridge [0604]: Intel Corporation Atom/Celeron/Pentium Processor x5-E8000/J3xxx/N3xxx Series PCI Express Port #1 [8086:22c8] (rev 20) (prog-if 00 [Normal decode])
#
@@ -103,7 +103,7 @@
DevSta: CorrErr+ NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend-
LnkCap: Port #1, Speed 5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <512ns, L1 <16us
ClockPM- Surprise- LLActRep+ BwNot+ ASPMOptComp-
- LnkCtl: ASPM L0s L1 Enabled; RCB 64 bytes, Disabled- CommClk+
+ LnkCtl: ASPM Disabled; RCB 64 bytes, Disabled- CommClk+
ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 2.5GT/s (downgraded), Width x1 (ok)
TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt-
#
# no changes on wifi regarding ASPM
#
As you see, the ASPM of wifi root port is disabled after fw reset (thus
breaks S0ix).
2) Creating a new device driver for the WSID device is better?
then export the reset function, and call it from mwifiex. As the comments
in code states, on S3, calling the _DSM methods immediately
remove/re-probe mwifiex. So, resetting the wifi externally is a better
idea? (Also in this way, hopefully supporting SP3 may become easier.)
drivers/net/wireless/marvell/mwifiex/pcie.c | 10 +++
.../wireless/marvell/mwifiex/pcie_quirks.c | 77 ++++++++++++++++++-
.../wireless/marvell/mwifiex/pcie_quirks.h | 5 ++
3 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index c0c4b5a9149ab..b3aacd19cc48f 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2966,6 +2966,16 @@ static void mwifiex_pcie_card_reset_work(struct mwifiex_adapter *adapter)
{
struct pcie_service_card *card = adapter->card;
+ /* On Surface 3, reset_wsid method removes then re-probes card by
+ * itself. So, need to place it here and skip performing any other
+ * reset-related works.
+ */
+ if (card->quirks & QUIRK_FW_RST_WSID_S3) {
+ mwifiex_pcie_reset_wsid_quirk(card->dev);
+ /* skip performing any other reset-related works */
+ return;
+ }
+
/* We can't afford to wait here; remove() might be waiting on us. If we
* can't grab the device lock, maybe we'll get another chance later.
*/
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
index edc739c542fea..f0a6fa0a7ae5f 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
@@ -9,10 +9,21 @@
* down, or causes NULL ptr deref).
*/
+#include <linux/acpi.h>
#include <linux/dmi.h>
#include "pcie_quirks.h"
+/* For reset_wsid quirk */
+#define ACPI_WSID_PATH "\\_SB.WSID"
+#define WSID_REV 0x0
+#define WSID_FUNC_WIFI_PWR_OFF 0x1
+#define WSID_FUNC_WIFI_PWR_ON 0x2
+/* WSID _DSM UUID: "534ea3bf-fcc2-4e7a-908f-a13978f0c7ef" */
+static const guid_t wsid_dsm_guid =
+ GUID_INIT(0x534ea3bf, 0xfcc2, 0x4e7a,
+ 0x90, 0x8f, 0xa1, 0x39, 0x78, 0xf0, 0xc7, 0xef);
+
/* quirk table based on DMI matching */
static const struct dmi_system_id mwifiex_quirk_table[] = {
{
@@ -87,7 +98,7 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
},
- .driver_data = 0,
+ .driver_data = (void *)QUIRK_FW_RST_WSID_S3,
},
{
.ident = "Surface Pro 3",
@@ -113,6 +124,9 @@ void mwifiex_initialize_quirks(struct pcie_service_card *card)
dev_info(&pdev->dev, "no quirks enabled\n");
if (card->quirks & QUIRK_FW_RST_D3COLD)
dev_info(&pdev->dev, "quirk reset_d3cold enabled\n");
+ if (card->quirks & QUIRK_FW_RST_WSID_S3)
+ dev_info(&pdev->dev,
+ "quirk reset_wsid for Surface 3 enabled\n");
}
static void mwifiex_pcie_set_power_d3cold(struct pci_dev *pdev)
@@ -169,3 +183,64 @@ int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev)
return 0;
}
+
+int mwifiex_pcie_reset_wsid_quirk(struct pci_dev *pdev)
+{
+ acpi_handle handle;
+ union acpi_object *obj;
+ acpi_status status;
+
+ dev_info(&pdev->dev, "Using reset_wsid quirk to perform FW reset\n");
+
+ status = acpi_get_handle(NULL, ACPI_WSID_PATH, &handle);
+ if (ACPI_FAILURE(status)) {
+ dev_err(&pdev->dev, "No ACPI handle for path %s\n",
+ ACPI_WSID_PATH);
+ return -ENODEV;
+ }
+
+ if (!acpi_has_method(handle, "_DSM")) {
+ dev_err(&pdev->dev, "_DSM method not found\n");
+ return -ENODEV;
+ }
+
+ if (!acpi_check_dsm(handle, &wsid_dsm_guid,
+ WSID_REV, WSID_FUNC_WIFI_PWR_OFF)) {
+ dev_err(&pdev->dev,
+ "_DSM method doesn't support wifi power off func\n");
+ return -ENODEV;
+ }
+
+ if (!acpi_check_dsm(handle, &wsid_dsm_guid,
+ WSID_REV, WSID_FUNC_WIFI_PWR_ON)) {
+ dev_err(&pdev->dev,
+ "_DSM method doesn't support wifi power on func\n");
+ return -ENODEV;
+ }
+
+ /* card will be removed immediately after this call on Surface 3 */
+ dev_info(&pdev->dev, "turning wifi off...\n");
+ obj = acpi_evaluate_dsm(handle, &wsid_dsm_guid,
+ WSID_REV, WSID_FUNC_WIFI_PWR_OFF,
+ NULL);
+ if (!obj) {
+ dev_err(&pdev->dev,
+ "device _DSM execution failed for turning wifi off\n");
+ return -EIO;
+ }
+ ACPI_FREE(obj);
+
+ /* card will be re-probed immediately after this call on Surface 3 */
+ dev_info(&pdev->dev, "turning wifi on...\n");
+ obj = acpi_evaluate_dsm(handle, &wsid_dsm_guid,
+ WSID_REV, WSID_FUNC_WIFI_PWR_ON,
+ NULL);
+ if (!obj) {
+ dev_err(&pdev->dev,
+ "device _DSM execution failed for turning wifi on\n");
+ return -EIO;
+ }
+ ACPI_FREE(obj);
+
+ return 0;
+}
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
index 8b9dcb5070d87..3ef7440418e32 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
@@ -7,6 +7,11 @@
/* quirks */
#define QUIRK_FW_RST_D3COLD BIT(0)
+/* Surface 3 and Surface Pro 3 have the same _DSM method but need to
+ * be handled differently. Currently, only S3 is supported.
+ */
+#define QUIRK_FW_RST_WSID_S3 BIT(1)
void mwifiex_initialize_quirks(struct pcie_service_card *card);
int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev);
+int mwifiex_pcie_reset_wsid_quirk(struct pci_dev *pdev);
--
2.29.1
On Sat, May 22, 2021 at 03:18:27PM +0200, Jonas Dre?ler wrote:
> From: Tsuchiya Yuto <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jonas Dre?ler <[email protected]>
> ---
> 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
>
On Sat, Jun 5, 2021 at 12:15 AM Bjorn Helgaas <[email protected]> wrote:
> On Sat, May 22, 2021 at 03:18:27PM +0200, Jonas Dreßler wrote:
> > From: Tsuchiya Yuto <[email protected]>
> >
> > 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