Re: [PATCH v5 13/13] coresight: Fix CTI module refcount leak by making it a helper device
From: Suzuki K Poulose
Date: Mon Apr 24 2023 - 06:43:22 EST
On 04/04/2023 16:51, James Clark wrote:
The CTI module has some hard coded refcounting code that has a leak.
For example running perf and then trying to unload it fails:
perf record -e cs_etm// -a -- ls
rmmod coresight_cti
rmmod: ERROR: Module coresight_cti is in use
The coresight core already handles references of devices in use, so by
making CTI a normal helper device, we get working refcounting for free.
Signed-off-by: James Clark <james.clark@xxxxxxx>
---
drivers/hwtracing/coresight/coresight-core.c | 104 ++++++------------
.../hwtracing/coresight/coresight-cti-core.c | 52 +++++----
.../hwtracing/coresight/coresight-cti-sysfs.c | 4 +-
drivers/hwtracing/coresight/coresight-cti.h | 4 +-
drivers/hwtracing/coresight/coresight-priv.h | 4 +-
drivers/hwtracing/coresight/coresight-sysfs.c | 4 +
include/linux/coresight.h | 30 +----
7 files changed, 75 insertions(+), 127 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 16689fe4ba98..2af416bba983 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -236,60 +236,44 @@ void coresight_disclaim_device(struct coresight_device *csdev)
}
EXPORT_SYMBOL_GPL(coresight_disclaim_device);
-/* enable or disable an associated CTI device of the supplied CS device */
-static int
-coresight_control_assoc_ectdev(struct coresight_device *csdev, bool enable)
+/*
+ * Add a helper as an output device. This function takes the @coresight_mutex
+ * because it's assumed that it's called from the helper device, outside of the
+ * core code where the mutex would already be held. Don't add new calls to this
+ * from inside the core code, instead try to add the new helper to the DT and
+ * ACPI where it will be picked up and linked automatically.
+ */
+void coresight_add_helper(struct coresight_device *csdev,
+ struct coresight_device *helper)
{
- int ect_ret = 0;
- struct coresight_device *ect_csdev = csdev->ect_dev;
- struct module *mod;
+ int i;
+ struct coresight_connection conn = {};
+ struct coresight_connection *new_conn;
- if (!ect_csdev)
- return 0;
- if ((!ect_ops(ect_csdev)->enable) || (!ect_ops(ect_csdev)->disable))
- return 0;
+ mutex_lock(&coresight_mutex);
+ conn.dest_fwnode = fwnode_handle_get(dev_fwnode(&helper->dev));
+ conn.dest_dev = helper;
+ conn.dest_port = conn.src_port = -1;
+ conn.src_dev = csdev;
- mod = ect_csdev->dev.parent->driver->owner;
- if (enable) {
- if (try_module_get(mod)) {
- ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);
- if (ect_ret) {
- module_put(mod);
- } else {
- get_device(ect_csdev->dev.parent);
- csdev->ect_enabled = true;
- }
- } else
- ect_ret = -ENODEV;
- } else {
- if (csdev->ect_enabled) {
- ect_ret = ect_ops(ect_csdev)->disable(ect_csdev);
- put_device(ect_csdev->dev.parent);
- module_put(mod);
- csdev->ect_enabled = false;
- }
- }
+ /*
+ * Check for duplicates because this is called every time a helper
+ * device is re-loaded. Existing connections will get re-linked
+ * automatically.
+ */
+ for (i = 0; i < csdev->pdata->nr_outconns; ++i)
+ if (csdev->pdata->out_conns[i]->dest_fwnode == conn.dest_fwnode)
+ goto unlock;
- /* output warning if ECT enable is preventing trace operation */
- if (ect_ret)
- dev_info(&csdev->dev, "Associated ECT device (%s) %s failed\n",
- dev_name(&ect_csdev->dev),
- enable ? "enable" : "disable");
- return ect_ret;
-}
+ new_conn =
+ coresight_add_out_conn(csdev->dev.parent, csdev->pdata, &conn);
ultra minor nit:
new_conn = coresight_add_out_conn(....,
.... );
+ if (!IS_ERR(new_conn))
+ coresight_add_in_conn(new_conn);
-/*
- * Set the associated ect / cti device while holding the coresight_mutex
- * to avoid a race with coresight_enable that may try to use this value.
- */
-void coresight_set_assoc_ectdev_mutex(struct coresight_device *csdev,
- struct coresight_device *ect_csdev)
-{
- mutex_lock(&coresight_mutex);
- csdev->ect_dev = ect_csdev;
+unlock:
mutex_unlock(&coresight_mutex);
}
-EXPORT_SYMBOL_GPL(coresight_set_assoc_ectdev_mutex);
+EXPORT_SYMBOL_GPL(coresight_add_helper);
static int coresight_enable_sink(struct coresight_device *csdev,
enum cs_mode mode, void *data)
@@ -303,12 +287,8 @@ static int coresight_enable_sink(struct coresight_device *csdev,
if (!sink_ops(csdev)->enable)
return -EINVAL;
- ret = coresight_control_assoc_ectdev(csdev, true);
- if (ret)
- return ret;
ret = sink_ops(csdev)->enable(csdev, mode, data);
if (ret) {
- coresight_control_assoc_ectdev(csdev, false);
return ret;
}
csdev->enable = true;
@@ -326,7 +306,6 @@ static void coresight_disable_sink(struct coresight_device *csdev)
ret = sink_ops(csdev)->disable(csdev);
if (ret)
return;
- coresight_control_assoc_ectdev(csdev, false);
csdev->enable = false;
}
@@ -351,17 +330,11 @@ static int coresight_enable_link(struct coresight_device *csdev,
return PTR_ERR(outconn);
if (link_ops(csdev)->enable) {
- ret = coresight_control_assoc_ectdev(csdev, true);
- if (!ret) {
- ret = link_ops(csdev)->enable(csdev, inconn, outconn);
- if (ret)
- coresight_control_assoc_ectdev(csdev, false);
- }
+ ret = link_ops(csdev)->enable(csdev, inconn, outconn);
+ if (!ret)
+ csdev->enable = true;
}
- if (!ret)
- csdev->enable = true;
-
return ret;
}
@@ -382,7 +355,6 @@ static void coresight_disable_link(struct coresight_device *csdev,
if (link_ops(csdev)->disable) {
link_ops(csdev)->disable(csdev, inconn, outconn);
- coresight_control_assoc_ectdev(csdev, false);
}
if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG) {
@@ -410,14 +382,9 @@ int coresight_enable_source(struct coresight_device *csdev, enum cs_mode mode,
if (!csdev->enable) {
if (source_ops(csdev)->enable) {
- ret = coresight_control_assoc_ectdev(csdev, true);
- if (ret)
- return ret;
ret = source_ops(csdev)->enable(csdev, data, mode);
- if (ret) {
- coresight_control_assoc_ectdev(csdev, false);
+ if (ret)
return ret;
- }
}
csdev->enable = true;
}
@@ -488,7 +455,6 @@ bool coresight_disable_source(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_control_assoc_ectdev(csdev, false);
coresight_disable_helpers(csdev);
csdev->enable = false;
}
You may also remove the "ect" from the
static struct device_type coresight_dev_type[]
Suzuki