[PATCH 5/8] coresight: Remove the 'enable' field.
Suzuki K Poulose
suzuki.poulose at arm.com
Mon Jan 8 06:42:01 PST 2024
Hi James
+Cc: Tao Zhang <quic_taozha at quicinc.com>
+Cc: Mao Jinlong <quic_jinlmao at quicinc.com>
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 at arm.com>
> ---
> 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;
More information about the linux-arm-kernel
mailing list