On 09/04/2026 2:14 pm, Suzuki K Poulose wrote:
On 09/04/2026 11:52, James Clark wrote:
On 05/04/2026 4:02 pm, Leo Yan wrote:
Unlike system level sinks, per-CPU sinks may lose power during CPU idle states. Currently, this applies specifically to TRBE. This commit invokes save and restore callbacks for the sink in the CPU PM notifier.
If the sink provides PM callbacks but the source does not, this is unsafe because the sink cannot be disabled safely unless the source can also be controlled, so veto low power entry to avoid lockups.
Tested-by: James Clark james.clark@linaro.org Reviewed-by: Yeoreum Yun yeoreum.yun@arm.com Reviewed-by: James Clark james.clark@linaro.org Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-core.c | 46 +++++++++++++++++
- ++++++++--
1 file changed, 43 insertions(+), 3 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/ hwtracing/coresight/coresight-core.c index c1e8debc76aba7eb5ecf7efe2a3b9b8b3e11b10c..a918bf6398a932de30fe9b4947020cc4c1cfb2f7 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -1736,14 +1736,15 @@ static void coresight_release_device_list(void) /* Return: 1 if PM is required, 0 if skip, <0 on error */ static int coresight_pm_check(struct coresight_path *path) { - struct coresight_device *source; - bool source_has_cb; + struct coresight_device *source, *sink; + bool source_has_cb, sink_has_cb; if (!path) return 0; source = coresight_get_source(path); - if (!source) + sink = coresight_get_sink(path); + if (!source || !sink) return 0; /* Don't save and restore if the source is inactive */ @@ -1759,16 +1760,36 @@ static int coresight_pm_check(struct coresight_path *path) if (source_has_cb) return 1; + sink_has_cb = coresight_ops(sink)->pm_save_disable && + coresight_ops(sink)->pm_restore_enable; + /* + * It is not permitted that the source has no callbacks while the sink + * does, as the sink cannot be disabled without disabling the source, + * which may lead to lockups. Alternatively, the ETM driver should + * enable self-hosted PM mode at probe (see etm4_probe()). + */ + if (sink_has_cb) { + pr_warn_once("coresight PM failed: source has no PM callbacks; " + "cannot safely control sink\n");
This prints out on my Orion board on a fresh boot because of how pm_save_enable is setup there. Do we really need the configuration of pm_save_enable for ETE/TRBE if we know that it always needs saving?
It also stops warning if I rmmod and modprobe the module after booting. Seems like pm_save_enable is different depending on how the module is loaded which doesn't seem right.
Thats because the warning is pr_warn_*once*()
Suzuki
I don't think so, I tested it with a printf instead of a warn once and also tested modprobeing straight after a reboot.
+ return -EINVAL; + }
return 0; } static int coresight_pm_device_save(struct coresight_device *csdev) { + if (!csdev || !coresight_ops(csdev)->pm_save_disable) + return 0;
return coresight_ops(csdev)->pm_save_disable(csdev); } static void coresight_pm_device_restore(struct coresight_device *csdev) { + if (!csdev || !coresight_ops(csdev)->pm_restore_enable) + return;
coresight_ops(csdev)->pm_restore_enable(csdev); } @@ -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?
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. 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.
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.
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.
+ return ret; } static void coresight_pm_restore(struct coresight_path *path) { struct coresight_device *source = coresight_get_source(path); + struct coresight_device *sink = coresight_get_sink(path); struct coresight_node *from, *to; int ret; + coresight_pm_device_restore(sink);
from = coresight_path_first_node(path); /* Up to the node before sink to avoid latency */ to = list_prev_entry(coresight_path_last_node(path), link); @@ -1808,6 +1846,8 @@ static void coresight_pm_restore(struct coresight_path *path) return; path_failed: + coresight_pm_device_save(sink);
pr_err("Failed in coresight PM restore on CPU%d: %d\n", smp_processor_id(), ret);