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@arm.com
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;}} elseect_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;coresight_control_assoc_ectdev(csdev, false);@@ -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;
}@@ -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);
You may also remove the "ect" from the
static struct device_type coresight_dev_type[]
Suzuki