On 6/17/2024 9:43 PM, Kalle Valo wrote:
> Baochen Qiang <quic_bqiang@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>> Acked-by: Jeff Johnson <quic_jjohnson@xxxxxxxxxxx>
>
> [...]
>
>> +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.
>
