Received-SPF: pass (google.com: domain of linux-kernel+bounces-19435-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <cbe648c7-91b9-403c-9948-c3ba1d8d4fcc@arm.com>
Date: Mon, 8 Jan 2024 11:32:16 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 4/8] coresight: Move mode to struct coresight_device
Content-Language: en-US
To: James Clark <james.clark@arm.com>, coresight@lists.linaro.org
Cc: Mike Leach <mike.leach@linaro.org>,
 Alexander Shishkin <alexander.shishkin@linux.intel.com>,
 Maxime Coquelin <mcoquelin.stm32@gmail.com>,
 Alexandre Torgue <alexandre.torgue@foss.st.com>,
 linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
 linux-stm32@st-md-mailman.stormreply.com
References: <20231212155407.1429121-1-james.clark@arm.com>
 <20231212155407.1429121-5-james.clark@arm.com>
From: Suzuki K Poulose <suzuki.poulose@arm.com>
In-Reply-To: <20231212155407.1429121-5-james.clark@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 12/12/2023 15:54, James Clark wrote:
> Most devices use mode, so move the mode definition out of the individual
> devices and up to the Coresight device. This will allow the core code to
> also know the mode which will be useful in a later commit.
> 
> This also fixes the inconsistency of the documentation of the mode field
> on the individual device types. For example ETB10 had "this ETB is being
> used".
> 
> Two devices didn't require an atomic mode type, so these usages have
> been converted to atomic_get() and atomic_set() only to make it compile,
> but the documentation of the field in struct coresight_device explains
> this type of usage.
> 
> In the future, manipulation of the mode could be completely moved out of
> the individual devices and into the core code because it's almost all
> duplicate code, and this change is a step towards that.

The change looks good to me. I am wondering if we should abstract it out ?

i.e.,
	csdev_get_mode(struct coresight_device *csdev)
	csdev_set_mode(struct coresight_device *csdev, mode)
	csdev_{cmpxchg or set_if}_mode(csdev, old_mode, new_mode)


> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-etb10.c | 18 ++++++-------
>   drivers/hwtracing/coresight/coresight-etm.h   |  2 --
>   .../coresight/coresight-etm3x-core.c          | 13 +++++-----
>   .../coresight/coresight-etm3x-sysfs.c         |  4 +--
>   drivers/hwtracing/coresight/coresight-stm.c   | 20 +++++++-------
>   .../hwtracing/coresight/coresight-tmc-core.c  |  2 +-
>   .../hwtracing/coresight/coresight-tmc-etf.c   | 26 +++++++++----------
>   .../hwtracing/coresight/coresight-tmc-etr.c   | 20 +++++++-------
>   drivers/hwtracing/coresight/coresight-tmc.h   |  2 --
>   drivers/hwtracing/coresight/ultrasoc-smb.c    | 13 +++++-----
>   drivers/hwtracing/coresight/ultrasoc-smb.h    |  2 --
>   include/linux/coresight.h                     |  6 +++++
>   12 files changed, 62 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
> index fa80039e0821..08e5bba862db 100644
> --- a/drivers/hwtracing/coresight/coresight-etb10.c
> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
> @@ -76,7 +76,6 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "etb");
>    * @pid:	Process ID of the process being monitored by the session
>    *		that is using this component.
>    * @buf:	area of memory where ETB buffer content gets sent.
> - * @mode:	this ETB is being used.
>    * @buffer_depth: size of @buf.
>    * @trigger_cntr: amount of words to store after a trigger.
>    */
> @@ -89,7 +88,6 @@ struct etb_drvdata {
>   	local_t			reading;
>   	pid_t			pid;
>   	u8			*buf;
> -	u32			mode;
>   	u32			buffer_depth;
>   	u32			trigger_cntr;
>   };
> @@ -150,17 +148,17 @@ static int etb_enable_sysfs(struct coresight_device *csdev)
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
>   
>   	/* Don't messup with perf sessions. */
> -	if (drvdata->mode == CS_MODE_PERF) {
> +	if (local_read(&csdev->mode) == CS_MODE_PERF) {
>   		ret = -EBUSY;
>   		goto out;
>   	}
>   
> -	if (drvdata->mode == CS_MODE_DISABLED) {
> +	if (local_read(&csdev->mode) == CS_MODE_DISABLED) {
>   		ret = etb_enable_hw(drvdata);
>   		if (ret)
>   			goto out;
>   
> -		drvdata->mode = CS_MODE_SYSFS;
> +		local_set(&csdev->mode, CS_MODE_SYSFS);
>   	}
>   
>   	atomic_inc(&csdev->refcnt);
> @@ -181,7 +179,7 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
>   
>   	/* No need to continue if the component is already in used by sysFS. */
> -	if (drvdata->mode == CS_MODE_SYSFS) {
> +	if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
>   		ret = -EBUSY;
>   		goto out;
>   	}
> @@ -216,7 +214,7 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
>   	if (!ret) {
>   		/* Associate with monitored process. */
>   		drvdata->pid = pid;
> -		drvdata->mode = CS_MODE_PERF;
> +		local_set(&drvdata->csdev->mode, CS_MODE_PERF);
>   		atomic_inc(&csdev->refcnt);
>   	}
>   
> @@ -362,11 +360,11 @@ static int etb_disable(struct coresight_device *csdev)
>   	}
>   
>   	/* Complain if we (somehow) got out of sync */
> -	WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
> +	WARN_ON_ONCE(local_read(&csdev->mode) == CS_MODE_DISABLED);
>   	etb_disable_hw(drvdata);
>   	/* Dissociate from monitored process. */
>   	drvdata->pid = -1;
> -	drvdata->mode = CS_MODE_DISABLED;
> +	local_set(&csdev->mode, CS_MODE_DISABLED);
>   	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>   
>   	dev_dbg(&csdev->dev, "ETB disabled\n");
> @@ -589,7 +587,7 @@ static void etb_dump(struct etb_drvdata *drvdata)
>   	unsigned long flags;
>   
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
> -	if (drvdata->mode == CS_MODE_SYSFS) {
> +	if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
>   		__etb_disable_hw(drvdata);
>   		etb_dump_hw(drvdata);
>   		__etb_enable_hw(drvdata);
> diff --git a/drivers/hwtracing/coresight/coresight-etm.h b/drivers/hwtracing/coresight/coresight-etm.h
> index 9a0d08b092ae..e02c3ea972c9 100644
> --- a/drivers/hwtracing/coresight/coresight-etm.h
> +++ b/drivers/hwtracing/coresight/coresight-etm.h
> @@ -215,7 +215,6 @@ struct etm_config {
>    * @port_size:	port size as reported by ETMCR bit 4-6 and 21.
>    * @arch:	ETM/PTM version number.
>    * @use_cpu14:	true if management registers need to be accessed via CP14.
> - * @mode:	this tracer's mode, i.e sysFS, Perf or disabled.
>    * @sticky_enable: true if ETM base configuration has been done.
>    * @boot_enable:true if we should start tracing at boot time.
>    * @os_unlock:	true if access to management registers is allowed.
> @@ -238,7 +237,6 @@ struct etm_drvdata {
>   	int				port_size;
>   	u8				arch;
>   	bool				use_cp14;
> -	local_t				mode;
>   	bool				sticky_enable;
>   	bool				boot_enable;
>   	bool				os_unlock;
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> index 116a91d90ac2..29f62dfac673 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
> @@ -559,7 +559,7 @@ static int etm_enable(struct coresight_device *csdev, struct perf_event *event,
>   	u32 val;
>   	struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>   
> -	val = local_cmpxchg(&drvdata->mode, CS_MODE_DISABLED, mode);
> +	val = local_cmpxchg(&drvdata->csdev->mode, CS_MODE_DISABLED, mode);
>   
>   	/* Someone is already using the tracer */
>   	if (val)
> @@ -578,7 +578,7 @@ static int etm_enable(struct coresight_device *csdev, struct perf_event *event,
>   
>   	/* The tracer didn't start */
>   	if (ret)
> -		local_set(&drvdata->mode, CS_MODE_DISABLED);
> +		local_set(&drvdata->csdev->mode, CS_MODE_DISABLED);
>   
>   	return ret;
>   }
> @@ -672,14 +672,13 @@ static void etm_disable(struct coresight_device *csdev,
>   			struct perf_event *event)
>   {
>   	enum cs_mode mode;
> -	struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>   
>   	/*
>   	 * For as long as the tracer isn't disabled another entity can't
>   	 * change its status.  As such we can read the status here without
>   	 * fearing it will change under us.
>   	 */
> -	mode = local_read(&drvdata->mode);
> +	mode = local_read(&csdev->mode);
>   
>   	switch (mode) {
>   	case CS_MODE_DISABLED:
> @@ -696,7 +695,7 @@ static void etm_disable(struct coresight_device *csdev,
>   	}
>   
>   	if (mode)
> -		local_set(&drvdata->mode, CS_MODE_DISABLED);
> +		local_set(&csdev->mode, CS_MODE_DISABLED);
>   }
>   
>   static const struct coresight_ops_source etm_source_ops = {
> @@ -730,7 +729,7 @@ static int etm_starting_cpu(unsigned int cpu)
>   		etmdrvdata[cpu]->os_unlock = true;
>   	}
>   
> -	if (local_read(&etmdrvdata[cpu]->mode))
> +	if (local_read(&etmdrvdata[cpu]->csdev->mode))
>   		etm_enable_hw(etmdrvdata[cpu]);
>   	spin_unlock(&etmdrvdata[cpu]->spinlock);
>   	return 0;
> @@ -742,7 +741,7 @@ static int etm_dying_cpu(unsigned int cpu)
>   		return 0;
>   
>   	spin_lock(&etmdrvdata[cpu]->spinlock);
> -	if (local_read(&etmdrvdata[cpu]->mode))
> +	if (local_read(&etmdrvdata[cpu]->csdev->mode))
>   		etm_disable_hw(etmdrvdata[cpu]);
>   	spin_unlock(&etmdrvdata[cpu]->spinlock);
>   	return 0;
> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> index 2f271b7fb048..6c8429c980b1 100644
> --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c
> @@ -722,7 +722,7 @@ static ssize_t cntr_val_show(struct device *dev,
>   	struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>   	struct etm_config *config = &drvdata->config;
>   
> -	if (!local_read(&drvdata->mode)) {
> +	if (!local_read(&drvdata->csdev->mode)) {
>   		spin_lock(&drvdata->spinlock);
>   		for (i = 0; i < drvdata->nr_cntr; i++)
>   			ret += sprintf(buf, "counter %d: %x\n",
> @@ -941,7 +941,7 @@ static ssize_t seq_curr_state_show(struct device *dev,
>   	struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>   	struct etm_config *config = &drvdata->config;
>   
> -	if (!local_read(&drvdata->mode)) {
> +	if (!local_read(&drvdata->csdev->mode)) {
>   		val = config->seq_curr_state;
>   		goto out;
>   	}
> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
> index a1c27c901ad1..190ba569b05a 100644
> --- a/drivers/hwtracing/coresight/coresight-stm.c
> +++ b/drivers/hwtracing/coresight/coresight-stm.c
> @@ -119,7 +119,6 @@ DEFINE_CORESIGHT_DEVLIST(stm_devs, "stm");
>    * @spinlock:		only one at a time pls.
>    * @chs:		the channels accociated to this STM.
>    * @stm:		structure associated to the generic STM interface.
> - * @mode:		this tracer's mode (enum cs_mode), i.e sysFS, or disabled.
>    * @traceid:		value of the current ID for this component.
>    * @write_bytes:	Maximus bytes this STM can write at a time.
>    * @stmsper:		settings for register STMSPER.
> @@ -136,7 +135,6 @@ struct stm_drvdata {
>   	spinlock_t		spinlock;
>   	struct channel_space	chs;
>   	struct stm_data		stm;
> -	local_t			mode;
>   	u8			traceid;
>   	u32			write_bytes;
>   	u32			stmsper;
> @@ -201,7 +199,7 @@ static int stm_enable(struct coresight_device *csdev, struct perf_event *event,
>   	if (mode != CS_MODE_SYSFS)
>   		return -EINVAL;
>   
> -	val = local_cmpxchg(&drvdata->mode, CS_MODE_DISABLED, mode);
> +	val = local_cmpxchg(&csdev->mode, CS_MODE_DISABLED, mode);
>   
>   	/* Someone is already using the tracer */
>   	if (val)
> @@ -266,7 +264,7 @@ static void stm_disable(struct coresight_device *csdev,
>   	 * change its status.  As such we can read the status here without
>   	 * fearing it will change under us.
>   	 */
> -	if (local_read(&drvdata->mode) == CS_MODE_SYSFS) {
> +	if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
>   		spin_lock(&drvdata->spinlock);
>   		stm_disable_hw(drvdata);
>   		spin_unlock(&drvdata->spinlock);
> @@ -276,7 +274,7 @@ static void stm_disable(struct coresight_device *csdev,
>   
>   		pm_runtime_put(csdev->dev.parent);
>   
> -		local_set(&drvdata->mode, CS_MODE_DISABLED);
> +		local_set(&csdev->mode, CS_MODE_DISABLED);
>   		dev_dbg(&csdev->dev, "STM tracing disabled\n");
>   	}
>   }
> @@ -373,7 +371,7 @@ static long stm_generic_set_options(struct stm_data *stm_data,
>   {
>   	struct stm_drvdata *drvdata = container_of(stm_data,
>   						   struct stm_drvdata, stm);
> -	if (!(drvdata && local_read(&drvdata->mode)))
> +	if (!(drvdata && local_read(&drvdata->csdev->mode)))
>   		return -EINVAL;
>   
>   	if (channel >= drvdata->numsp)
> @@ -408,7 +406,7 @@ static ssize_t notrace stm_generic_packet(struct stm_data *stm_data,
>   						   struct stm_drvdata, stm);
>   	unsigned int stm_flags;
>   
> -	if (!(drvdata && local_read(&drvdata->mode)))
> +	if (!(drvdata && local_read(&drvdata->csdev->mode)))
>   		return -EACCES;
>   
>   	if (channel >= drvdata->numsp)
> @@ -515,7 +513,7 @@ static ssize_t port_select_show(struct device *dev,
>   	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>   	unsigned long val;
>   
> -	if (!local_read(&drvdata->mode)) {
> +	if (!local_read(&drvdata->csdev->mode)) {
>   		val = drvdata->stmspscr;
>   	} else {
>   		spin_lock(&drvdata->spinlock);
> @@ -541,7 +539,7 @@ static ssize_t port_select_store(struct device *dev,
>   	spin_lock(&drvdata->spinlock);
>   	drvdata->stmspscr = val;
>   
> -	if (local_read(&drvdata->mode)) {
> +	if (local_read(&drvdata->csdev->mode)) {
>   		CS_UNLOCK(drvdata->base);
>   		/* Process as per ARM's TRM recommendation */
>   		stmsper = readl_relaxed(drvdata->base + STMSPER);
> @@ -562,7 +560,7 @@ static ssize_t port_enable_show(struct device *dev,
>   	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>   	unsigned long val;
>   
> -	if (!local_read(&drvdata->mode)) {
> +	if (!local_read(&drvdata->csdev->mode)) {
>   		val = drvdata->stmsper;
>   	} else {
>   		spin_lock(&drvdata->spinlock);
> @@ -588,7 +586,7 @@ static ssize_t port_enable_store(struct device *dev,
>   	spin_lock(&drvdata->spinlock);
>   	drvdata->stmsper = val;
>   
> -	if (local_read(&drvdata->mode)) {
> +	if (local_read(&drvdata->csdev->mode)) {
>   		CS_UNLOCK(drvdata->base);
>   		writel_relaxed(drvdata->stmsper, drvdata->base + STMSPER);
>   		CS_LOCK(drvdata->base);
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index 7ec5365e2b64..e5d47f61f9f3 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> @@ -558,7 +558,7 @@ static void tmc_shutdown(struct amba_device *adev)
>   
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
>   
> -	if (drvdata->mode == CS_MODE_DISABLED)
> +	if (local_read(&drvdata->csdev->mode) == CS_MODE_DISABLED)
>   		goto out;
>   
>   	if (drvdata->config_type == TMC_CONFIG_TYPE_ETR)
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index 7406b65e2cdd..2a7e516052a2 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -89,7 +89,7 @@ static void __tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
>   	 * When operating in sysFS mode the content of the buffer needs to be
>   	 * read before the TMC is disabled.
>   	 */
> -	if (drvdata->mode == CS_MODE_SYSFS)
> +	if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS)
>   		tmc_etb_dump_hw(drvdata);
>   	tmc_disable_hw(drvdata);
>   
> @@ -205,7 +205,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
>   	 * sink is already enabled no memory is needed and the HW need not be
>   	 * touched.
>   	 */
> -	if (drvdata->mode == CS_MODE_SYSFS) {
> +	if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
>   		atomic_inc(&csdev->refcnt);
>   		goto out;
>   	}
> @@ -228,7 +228,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
>   
>   	ret = tmc_etb_enable_hw(drvdata);
>   	if (!ret) {
> -		drvdata->mode = CS_MODE_SYSFS;
> +		local_set(&csdev->mode, CS_MODE_SYSFS);
>   		atomic_inc(&csdev->refcnt);
>   	} else {
>   		/* Free up the buffer if we failed to enable */
> @@ -262,7 +262,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
>   		 * No need to continue if the ETB/ETF is already operated
>   		 * from sysFS.
>   		 */
> -		if (drvdata->mode == CS_MODE_SYSFS) {
> +		if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
>   			ret = -EBUSY;
>   			break;
>   		}
> @@ -292,7 +292,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
>   		if (!ret) {
>   			/* Associate with monitored process. */
>   			drvdata->pid = pid;
> -			drvdata->mode = CS_MODE_PERF;
> +			local_set(&csdev->mode, CS_MODE_PERF);
>   			atomic_inc(&csdev->refcnt);
>   		}
>   	} while (0);
> @@ -344,11 +344,11 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev)
>   	}
>   
>   	/* Complain if we (somehow) got out of sync */
> -	WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
> +	WARN_ON_ONCE(local_read(&csdev->mode) == CS_MODE_DISABLED);
>   	tmc_etb_disable_hw(drvdata);
>   	/* Dissociate from monitored process. */
>   	drvdata->pid = -1;
> -	drvdata->mode = CS_MODE_DISABLED;
> +	local_set(&csdev->mode, CS_MODE_DISABLED);
>   
>   	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>   
> @@ -374,7 +374,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
>   	if (atomic_read(&csdev->refcnt) == 0) {
>   		ret = tmc_etf_enable_hw(drvdata);
>   		if (!ret) {
> -			drvdata->mode = CS_MODE_SYSFS;
> +			local_set(&csdev->mode, CS_MODE_SYSFS);
>   			first_enable = true;
>   		}
>   	}
> @@ -403,7 +403,7 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
>   
>   	if (atomic_dec_return(&csdev->refcnt) == 0) {
>   		tmc_etf_disable_hw(drvdata);
> -		drvdata->mode = CS_MODE_DISABLED;
> +		local_set(&csdev->mode, CS_MODE_DISABLED);
>   		last_disable = true;
>   	}
>   	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> @@ -483,7 +483,7 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
>   		return 0;
>   
>   	/* This shouldn't happen */
> -	if (WARN_ON_ONCE(drvdata->mode != CS_MODE_PERF))
> +	if (WARN_ON_ONCE(local_read(&csdev->mode) != CS_MODE_PERF))
>   		return 0;
>   
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
> @@ -629,7 +629,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
>   	}
>   
>   	/* Don't interfere if operated from Perf */
> -	if (drvdata->mode == CS_MODE_PERF) {
> +	if (local_read(&drvdata->csdev->mode) == CS_MODE_PERF) {
>   		ret = -EINVAL;
>   		goto out;
>   	}
> @@ -641,7 +641,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
>   	}
>   
>   	/* Disable the TMC if need be */
> -	if (drvdata->mode == CS_MODE_SYSFS) {
> +	if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
>   		/* There is no point in reading a TMC in HW FIFO mode */
>   		mode = readl_relaxed(drvdata->base + TMC_MODE);
>   		if (mode != TMC_MODE_CIRCULAR_BUFFER) {
> @@ -673,7 +673,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
>   
>   	/* Re-enable the TMC if need be */
> -	if (drvdata->mode == CS_MODE_SYSFS) {
> +	if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
>   		/* There is no point in reading a TMC in HW FIFO mode */
>   		mode = readl_relaxed(drvdata->base + TMC_MODE);
>   		if (mode != TMC_MODE_CIRCULAR_BUFFER) {
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index af02ba5d5f15..3dc989d4fcab 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1143,7 +1143,7 @@ static void __tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
>   	 * When operating in sysFS mode the content of the buffer needs to be
>   	 * read before the TMC is disabled.
>   	 */
> -	if (drvdata->mode == CS_MODE_SYSFS)
> +	if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS)
>   		tmc_etr_sync_sysfs_buf(drvdata);
>   
>   	tmc_disable_hw(drvdata);
> @@ -1189,7 +1189,7 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev)
>   		spin_lock_irqsave(&drvdata->spinlock, flags);
>   	}
>   
> -	if (drvdata->reading || drvdata->mode == CS_MODE_PERF) {
> +	if (drvdata->reading || local_read(&csdev->mode) == CS_MODE_PERF) {
>   		ret = -EBUSY;
>   		goto out;
>   	}
> @@ -1230,14 +1230,14 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
>   	 * sink is already enabled no memory is needed and the HW need not be
>   	 * touched, even if the buffer size has changed.
>   	 */
> -	if (drvdata->mode == CS_MODE_SYSFS) {
> +	if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
>   		atomic_inc(&csdev->refcnt);
>   		goto out;
>   	}
>   
>   	ret = tmc_etr_enable_hw(drvdata, sysfs_buf);
>   	if (!ret) {
> -		drvdata->mode = CS_MODE_SYSFS;
> +		local_set(&csdev->mode, CS_MODE_SYSFS);
>   		atomic_inc(&csdev->refcnt);
>   	}
>   
> @@ -1652,7 +1652,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
>   
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
>   	 /* Don't use this sink if it is already claimed by sysFS */
> -	if (drvdata->mode == CS_MODE_SYSFS) {
> +	if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
>   		rc = -EBUSY;
>   		goto unlock_out;
>   	}
> @@ -1684,7 +1684,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
>   	if (!rc) {
>   		/* Associate with monitored process. */
>   		drvdata->pid = pid;
> -		drvdata->mode = CS_MODE_PERF;
> +		local_set(&csdev->mode, CS_MODE_PERF);
>   		drvdata->perf_buf = etr_perf->etr_buf;
>   		atomic_inc(&csdev->refcnt);
>   	}
> @@ -1725,11 +1725,11 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev)
>   	}
>   
>   	/* Complain if we (somehow) got out of sync */
> -	WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
> +	WARN_ON_ONCE(local_read(&csdev->mode) == CS_MODE_DISABLED);
>   	tmc_etr_disable_hw(drvdata);
>   	/* Dissociate from monitored process. */
>   	drvdata->pid = -1;
> -	drvdata->mode = CS_MODE_DISABLED;
> +	local_set(&csdev->mode, CS_MODE_DISABLED);
>   	/* Reset perf specific data */
>   	drvdata->perf_buf = NULL;
>   
> @@ -1777,7 +1777,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
>   	}
>   
>   	/* Disable the TMC if we are trying to read from a running session. */
> -	if (drvdata->mode == CS_MODE_SYSFS)
> +	if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS)
>   		__tmc_etr_disable_hw(drvdata);
>   
>   	drvdata->reading = true;
> @@ -1799,7 +1799,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
>   
>   	/* RE-enable the TMC if need be */
> -	if (drvdata->mode == CS_MODE_SYSFS) {
> +	if (local_read(&drvdata->csdev->mode) == CS_MODE_SYSFS) {
>   		/*
>   		 * The trace run will continue with the same allocated trace
>   		 * buffer. Since the tracer is still enabled drvdata::buf can't
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 8dcb426ac3e7..cef979c897e6 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -178,7 +178,6 @@ struct etr_buf {
>    * @size:	trace buffer size for this TMC (common for all modes).
>    * @max_burst_size: The maximum burst size that can be initiated by
>    *		TMC-ETR on AXI bus.
> - * @mode:	how this TMC is being used.
>    * @config_type: TMC variant, must be of type @tmc_config_type.
>    * @memwidth:	width of the memory interface databus, in bytes.
>    * @trigger_cntr: amount of words to store after a trigger.
> @@ -203,7 +202,6 @@ struct tmc_drvdata {
>   	u32			len;
>   	u32			size;
>   	u32			max_burst_size;
> -	u32			mode;
>   	enum tmc_config_type	config_type;
>   	enum tmc_mem_intf_width	memwidth;
>   	u32			trigger_cntr;
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index 10e886455b8b..1a8c64645b92 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -207,11 +207,11 @@ static void smb_enable_sysfs(struct coresight_device *csdev)
>   {
>   	struct smb_drv_data *drvdata = dev_get_drvdata(csdev->dev.parent);
>   
> -	if (drvdata->mode != CS_MODE_DISABLED)
> +	if (local_read(&csdev->mode) != CS_MODE_DISABLED)
>   		return;
>   
>   	smb_enable_hw(drvdata);
> -	drvdata->mode = CS_MODE_SYSFS;
> +	local_set(&csdev->mode, CS_MODE_SYSFS);
>   }
>   
>   static int smb_enable_perf(struct coresight_device *csdev, void *data)
> @@ -234,7 +234,7 @@ static int smb_enable_perf(struct coresight_device *csdev, void *data)
>   	if (drvdata->pid == -1) {
>   		smb_enable_hw(drvdata);
>   		drvdata->pid = pid;
> -		drvdata->mode = CS_MODE_PERF;
> +		local_set(&csdev->mode, CS_MODE_PERF);
>   	}
>   
>   	return 0;
> @@ -253,7 +253,8 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
>   		return -EBUSY;
>   
>   	/* Do nothing, the SMB is already enabled as other mode */
> -	if (drvdata->mode != CS_MODE_DISABLED && drvdata->mode != mode)
> +	if (local_read(&csdev->mode) != CS_MODE_DISABLED &&
> +	    local_read(&csdev->mode) != mode)
>   		return -EBUSY;
>   
>   	switch (mode) {
> @@ -289,13 +290,13 @@ static int smb_disable(struct coresight_device *csdev)
>   		return -EBUSY;
>   
>   	/* Complain if we (somehow) got out of sync */
> -	WARN_ON_ONCE(drvdata->mode == CS_MODE_DISABLED);
> +	WARN_ON_ONCE(local_read(&csdev->mode) == CS_MODE_DISABLED);
>   
>   	smb_disable_hw(drvdata);
>   
>   	/* Dissociate from the target process. */
>   	drvdata->pid = -1;
> -	drvdata->mode = CS_MODE_DISABLED;
> +	local_set(&csdev->mode, CS_MODE_DISABLED);
>   	dev_dbg(&csdev->dev, "Ultrasoc SMB disabled\n");
>   
>   	return 0;
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.h b/drivers/hwtracing/coresight/ultrasoc-smb.h
> index 82a44c14a882..a91d39cfccb8 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.h
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.h
> @@ -109,7 +109,6 @@ struct smb_data_buffer {
>    * @reading:	Synchronise user space access to SMB buffer.
>    * @pid:	Process ID of the process being monitored by the
>    *		session that is using this component.
> - * @mode:	How this SMB is being used, perf mode or sysfs mode.
>    */
>   struct smb_drv_data {
>   	void __iomem *base;
> @@ -119,7 +118,6 @@ struct smb_drv_data {
>   	spinlock_t spinlock;
>   	bool reading;
>   	pid_t pid;
> -	enum cs_mode mode;
>   };
>   
>   #endif
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 65131bfbd904..ba817f563ff7 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -226,6 +226,11 @@ struct coresight_sysfs_link {
>    *		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.

By abstracting it, we could enforce this change. Eitherways, the change 
as such is fine by me.

Suzuki


>    * @refcnt:	keep track of what is in use.
>    * @orphan:	true if the component has connections that haven't been linked.
>    * @enable:	'true' if component is currently part of an active path.
> @@ -252,6 +257,7 @@ struct coresight_device {
>   	const struct coresight_ops *ops;
>   	struct csdev_access access;
>   	struct device dev;
> +	local_t	mode;
>   	atomic_t refcnt;
>   	bool orphan;
>   	bool enable;