Re: [PATCH 7/8] remoteproc: qcom: pas: Enforce coupled stop/crash for HPASS ADSP clusters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Canceling `pas->stop_work` before removing the member from `cluster->members` creates a TOCTOU race window that leads to a Use-After-Free.
- [High] Coordinated stop state machine permanently deadlocks if a participant has multiple power references due to the use of `rproc_shutdown()`.
- [High] Concurrent crashes bypass the coordination barrier and prematurely power off hardware, violating the cluster constraint.
- [High] Sibling auto-restart times out prematurely when the cluster root crashes, leaving siblings permanently offline.
- [High] Lockless iteration of `cluster->members` in `qcom_pas_cluster_cascade_work()` races with `qcom_pas_cluster_init()`, causing list corruption.
- [Medium] Concurrent crash of a cluster member inflates its `rproc->power` refcount during auto-restart, preventing it from being successfully stopped by the user.
--

--- Patch [7]: [PATCH 7/8] remoteproc: qcom: pas: Enforce coupled stop/crash for HPASS ADSP clusters ---
commit 6bd05de74159725dcc440502485f8bb8e3c952cb
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

remoteproc: qcom: pas: Enforce coupled stop/crash for HPASS ADSP clusters

This commit enforces a coupled stop and crash mechanism for Nord HPASS ADSP
clusters. It orchestrates a two-phase stop and hardware power-off sequence
across cluster members to prevent sysmon timeouts and recovery failures. It
also implements an automatic root-first restart sequence upon a crash.

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index ef38a1929f56b..ac3b92c7e7e79 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c

[ ... ]

> +static void qcom_pas_stop_work_fn(struct work_struct *work)
> +{
> +	struct qcom_pas *pas = container_of(work, struct qcom_pas, stop_work);
> +
> +	rproc_shutdown(pas->rproc);

[Severity: High]
If a sibling has an rproc->power refcount greater than 1 (e.g., booted by
multiple clients), rproc_shutdown() will simply decrement the refcount and
return without invoking qcom_pas_stop().

Since qcom_pas_cluster_stop_complete() would never be called for that
sibling, cluster->stop_done_pending will never reach 0.

Could this leave cluster->stop_in_progress permanently set to true and
deadlock the coordinated stop state machine for all future operations?

> +}
> +
> +/**
> + * qcom_pas_cluster_cascade_work() - restart a crashed cluster, root first
> + * @work:	the cluster's cascade_work

[ ... ]

> +static void qcom_pas_cluster_cascade_work(struct work_struct *work)
> +{
> +	struct qcom_pas_cluster *cluster = container_of(work, struct qcom_pas_cluster,
> +						       cascade_work);
> +	struct qcom_pas *pas, *origin, *root;
> +
> +	mutex_lock(&cluster->lock);
> +	origin = cluster->cascade_origin;
> +	root = cluster->root;
> +	mutex_unlock(&cluster->lock);
> +
> +	/*
> +	 * Membership is stable here: qcom_pas_cluster_exit() always
> +	 * cancel_work_sync()s this work before touching cluster->members, so no

[ ... ]

> +	if (root && root != origin) {
> +		int ret;
> +
> +		ret = rproc_boot(root->rproc);
> +		if (ret) {
> +			dev_err(root->dev, "failed to restart cluster root: %d\n", ret);
> +			return;
> +		}
> +	}
> +
> +	list_for_each_entry(pas, &cluster->members, cluster_node) {

[Severity: High]
Is it safe to iterate cluster->members locklessly here?

If a cluster crash recovery executes concurrently with the probe of a new
cluster member, qcom_pas_cluster_init() could call list_add_tail() while
holding cluster->lock.

Could traversing this list without holding cluster->lock result in list
corruption or infinite loops?

> +		int ret;
> +
> +		if (pas == origin || pas == root)
> +			continue;
> +
> +		ret = rproc_boot(pas->rproc);

[Severity: High]
If the cluster root crashes, its recovery thread will trigger a coordinated
stop and schedule this cascade_work. Siblings are immediately booted here,
which eventually calls qcom_pas_cluster_wait_for_root(), enforcing a 1-second
timeout waiting for the root to boot.

However, the root's recovery thread must still generate a coredump and load
firmware, which typically takes significantly longer than 1 second.

Will this cause the auto-restarting siblings to prematurely time out and
fail to boot?

[Severity: Medium]
When auto-restarting siblings, this loops through the entire cluster without
checking if the members actually participated in the coordinated stop.

If a sibling crashed concurrently and was excluded from the stop (its
stop_work didn't execute and its refcount wasn't decremented), unconditionally
calling rproc_boot() on it here will artificially inflate its rproc->power
refcount.

Could this permanent refcount leak prevent the remote processor from being
successfully shut down by the user later?

> +		if (ret)
> +			dev_err(pas->dev, "failed to restart cluster sibling: %d\n", ret);
> +	}
> +}

[ ... ]

> @@ -273,6 +364,9 @@ static void qcom_pas_cluster_exit(struct qcom_pas *pas)
>  	if (!cluster)
>  		return;
>  
> +	cancel_work_sync(&pas->stop_work);
> +	cancel_work_sync(&cluster->cascade_work);
> +
>  	mutex_lock(&cluster->lock);
>  	list_del(&pas->cluster_node);

[Severity: High]
Does qcom_pas_cluster_exit() have a race condition that could lead to a
use-after-free?

If a member is being removed, cancel_work_sync() is called while the member
is still in the cluster->members list. If a sibling concurrently triggers
a cluster stop, it can find this member in the list and schedule
pas->stop_work via qcom_pas_cluster_trigger_stop():

    list_for_each_entry(member, &cluster->members, cluster_node) {
        if (member == pas || !member->in_cluster_stop)
            continue;
        schedule_work(&member->stop_work);
    }

Could this result in the work executing on a freed pas structure after
device removal completes?

[ ... ]

> @@ -625,10 +866,18 @@ static int qcom_pas_stop(struct rproc *rproc)
>  		mutex_unlock(&pas->cluster->lock);
>  	}
>  
> +	if (pas->cluster)
> +		qcom_pas_cluster_trigger_stop(pas, rproc->state == RPROC_CRASHED);
> +
> +	/* Phase 1: request and await this member's own graceful ack */
>  	ret = qcom_q6v5_request_stop(&pas->q6v5, pas->sysmon);
>  	if (ret == -ETIMEDOUT)
>  		dev_err(pas->dev, "timed out on wait\n");
>  
> +	if (pas->cluster)
> +		qcom_pas_cluster_stop_barrier(pas);
> +
> +	/* Phase 2: the whole cluster has acked, power the hardware off */
>  	ret = qcom_pas_shutdown(pas->pas_id);

[Severity: High]
If a sibling crashes concurrently with the original stopping member,
qcom_pas_cluster_member_stops() returns false for it, excluding it from
the coordinated stop.

When that sibling's own recovery thread later calls qcom_pas_stop(), it sees
stop_in_progress is true but its in_cluster_stop is false. This causes it
to bypass qcom_pas_cluster_stop_barrier() and immediately call
qcom_pas_shutdown().

Doesn't this power off its hardware instantly while other participants are
still executing Phase 1, violating the constraint that no member shuts down
before the whole cluster has acked?

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