| Message ID | 20170919232416.108247-1-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <briannorris@chromium.org> > Cc: Ryan Hsu <ryanhsu@qti.qualcomm.com> > Cc: Kalle Valo <kvalo@qca.qualcomm.com> > Cc: Michal Kazior <michal.kazior@tieto.com> > --- > 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 >
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 <briannorris@chromium.org> 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 <briannorris@chromium.org> >> Cc: Ryan Hsu <ryanhsu@qti.qualcomm.com> >> Cc: Kalle Valo <kvalo@qca.qualcomm.com> >> Cc: Michal Kazior <michal.kazior@tieto.com> >> --- >> 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 <briannorris@chromium.org> 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.
Brian Norris <briannorris@chromium.org> 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 <briannorris@chromium.org> > Cc: Ryan Hsu <ryanhsu@qti.qualcomm.com> > Cc: Kalle Valo <kvalo@qca.qualcomm.com> > Cc: Michal Kazior <michal.kazior@tieto.com> > Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com> Patch applied to ath-next branch of ath.git, thanks. 96378bd2c6cd ath10k: fix core PCI suspend when WoWLAN is supported but disabled
Kalle Valo <kvalo@qca.qualcomm.com> writes: > Brian Norris <briannorris@chromium.org> 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 <briannorris@chromium.org> >> Cc: Ryan Hsu <ryanhsu@qti.qualcomm.com> >> Cc: Kalle Valo <kvalo@qca.qualcomm.com> >> Cc: Michal Kazior <michal.kazior@tieto.com> >> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com> > > 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?
+ Arnd On Thu, Oct 19, 2017 at 02:32:45PM +0000, Kalle Valo wrote: > Kalle Valo <kvalo@qca.qualcomm.com> writes: > > > Brian Norris <briannorris@chromium.org> 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 <briannorris@chromium.org> > >> Cc: Ryan Hsu <ryanhsu@qti.qualcomm.com> > >> Cc: Kalle Valo <kvalo@qca.qualcomm.com> > >> Cc: Michal Kazior <michal.kazior@tieto.com> > >> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com> > > > > 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 <briannorris@chromium.org> writes: > + Arnd > > On Thu, Oct 19, 2017 at 02:32:45PM +0000, Kalle Valo wrote: >> Kalle Valo <kvalo@qca.qualcomm.com> writes: >> >> > Brian Norris <briannorris@chromium.org> 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 <briannorris@chromium.org> >> >> Cc: Ryan Hsu <ryanhsu@qti.qualcomm.com> >> >> Cc: Kalle Valo <kvalo@qca.qualcomm.com> >> >> Cc: Michal Kazior <michal.kazior@tieto.com> >> >> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com> >> > >> > 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' 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=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. I see that you already sent the patch, thanks!
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);
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 <briannorris@chromium.org> Cc: Ryan Hsu <ryanhsu@qti.qualcomm.com> Cc: Kalle Valo <kvalo@qca.qualcomm.com> Cc: Michal Kazior <michal.kazior@tieto.com> --- drivers/net/wireless/ath/ath10k/pci.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)