Re: [PATCH] remoteproc: qcom_q6v5_pas: add HPASS ADSP cluster boot-order and SSR coupling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unprotected iteration over `cluster->members` in `qcom_pas_cluster_stop()` leads to list corruption and use-after-free.
- [High] Premature clearing of `restart_pending` by the root instance causes an infinite cascade of crashes during recovery.
--

--- Patch [1]: [PATCH] remoteproc: qcom_q6v5_pas: add HPASS ADSP cluster boot-order and SSR coupling ---
commit 644aec14bb718175878ea77cb26cb7732aee67b2
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

remoteproc: qcom_q6v5_pas: add HPASS ADSP cluster boot-order and SSR coupling

This commit introduces a boot-ordering and coupled-SSR (Subsystem Restart)
mechanism for Qualcomm SoCs with multiple PAS instances grouped into a cluster.
It uses a shared qcom_pas_cluster object to serialize boot so that the root
instance boots first, and ensures that a crash or stop on one member brings
down and restarts the entire cluster together.

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index ca8e61254c449..61e8dc64e2ad9 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
[ ... ]
> @@ -433,6 +433,18 @@ static void qcom_pas_cluster_mark_booted(struct qcom_pas *pas)
>  {
>  	struct qcom_pas_cluster *cluster = pas->cluster;
>  
>  	if (cluster->root_node != pas->dev->of_node)
>  		return;
>  
>  	mutex_lock(&cluster->lock);
>  	cluster->booted = true;
>  	cluster->restart_pending = false;
>  	mutex_unlock(&cluster->lock);

[Severity: High]
If a member crashes, qcom_pas_cluster_stop() sets restart_pending = true and
initiates recovery on all members.

If the root member completes its recovery (rproc_stop, coredump, rproc_start)
and calls qcom_pas_cluster_mark_booted() before another member enters
qcom_pas_cluster_stop(), won't the root clear restart_pending prematurely?

When the slower member finally executes qcom_pas_cluster_stop(), it could
observe restart_pending == false, assume it's a new crash, set it to true
again, and trigger another round of crashes.

Could this regression lead to an infinite cascade of crashes?

>  
>  	wake_up_interruptible(&cluster->wq);
>  }
[ ... ]
> @@ -1036,6 +1036,36 @@ static void qcom_pas_cluster_stop(struct rproc_subdev *subdev, bool crashed)
>  {
>  	struct qcom_pas *pas = container_of(subdev, struct qcom_pas, cluster_subdev);
>  	struct qcom_pas_cluster *cluster = pas->cluster;
>  	struct qcom_pas *member;
>  
>  	mutex_lock(&cluster->lock);
>  	if (cluster->restart_pending) {
>  		mutex_unlock(&cluster->lock);
>  		return;
>  	}
>  	cluster->restart_pending = true;
>  	cluster->booted = false;
>  	mutex_unlock(&cluster->lock);
>  
>  	list_for_each_entry(member, &cluster->members, cluster_node) {

[Severity: High]
Can this iterate over cluster->members concurrently with list modifications?

Since the iteration occurs entirely outside of cluster->lock, it seems to open a
race window if a user unbinds a PAS driver while the cluster is stopping.

qcom_pas_remove() takes cluster->lock, removes the node, and frees the object:

drivers/remoteproc/qcom_q6v5_pas.c:qcom_pas_remove() {
    ...
    mutex_lock(&pas->cluster->lock);
    list_del(&pas->cluster_node);
    mutex_unlock(&pas->cluster->lock);
    ...
}

Would it be possible for this regression to cause the unprotected
list_for_each_entry() to hit a poisoned list pointer or call
rproc_report_crash() / schedule_work() on a freed member?

>  		if (member == pas)
>  			continue;
>  
>  		if (crashed)
>  			rproc_report_crash(member->rproc, RPROC_FATAL_ERROR);
>  		else
>  			schedule_work(&member->cluster_stop_work);
>  	}
>  }

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