Re: [PATCH v4 2/8] wifi: ath12k: implement WoW enable and wakeup commands

[Kalle Valo <kvalo@kernel.org>]

Baochen Qiang <quic_bqiang@quicinc.com> writes:

>>> +int ath12k_wow_enable(struct ath12k *ar)
>>> +{
>>> +	struct ath12k_base *ab = ar->ab;
>>> +	int i, ret;
>>> +
>>> +	clear_bit(ATH12K_FLAG_HTC_SUSPEND_COMPLETE, &ab->dev_flags);
>>> +
>>> +	for (i = 0; i < ATH12K_WOW_RETRY_NUM; i++) {
>>> +		reinit_completion(&ab->htc_suspend);
>>> +
>>> +		ret = ath12k_wmi_wow_enable(ar);
>>> +		if (ret) {
>>> +			ath12k_warn(ab, "failed to issue wow enable: %d\n", ret);
>>> +			return ret;
>>> +		}
>>> +
>>> +		ret = wait_for_completion_timeout(&ab->htc_suspend, 3 * HZ);
>>> +		if (ret == 0) {
>>> +			ath12k_warn(ab,
>>> + "timed out while waiting for htc suspend completion\n");
>>> +			return -ETIMEDOUT;
>>> +		}
>>> +
>>> +		if (test_bit(ATH12K_FLAG_HTC_SUSPEND_COMPLETE, &ab->dev_flags))
>>> +			/* success, suspend complete received */
>>> +			return 0;
>>> +
>>> +		ath12k_warn(ab, "htc suspend not complete, retrying (try %d)\n",
>>> +			    i);
>>> +		msleep(ATH12K_WOW_RETRY_WAIT_MS);
>>> +	}
>>> +
>>> +	ath12k_warn(ab, "htc suspend not complete, failing after %d tries\n", i);
>>> +
>>> +	return -ETIMEDOUT;
>>> +}
>> 
>> Why the loop here? Looks really odd to me and no explanation why it's
>> needed. ATH12K_WOW_RETRY_NUM seems to be 10 so this can loop a lot.
>
> Host asks firmware to enter WoW mode using a WMI command. While
> receiving it, firmware might be busy so that can not enter WoW
> immediately. In that case firmware notifies host of
> ATH12K_HTC_MSG_NACK_SUSPEND message, asking host to try again later.
> Per firmware team there could be up to 10 loops.

Not really a good firmware interface, oh well. I added a comment to
ath12k_wow_enable() explaining, thanks.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches