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

[James Clark <james.clark@linaro.org>]

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