Re: [PATCH 6/8] remoteproc: qcom: pas: Sequence HPASS ADSP cluster boot root-first

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Accessing `root->state` outside `cluster->lock` without holding an `rproc` device reference causes a Use-After-Free if the root device is unbound concurrently.
- [High] Missing state check after `wait_for_completion_timeout()` completely breaks boot sequencing, allowing dependent members to cold boot before the root.
- [Medium] Concurrent execution of `reinit_completion()` and `wait_for_completion_timeout()` causes a data race on `cluster->root_booted`.
--

--- Patch [6]: [PATCH 6/8] remoteproc: qcom: pas: Sequence HPASS ADSP cluster boot root-first ---
commit 7abf00410ddb06041e837e4d053d61bb3ef43922
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

remoteproc: qcom: pas: Sequence HPASS ADSP cluster boot root-first

This commit gates dependent member boots on the cluster root having started.
It introduces a wait mechanism using a completion to handle racing dependent
member restarts with the root's start and stop paths.

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index dc6e7d601a9bd..ef38a1929f56b 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
[ ... ]
> @@ -267,6 +283,51 @@ static void qcom_pas_cluster_exit(struct qcom_pas *pas)
>  	pas->cluster = NULL;
>  }
>  
> +/**
> + * qcom_pas_cluster_wait_for_root() - gate a dependent member's boot on its root
> + * @pas:	the non-root cluster member being started
[ ... ]
> +static int qcom_pas_cluster_wait_for_root(struct qcom_pas *pas)
> +{
> +	struct qcom_pas_cluster *cluster = pas->cluster;
> +	struct rproc *root;
> +
> +	/*
> +	 * The root's PAS instance is enabled in DT (checked in
> +	 * qcom_pas_cluster_init()) but has not necessarily bound yet, and may
> +	 * have unbound again. Without it there is nothing to sequence this
> +	 * member's boot against.
> +	 *
> +	 * Note that nothing refcounts members across unbind, so a root freed
> +	 * under a concurrent sibling boot remains unhandled.
> +	 */
> +	mutex_lock(&cluster->lock);
> +	root = cluster->root ? cluster->root->rproc : NULL;
> +	mutex_unlock(&cluster->lock);
> +
> +	if (!root) {
> +		dev_err(pas->dev, "cluster root not bound\n");
> +		return -ENODEV;
> +	}
> +
[Severity: High]
Is there a risk of a use-after-free here if the root device is unbound
concurrently? 

Since the cluster->lock is dropped above and no reference is taken on
the rproc device (e.g., via get_device(&root->dev)), could root be freed via
devres before we evaluate root->state?

> +	if (root->state == RPROC_RUNNING || root->state == RPROC_ATTACHED)
> +		return 0;
> +
[Severity: Medium]
Could this wait_for_completion_timeout() race with the reinit_completion()
called in qcom_pas_stop()? 

In qcom_pas_stop():

	if (pas->cluster && pas->is_cluster_root) {
		mutex_lock(&pas->cluster->lock);
		reinit_completion(&pas->cluster->root_booted);
		mutex_unlock(&pas->cluster->lock);
	}

Since the wait happens outside the cluster->lock but reinit_completion()
modifies the completion directly, does this result in a data race on
cluster->root_booted?

> +	if (!wait_for_completion_timeout(&cluster->root_booted,
> +					 QCOM_PAS_CLUSTER_ROOT_BOOT_TIMEOUT)) {
> +		dev_err(pas->dev, "cluster root not started\n");
> +		return -ENODEV;
> +	}
> +
[Severity: High]
The commit message mentions that the dependent member "falls back to failing
on the root's state" when it observes a stale completion. 

Since cluster->root_booted is initialized as signaled during probe via
complete_all(), wait_for_completion_timeout() will immediately return success
here on a cold boot.

Are we missing the fallback check to re-evaluate root->state before returning
0? Without it, dependent members might successfully bypass the wait and
cold boot before the root.

> +	return 0;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260815132541.1575121-1-shengchao.guo@oss.qualcomm.com?part=6