Re: [PATCH 4/7] ath11k: add PCIe link recovery retry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ath11k: 5-second busy-wait loop using mdelay() causes CPU stalls and soft lockups
- [Medium] ath11k: polling loop misses final condition check on timeout causing false failures
--

commit c8d0b71714f3dd812345dc63d33d90b3fe00f198
Author: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
Subject: ath11k: add PCIe link recovery retry

This commit adds a retry loop (up to 5000ms) to wait for the PCIe link
to recover after a global reset. This aims to fix initialization failures
in virtualized environments where link stabilization may take longer.

> diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
> index a163168f36176..c372676b097f5 100644
> --- a/drivers/net/wireless/ath/ath11k/pci.c
> +++ b/drivers/net/wireless/ath/ath11k/pci.c

[ ... ]

> @@ -215,14 +217,29 @@ static void ath11k_pci_soc_global_reset(struct ath11k_base *ab)
>  
>  	mdelay(delay);
>  
> -	val = ath11k_pcic_read32(ab, PCIE_SOC_GLOBAL_RESET);
> -	if (val == 0xffffffff)
> -		ath11k_warn(ab, "link down error during global reset\n");
> +	/* Wait for PCIe link to recover, especially important in VM environments
> +	 * where timing can vary significantly from bare metal.
> +	 */
> +	timeout = jiffies + msecs_to_jiffies(5000);
> +	while (time_before(jiffies, timeout)) {

[Severity: Medium]
Will this polling loop incorrectly return -ETIMEDOUT if the thread is
preempted?

If the thread is preempted during the delay, jiffies could advance past
timeout before the next iteration. When the thread resumes, time_before()
will evaluate to false immediately, and the loop will exit without a final
register check despite the hardware having potentially recovered.

Should the hardware state be evaluated one final time after exiting the
time bounds check to avoid false timeouts?

> +		val = ath11k_pcic_read32(ab, PCIE_SOC_GLOBAL_RESET);
> +		if (val != 0xffffffff) {
> +			link_recovered = true;
> +			break;
> +		}
> +		mdelay(20);

[Severity: High]
Does this mdelay() in a loop of up to 5 seconds cause soft lockups?

During device unbind or surprise removal, the following call chain is
executed:

ath11k_pci_remove()
  ath11k_core_deinit()
    ath11k_hif_power_down()
      ath11k_pci_power_down()
        ath11k_pci_sw_reset()
          ath11k_pci_soc_global_reset()

In these cases, the PCIe link is permanently dead and will never recover,
resulting in an unconditional 5-second CPU stall using a busy-wait.

Since ath11k_pci_soc_global_reset() is called from sleepable process and
workqueue contexts, could msleep() or read_poll_timeout() be used instead
of mdelay() to avoid blocking the CPU?

> +	}
> +	if (!link_recovered) {
> +		ath11k_err(ab, "PCIe link failed to recover - device may need power cycle\n");
> +		return -ETIMEDOUT;
> +	}
>  
>  	/* Restore window register as its content is cleared during
>  	 * hardware global reset, such that it aligns with host cache.
>  	 */
>  	ath11k_pci_restore_window(ab);
> +
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930140833.576941-1-jtornosm@redhat.com?part=4