mwifiex: add fw reset quirks for Surface devices

[kitakar5525]

This PR adds

1. DMI tables of Surface devices (first and second patch)
2. fw reset quirk for Surface gen4+ devices (third patch)
3. fw reset quirk for Surface 3 (fourth patch)

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.

For Surface gen4+ devices, we can reset fw by putting wifi into D3cold then bring back into D0 (power-cycle).

For Surface 3, we can reset fw by calling _DSM method available in \_SB.WSID device (https://github.com/linux-surface/acpidumps/blob/05cba925f3a515f222acb5b3551a032ddde958fe/surface_3/dsdt.dsl#L11947-L12011).

Note that for Surface Pro 3, there is the same WSID device exists (https://github.com/linux-surface/acpidumps/blob/05cba925f3a515f222acb5b3551a032ddde958fe/surface_pro_3/dsdt.dsl#L12164-L12216). However, I'm not sure if it will work in the same way as Surface 3. So, I didn't add support for SP3 in this PR. Might look into it later.

[kitakar5525]

You can trigger the card reset manually via sysfs entry:

$ echo 1 | sudo tee /sys/kernel/debug/mwifiex/mlan0/reset

[kitakar5525]

mwifiex: pcie: add reset_wsid quirk for Surface 3

Forgot to mention that why this quirk is required.

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

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

EDIT: Added.

[kitakar5525]

mwifiex: pcie: add reset_wsid quirk for Surface 3

Hmm, some indentations look weird. I'll fix them later.

EDIT: Fixed. (Changed tab size setting to 8 in my editor)

[kitakar5525]

Addressed the following checkpatch warnings:
mwifiex: pcie: add Surface devices DMI table for quirks

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#33: FILE: drivers/net/wireless/marvell/mwifiex/pcie_quirks.h:1:
+/*

mwifiex: pcie: add reset_d3cold quirk for Surface gen4+ devices

WARNING: Block comments use a trailing */ on a separate line
#53: FILE: drivers/net/wireless/marvell/mwifiex/pcie.c:398:
+	 * before performing FLR */

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#66: FILE: drivers/net/wireless/marvell/mwifiex/pcie_quirks.c:1:
+/*

WARNING: Block comments use a trailing */ on a separate line
#109: FILE: drivers/net/wireless/marvell/mwifiex/pcie_quirks.c:44:
+	 * "Cannot transition to power state D0 for parent in D3hot". */

mwifiex: pcie: add reset_wsid quirk for Surface 3

WARNING: Block comments use a trailing */ on a separate line
#39: FILE: drivers/net/wireless/marvell/mwifiex/pcie.c:2830:
+	 * reset-related works. */

Ignoring WARNING: networking block comments don't use an empty /* line, use /* Comment... at top of files in accordance with the other upstream files:

WARNING: networking block comments don't use an empty /* line, use /* Comment...
#35: FILE: drivers/net/wireless/marvell/mwifiex/pcie_quirks.h:3:
+/*
+ * Header file for PCIe quirks.

[kitakar5525]

Thanks for the review! I've pushed four new patches.

The first two patches addressed the review.
The rest is for simplification/upstreaming.

If they look OK, I'll squash the patches.

[qzed]

Looks mostly good, a couple of nitpicks.

[kitakar5525]

Squashed patches with addressing the review feedbacks:

• moved mwifiex_initialize_quirks() before mwifiex_add_card()
• switched to using GUID_INIT()
• changed acpi_evaluate_dsm() failure return value to -EFAULT
• added ACPI_FREE(obj); after acpi_evaluate_dsm() success

Other changes:

• do not use mwifiex_dbg() to avoid troubles in pcie_quirks.c
  (e.g. to avoid using mwifiex_adapter struct before init or wifi is powered down, or causes NULL ptr deref).
  Added comment regarding this to the top of pcie_quirks.c:
  

  kernel/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c

  Lines 21 to 25 in 5dcea7f

  	/* 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).
  	*/

[kitakar5525]

I've rebased again.

Changes:

• Fixed changes squashed into wrong patch (the print dev_info(&pdev->dev, "no quirks applied\n"); went into second patch by mistake, now in first patch)
• Moved comment for (do not use mwifiex_dbg() to avoid troubles in pcie_quirks.c) higher (now above #include):
  

  kernel/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c

  Lines 1 to 15 in a90d243

  	// 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/acpi.h>
  	#include <linux/dmi.h>
  	#include "pcie_quirks.h"

• Use result of mwifiex_pcie_set_power_d0()
  

  kernel/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c

  Lines 186 to 191 in a90d243

  	ret = mwifiex_pcie_set_power_d0(parent_pdev);
  	if (ret)
  	return ret;
  	ret = mwifiex_pcie_set_power_d0(pdev);
  	if (ret)
  	return ret;

[kitakar5525]

Rebased with addressing review feedbacks:

• dropped QUIRK_NONE and used .driver_data = 0 for the skeleton
• dropped path variable in mwifiex_pcie_reset_wsid_quirk()
• changed return value of acpi_evaluate_dsm() failure to -EIO

Other changes:

• added _DSM check for also wifi power on function
  There was only check for wifi power off function before, so also add for on accordingly
  

  kernel/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c

  Lines 216 to 228 in 5591da4

  	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;
  	}

• change wording no quirks applied -> no quirks enabled in mwifiex_initialize_quirks() in accordance with the other print there
• just use "NOT operator" instead of == in mwifiex_initialize_quirks()
card->quirks == 0 -> !card->quirks
• moved patch mwifiex: pcie: (OEMB) add quirk for Surface 3 with broken DMI table to last so that the first three patches can be sent upstream easily

[qzed]

Okay, looks good to me. Feel free to merge when you're ready.

[kitakar5525]

Thanks!

There are still issues remaining:

1. For the reset_wsid quirk (for S3 and SP3), I think that 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 _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.)

2. After reset, ASPM settings don't get restored completely.
   sudo lspci -nnvvv diff before/after fw reset on SB1:

#
# 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
#

All of the L1 substates are disabled after fw reset. LTR value is also changed.

sudo lspci -nnvvv diff before/after fw reset on S3:

#
# 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
#

It's worse in this case. The ASPM of wifi root port is disabled after fw reset (thus breaks S0ix).

Anyway, we can improve these later. This PR just works regarding fw reset. So, I'll go this way for now.
I'll merge this later when the other open PR becomes ready.

[qzed]

Keep in mind that with an external driver you might need to ensure correct suspend/resume ordering though, which might complicate things. I know too little about the whole situation to say what's better here though-

[kitakar5525]

I think what we need to do is just exposing reset function via EXPORT_SYMBOL_GPL(). So, I hope there are no issues with suspend/resume ordering, but I may be wrong though.

[qzed]

You'd have to make sure that reset can be called both before and after mwifiex has resumed and also executed concurrently. At least if you plan on calling it during resume. Otherwise you'd have to enforce an ordering. You could probably also call it during the complete phase, assuming that mwifiex doesn't have a handler for that phase (then it's guaranteed to run after the basic resume steps have been executed). You probably still want to make sure that it can executed concurrently though.

[kitakar5525]

Hmm, anyway I'll try something first.

[kitakar5525]

Rebased on top of current v5.8-surface-devel due to conflict with #59 (with correcting grammar in commit messages)

[katamayko]

friends
i installed debian 12 for the first time this week and i'm having wifi problems on my surface pro 4
I don't understand how to overcome this, can you give me a step-by-step guide if there is a solution