[PATCH v3 01/31] coresight: Change device mode to atomic type

[James Clark]

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