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

[Kalle Valo]

Brian Norris <briannorris@xxxxxxxxxxxx> writes:

> + Arnd
>
> On Thu, Oct 19, 2017 at 02:32:45PM +0000, Kalle Valo wrote:
>> Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> writes:
>> 
>> > Brian Norris <briannorris@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> >> Cc: Ryan Hsu <ryanhsu@xxxxxxxxxxxxxxxx>
>> >> Cc: Kalle Valo <kvalo@xxxxxxxxxxxxxxxx>
>> >> Cc: Michal Kazior <michal.kazior@xxxxxxxxx>
>> >> Signed-off-by: Kalle Valo <kvalo@xxxxxxxxxxxxxxxx>
>> >
>> > 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!

-- 
Kalle Valo