linux-kernel - Re: [PATCH] ath10k: fix core PCI suspend when WoWLAN is supported but disabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ-Vmo=HF6g9cOsxyn_xkRvpqd0PVgXWESvASi51ccUwNA3hxg@mail.gmail.com>
Date:   Wed, 11 Oct 2017 17:43:40 -0700
From:   Adrian Chadd <adrian@...ebsd.org>
To:     Brian Norris <briannorris@...omium.org>
Cc:     Kalle Valo <kvalo@....qualcomm.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        "ath10k@...ts.infradead.org" <ath10k@...ts.infradead.org>,
        "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        Grant Grundler <grundler@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ryan Hsu <ryanhsu@....qualcomm.com>,
        Michal Kazior <michal.kazior@...to.com>
Subject: Re: [PATCH] ath10k: fix core PCI suspend when WoWLAN is supported but disabled

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@...omium.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@...omium.org>
>> Cc: Ryan Hsu <ryanhsu@....qualcomm.com>
>> Cc: Kalle Valo <kvalo@....qualcomm.com>
>> Cc: Michal Kazior <michal.kazior@...to.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
>>