On Thu, Oct 02, 2025 at 01:51:22PM +0100, Suzuki Kuruppassery Poulose wrote:
On 15/09/2025 11:33, Leo Yan wrote:
The device mode is defined as local type. This type cannot promise SMP-safe access.
Change to atomic type and impose relax ordering, which ensures the SMP-safe synchronisation and the ordering between the mode setting and relevant operations.
Fixes: 22fd532eaa0c ("coresight: etm3x: adding operation mode for etm_enable()")
Do we really have an issue here ? I thought we modify the mode under spinlock. Also the fixes commit looks odd. The mode was added to the coresight device only recently. Prior to that, it was part of drvdata and was serialized by the respective spinlocks. So I don't see how this was an issue.
Below is the sequence in which CPU0 enables the path with CPU1 ETM via sysfs knob, while CPU1 is executing the CPUidle notifier:
CPU0 | CPU1 | coresight_enable_sysfs() | `> mutex_lock(&coresight_mutex) | etm4_cpu_pm_notify() `> etm4_enable() | `> etm4_cpu_save() `> coresight_take_mode(csdev, mode) | `> coresight_get_mode(csdev) `> Call SMP call | `> mutex_unlock(&coresight_mutex) |
(1) The mode is not protected by spinlock. Although the sysfs flow acquires a mutex, it does not provide exclusive access to the mode.
(2) On SMP systems, the mode is accessed by multiple CPUs. We currently use the local_* accessors, which are suitable only for per-CPU (local) access, not for SMP access.
If a vairable is not protected by spinlock, and accessed by SMP cores, then atomic type would be a right choice.
Thanks, Leo