Re: [PATCH 3/8] remoteproc: qcom: sysmon: Suppress notify between cluster siblings

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch's fix for the QMI timeout is incomplete. The patch adds a cluster-based suppression mechanism in `sysmon_notify()` to prevent QMI timeouts caused by peer SSR notifications between remote processors in the same cluster. However, it fails to add the same suppression check in the state synchronization loop inside `sysmon_start()`. When a remote processor powers up, `sysmon_start()` iterates over all already running instances and directly calls `ssctl_send_event()` (or `sysmon_send_event()`). If a sibling in the same cluster is already running, the newly starting processor will explicitly receive a peer notification about its sibling. This will trigger the exact same unhandled QMI message and timeout the patch intended to fix, making the timeout persist depending on the boot order of cluster members.
--

--- Patch [3]: [PATCH 3/8] remoteproc: qcom: sysmon: Suppress notify between cluster siblings ---
commit 8023059af35265ed9a7ebd6a255bd4e8dfa00c5b
Author: Shawn Guo <[email protected]>

remoteproc: qcom: sysmon: Suppress notify between cluster siblings

Firmware on clustered remote processors (e.g. Nord's HPASS ADSP0/1/2)
cannot handle the SSCTL peer-shutdown notification sysmon sends
whenever a sibling in the same cluster starts or stops, and the QMI
request instead times out.

> diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
> index 61e1038328e83..5d4443ef3bc18 100644
> --- a/drivers/remoteproc/qcom_sysmon.c
> +++ b/drivers/remoteproc/qcom_sysmon.c

[ ... ]

> @@ -500,7 +502,8 @@ static int sysmon_start(struct rproc_subdev *subdev)
>  	struct qcom_sysmon *target;
>  	struct sysmon_event event = {
>  		.subsys_name = sysmon->name,
> -		.ssr_event = SSCTL_SSR_EVENT_AFTER_POWERUP
> +		.ssr_event = SSCTL_SSR_EVENT_AFTER_POWERUP,
> +		.cluster = sysmon->rproc->cluster,
>  	};
>  
>  	reinit_completion(&sysmon->ssctl_comp);

[Severity: High]
This is a pre-existing issue, but does the patch leave the QMI timeout
unresolved during power-up?

When the second processor in a cluster starts, sysmon_start() iterates over
all running instances in sysmon_list and directly sends them events:

drivers/remoteproc/qcom_sysmon.c:sysmon_start() {
    ...
	list_for_each_entry(target, &sysmon_list, node) {
		mutex_lock(&target->state_lock);
		if (target == sysmon || target->state != SSCTL_SSR_EVENT_AFTER_POWERUP) {
			mutex_unlock(&target->state_lock);
			continue;
		}

		event.subsys_name = target->name;
		event.ssr_event = target->state;

		if (sysmon->ssctl_version == 2)
			ssctl_send_event(sysmon, &event);
		else if (sysmon->ept)
			sysmon_send_event(sysmon, &event);
		mutex_unlock(&target->state_lock);
	}
    ...
}

Because this manual synchronization loop bypasses sysmon_notify() and
directly calls ssctl_send_event() or sysmon_send_event(), it misses the
new cluster sibling suppression logic.

If a sibling in the same cluster is already running, will the newly starting
processor explicitly receive a peer notification about its sibling and
trigger the exact same unhandled QMI message and timeout this patch intends
to fix?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3