Re: [PATCH v8 12/14] wifi: ath12k: Switch to generic PAS TZ APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `qcom_pas_shutdown()` on error and timeout paths in `ath12k_ahb_power_up()` and `ath12k_ahb_power_down()`.
- [High] Missing `reinit_completion()` for UserPD completions before reuse.
--

commit 64d57b3ec944d7af7a51a4d0a20ee4f202c079f4
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>

wifi: ath12k: Switch to generic PAS TZ APIs

Switch ath12k client driver over to generic PAS TZ APIs. Generic PAS TZ
service allows to support multiple TZ implementation backends like QTEE
based SCM PAS service, OP-TEE based PAS service and any further future TZ
backend service.

> diff --git a/drivers/net/wireless/ath/ath12k/ahb.c b/drivers/net/wireless/ath/ath12k/ahb.c
> index 30733a244454e..d9717ea999d72 100644
> --- a/drivers/net/wireless/ath/ath12k/ahb.c
> +++ b/drivers/net/wireless/ath/ath12k/ahb.c
> @@ -420,7 +420,7 @@ static int ath12k_ahb_power_up(struct ath12k_base *ab)
>  
>  	if (ab_ahb->scm_auth_enabled) {
>  		/* Authenticate FW image using peripheral ID */
> -		ret = qcom_scm_pas_auth_and_reset(pasid);
> +		ret = qcom_pas_auth_and_reset(pasid);
>  		if (ret) {
>  			ath12k_err(ab, "failed to boot the remote processor %d\n", ret);
>  			goto err_fw2;
>  		}
>  	}
> 
>  	/* Instruct Q6 to spawn userPD thread */
>  	ret = qcom_smem_state_update_bits(ab_ahb->spawn_state, BIT(ab_ahb->spawn_bit),
>  					  BIT(ab_ahb->spawn_bit));
>  	if (ret) {
>  		ath12k_err(ab, "Failed to update spawn state %d\n", ret);
>  		goto err_fw2;
>  	}
> 
>  	time_left = wait_for_completion_timeout(&ab_ahb->userpd_spawned,
>  						ATH12K_USERPD_SPAWN_TIMEOUT);

[Severity: High]
This is a pre-existing issue, but does this wait_for_completion_timeout()
need a matching reinit_completion() before it?

If a previous wait timed out but the interrupt fired late, the completion
might contain a stale done count. Would the next attempt to power up
incorrectly return immediately, allowing the driver to proceed before the
firmware is actually ready?

>  	if (!time_left) {
>  		ath12k_err(ab, "UserPD spawn wait timed out\n");
>  		ret = -ETIMEDOUT;
>  		goto err_fw2;

[Severity: High]
This is also a pre-existing issue, but if the wait times out, this path
jumps to err_fw2.

Does jumping to err_fw2 without calling qcom_pas_shutdown() leak the PAS
resources? If the TrustZone state machine is left in an active state, will
it permanently reject subsequent authentication attempts until a reboot?

[ ... ]

> @@ -485,10 +485,10 @@ static void ath12k_ahb_power_down(struct ath12k_base *ab, bool is_suspend)
>  	unsigned long time_left;
>  	u32 pasid;
>  	int ret;
> 
>  	qcom_smem_state_update_bits(ab_ahb->stop_state, BIT(ab_ahb->stop_bit),
>  				    BIT(ab_ahb->stop_bit));
> 
>  	time_left = wait_for_completion_timeout(&ab_ahb->userpd_stopped,
>  						ATH12K_USERPD_STOP_TIMEOUT);
>  	if (!time_left) {
>  		ath12k_err(ab, "UserPD stop wait timed out\n");
>  		return;

[Severity: High]
This is a pre-existing issue, but if the stop wait times out, the function
returns early without clearing the stop smem state bit or calling
qcom_pas_shutdown().

Can this cause the PAS memory and remote processor resources to leak and
break the TrustZone state machine on subsequent power ups?

>  	}
> 
>  	qcom_smem_state_update_bits(ab_ahb->stop_state, BIT(ab_ahb->stop_bit), 0);
> 
>  	if (ab_ahb->scm_auth_enabled) {
>  		pasid = (u32_encode_bits(ab_ahb->userpd_id, ATH12K_USERPD_ID_MASK)) |
>  			 ATH12K_AHB_UPD_SWID;
>  		/* Release the firmware */
> -		ret = qcom_scm_pas_shutdown(pasid);
> +		ret = qcom_pas_shutdown(pasid);
>  		if (ret)
> -			ath12k_err(ab, "scm pas shutdown failed for userPD%d\n",
> -				   ab_ahb->userpd_id);
> +			ath12k_err(ab, "pas shutdown failed for userPD%d: %d\n",
> +				   ab_ahb->userpd_id, ret);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626133440.692849-1-sumit.garg@kernel.org?part=12