ath10k: fix core PCI suspend when WoWLAN is supported but disabled

For devices where the FW supports WoWLAN but user-space has not configured it, we don't do any PCI-specific suspend/resume operations, because mac80211 doesn't call drv_suspend() when !wowlan. This has particularly bad effects for some platforms, because we don't stop the power-save timer, and if this timer goes off after the PCI controller has suspended the link, Bad Things will happen.

Commit 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops") got some of this right, in that it understood there was a problem on non-WoWLAN firmware. But it forgot the $subject case.

Fix this by moving all the PCI driver suspend/resume logic exclusively into the driver PM hooks. This shouldn't affect WoWLAN support much (this just gets executed later on).

I would just as well kill the entirety of ath10k_hif_suspend(), as it's not even implemented on the USB or SDIO drivers. I expect that we don't need the callback, except to return "supported" (i.e., 0) or "not supported" (i.e., -EOPNOTSUPP).

Fixes 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")

Fixes 77258d409ce4 ("ath10k: enable pci soc powersaving")

Signed-off-by Brian Norris <[email protected]>

Cc Ryan Hsu <[email protected]>

Cc Kalle Valo <[email protected]>

Cc Michal Kazior <[email protected]>

---
 drivers/net/wireless/ath/ath10k/pci.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

drivers/net/wireless/ath/ath10k/pci.c +14 -10

index bc1633945a56..4655c944e3fd 100644
@@ -2580,6 +2580,12 @@ void ath10k_pci_hif_power_down(struct ath10k *ar)
 #ifdef CONFIG_PM
 
 static int ath10k_pci_hif_suspend(struct ath10k *ar)
+{
+	/* Nothing to do; the important stuff is in the driver suspend. */
+	return 0;
+}
+
+static int ath10k_pci_suspend(struct ath10k *ar)
 {
 	/* The grace timer can still be counting down and ar->ps_awake be true.
 	 * It is known that the device may be asleep after resuming regardless

@@ -2592,6 +2598,12 @@ static int ath10k_pci_hif_suspend(struct ath10k *ar)
 }
 
 static int ath10k_pci_hif_resume(struct ath10k *ar)
+{
+	/* Nothing to do; the important stuff is in the driver resume. */
+	return 0;
+}
+
+static int ath10k_pci_resume(struct ath10k *ar)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
 	struct pci_dev *pdev = ar_pci->pdev;

@@ -3403,11 +3415,7 @@ static int ath10k_pci_pm_suspend(struct device *dev)
 	struct ath10k *ar = dev_get_drvdata(dev);
 	int ret;
 
-	if (test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
-		     ar->running_fw->fw_file.fw_features))
-		return 0;
-
-	ret = ath10k_hif_suspend(ar);
+	ret = ath10k_pci_suspend(ar);
 	if (ret)
 		ath10k_warn(ar, "failed to suspend hif: %d\n", ret);
 

@@ -3419,11 +3427,7 @@ static int ath10k_pci_pm_resume(struct device *dev)
 	struct ath10k *ar = dev_get_drvdata(dev);
 	int ret;
 
-	if (test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
-		     ar->running_fw->fw_file.fw_features))
-		return 0;
-
-	ret = ath10k_hif_resume(ar);
+	ret = ath10k_pci_resume(ar);
 	if (ret)
 		ath10k_warn(ar, "failed to resume hif: %d\n", ret);
 

Signature · 1 line

2.14.1.690.gbb1197296e-goog

Ping? Any comments? I know there's more than one way to slice this problem, but it's most definitely a problem...

Brian

Quoted 89 lines · Brian Norris

For devices where the FW supports WoWLAN but user-space has not configured it, we don't do any PCI-specific suspend/resume operations, because mac80211 doesn't call drv_suspend() when !wowlan. This has particularly bad effects for some platforms, because we don't stop the power-save timer, and if this timer goes off after the PCI controller has suspended the link, Bad Things will happen.

Commit 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops") got some of this right, in that it understood there was a problem on non-WoWLAN firmware. But it forgot the $subject case.

Fix this by moving all the PCI driver suspend/resume logic exclusively into the driver PM hooks. This shouldn't affect WoWLAN support much (this just gets executed later on).

I would just as well kill the entirety of ath10k_hif_suspend(), as it's not even implemented on the USB or SDIO drivers. I expect that we don't need the callback, except to return "supported" (i.e., 0) or "not supported" (i.e., -EOPNOTSUPP).

Fixes 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")

Fixes 77258d409ce4 ("ath10k: enable pci soc powersaving")

Signed-off-by Brian Norris <[email protected]>

Cc Ryan Hsu <[email protected]>

Cc Kalle Valo <[email protected]>

Cc Michal Kazior <[email protected]>

---
 drivers/net/wireless/ath/ath10k/pci.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

drivers/net/wireless/ath/ath10k/pci.c +14 -11

index bc1633945a56..4655c944e3fd 100644
@@ -2580,6 +2580,12 @@ void ath10k_pci_hif_power_down(struct ath10k *ar)
 #ifdef CONFIG_PM
 
 static int ath10k_pci_hif_suspend(struct ath10k *ar)
+{
+     /* Nothing to do; the important stuff is in the driver suspend. */
+     return 0;
+}
+
+static int ath10k_pci_suspend(struct ath10k *ar)
 {
      /* The grace timer can still be counting down and ar->ps_awake be true.
       * It is known that the device may be asleep after resuming regardless

@@ -2592,6 +2598,12 @@ static int ath10k_pci_hif_suspend(struct ath10k *ar)
 }
 
 static int ath10k_pci_hif_resume(struct ath10k *ar)
+{
+     /* Nothing to do; the important stuff is in the driver resume. */
+     return 0;
+}
+
+static int ath10k_pci_resume(struct ath10k *ar)
 {
      struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
      struct pci_dev *pdev = ar_pci->pdev;

@@ -3403,11 +3415,7 @@ static int ath10k_pci_pm_suspend(struct device *dev)
      struct ath10k *ar = dev_get_drvdata(dev);
      int ret;
 
-     if (test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
-                  ar->running_fw->fw_file.fw_features))
-             return 0;
-
-     ret = ath10k_hif_suspend(ar);
+     ret = ath10k_pci_suspend(ar);
      if (ret)
              ath10k_warn(ar, "failed to suspend hif: %d\n", ret);
 

@@ -3419,11 +3427,7 @@ static int ath10k_pci_pm_resume(struct device *dev)
      struct ath10k *ar = dev_get_drvdata(dev);
      int ret;
 
-     if (test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
-                  ar->running_fw->fw_file.fw_features))
-             return 0;
-
-     ret = ath10k_hif_resume(ar);
+     ret = ath10k_pci_resume(ar);
      if (ret)
              ath10k_warn(ar, "failed to resume hif: %d\n", ret);
 
--

2.14.1.690.gbb1197296e-goog

Ping? Any comments? I know there's more than one way to slice this problem, but it's most definitely a problem...

For devices where the FW supports WoWLAN but user-space has not configured it, we don't do any PCI-specific suspend/resume operations, because mac80211 doesn't call drv_suspend() when !wowlan. This has particularly bad effects for some platforms, because we don't stop the power-save timer, and if this timer goes off after the PCI controller has suspended the link, Bad Things will happen.

Commit 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops") got some of this right, in that it understood there was a problem on non-WoWLAN firmware. But it forgot the $subject case.

Fix this by moving all the PCI driver suspend/resume logic exclusively into the driver PM hooks. This shouldn't affect WoWLAN support much (this just gets executed later on).

I would just as well kill the entirety of ath10k_hif_suspend(), as it's not even implemented on the USB or SDIO drivers. I expect that we don't need the callback, except to return "supported" (i.e., 0) or "not supported" (i.e., -EOPNOTSUPP).

Fixes 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")

Fixes 77258d409ce4 ("ath10k: enable pci soc powersaving")

Signed-off-by Brian Norris <[email protected]>

Cc Ryan Hsu <[email protected]>

Cc Kalle Valo <[email protected]>

Cc Michal Kazior <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

Quoted 20 lines · Brian Norris

For devices where the FW supports WoWLAN but user-space has not configured it, we don't do any PCI-specific suspend/resume operations, because mac80211 doesn't call drv_suspend() when !wowlan. This has particularly bad effects for some platforms, because we don't stop the power-save timer, and if this timer goes off after the PCI controller has suspended the link, Bad Things will happen.
=20
Commit 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops") got some of this right, in that it understood there was a problem on non-WoWLAN firmware. But it forgot the $subject case.
=20
Fix this by moving all the PCI driver suspend/resume logic exclusively into the driver PM hooks. This shouldn't affect WoWLAN support much (this just gets executed later on).
=20
I would just as well kill the entirety of ath10k_hif_suspend(), as it's not even implemented on the USB or SDIO drivers. I expect that we don't need the callback, except to return "supported" (i.e., 0) or "not supported" (i.e., -EOPNOTSUPP).
=20

Quoted 7 lines

Fixes 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")

Fixes 77258d409ce4 ("ath10k: enable pci soc powersaving")

Signed-off-by Brian Norris <[email protected]>

Cc Ryan Hsu <[email protected]>

Cc Kalle Valo <[email protected]>

Cc Michal Kazior <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

Patch applied to ath-next branch of ath.git, thanks.

96378bd2c6cd ath10k: fix core PCI suspend when WoWLAN is supported but di=

Quoted 30 lines · Kalle Valo

Quoted 27 lines · Brian Norris

For devices where the FW supports WoWLAN but user-space has not configured it, we don't do any PCI-specific suspend/resume operations, because mac80211 doesn't call drv_suspend() when !wowlan. This has particularly bad effects for some platforms, because we don't stop the power-save timer, and if this timer goes off after the PCI controller has suspended the link, Bad Things will happen.

Commit 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops") got some of this right, in that it understood there was a problem on non-WoWLAN firmware. But it forgot the $subject case.

Fix this by moving all the PCI driver suspend/resume logic exclusively into the driver PM hooks. This shouldn't affect WoWLAN support much (this just gets executed later on).

I would just as well kill the entirety of ath10k_hif_suspend(), as it's not even implemented on the USB or SDIO drivers. I expect that we don't need the callback, except to return "supported" (i.e., 0) or "not supported" (i.e., -EOPNOTSUPP).

Fixes 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")

Fixes 77258d409ce4 ("ath10k: enable pci soc powersaving")

Signed-off-by Brian Norris <[email protected]>

Cc Ryan Hsu <[email protected]>

Cc Kalle Valo <[email protected]>

Cc Michal Kazior <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

Quoted 3 lines

Patch applied to ath-next branch of ath.git, thanks.

96378bd2c6cd ath10k: fix core PCI suspend when WoWLAN is supported but disabled

Kbuild found a build problem, I suspect it's caused by this patch:

drivers/net/wireless/ath/ath10k/pci.c:3416:8: error: implicit declaration of function 'ath10k_pci_suspend' [-Werror=implicit-function-declaration]

drivers/net/wireless/ath/ath10k/pci.c:3428:8: error: implicit declaration of function 'ath10k_pci_resume' [-Werror=implicit-function-declaration]

http://lists.infradead.org/pipermail/ath10k/2017-October/010269.html

The .config.gz there doesn't have CONFIG_PM set, maybe that's the problem?

The solution would seem to be either to kill the #ifdefs around ath10k_pci_{suspend,resume}() and friends (and use __maybe_unused instead, to further extend Arnd's patch), or else revert Arnd's stuff and go with CONFIG_PM_SLEEP everywhere, which would resolve the original warning (promoted to error) that Arnd was resolving.

I can send out one of these if you'd like.

Build errors have been reported with CONFIG_PM=3Dn:

drivers/net/wireless/ath/ath10k/pci.c:3416:8: error: implicit declaration of function 'ath10k_pci_suspend' [-Werror=3Dimplicit-function-declaration]

drivers/net/wireless/ath/ath10k/pci.c:3428:8: error: implicit declaration of function 'ath10k_pci_resume' [-Werror=3Dimplicit-function-declaration]

These are caused by the combination of the following two commits:

6af1de2e4ec4 ("ath10k: mark PM functions as __maybe_unused") 96378bd2c6cd ("ath10k: fix core PCI suspend when WoWLAN is supported but disabled")

Both build fine on their own.

But now that ath10k_pci_pm_{suspend,resume}() is compiled unconditionally, we should also compile ath10k_pci_{suspend,resume}() unconditionally.

And drop the #ifdef around ath10k_pci_hif_{suspend,resume}() too; they are trivial (empty), so we're not saving much space by compiling them out. And the alternatives would be to sprinkle more __maybe_unused, or spread the #ifdef's further.

Build tested with the following combinations:
CONFIG_PM=3Dy && CONFIG_PM_SLEEP=3Dy
CONFIG_PM=3Dy && CONFIG_PM_SLEEP=3Dn
CONFIG_PM=3Dn

Fixes 96378bd2c6cd ("ath10k: fix core PCI suspend when WoWLAN is

supported but disabled")

Fixes 096ad2a15fd8 ("Merge branch 'ath-next'")

Signed-off-by Brian Norris <[email protected]>

---
 drivers/net/wireless/ath/ath10k/pci.c | 5 -----
 1 file changed, 5 deletions(-)

Quoted 7 lines · Brian Norris

The solution would seem to be either to kill the #ifdefs around ath10k_pci_{suspend,resume}() and friends (and use __maybe_unused instead, to further extend Arnd's patch), or else revert Arnd's stuff and go with CONFIG_PM_SLEEP everywhere, which would resolve the original warning (promoted to error) that Arnd was resolving.
=20
I can send out one of these if you'd like.

Here you go :)

Build errors have been reported with CONFIG_PM=n:

drivers/net/wireless/ath/ath10k/pci.c:3416:8: error: implicit declaration of function 'ath10k_pci_suspend' [-Werror=implicit-function-declaration]

drivers/net/wireless/ath/ath10k/pci.c:3428:8: error: implicit declaration of function 'ath10k_pci_resume' [-Werror=implicit-function-declaration]

These are caused by the combination of the following two commits:

6af1de2e4ec4 ("ath10k: mark PM functions as __maybe_unused") 96378bd2c6cd ("ath10k: fix core PCI suspend when WoWLAN is supported but disabled")

Both build fine on their own.

But now that ath10k_pci_pm_{suspend,resume}() is compiled unconditionally, we should also compile ath10k_pci_{suspend,resume}() unconditionally.

And drop the #ifdef around ath10k_pci_hif_{suspend,resume}() too; they are trivial (empty), so we're not saving much space by compiling them out. And the alternatives would be to sprinkle more __maybe_unused, or spread the #ifdef's further.

Build tested with the following combinations:
CONFIG_PM=y && CONFIG_PM_SLEEP=y
CONFIG_PM=y && CONFIG_PM_SLEEP=n
CONFIG_PM=n

Fixes 96378bd2c6cd ("ath10k: fix core PCI suspend when WoWLAN is supported but disabled")

Fixes 096ad2a15fd8 ("Merge branch 'ath-next'")

Signed-off-by Brian Norris <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

+ Arnd

Quoted 6 lines · Kalle Valo

Kalle Valo <[email protected]> writes:
=20

Quoted 4 lines

Quoted 2 lines · Brian Norris

For devices where the FW supports WoWLAN but user-space has not configured it, we don't do any PCI-specific suspend/resume operations=

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

because mac80211 doesn't call drv_suspend() when !wowlan. This has particularly bad effects for some platforms, because we don't stop th=

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

power-save timer, and if this timer goes off after the PCI controller has suspended the link, Bad Things will happen.
=20
Commit 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops"=

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

got some of this right, in that it understood there was a problem on non-WoWLAN firmware. But it forgot the $subject case.
=20
Fix this by moving all the PCI driver suspend/resume logic exclusivel=

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

into the driver PM hooks. This shouldn't affect WoWLAN support much (this just gets executed later on).
=20
I would just as well kill the entirety of ath10k_hif_suspend(), as it=

Quoted 1 line

Quoted 1 line

Quoted 1 line

not even implemented on the USB or SDIO drivers. I expect that we don=

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

need the callback, except to return "supported" (i.e., 0) or "not supported" (i.e., -EOPNOTSUPP).
=20

Fixes 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops"=

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

Fixes 77258d409ce4 ("ath10k: enable pci soc powersaving")

Signed-off-by Brian Norris <[email protected]>

Cc Ryan Hsu <[email protected]>

Cc Kalle Valo <[email protected]>

Cc Michal Kazior <[email protected]>

Signed-off-by Kalle Valo <[email protected]>

Patch applied to ath-next branch of ath.git, thanks.

96378bd2c6cd ath10k: fix core PCI suspend when WoWLAN is supported but=

Quoted 2 lines

=20
Kbuild found a build problem, I suspect it's caused by this patch:

Actually, it's the interaction of this patch and Arnd's patch:

6af1de2e4ec4 ath10k: mark PM functions as __maybe_unused

I see that's now in these branches:

ath/ath-current
ath/ath-qca
ath/master
ath/master-pending
wireless-drivers-next/master
wireless-drivers-next/pending

Whereas mine got applied to:

ath/ath-next

So technically, the problem is in your merge here :)

096ad2a15fd8 Merge branch 'ath-next'

Quoted 4 lines

drivers/net/wireless/ath/ath10k/pci.c:3416:8: error: implicit declaration of function 'ath10k_pci_suspend' [-Werror=3Dimplicit-function-declaration]
=20

Quoted 8 lines

drivers/net/wireless/ath/ath10k/pci.c:3428:8: error: implicit declaration of function 'ath10k_pci_resume' [-Werror=3Dimplicit-function-declaration]
=20
http://lists.infradead.org/pipermail/ath10k/2017-October/010269.html
=20
The .config.gz there doesn't have CONFIG_PM set, maybe that's the problem?

Yes, indirectly that's also the problem.

The solution would seem to be either to kill the #ifdefs around ath10k_pci_{suspend,resume}() and friends (and use __maybe_unused instead, to further extend Arnd's patch), or else revert Arnd's stuff and go with CONFIG_PM_SLEEP everywhere, which would resolve the original warning (promoted to error) that Arnd was resolving.

I can send out one of these if you'd like.