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 <[email protected]> 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/[email protected]?part=12
