Re: [PATCH ath-next] wifi: ath10k: avoid unnecessary wait for service ready message

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Baochen,


Thank you for your patch, and sorry for the late reply.


Am 11.08.25 um 11:26 schrieb Baochen Qiang:
> Commit e57b7d62a1b2 ("wifi: ath10k: poll service ready message before
> failing") works around the failure in waiting for the service ready
> message by active polling. Note the polling is triggered after initial
> wait timeout, which means that the wait-till-timeout can not be avoided
> even the message is ready.
> 
> A possible fix is to do polling once before wait as well, however this
> can not handle the race that the message arrives right after polling.
> So the solution is to do periodic polling until timeout.
> 
> Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00309-QCARMSWPZ-1
> 
> Fixes: e57b7d62a1b2 ("wifi: ath10k: poll service ready message before failing")
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Closes: https://lore.kernel.org/all/97a15967-5518-4731-a8ff-d43ff7f437b0@molgen.mpg.de
> Signed-off-by: Baochen Qiang <baochen.qiang@oss.qualcomm.com>
> ---
>   drivers/net/wireless/ath/ath10k/wmi.c | 39 +++++++++++++++++------------------
>   1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index cb8ae751eb312109f74985580065c3b9d3806d51..e595b0979a56d3110ce0acf534e718a4a1f36a0b 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -1764,33 +1764,32 @@ void ath10k_wmi_put_wmi_channel(struct ath10k *ar, struct wmi_channel *ch,
>   
>   int ath10k_wmi_wait_for_service_ready(struct ath10k *ar)
>   {
> +	unsigned long timeout = jiffies + WMI_SERVICE_READY_TIMEOUT_HZ;
>   	unsigned long time_left, i;
>   
> -	time_left = wait_for_completion_timeout(&ar->wmi.service_ready,
> -						WMI_SERVICE_READY_TIMEOUT_HZ);
> -	if (!time_left) {
> -		/* Sometimes the PCI HIF doesn't receive interrupt
> -		 * for the service ready message even if the buffer
> -		 * was completed. PCIe sniffer shows that it's
> -		 * because the corresponding CE ring doesn't fires
> -		 * it. Workaround here by polling CE rings once.
> -		 */
> -		ath10k_warn(ar, "failed to receive service ready completion, polling..\n");
> -
> +	/* Sometimes the PCI HIF doesn't receive interrupt
> +	 * for the service ready message even if the buffer
> +	 * was completed. PCIe sniffer shows that it's
> +	 * because the corresponding CE ring doesn't fires
> +	 * it. Workaround here by polling CE rings. Since
> +	 * the message could arrive at any time, continue
> +	 * polling until timeout.

I would have also re-flowed the comment to make it take up less lines.

> +	 */
> +	do {
>   		for (i = 0; i < CE_COUNT; i++)
>   			ath10k_hif_send_complete_check(ar, i, 1);
>   
> +		/* The 100 ms granularity is a tradeoff considering scheduler
> +		 * overhead and response latency
> +		 */
>   		time_left = wait_for_completion_timeout(&ar->wmi.service_ready,
> -							WMI_SERVICE_READY_TIMEOUT_HZ);
> -		if (!time_left) {
> -			ath10k_warn(ar, "polling timed out\n");
> -			return -ETIMEDOUT;
> -		}
> -
> -		ath10k_warn(ar, "service ready completion received, continuing normally\n");
> -	}
> +							msecs_to_jiffies(100));
> +		if (time_left)
> +			return 0;
> +	} while (time_before(jiffies, timeout));
>   
> -	return 0;
> +	ath10k_warn(ar, "failed to receive service ready completion\n");
> +	return -ETIMEDOUT;
>   }
>   
>   int ath10k_wmi_wait_for_unified_ready(struct ath10k *ar)

Great to have this improved upstream!

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul