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:

> 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@quicinc.com>
> Acked-by: Jeff Johnson <quic_jjohnson@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.

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

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

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