Re: [PATCH v2 01/28] coresight: Change device mode to atomic type

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 01/07/25 8:23 PM, 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.


But have we really faced such problems on real systems due to local_t
or this is just an improvement ?

> 
> Fixes: 22fd532eaa0c ("coresight: etm3x: adding operation mode for etm_enable()")
> 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 4ac65c68bbf44b98db22c3dad2d83a224ce5278e..5fd3d08824e5a91a197aa01daf0fc392392f3e55 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 */
> @@ -650,13 +646,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,
> @@ -672,7 +669,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);
>