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

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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


> 
>> +        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);
>>
>