On 02/10/2025 1:51 pm, Suzuki K 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.
Suzuki
Mode changes for sources are/were always done outside of any locking using the compare and swap, even before it was moved to coresight device. The reason was probably to do a quick early fail in the case of contention between sysfs and Perf.
I think mode changes for other device types may already be within in their spinlocks.
Unrelated to this change, I think the complexity isn't unnecessary and taking the spinlock could be the very first operation for any action. That would do away with the atomics. We don't need to optimise for any contention cases, because it never happens outside of artificial stress tests.
Signed-off-by: Leo Yan leo.yan@arm.com
include/linux/coresight.h | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 6de59ce8ef8ca45c29e2f09c1b979eb7686b685f..3e5e5acd0c7fcde7d312d440da4355faaf682c7b 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -251,15 +251,11 @@ struct coresight_trace_id_map { * by @coresight_ops. * @access: Device i/o access abstraction for this device. * @dev: The device entity associated to this component.
- @mode: This tracer's mode, i.e sysFS, Perf or disabled. This is
- * actually an 'enum cs_mode', but is stored in an atomic type.
- * This is always accessed through local_read() and local_set(),
- * but wherever it's done from within the Coresight device's
lock,
- * a non-atomic read would also work. This is the main point of
- * synchronisation between code happening inside the sysfs mode's
- * coresight_mutex and outside when running in Perf mode. A
compare
- * and exchange swap is done to atomically claim one mode or the
- * other.
- @mode: The device mode, i.e sysFS, Perf or disabled. This is
actually
- * an 'enum cs_mode' but stored in an atomic type. Access is
always
- * through atomic APIs, ensuring SMP-safe synchronisation between
- * racing from sysFS and Perf mode. A compare-and-exchange
- * operation is done to atomically claim one mode or the other.
* @refcnt: keep track of what is in use. Only access this outside of the * device's spinlock when the coresight_mutex held and mode == * CS_MODE_SYSFS. Otherwise it must be accessed from inside the @@ -288,7 +284,7 @@ struct coresight_device { const struct coresight_ops *ops; struct csdev_access access; struct device dev; - local_t mode; + atomic_t mode; int refcnt; bool orphan; /* sink specific fields */ @@ -621,13 +617,14 @@ static inline bool coresight_is_percpu_sink(struct coresight_device *csdev) static inline bool coresight_take_mode(struct coresight_device *csdev, enum cs_mode new_mode) { - return local_cmpxchg(&csdev->mode, CS_MODE_DISABLED, new_mode) == - CS_MODE_DISABLED; + int curr = CS_MODE_DISABLED;
+ return atomic_try_cmpxchg_acquire(&csdev->mode, &curr, new_mode); } static inline enum cs_mode coresight_get_mode(struct coresight_device *csdev) { - return local_read(&csdev->mode); + return atomic_read_acquire(&csdev->mode); } static inline void coresight_set_mode(struct coresight_device *csdev, @@ -643,7 +640,7 @@ static inline void coresight_set_mode(struct coresight_device *csdev, WARN(new_mode != CS_MODE_DISABLED && current_mode != CS_MODE_DISABLED && current_mode != new_mode, "Device already in use\n"); - local_set(&csdev->mode, new_mode); + atomic_set_release(&csdev->mode, new_mode); } struct coresight_device *coresight_register(struct coresight_desc *desc);