Re: [PATCH ath-next 08/16] wifi: ath11k: Power up userPD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ath11k: hardware power and SMEM state leak on error paths in ath11k_ahb_boot_user_pd
- [High] ath11k: missing reinit_completion() in ath11k_ahb_boot_user_pd
- [High] ath11k: NULL pointer dereference in userPD IRQ handler due to late completion init
--

commit 6b777a3c2abf7cc48a88c29957b57c461963887f
Author: George Moussalem <george.moussalem@outlook.com>

wifi: ath11k: Power up userPD
    
Set the spawn bit to instruct Q6 to spawn the userPD thread.
Wait for userPD to spawn which is indicated by spawn interrupt. The
ready interrupt is triggered once the userPD is powered up completely.

> diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c
> index 25f9a992a7d2f..2c41cf585d001 100644
> --- a/drivers/net/wireless/ath/ath11k/ahb.c
> +++ b/drivers/net/wireless/ath/ath11k/ahb.c
> @@ -430,8 +431,63 @@ static void ath11k_ahb_stop(struct ath11k_base *ab)
>  	ath11k_ce_cleanup_pipes(ab);
>  }
>  
> +static int ath11k_ahb_boot_user_pd(struct ath11k_base *ab)
> +{
> +	struct ath11k_ahb *ab_ahb = ath11k_ahb_priv(ab);
> +	unsigned long time_left;
> +	int ret;
> +
> +	if (ab->hw_rev == ATH11K_HW_IPQ5018_HW10) {
> +		ret = qcom_scm_pas_set_wifi_power_mode(MPD_WCNSS_PAS_ID, true);
> +		if (ret) {
> +			ath11k_err(ab, "failed to power up wifi: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	ret = qcom_smem_state_update_bits(ab_ahb->spawn_state, BIT(ab_ahb->spawn_bit),
> +					  BIT(ab_ahb->spawn_bit));
> +	if (ret) {
> +		ath11k_err(ab, "Failed to update spawn state %d\n", ret);
> +		return ret;
> +	}
> +
> +	time_left = wait_for_completion_timeout(&ab_ahb->userpd_spawned,
> +						ATH11K_USERPD_SPAWN_TIMEOUT);

[Severity: High]
Does this wait rely on a completion state that might carry over between driver
recoveries?

If a previous cycle timed out but the interrupt fired later, the completion's
done count might be > 0. Should reinit_completion() be called before setting the
SMEM state to prevent false positives?

> +	if (!time_left) {
> +		ath11k_err(ab, "UserPD spawn wait timed out\n");
> +		return -ETIMEDOUT;
> +	}

[Severity: High]
If this times out, should the SMEM spawn bit and Wi-Fi power mode be reverted?

Returning -ETIMEDOUT here without clearing ab_ahb->spawn_bit or calling
qcom_scm_pas_set_wifi_power_mode(MPD_WCNSS_PAS_ID, false) leaves the device
powered on and prevents subsequent recovery attempts from triggering the
edge-based spawn interrupt.

> +
> +	time_left = wait_for_completion_timeout(&ab_ahb->userpd_ready,
> +						ATH11K_USERPD_READY_TIMEOUT);
> +	if (!time_left) {
> +		ath11k_err(ab, "UserPD ready wait timed out\n");
> +		return -ETIMEDOUT;
> +	}

[Severity: High]
Similarly, if waiting for userpd_ready times out, should the SMEM spawn bit and
Wi-Fi power mode be cleaned up before returning -ETIMEDOUT?

> +
> +	qcom_smem_state_update_bits(ab_ahb->spawn_state, BIT(ab_ahb->spawn_bit), 0);
> +
> +	ath11k_dbg(ab, ATH11K_DBG_AHB, "UserPD%d is now UP\n", ab_ahb->userpd_id);
> +
> +	return 0;
> +}

[Severity: High]
This isn't a bug introduced by this patch, but does the following pre-existing
code risk a NULL pointer dereference?

drivers/net/wireless/ath/ath11k/ahb.c:ath11k_ahb_config_userpd_irq() {
	...
	for (i = 0; i < ATH11K_USERPD_MAX_IRQ; i++) {
		...
		ret = devm_request_threaded_irq(&ab->pdev->dev, ab_ahb->userpd_irq_num[i],
						NULL, ath11k_userpd_irq_handler,
						IRQF_TRIGGER_RISING | IRQF_ONESHOT,
						upd_irq_name, ab);
		...
	}

	init_completion(&ab_ahb->userpd_spawned);
	init_completion(&ab_ahb->userpd_ready);
	...
}

If the interrupt fires immediately after registration and before
init_completion() is called, ath11k_userpd_irq_handler() would call complete()
on an uninitialized completion. Since ath11k_ahb is zero-allocated, the
completion's waitqueue list head is NULL, which will crash when complete() calls
list_empty().

Should init_completion() be called before registering the IRQ handlers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-ipq5018-qcn6122-mpd-v1-0-86729521a75c@outlook.com?part=8