Re: [PATCH 6/7] ath12k: add PCIe link recovery retry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ath12k: infinite loop in panic handler due to jiffies-based timeout
- [Medium] ath12k: 5-second busy-wait loop using mdelay() causes severe CPU stall
- [Medium] ath12k: missing post-loop check causes spurious timeouts on preemption
--

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

ath12k: add PCIe link recovery retry

This patch modifies ath12k_pci_soc_global_reset() to add a retry loop
waiting up to 5000ms for the PCIe link to recover after a global reset.
It returns an error if the link remains down, preventing continued MHI
initialization with a dead link.

> diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
> index af0e882fd0b51..65712486d3127 100644
> --- a/drivers/net/wireless/ath/ath12k/pci.c
> +++ b/drivers/net/wireless/ath/ath12k/pci.c

[ ... ]

> @@ -206,14 +208,29 @@ 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.
> +	 */
> +	timeout = jiffies + msecs_to_jiffies(5000);
> +	while (time_before(jiffies, timeout)) {

[Severity: High]
Will this create an infinite loop during a kernel panic?

The panic handler ath12k_pci_panic_handler() executes via the panic notifier
chain:

ath12k_pci_panic_handler()
    ath12k_pci_sw_reset()
        ath12k_pci_soc_global_reset()

Since interrupts are disabled during a panic, jiffies does not advance. If
the PCIe link fails to recover, which is common during a system crash, does
this loop evaluate to true indefinitely and prevent kexec or reboot?

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

[Severity: Medium]
Does this busy-wait cause a severe CPU stall?

A 5000ms busy-wait monopolizes the CPU, causing a severe latency spike. In a
virtualized environment, which this patch targets, it can starve other vCPUs
or host tasks, and risks triggering RCU stall warnings.

Since mdelay() relies on a spinning loop and actively consumes 100% of the
CPU instead of yielding, should msleep() or context-aware wait functions be
used for delays this long outside of atomic contexts?

> +	}
> +	if (!link_recovered) {

[Severity: Medium]
Can this cause a spurious timeout if the thread is preempted?

If the thread is preempted or descheduled during or just after the mdelay()
call, which is common in VM environments due to vCPU scheduling, jiffies may
advance past the timeout.

When the loop terminates due to time_before() evaluating to false, it returns
-ETIMEDOUT without verifying the register one last time. Does this ignore the
possibility that the hardware condition was satisfied during the preemption
window?

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

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