2024-01-29 15:55:53

by James Clark

[permalink] [raw]
Subject: [PATCH v2 00/12] coresight: Separate sysfs and Perf usage and some other cleanups

Changes since V1:

* Clarify further "the selected sink" in _coresight_build_path()
* Move etm4x's mode to coresight device which was missing from V1
* Use explicit initialisers in coresight_dev_type
* Create functions for handling mode changes

I've been finding it quite difficult to reason about some of the state
and functions in coresight-core.c because they have generic names when
they are actually only relevant to the sysfs usage of Coresight rather
than usage through Perf. This is probably because sysfs came first and
Perf was added later. This has caused a couple of issues where these
things have been used in the wrong context, for example the first
commit is a fixup.

To fix this I've mainly just moved all of the sysfs stuff to the sysfs
file and removed the 'enable' state, which was just for sysfs. While
doing the refactor it became obvious that refcnt didn't need to be
atomic either, so that can be simplified along with some other comment
clarifications and simplifications.

Hopefully it's also a step towards to removing all of the duplicate
refcnt and mode tracking code from the individual devices. That tracking
pretty much always results in a one-shot enable/disable and fixes the
mode to either sysfs or Perf, and there is no reason that can't exist in
the core layer outside of the devices. I tried to finish that in this
set, but there turned out to be some complexities, so I cut it short at
a point where I can be sure that there are no behavioral changes.

James Clark (12):
coresight: Fix issue where a source device's helpers aren't disabled
coresight: Make language around "activated" sinks consistent
coresight: Remove ops callback checks
coresight: Move mode to struct coresight_device
coresight: Remove the 'enable' field.
coresight: Move all sysfs code to sysfs file
coresight: Remove atomic type from refcnt
coresight: Remove unused stubs
coresight: Add explicit member initializers to coresight_dev_type
coresight: Add helper for atomically taking the device
coresight: Add a helper for getting csdev->mode
coresight: Add helper for setting csdev->mode

drivers/hwtracing/coresight/coresight-core.c | 494 +-----------------
drivers/hwtracing/coresight/coresight-etb10.c | 29 +-
.../hwtracing/coresight/coresight-etm-perf.c | 2 +-
drivers/hwtracing/coresight/coresight-etm.h | 2 -
.../coresight/coresight-etm3x-core.c | 23 +-
.../coresight/coresight-etm3x-sysfs.c | 4 +-
.../coresight/coresight-etm4x-core.c | 26 +-
drivers/hwtracing/coresight/coresight-etm4x.h | 1 -
drivers/hwtracing/coresight/coresight-priv.h | 9 +-
drivers/hwtracing/coresight/coresight-stm.c | 30 +-
drivers/hwtracing/coresight/coresight-sysfs.c | 391 ++++++++++++++
.../hwtracing/coresight/coresight-tmc-core.c | 2 +-
.../hwtracing/coresight/coresight-tmc-etf.c | 46 +-
.../hwtracing/coresight/coresight-tmc-etr.c | 33 +-
drivers/hwtracing/coresight/coresight-tmc.h | 2 -
drivers/hwtracing/coresight/coresight-tpda.c | 13 +-
drivers/hwtracing/coresight/coresight-tpiu.c | 14 +-
drivers/hwtracing/coresight/ultrasoc-smb.c | 22 +-
drivers/hwtracing/coresight/ultrasoc-smb.h | 2 -
include/linux/coresight.h | 146 ++----
20 files changed, 606 insertions(+), 685 deletions(-)

--
2.34.1



2024-01-29 15:56:13

by James Clark

[permalink] [raw]
Subject: [PATCH v2 01/12] coresight: Fix issue where a source device's helpers aren't disabled

The linked commit reverts the change that accidentally used some sysfs
enable/disable functions from Perf which broke the refcounting, but it
also removes the fact that the sysfs disable function disabled the
helpers.

Add a new wrapper function that does both which is used by both Perf and
sysfs, and label the sysfs disable function appropriately. The naming of
all of the functions will be tidied up later to avoid this happening
again.

Fixes: 287e82cf69aa ("coresight: Fix crash when Perf and sysfs modes are used concurrently")
Signed-off-by: James Clark <[email protected]>
---
drivers/hwtracing/coresight/coresight-core.c | 30 ++++++++++++++-----
.../hwtracing/coresight/coresight-etm-perf.c | 2 +-
drivers/hwtracing/coresight/coresight-priv.h | 2 +-
3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index d7f0e231feb9..965bb6d4e1bf 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -441,8 +441,26 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
}
}

+/*
+ * Helper function to call source_ops(csdev)->disable and also disable the
+ * helpers.
+ *
+ * There is an imbalance between coresight_enable_path() and
+ * coresight_disable_path(). Enabling also enables the source's helpers as part
+ * of the path, but disabling always skips the first item in the path (which is
+ * the source), so sources and their helpers don't get disabled as part of that
+ * function and we need the extra step here.
+ */
+void coresight_disable_source(struct coresight_device *csdev, void *data)
+{
+ if (source_ops(csdev)->disable)
+ source_ops(csdev)->disable(csdev, data);
+ coresight_disable_helpers(csdev);
+}
+EXPORT_SYMBOL_GPL(coresight_disable_source);
+
/**
- * coresight_disable_source - Drop the reference count by 1 and disable
+ * coresight_disable_source_sysfs - Drop the reference count by 1 and disable
* the device if there are no users left.
*
* @csdev: The coresight device to disable
@@ -451,17 +469,15 @@ static void coresight_disable_helpers(struct coresight_device *csdev)
*
* Returns true if the device has been disabled.
*/
-bool coresight_disable_source(struct coresight_device *csdev, void *data)
+static bool coresight_disable_source_sysfs(struct coresight_device *csdev,
+ void *data)
{
if (atomic_dec_return(&csdev->refcnt) == 0) {
- if (source_ops(csdev)->disable)
- source_ops(csdev)->disable(csdev, data);
- coresight_disable_helpers(csdev);
+ coresight_disable_source(csdev, data);
csdev->enable = false;
}
return !csdev->enable;
}
-EXPORT_SYMBOL_GPL(coresight_disable_source);

/*
* coresight_disable_path_from : Disable components in the given path beyond
@@ -1204,7 +1220,7 @@ void coresight_disable(struct coresight_device *csdev)
if (ret)
goto out;

- if (!csdev->enable || !coresight_disable_source(csdev, NULL))
+ if (!csdev->enable || !coresight_disable_source_sysfs(csdev, NULL))
goto out;

switch (csdev->subtype.source_subtype) {
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index a52cfcce25d6..c0c60e6a1703 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -589,7 +589,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
return;

/* stop tracer */
- source_ops(csdev)->disable(csdev, event);
+ coresight_disable_source(csdev, event);

/* tell the core */
event->hw.state = PERF_HES_STOPPED;
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 767076e07970..30c051055e54 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -233,6 +233,6 @@ void coresight_set_percpu_sink(int cpu, struct coresight_device *csdev);
struct coresight_device *coresight_get_percpu_sink(int cpu);
int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
void *data);
-bool coresight_disable_source(struct coresight_device *csdev, void *data);
+void coresight_disable_source(struct coresight_device *csdev, void *data);

#endif
--
2.34.1


2024-01-29 15:57:04

by James Clark

[permalink] [raw]
Subject: [PATCH v2 04/12] coresight: Move mode to struct coresight_device

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.

Signed-off-by: James Clark <[email protected]>
---
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 +--
.../coresight/coresight-etm4x-core.c | 16 +++++-------
drivers/hwtracing/coresight/coresight-etm4x.h | 1 -
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 +++++
14 files changed, 69 insertions(+), 76 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-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index ce1995a2827f..ebf55b1f67d7 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -841,9 +841,8 @@ static int etm4_enable(struct coresight_device *csdev, struct perf_event *event,
{
int ret;
u32 val;
- struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);

- 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)
@@ -862,7 +861,7 @@ static int etm4_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(&csdev->mode, CS_MODE_DISABLED);

return ret;
}
@@ -1004,14 +1003,13 @@ static void etm4_disable(struct coresight_device *csdev,
struct perf_event *event)
{
enum cs_mode mode;
- struct etmv4_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:
@@ -1025,7 +1023,7 @@ static void etm4_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 etm4_source_ops = {
@@ -1663,7 +1661,7 @@ static int etm4_starting_cpu(unsigned int cpu)
if (!etmdrvdata[cpu]->os_unlock)
etm4_os_unlock(etmdrvdata[cpu]);

- if (local_read(&etmdrvdata[cpu]->mode))
+ if (local_read(&etmdrvdata[cpu]->csdev->mode))
etm4_enable_hw(etmdrvdata[cpu]);
spin_unlock(&etmdrvdata[cpu]->spinlock);
return 0;
@@ -1675,7 +1673,7 @@ static int etm4_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))
etm4_disable_hw(etmdrvdata[cpu]);
spin_unlock(&etmdrvdata[cpu]->spinlock);
return 0;
@@ -1833,7 +1831,7 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
* Save and restore the ETM Trace registers only if
* the ETM is active.
*/
- if (local_read(&drvdata->mode) && drvdata->save_state)
+ if (local_read(&drvdata->csdev->mode) && drvdata->save_state)
ret = __etm4_cpu_save(drvdata);
return ret;
}
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index da17b6c49b0f..9ea678bc2e8e 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -1016,7 +1016,6 @@ struct etmv4_drvdata {
void __iomem *base;
struct coresight_device *csdev;
spinlock_t spinlock;
- local_t mode;
int cpu;
u8 arch;
u8 nr_pe;
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.
* @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;
--
2.34.1


2024-01-29 15:58:00

by James Clark

[permalink] [raw]
Subject: [PATCH v2 07/12] coresight: Remove atomic type from refcnt

Refcnt is only ever accessed from either inside the coresight_mutex, or
the device's spinlock, making the atomic type and atomic_dec_return()
calls confusing and unnecessary. The only point of synchronisation
outside of these two types of locks is already done with a compare and
swap on 'mode', which a comment has been added for.

There was one instance of refcnt being used outside of a lock in TPIU,
but that can easily be fixed by making it the same as all the other
devices and adding a spinlock. Potentially in the future all the
refcounting and locking can be moved up into the core code, and all the
mostly duplicate code from the individual devices can be removed.

Signed-off-by: James Clark <[email protected]>
---
drivers/hwtracing/coresight/coresight-etb10.c | 11 +++++-----
drivers/hwtracing/coresight/coresight-sysfs.c | 7 ++++---
.../hwtracing/coresight/coresight-tmc-etf.c | 20 ++++++++++---------
.../hwtracing/coresight/coresight-tmc-etr.c | 13 ++++++------
drivers/hwtracing/coresight/coresight-tpda.c | 7 ++++---
drivers/hwtracing/coresight/coresight-tpiu.c | 14 +++++++++++--
drivers/hwtracing/coresight/ultrasoc-smb.c | 9 +++++----
include/linux/coresight.h | 13 +++++++++---
8 files changed, 59 insertions(+), 35 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index 08e5bba862db..5f2bb95955b7 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -161,7 +161,7 @@ static int etb_enable_sysfs(struct coresight_device *csdev)
local_set(&csdev->mode, CS_MODE_SYSFS);
}

- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
out:
spin_unlock_irqrestore(&drvdata->spinlock, flags);
return ret;
@@ -197,7 +197,7 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
* use for this session.
*/
if (drvdata->pid == pid) {
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
goto out;
}

@@ -215,7 +215,7 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data)
/* Associate with monitored process. */
drvdata->pid = pid;
local_set(&drvdata->csdev->mode, CS_MODE_PERF);
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
}

out:
@@ -354,7 +354,8 @@ static int etb_disable(struct coresight_device *csdev)

spin_lock_irqsave(&drvdata->spinlock, flags);

- if (atomic_dec_return(&csdev->refcnt)) {
+ csdev->refcnt--;
+ if (csdev->refcnt) {
spin_unlock_irqrestore(&drvdata->spinlock, flags);
return -EBUSY;
}
@@ -445,7 +446,7 @@ static unsigned long etb_update_buffer(struct coresight_device *csdev,
spin_lock_irqsave(&drvdata->spinlock, flags);

/* Don't do anything if another tracer is using this sink */
- if (atomic_read(&csdev->refcnt) != 1)
+ if (csdev->refcnt != 1)
goto out;

__etb_disable_hw(drvdata);
diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
index 92cdf8139f23..5992f2c2200a 100644
--- a/drivers/hwtracing/coresight/coresight-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-sysfs.c
@@ -68,7 +68,7 @@ static int coresight_enable_source_sysfs(struct coresight_device *csdev,
return ret;
}

- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;

return 0;
}
@@ -90,7 +90,8 @@ static bool coresight_disable_source_sysfs(struct coresight_device *csdev,
if (local_read(&csdev->mode) != CS_MODE_SYSFS)
return false;

- if (atomic_dec_return(&csdev->refcnt) == 0) {
+ csdev->refcnt--;
+ if (csdev->refcnt == 0) {
coresight_disable_source(csdev, data);
return true;
}
@@ -190,7 +191,7 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
* source is already enabled.
*/
if (subtype == CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE)
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
goto out;
}

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 2a7e516052a2..f3281c958a57 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -206,7 +206,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
* touched.
*/
if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
goto out;
}

@@ -229,7 +229,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
ret = tmc_etb_enable_hw(drvdata);
if (!ret) {
local_set(&csdev->mode, CS_MODE_SYSFS);
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
} else {
/* Free up the buffer if we failed to enable */
used = false;
@@ -284,7 +284,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
* use for this session.
*/
if (drvdata->pid == pid) {
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
break;
}

@@ -293,7 +293,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data)
/* Associate with monitored process. */
drvdata->pid = pid;
local_set(&csdev->mode, CS_MODE_PERF);
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
}
} while (0);
spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -338,7 +338,8 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev)
return -EBUSY;
}

- if (atomic_dec_return(&csdev->refcnt)) {
+ csdev->refcnt--;
+ if (csdev->refcnt) {
spin_unlock_irqrestore(&drvdata->spinlock, flags);
return -EBUSY;
}
@@ -371,7 +372,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
return -EBUSY;
}

- if (atomic_read(&csdev->refcnt) == 0) {
+ if (csdev->refcnt == 0) {
ret = tmc_etf_enable_hw(drvdata);
if (!ret) {
local_set(&csdev->mode, CS_MODE_SYSFS);
@@ -379,7 +380,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
}
}
if (!ret)
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
spin_unlock_irqrestore(&drvdata->spinlock, flags);

if (first_enable)
@@ -401,7 +402,8 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
return;
}

- if (atomic_dec_return(&csdev->refcnt) == 0) {
+ csdev->refcnt--;
+ if (csdev->refcnt == 0) {
tmc_etf_disable_hw(drvdata);
local_set(&csdev->mode, CS_MODE_DISABLED);
last_disable = true;
@@ -489,7 +491,7 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
spin_lock_irqsave(&drvdata->spinlock, flags);

/* Don't do anything if another tracer is using this sink */
- if (atomic_read(&csdev->refcnt) != 1)
+ if (csdev->refcnt != 1)
goto out;

CS_UNLOCK(drvdata->base);
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 3dc989d4fcab..88a0fc375b4d 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1231,14 +1231,14 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
* touched, even if the buffer size has changed.
*/
if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
goto out;
}

ret = tmc_etr_enable_hw(drvdata, sysfs_buf);
if (!ret) {
local_set(&csdev->mode, CS_MODE_SYSFS);
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
}

out:
@@ -1564,7 +1564,7 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
spin_lock_irqsave(&drvdata->spinlock, flags);

/* Don't do anything if another tracer is using this sink */
- if (atomic_read(&csdev->refcnt) != 1) {
+ if (csdev->refcnt != 1) {
spin_unlock_irqrestore(&drvdata->spinlock, flags);
goto out;
}
@@ -1676,7 +1676,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
* use for this session.
*/
if (drvdata->pid == pid) {
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
goto unlock_out;
}

@@ -1686,7 +1686,7 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data)
drvdata->pid = pid;
local_set(&csdev->mode, CS_MODE_PERF);
drvdata->perf_buf = etr_perf->etr_buf;
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
}

unlock_out:
@@ -1719,7 +1719,8 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev)
return -EBUSY;
}

- if (atomic_dec_return(&csdev->refcnt)) {
+ csdev->refcnt--;
+ if (csdev->refcnt) {
spin_unlock_irqrestore(&drvdata->spinlock, flags);
return -EBUSY;
}
diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index 65c70995ab00..135a89654c4c 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -152,7 +152,8 @@ static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
* Only do pre-port enable for first port that calls enable when the
* device's main refcount is still 0
*/
- if (!atomic_read(&drvdata->csdev->refcnt))
+ lockdep_assert_held(&drvdata->spinlock);
+ if (!drvdata->csdev->refcnt)
tpda_enable_pre_port(drvdata);

ret = tpda_enable_port(drvdata, port);
@@ -173,7 +174,7 @@ static int tpda_enable(struct coresight_device *csdev,
ret = __tpda_enable(drvdata, in->dest_port);
if (!ret) {
atomic_inc(&in->dest_refcnt);
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port);
}
}
@@ -204,7 +205,7 @@ static void tpda_disable(struct coresight_device *csdev,
spin_lock(&drvdata->spinlock);
if (atomic_dec_return(&in->dest_refcnt) == 0) {
__tpda_disable(drvdata, in->dest_port);
- atomic_dec(&csdev->refcnt);
+ csdev->refcnt--;
}
spin_unlock(&drvdata->spinlock);

diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
index 59eac93fd6bb..c23a6b9b41fe 100644
--- a/drivers/hwtracing/coresight/coresight-tpiu.c
+++ b/drivers/hwtracing/coresight/coresight-tpiu.c
@@ -58,6 +58,7 @@ struct tpiu_drvdata {
void __iomem *base;
struct clk *atclk;
struct coresight_device *csdev;
+ spinlock_t spinlock;
};

static void tpiu_enable_hw(struct csdev_access *csa)
@@ -72,8 +73,11 @@ static void tpiu_enable_hw(struct csdev_access *csa)
static int tpiu_enable(struct coresight_device *csdev, enum cs_mode mode,
void *__unused)
{
+ struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+ guard(spinlock)(&drvdata->spinlock);
tpiu_enable_hw(&csdev->access);
- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
dev_dbg(&csdev->dev, "TPIU enabled\n");
return 0;
}
@@ -96,7 +100,11 @@ static void tpiu_disable_hw(struct csdev_access *csa)

static int tpiu_disable(struct coresight_device *csdev)
{
- if (atomic_dec_return(&csdev->refcnt))
+ struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+ guard(spinlock)(&drvdata->spinlock);
+ csdev->refcnt--;
+ if (csdev->refcnt)
return -EBUSY;

tpiu_disable_hw(&csdev->access);
@@ -132,6 +140,8 @@ static int tpiu_probe(struct amba_device *adev, const struct amba_id *id)
if (!drvdata)
return -ENOMEM;

+ spin_lock_init(&drvdata->spinlock);
+
drvdata->atclk = devm_clk_get(&adev->dev, "atclk"); /* optional */
if (!IS_ERR(drvdata->atclk)) {
ret = clk_prepare_enable(drvdata->atclk);
diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
index 1a8c64645b92..1146584a1745 100644
--- a/drivers/hwtracing/coresight/ultrasoc-smb.c
+++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
@@ -103,7 +103,7 @@ static int smb_open(struct inode *inode, struct file *file)
if (drvdata->reading)
return -EBUSY;

- if (atomic_read(&drvdata->csdev->refcnt))
+ if (drvdata->csdev->refcnt)
return -EBUSY;

smb_update_data_size(drvdata);
@@ -271,7 +271,7 @@ static int smb_enable(struct coresight_device *csdev, enum cs_mode mode,
if (ret)
return ret;

- atomic_inc(&csdev->refcnt);
+ csdev->refcnt++;
dev_dbg(&csdev->dev, "Ultrasoc SMB enabled\n");

return ret;
@@ -286,7 +286,8 @@ static int smb_disable(struct coresight_device *csdev)
if (drvdata->reading)
return -EBUSY;

- if (atomic_dec_return(&csdev->refcnt))
+ csdev->refcnt--;
+ if (csdev->refcnt)
return -EBUSY;

/* Complain if we (somehow) got out of sync */
@@ -381,7 +382,7 @@ static unsigned long smb_update_buffer(struct coresight_device *csdev,
guard(spinlock)(&drvdata->spinlock);

/* Don't do anything if another tracer is using this sink. */
- if (atomic_read(&csdev->refcnt) != 1)
+ if (csdev->refcnt != 1)
return 0;

smb_disable_hw(drvdata);
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index e8dd0df98881..4400d554a16b 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -230,8 +230,15 @@ struct coresight_sysfs_link {
* 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.
- * @refcnt: keep track of what is in use.
+ * 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.
+ * @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
+ * spinlock.
* @orphan: true if the component has connections that haven't been linked.
* @sysfs_sink_activated: 'true' when a sink has been selected for use via sysfs
* by writing a 1 to the 'enable_sink' file. A sink can be
@@ -257,7 +264,7 @@ struct coresight_device {
struct csdev_access access;
struct device dev;
local_t mode;
- atomic_t refcnt;
+ int refcnt;
bool orphan;
/* sink specific fields */
bool sysfs_sink_activated;
--
2.34.1


2024-01-29 15:58:31

by James Clark

[permalink] [raw]
Subject: [PATCH v2 09/12] coresight: Add explicit member initializers to coresight_dev_type

These could potentially become wrong silently if the enum is changed,
so explicitly initialize them.

Signed-off-by: James Clark <[email protected]>
---
drivers/hwtracing/coresight/coresight-sysfs.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
index 5992f2c2200a..fa52297c73d2 100644
--- a/drivers/hwtracing/coresight/coresight-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-sysfs.c
@@ -378,22 +378,22 @@ static struct attribute *coresight_source_attrs[] = {
ATTRIBUTE_GROUPS(coresight_source);

struct device_type coresight_dev_type[] = {
- {
+ [CORESIGHT_DEV_TYPE_SINK] = {
.name = "sink",
.groups = coresight_sink_groups,
},
- {
+ [CORESIGHT_DEV_TYPE_LINK] = {
.name = "link",
},
- {
+ [CORESIGHT_DEV_TYPE_LINKSINK] = {
.name = "linksink",
.groups = coresight_sink_groups,
},
- {
+ [CORESIGHT_DEV_TYPE_SOURCE] = {
.name = "source",
.groups = coresight_source_groups,
},
- {
+ [CORESIGHT_DEV_TYPE_HELPER] = {
.name = "helper",
}
};
--
2.34.1


2024-02-12 11:28:10

by Suzuki K Poulose

[permalink] [raw]
Subject: Re: [PATCH v2 00/12] coresight: Separate sysfs and Perf usage and some other cleanups

On Mon, 29 Jan 2024 15:40:31 +0000, James Clark wrote:
> Changes since V1:
>
> * Clarify further "the selected sink" in _coresight_build_path()
> * Move etm4x's mode to coresight device which was missing from V1
> * Use explicit initialisers in coresight_dev_type
> * Create functions for handling mode changes
>
> [...]

Applied, thanks!

[01/12] coresight: Fix issue where a source device's helpers aren't disabled
https://git.kernel.org/coresight/c/f68bbe4dcfa3
[02/12] coresight: Make language around "activated" sinks consistent
https://git.kernel.org/coresight/c/a0fef3f05cf3
[03/12] coresight: Remove ops callback checks
https://git.kernel.org/coresight/c/a11ebe138b8d
[04/12] coresight: Move mode to struct coresight_device
https://git.kernel.org/coresight/c/9cae77cf23e3
[05/12] coresight: Remove the 'enable' field.
https://git.kernel.org/coresight/c/d5e83f97eb56
[06/12] coresight: Move all sysfs code to sysfs file
https://git.kernel.org/coresight/c/1f5149c7751c
[07/12] coresight: Remove atomic type from refcnt
https://git.kernel.org/coresight/c/4545b38ef004
[08/12] coresight: Remove unused stubs
https://git.kernel.org/coresight/c/053ad9ad1d13
[09/12] coresight: Add explicit member initializers to coresight_dev_type
https://git.kernel.org/coresight/c/812265e26ed3
[10/12] coresight: Add helper for atomically taking the device
https://git.kernel.org/coresight/c/d724f65218b9
[11/12] coresight: Add a helper for getting csdev->mode
https://git.kernel.org/coresight/c/c95c2733e5fe
[12/12] coresight: Add helper for setting csdev->mode
https://git.kernel.org/coresight/c/bcaabb95f0c9

Best regards,
--
Suzuki K Poulose <[email protected]>