From mboxrd@z Thu Jan  1 00:00:00 1970
From: Kalle Valo <kvalo@qca.qualcomm.com>
Date: Fri, 5 Jul 2013 09:51:17 +0300
Subject: [ath9k-devel] [PATCH v3 5/9] ath10k: decouple suspend code
In-Reply-To: <1372147152-26799-6-git-send-email-michal.kazior@tieto.com>
	(Michal Kazior's message of "Tue, 25 Jun 2013 09:59:08 +0200")
References: <1371041642-20273-1-git-send-email-michal.kazior@tieto.com>
	<1372147152-26799-1-git-send-email-michal.kazior@tieto.com>
	<1372147152-26799-6-git-send-email-michal.kazior@tieto.com>
Message-ID: <87vc4ph53u.fsf@kamboji.qca.qualcomm.com>
List-Id: <ath9k-devel.lists.ath9k.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: ath9k-devel@lists.ath9k.org

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

> Split up fw-related and hw-related suspension code.
>
> Although we don't advertise WoW support to
> mac80211 yet it's useful to keep the code in
> suspend/resume hooks.
>
> At this point there's no need to keep pci pm ops.
> In case of WoW mac80211 calls ath10k_suspend()
> which should take care of entering low-power mode.
> In case WoW is not available mac80211 will go
> through regular interface teradown and use start/stop.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> +#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 :)

-- 
Kalle Valo