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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ath11k: 5000ms mdelay() causes severe CPU stall in normal contexts
--

commit 457eae9bbd9159c5c25a5626326a881f1232316d
Author: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>

ath11k: add PCIe link recovery retry

When ath11k devices are passed through to VMs via PCIe passthrough, the
PCIe link may not stabilize immediately after global reset. This patch
adds a retry loop to wait for the PCIe link to recover after global
reset and prevents continuing with a dead link.

Link: https://lore.kernel.org/all/20260930140833.576941-5-jtornosm@redhat.com/

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

[ ... ]

> @@ -215,14 +217,28 @@ 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.
> +	 */
> +	while (retries-- > 0) {
> +		val = ath11k_pcic_read32(ab, PCIE_SOC_GLOBAL_RESET);
> +		if (val != 0xffffffff) {
> +			link_recovered = true;
> +			break;
> +		}
> +		mdelay(20);
> +	}

[Severity: High]
Does this mdelay() loop cause a severe CPU stall?

The commit notes indicate this loop was changed to an iteration counter to
work in atomic/panic contexts. However, looking at the callers of
ath11k_pci_soc_global_reset() (such as probe, core_reset, core_restart,
and resume_early), these execute in contexts where it seems safe to sleep.
Even in resume_early, interrupts are enabled and jiffies advance normally.

Furthermore, immediately after this loop, the code calls
ath11k_pci_restore_window(), which unconditionally takes a spinlock with
spin_lock_bh():

ath11k_pci_restore_window(struct ath11k_base *ab) {
    ...
    spin_lock_bh(&ab_pci->window_lock);
    ...
}

Since taking a bottom-half disabling spinlock would be invalid in a hardirq
or panic context, doesn't this mean the code runs in normal process context?
If so, spinning the CPU without yielding for up to 5000ms (250 * 20ms) might
degrade responsiveness and risk triggering the soft lockup watchdog.

Could msleep() or fsleep() be used here instead?

> +	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;
>  }
>  
>  static void ath11k_pci_clear_dbg_registers(struct ath11k_base *ab)

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