Re: [PATCH v10 16/20] coresight: Add PM callbacks for sink device

[Leo Yan <leo.yan@arm.com>]

On Thu, Apr 09, 2026 at 11:52:00AM +0100, James Clark wrote:

[...]

> > @@ -1787,15 +1808,32 @@ static int coresight_pm_save(struct coresight_path *path)
> >   	to = list_prev_entry(coresight_path_last_node(path), link);
> >   	coresight_disable_path_from_to(path, from, to);
> > +	ret = coresight_pm_device_save(coresight_get_sink(path));
> > +	if (ret)
> > +		goto sink_failed;
> > +
> 
> The comment directly above this says "Up to the node before sink to avoid
> latency". But then this line goes and saves the sink anyway. So I'm not sure
> what's meant by the comment?

I will refine the comment to: "Control the path up to the node before
the _system_ sink to avoid latency caused by memory copying; afterwards,
saves context if the sink provides a callback".


> >   	return 0;
> > +
> > +sink_failed:
> > +	if (!coresight_enable_path_from_to(path, coresight_get_mode(source),
> > +					   from, to))
> > +		coresight_pm_device_restore(source);
> > +
> > +	pr_err("Failed in coresight PM save on CPU%d: %d\n",
> > +	       smp_processor_id(), ret);
> > +	this_cpu_write(percpu_pm_failed, true);
> 
> Why does only a failing sink set percpu_pm_failed when failing to save the
> source exits early.

My purpose is to use "percpu_pm_failed == true" to prevent further
issues if we already know the link has failed.  To be clear, this is not
a recovery mechanism; it simply means "if link enabling or disabling
fails in CPU PM, do not continue to avoid potential hardware lockups.”

The source save failure is a bit special, it fails at the beginning of
the CoreSight CPU PM flow, and the returned error prevents the CPU idle
flow from proceeding.  As a result, there is nothing further to do.

> Sashiko has a similar comment that this could result in
> restoring uninitialised source save data later, but a comment in this
> function about why the flow is like this would be helpful.

The comment of "Restoring uninitialised source save data later" does not
make sense.

When the save fails, the notifier only rolls back the callbacks that ran
before the failing callback by invoking CPU_PM_ENTER_FAILED.  And there
is no CPU_PM_EXIT, as the idle state is never entered.

> We have coresight_disable_path_from_to() which always succeeds and doesn't
> return an error. TRBE is the only sink with a pm_save_disable()
> callback, but it always succeeds anyway.
> 
> Would it not be much simpler to require that sink save/restore callbacks
> always succeed and don't return anything? Seems like this percpu_pm_failed
> stuff is extra complexity for a scenario that doesn't exist? The only thing
> that can fail is saving the source but it doesn't goto sink_failed when that
> happens.

As you suggested, I can simplify the code with assuming sink ops
always success, but we might expect complaints from Sashiko.

> Ideally etm4_cpu_save() wouldn't have a return value either. It would be
> good if we could find away to skip or ignore the timeouts in there somehow
> because that's the only reason it can fail.

Seems to me, ETMv4 has a timeout log and returns error is not bad, as
it can be helpful to locate issue in a cheap way (sometimes, hang
issue caused by lockup might be a time-consumed debugging).

Thanks,
Leo