[ath9k-devel] [PATCH v3 5/9] ath10k: decouple suspend code

[Kalle Valo <kvalo@qca.qualcomm.com>]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 5 July 2013 08:51, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>> +#ifdef CONFIG_PM
>>> +static int ath10k_suspend(struct ieee80211_hw *hw,
>>> +                       struct cfg80211_wowlan *wowlan)
>>> +{
>>> +     struct ath10k *ar = hw->priv;
>>> +     int ret;
>>> +
>>> +     ar->is_target_paused = false;
>>> +
>>> +     ret = ath10k_wmi_pdev_suspend_target(ar);
>>> +     if (ret) {
>>> +             ath10k_warn("could not suspend target (%d)\n", ret);
>>> +             return 1;
>>> +     }
>>> +
>>> +     ret = wait_event_interruptible_timeout(ar->event_queue,
>>> +                                            ar->is_target_paused == true,
>>> +                                            1 * HZ);
>>> +     if (ret < 0) {
>>> +             ath10k_warn("suspend interrupted (%d)\n", ret);
>>> +             goto resume;
>>> +     } else if (ret == 0) {
>>> +             ath10k_warn("suspend timed out - target pause event never came\n");
>>> +             goto resume;
>>> +     }
>>> +
>>> +     ret = ath10k_hif_suspend(ar);
>>> +     if (ret) {
>>> +             ath10k_warn("could not suspend hif (%d)\n", ret);
>>> +             goto resume;
>>> +     }
>>> +
>>> +     return 0;
>>> +resume:
>>> +     ret = ath10k_wmi_pdev_resume_target(ar);
>>> +     if (ret)
>>> +             ath10k_warn("could not resume target (%d)\n", ret);
>>> +     return 1;
>>> +}
>>
>> No need to change anything now, it's just moving code around anyway, but
>> from a quick look this function looks racy. Can we trust that there's no
>> other code path which could create problems during suspend? Or should we
>> properly serialise this, eg. by using conf_mutex and adding a new state?
>>
>> I hope I didn't ask this already before, I just came back from vacation :)
>
> Right. We could use a conf_mutex here. However this code is not used
> at all. Nothing should trigger it (it's just left for reference) since
> we don't advertise any WoWLAN capabilities to mac80211. I can update
> the patch and include conf_mutex in suspend/resume functions if you
> think that's necessary. Or perhaps we should just scrap the code. The
> code will still remain in git history.

No need to change anything now, the patch is fine as it is. I was just
thinking for the future.

-- 
Kalle Valo