Messages in this thread Patch in this message | | | Date | Mon, 24 Apr 2023 11:43:11 +0100 | Subject | Re: [PATCH v5 13/13] coresight: Fix CTI module refcount leak by making it a helper device | From | Suzuki K Poulose <> |
| |
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; > - } > - } 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
| |