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

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

On 09/04/2026 3:30 pm, James Clark wrote:
> 
> 
> 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.
> 

I also printed the pointers to the callbacks and they really do change.

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