[PATCH v1 05/11] coresight: Save activated path into source device

[James Clark]

On 20/06/2025 3:24 pm, Leo Yan wrote:
> On Tue, Jun 03, 2025 at 03:49:26PM +0100, James Clark wrote:
> 
> [...]
> 
>>> @@ -577,6 +583,10 @@ int coresight_enable_path(struct coresight_path *path, enum cs_mode mode,
>>>    		}
>>>    	}
>>> +	source = coresight_get_source(path);
>>> +	if (coresight_is_percpu_source(source))
>>> +		source->path = path;
>>> +
>>>    out:
>>>    	return ret;
>>>    err_disable_helpers:
>>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>>> index 64cee4040daa..cd9709a36b9c 100644
>>> --- a/include/linux/coresight.h
>>> +++ b/include/linux/coresight.h
>>> @@ -265,6 +265,7 @@ struct coresight_trace_id_map {
>>>     *		CS_MODE_SYSFS. Otherwise it must be accessed from inside the
>>>     *		spinlock.
>>>     * @orphan:	true if the component has connections that haven't been linked.
>>> + * @path:	activated path pointer (only used for source device).
>>>     * @sysfs_sink_activated: 'true' when a sink has been selected for use via sysfs
>>>     *		by writing a 1 to the 'enable_sink' file.  A sink can be
>>>     *		activated but not yet enabled.  Enabling for a _sink_ happens
>>> @@ -291,6 +292,8 @@ struct coresight_device {
>>>    	local_t	mode;
>>>    	int refcnt;
>>>    	bool orphan;
>>> +	/* activated path (for source only) */
>>> +	struct coresight_path *path;
>>
>> This is probably cleaner if it's saved in the new global per-cpu you added
>> in patch 2. It's even more specific than "for source only", it's actually
>> only for per-cpu sources so it's not worth having it on every device.
> 
> We can maintain Coresight path pointers in a global per-CPU data
> structure. These path pointers are set and cleared during path enable
> and disable operations, respectively. Idle threads can then access
> these pointers for power management purposes.
> 
> However, the challenge is how we can safely access the path pointer from
> an idle thread - particularly because idle threads cannot acquire
> coresight_mutex, which is a sleepable lock. Consider a race condition
> scenario where SysFS knobs frequently enable and disable paths, while
> the corresponding CPUs enter and exit idle states. In such cases, if
> an idle thread attempts to access the path pointers, we would need to
> use raw spinlocks to ensure safety. This is problematic, as it could
> result in the CPU suspend/resume flow waiting on SysFS operations.
> 
> This is why it turns out to use lockless approach. Two key background
> info:
> 
> - Path pointers are not guaranteed to be safe to access in CPU power
>    management (PM) notifiers. In contrast, the coresight_device pointer
>    of a source device (e.g., an ETM) is safe to access.
> 
>    A path's lifecycle begins and ends with SysFS enable and disable
>    operations. OTOH, an ETM’s coresight_device pointer is initialized
>    during the probe phase and cleared during module removal. Module
>    loading and unloading trigger SMP calls for initialization and
>    cleanup. These synchronized SMP calls ensure that it is safe for CPU
>    PM notifiers to access the source device structure.
> 
> - Another factor is that the device mode is used to determine whether
>    it is safe to access the path pointer. When a source device is
>    enabled, we must ensure that its state transitions from DISABLED to
>    an enabled state on the target CPU. This guarantees that the mode,
>    and any subsequent operations dependent on it, follow a strictly
>    sequential order.
> 
>    CPU0                                    CPU1
>    etm4_enable()
>     ` etm4_enable_sysfs()
>        ` smp_call_function_single() ---->  etm4_enable_hw_smp_call()
>                                             ` coresight_take_mode(SYSFS)
> 
>                                             CPU idle:
>                                             etm4_cpu_save()
>                                              ` coresight_get_mode()
>                                                 ^^^
>                                                 Read out the SYSFS mode
>                                                 Safely access coresight_device::path
> 
> Therefore, safe access to the Coresight path heavily relies on the
> mode (as part of the state machine). This is why the patch adds the
> path pointer to the same structure as the mode.
> 
> Thanks,
> Leo
> 

I'm not suggesting to change the flow for accessing it, only where it's 
stored to not waste space storing it for devices that never need it. It 
also makes the code a bit more readable because you know that variable 
is never accessed for other device types by the fact that it doesn't exist.

Because we only have 1 per-CPU source for each CPU, there is no 
functional difference between storing it in the device or in a global 
per-cpu area.

If we imagine that the path pointer saved in the device is actually a 
pointer to a pointer, and that pointer happens to point to the per-CPU area,

  struct coresight_device {
     struct coresight_path **path;  ---------------------
  }                                                      |
                                                         |
                                                        \ /
  static DEFINE_PER_CPU(struct coresight_device *, csdev_cpu_path);

then the argument for safely accessing doesn't really make sense, it's 
just a stored somewhere else. Moving it doesn't add or remove any safety 
or concurrency issues, it just has a different address than the one in 
the struct coresight_device and looks a bit different in the code.


>>>    	/* sink specific fields */
>>>    	bool sysfs_sink_activated;
>>>    	struct dev_ext_attribute *ea;
>>