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

[Baochen Qiang]

On 6/17/2024 9:43 PM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang at quicinc.com> writes:
> 
>> Implement WoW enable and WoW wakeup commands which are needed
>> for suspend/resume.
>>
>> Tested-on: WCN7850 hw2.0 PCI
>> WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4
>>
>> Signed-off-by: Baochen Qiang <quic_bqiang at quicinc.com>
>> Acked-by: Jeff Johnson <quic_jjohnson at quicinc.com>
> 
> [...]
> 
>> +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.

> 
> ath11k seems to have a similar loop and no comments there either. I
> clearly missed that in review.
>