Re: [PATCH v2 6/7] ath12k: 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] ath12k: 5-second CPU stall during surprise removal due to mdelay()
--

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

ath12k: add PCIe link recovery retry

This commit adds a retry loop to ath12k_pci_soc_global_reset() that waits up
to 5000ms for the PCIe link to recover after a global reset. This addresses
MHI initialization failures caused by virtualization layer timing variations
in PCIe passthrough environments.

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

> diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
> --- a/drivers/net/wireless/ath/ath12k/pci.c
> +++ b/drivers/net/wireless/ath/ath12k/pci.c
[ ... ]
> @@ -206,14 +208,28 @@ static void ath12k_pci_soc_global_reset(struct ath12k_base *ab)
>  
>  	mdelay(delay);
>  
> -	val = ath12k_pci_read32(ab, PCIE_SOC_GLOBAL_RESET);
> -	if (val == 0xffffffff)
> -		ath12k_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 = ath12k_pci_read32(ab, PCIE_SOC_GLOBAL_RESET);
> +		if (val != 0xffffffff) {
> +			link_recovered = true;
> +			break;
> +		}
> +		mdelay(20);
> +	}

[Severity: High]
Will this loop cause a 5-second CPU stall during surprise removal?

During device teardown or module unload (such as ath12k_pci_remove() ->
ath12k_core_hw_group_cleanup() -> ath12k_core_hw_group_stop() ->
ath12k_core_soc_destroy() -> ath12k_hif_power_down() ->
ath12k_pci_power_down()), if the device has been physically removed, the
memory-mapped read will continuously return 0xffffffff.

This forces the loop to exhaust all 250 retries, resulting in a 5000ms
busy-wait (mdelay) in process context, which severely disrupts system
responsiveness and can trigger soft lockup watchdogs.

The commit notes mention this was changed to use an iteration counter
for ath12k_pci_panic_handler(), but it doesn't appear that such a
handler currently exists in the driver. Even if one were added later,
stalling the CPU for 5 seconds on all teardowns and suspends might be
unacceptable.

Could this code use msleep() instead, or conditionally check
in_atomic() / is_panic before busy-waiting?

> +	if (!link_recovered) {
> +		ath12k_err(ab, "PCIe link failed to recover - device may need power cycle\n");
> +		return -ETIMEDOUT;
> +	}

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