Re: [PATCH 5/8] coresight: Remove the 'enable' field.

From: Suzuki K Poulose
Date: Mon Jan 08 2024 - 09:42:14 EST

Next message: Alice Ryhl: "[PATCH v4 0/4] Additional CondVar methods needed by Rust Binder"
Previous message: Mark Pearson: "Re: [PATCH RFC] platform/x86: Lenovo sub directory"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi James

+Cc: Tao Zhang <quic_taozha@xxxxxxxxxxx>
+Cc: Mao Jinlong <quic_jinlmao@xxxxxxxxxxx>

On 12/12/2023 15:54, James Clark wrote:

'enable', which probably should have been 'enabled', is only ever read
in the core code in relation to controlling sources, and specifically
only sources in sysfs mode. Confusingly it's not labelled as such and
relying on it can be a source of bugs like the one fixed by
commit 078dbba3f0c9 ("coresight: Fix crash when Perf and sysfs modes are
used concurrently").

Most importantly, it can only be used when the coresight_mutex is held
which is only done when enabling and disabling paths in sysfs mode, and
not Perf mode.

I think we may be able to relax this a bit for the syfs. The sole reason
for holding the mutex is for the "build_path" (and get_enabled_sink)
where we need to make sure that no devices are removed/added. We hold
necessary refcount on the device and the module (via coresight_grab_device()). After which, we should be able to release the
mutex and perform the rest without it in coresight_enable()

So to prevent its usage spreading and leaking out to
other devices, remove it.

It's use is equivalent to checking if the mode is currently sysfs, as
due to the coresight_mutex lock, mode == CS_MODE_SYSFS can only become
true or untrue when that lock is held, and when mode == CS_MODE_SYSFS
the device is both enabled and in sysfs mode.

All of the above makes sense and looks good to me.

The one place it was used outside of the core code is in TPDA, but that
pattern is more appropriately represented using refcounts inside the
device's own spinlock.

But, I think we can clean up this code a bit more better. See below.

Signed-off-by: James Clark <james.clark@xxxxxxx>
---
drivers/hwtracing/coresight/coresight-core.c | 86 +++++++-------------
drivers/hwtracing/coresight/coresight-tpda.c | 12 ++-
include/linux/coresight.h | 2 -
3 files changed, 38 insertions(+), 62 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index ab226441e5f4..1d0bd1586590 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -279,29 +279,18 @@ EXPORT_SYMBOL_GPL(coresight_add_helper);
static int coresight_enable_sink(struct coresight_device *csdev,
enum cs_mode mode, void *data)
{
- int ret = sink_ops(csdev)->enable(csdev, mode, data);
-
- if (ret)
- return ret;
-
- csdev->enable = true;
-
- return 0;
+ return sink_ops(csdev)->enable(csdev, mode, data);
}
static void coresight_disable_sink(struct coresight_device *csdev)
{
- int ret = sink_ops(csdev)->disable(csdev);
- if (ret)
- return;
- csdev->enable = false;
+ sink_ops(csdev)->disable(csdev);
}
static int coresight_enable_link(struct coresight_device *csdev,
struct coresight_device *parent,
struct coresight_device *child)
{
- int ret = 0;
int link_subtype;
struct coresight_connection *inconn, *outconn;
@@ -317,19 +306,13 @@ static int coresight_enable_link(struct coresight_device *csdev,
if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT && IS_ERR(outconn))
return PTR_ERR(outconn);
- ret = link_ops(csdev)->enable(csdev, inconn, outconn);
- if (!ret)
- csdev->enable = true;
-
- return ret;
+ return link_ops(csdev)->enable(csdev, inconn, outconn);
}
static void coresight_disable_link(struct coresight_device *csdev,
struct coresight_device *parent,
struct coresight_device *child)
{
- int i;
- int link_subtype;
struct coresight_connection *inconn, *outconn;
if (!parent || !child)
@@ -337,26 +320,8 @@ static void coresight_disable_link(struct coresight_device *csdev,
inconn = coresight_find_out_connection(parent, csdev);
outconn = coresight_find_out_connection(csdev, child);
- link_subtype = csdev->subtype.link_subtype;
link_ops(csdev)->disable(csdev, inconn, outconn);
-
- if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG) {
- for (i = 0; i < csdev->pdata->nr_inconns; i++)
- if (atomic_read(&csdev->pdata->in_conns[i]->dest_refcnt) !=
- 0)
- return;
- } else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT) {
- for (i = 0; i < csdev->pdata->nr_outconns; i++)
- if (atomic_read(&csdev->pdata->out_conns[i]->src_refcnt) !=
- 0)
- return;
- } else {
- if (atomic_read(&csdev->refcnt) != 0)
- return;
- }
-
- csdev->enable = false;
}
int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
@@ -364,11 +329,16 @@ int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
{
int ret;
- if (!csdev->enable) {
+ /*
+ * Comparison with CS_MODE_SYSFS works without taking any device
+ * specific spinlock because the truthyness of that comparison can only
+ * change with coresight_mutex held, which we already have here.
+ */
+ lockdep_assert_held(&coresight_mutex);
+ if (local_read(&csdev->mode) != CS_MODE_SYSFS) {
ret = source_ops(csdev)->enable(csdev, data, mode);
if (ret)
return ret;
- csdev->enable = true;
}
atomic_inc(&csdev->refcnt);
@@ -385,22 +355,12 @@ static bool coresight_is_helper(struct coresight_device *csdev)
static int coresight_enable_helper(struct coresight_device *csdev,
enum cs_mode mode, void *data)
{
- int ret = helper_ops(csdev)->enable(csdev, mode, data);
-
- if (ret)
- return ret;
-
- csdev->enable = true;
- return 0;
+ return helper_ops(csdev)->enable(csdev, mode, data);
}
static void coresight_disable_helper(struct coresight_device *csdev)
{
- int ret = helper_ops(csdev)->disable(csdev, NULL);
-
- if (ret)
- return;
- csdev->enable = false;
+ helper_ops(csdev)->disable(csdev, NULL);
}
static void coresight_disable_helpers(struct coresight_device *csdev)
@@ -445,11 +405,15 @@ EXPORT_SYMBOL_GPL(coresight_disable_source);
static bool coresight_disable_source_sysfs(struct coresight_device *csdev,
void *data)
{
+ lockdep_assert_held(&coresight_mutex);
+ if (local_read(&csdev->mode) != CS_MODE_SYSFS)
+ return false;
+
if (atomic_dec_return(&csdev->refcnt) == 0) {
coresight_disable_source(csdev, data);
- csdev->enable = false;
+ return true;
}
- return !csdev->enable;
+ return false;
}
/*
@@ -1097,7 +1061,13 @@ int coresight_enable(struct coresight_device *csdev)
if (ret)
goto out;
- if (csdev->enable) {
+ /*
+ * mode == SYSFS implies that it's already enabled. Don't look at the
+ * refcount to determine this because we don't claim the source until
+ * coresight_enable_source() so can still race with Perf mode which
+ * doesn't hold coresight_mutex.
+ */
+ if (local_read(&csdev->mode) == CS_MODE_SYSFS) {
/*
* There could be multiple applications driving the software
* source. So keep the refcount for each such user when the
@@ -1183,7 +1153,7 @@ void coresight_disable(struct coresight_device *csdev)
if (ret)
goto out;
- if (!csdev->enable || !coresight_disable_source_sysfs(csdev, NULL))
+ if (!coresight_disable_source_sysfs(csdev, NULL))
goto out;
switch (csdev->subtype.source_subtype) {
@@ -1249,7 +1219,9 @@ static ssize_t enable_source_show(struct device *dev,
{
struct coresight_device *csdev = to_coresight_device(dev);
- return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->enable);
+ guard(mutex)(&coresight_mutex);
+ return scnprintf(buf, PAGE_SIZE, "%u\n",
+ local_read(&csdev->mode) == CS_MODE_SYSFS);
}
static ssize_t enable_source_store(struct device *dev,
diff --git a/drivers/hwtracing/coresight/coresight-tpda.c b/drivers/hwtracing/coresight/coresight-tpda.c
index 5f82737c37bb..65c70995ab00 100644
--- a/drivers/hwtracing/coresight/coresight-tpda.c
+++ b/drivers/hwtracing/coresight/coresight-tpda.c
@@ -148,7 +148,11 @@ static int __tpda_enable(struct tpda_drvdata *drvdata, int port)
CS_UNLOCK(drvdata->base);
- if (!drvdata->csdev->enable)
+ /*
+ * 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))
tpda_enable_pre_port(drvdata);

Relying on the "csdev->enable" to do pre-port configuration looks like
a complete hack to me. This could have been performed once and for all
during the probe time, at say, tpda_init_default_data(). That value is
(drvdata->atid) never updated and need not be re-written unless the
value is lost during a power idle.

Mao, Tao, are you able to confirm this ? If that is the case, we don't need this csdev->refcnt.

ret = tpda_enable_port(drvdata, port);
@@ -169,6 +173,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);
dev_dbg(drvdata->dev, "TPDA inport %d enabled.\n", in->dest_port);
}
}
@@ -197,9 +202,10 @@ static void tpda_disable(struct coresight_device *csdev,
struct tpda_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
spin_lock(&drvdata->spinlock);
- if (atomic_dec_return(&in->dest_refcnt) == 0)
+ if (atomic_dec_return(&in->dest_refcnt) == 0) {
__tpda_disable(drvdata, in->dest_port);
-
+ atomic_dec(&csdev->refcnt);

If we need this, then: This should be performed outside the if () isn't it ?

e.g.,

Operation: in_conn->refcnt csdev->refcnt
port_enable : port0 0 - > 1 0 - > 1
port_enable : port0 1 - > 2 1 - > 2
port_disable: port0 2 - > 1 2 (no change)
port_disable: port0 1 - > 0 2 - > 1

As you can see the csdev->refcnt skipped a dec.

Suzuki

+ }
spin_unlock(&drvdata->spinlock);
dev_dbg(drvdata->dev, "TPDA inport %d disabled\n", in->dest_port);
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index ba817f563ff7..46e6667f72ce 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -233,7 +233,6 @@ struct coresight_sysfs_link {
* 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.
* @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
* activated but not yet enabled. Enabling for a _sink_ happens
@@ -260,7 +259,6 @@ struct coresight_device {
local_t mode;
atomic_t refcnt;
bool orphan;
- bool enable;
/* sink specific fields */
bool sysfs_sink_activated;
struct dev_ext_attribute *ea;

Next message: Alice Ryhl: "[PATCH v4 0/4] Additional CondVar methods needed by Rust Binder"
Previous message: Mark Pearson: "Re: [PATCH RFC] platform/x86: Lenovo sub directory"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]