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 <[email protected]>
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/[email protected]?part=7
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.