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(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index bc1633945a56..4655c944e3fd 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -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
Brian Norris <[email protected]> wrote:
> 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]>
Patch applied to ath-next branch of ath.git, thanks.
96378bd2c6cd ath10k: fix core PCI suspend when WoWLAN is supported but disabled
--
https://patchwork.kernel.org/patch/9960481/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Brian Norris <[email protected]> writes:
> + Arnd
>
> On Thu, Oct 19, 2017 at 02:32:45PM +0000, Kalle Valo wrote:
>> Kalle Valo <[email protected]> writes:
>>=20
>> > Brian Norris <[email protected]> wrote:
>> >
>> >> 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 th=
e
>> >> 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 exclusivel=
y
>> >> 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
>> >> 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=
disabled
>>=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'
Ah, that's why kbuild bot didn't report about this problem while your
patch was in my pending branch. And I was also really puzzled why it
claimed that my merge was at fault :) Thanks for the good explanation.
Just to clarify: ath-current is for patches going to 4.14 and ath-next
to 4.15, that's why they were applied to a different branch.
>> drivers/net/wireless/ath/ath10k/pci.c:3416:8: error: implicit
>> declaration of function 'ath10k_pci_suspend'
>> [-Werror=3Dimplicit-function-declaration]
>>=20
>> 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.
I see that you already sent the patch, thanks!
--=20
Kalle Valo=
Ping? Any comments? I know there's more than one way to slice this
problem, but it's most definitely a problem...
Brian
On Tue, Sep 19, 2017 at 04:24:16PM -0700, Brian Norris wrote:
> 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(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index bc1633945a56..4655c944e3fd 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -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
>
Brian Norris <[email protected]> writes:
> 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(-)
>
> On Thu, Oct 19, 2017 at 10:12:25AM -0700, Brian Norris wrote:
>> 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 :)
Thanks! As this an unusual merge problem between two branches I applied
this manually to ath.git master-pending branch for now. Let's see if
kbuild bot is happy now.
--=20
Kalle Valo=
fwiw - I did this on my in progress ath10k port to freebsd, and I've
tested it on Rome and Peregrine. It seems to be the right thing to do
during suspend to at least cleanly shut things down.
-adrian
On 11 October 2017 at 17:38, Brian Norris <[email protected]> wrote:
> Ping? Any comments? I know there's more than one way to slice this
> problem, but it's most definitely a problem...
>
> Brian
>
> On Tue, Sep 19, 2017 at 04:24:16PM -0700, Brian Norris wrote:
>> 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(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
>> index bc1633945a56..4655c944e3fd 100644
>> --- a/drivers/net/wireless/ath/ath10k/pci.c
>> +++ b/drivers/net/wireless/ath/ath10k/pci.c
>> @@ -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
>>
Brian Norris <[email protected]> wrote:
> 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]>
Patch applied to ath-next branch of ath.git, thanks.
20665a9076d4 ath10k: fix build errors with !CONFIG_PM
--
https://patchwork.kernel.org/patch/10018169/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kalle Valo <[email protected]> writes:
> Brian Norris <[email protected]> wrote:
>
>> 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
>> 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=
sabled
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=3Dimplicit-function-declaration]
drivers/net/wireless/ath/ath10k/pci.c:3428:8: error: implicit
declaration of function 'ath10k_pci_resume'
[-Werror=3Dimplicit-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?
--=20
Kalle Valo=
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]>
---
drivers/net/wireless/ath/ath10k/pci.c | 5 -----
1 file changed, 5 deletions(-)
On Thu, Oct 19, 2017 at 10:12:25AM -0700, Brian Norris wrote:
> 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.
Here you go :)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index b18a9b690df4..d790ea20b95d 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2577,8 +2577,6 @@ 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. */
@@ -2627,7 +2625,6 @@ static int ath10k_pci_resume(struct ath10k *ar)
return ret;
}
-#endif
static bool ath10k_pci_validate_cal(void *data, size_t size)
{
@@ -2782,10 +2779,8 @@ static const struct ath10k_hif_ops ath10k_pci_hif_ops = {
.power_down = ath10k_pci_hif_power_down,
.read32 = ath10k_pci_read32,
.write32 = ath10k_pci_write32,
-#ifdef CONFIG_PM
.suspend = ath10k_pci_hif_suspend,
.resume = ath10k_pci_hif_resume,
-#endif
.fetch_cal_eeprom = ath10k_pci_hif_fetch_cal_eeprom,
};
--
2.15.0.rc1.287.g2b38de12cc-goog
+ Arnd
On Thu, Oct 19, 2017 at 02:32:45PM +0000, Kalle Valo wrote:
> Kalle Valo <[email protected]> writes:
>
> > Brian Norris <[email protected]> wrote:
> >
> >> 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]>
> >
> > 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:
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'
> 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?
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.
Brian
Brian Norris <[email protected]> writes:
> Ping? Any comments? I know there's more than one way to slice this
> problem, but it's most definitely a problem...
I'm lagging behind with patches but I'll try to catch up this week.
Sorry.
--=20
Kalle Valo=