On 08/01/2024 11:21, Suzuki K Poulose wrote:
Hi James,
On 12/12/2023 15:53, James Clark wrote:
Activated has the specific meaning of a sink that's selected for use by the user via sysfs. But comments in some code that's shared by Perf use the same word, so in those cases change them to just say "selected" instead. With selected implying either via Perf or "activated" via sysfs.
coresight_get_enabled_sink() doesn't actually get an enabled sink, it only gets an activated one, so change that too.
These changes look good to me. Please see a minor nit below.
And change the activated variable name to include "sysfs" so it can't be confused as a general status.
Signed-off-by: James Clark james.clark@arm.com
drivers/hwtracing/coresight/coresight-core.c | 51 ++++++++------------ drivers/hwtracing/coresight/coresight-priv.h | 2 - include/linux/coresight.h | 14 +++--- 3 files changed, 27 insertions(+), 40 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c index 965bb6d4e1bf..f79aa9ff9b64 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -500,7 +500,7 @@ static void coresight_disable_path_from(struct list_head *path, /* * ETF devices are tricky... They can be a link or a sink, * depending on how they are configured. If an ETF has been - * "activated" it will be configured as a sink, otherwise + * selected as a sink it will be configured as a sink, otherwise * go ahead with the link configuration. */ if (type == CORESIGHT_DEV_TYPE_LINKSINK) @@ -578,7 +578,7 @@ int coresight_enable_path(struct list_head *path, enum cs_mode mode, /* * ETF devices are tricky... They can be a link or a sink, * depending on how they are configured. If an ETF has been - * "activated" it will be configured as a sink, otherwise + * selected as a sink it will be configured as a sink, otherwise * go ahead with the link configuration. */ if (type == CORESIGHT_DEV_TYPE_LINKSINK) @@ -635,15 +635,21 @@ struct coresight_device *coresight_get_sink(struct list_head *path) return csdev; } +/**
- coresight_find_activated_sysfs_sink - returns the first sink
activated via
- sysfs using connection based search starting from the source
reference.
- @csdev: Coresight source device reference
- */
static struct coresight_device * -coresight_find_enabled_sink(struct coresight_device *csdev) +coresight_find_activated_sysfs_sink(struct coresight_device *csdev) { int i; struct coresight_device *sink = NULL; if ((csdev->type == CORESIGHT_DEV_TYPE_SINK || csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) && - csdev->activated) + csdev->sysfs_sink_activated) return csdev; /* @@ -654,7 +660,7 @@ coresight_find_enabled_sink(struct coresight_device *csdev) child_dev = csdev->pdata->out_conns[i]->dest_dev; if (child_dev) - sink = coresight_find_enabled_sink(child_dev); + sink = coresight_find_activated_sysfs_sink(child_dev); if (sink) return sink; } @@ -662,21 +668,6 @@ coresight_find_enabled_sink(struct coresight_device *csdev) return NULL; } -/**
- coresight_get_enabled_sink - returns the first enabled sink using
- connection based search starting from the source reference
- @source: Coresight source device reference
- */
-struct coresight_device * -coresight_get_enabled_sink(struct coresight_device *source) -{ - if (!source) - return NULL;
- return coresight_find_enabled_sink(source); -}
static int coresight_sink_by_id(struct device *dev, const void *data) { struct coresight_device *csdev = to_coresight_device(dev); @@ -810,11 +801,10 @@ static void coresight_drop_device(struct coresight_device *csdev) * @sink: The final sink we want in this path. * @path: The list to add devices to. *
- The tree of Coresight device is traversed until an activated sink is
- found. From there the sink is added to the list along with all the
- devices that led to that point - the end result is a list from source
- to sink. In that list the source is the first device and the sink the
- last one.
- The tree of Coresight device is traversed until the selected sink
is found.
minor nit: s/until the selected/until *a* selected/
There could be multiple sinks activated that can be reached from the source. But we choose the "closest" possible selected sink for a source. So, it may be better to drop "the".
For _coresight_build_path() this isn't strictly true because the sink is passed in as an argument so there is only one. The only function that behaves like that is coresight_find_activated_sysfs_sink() which already seems to be documented properly.
To clarify it I'll just change "until the selected" to "until @sink" is found.
- From there the sink is added to the list along with all the
devices that led
- to that point - the end result is a list from source to sink. In
that list
- the source is the first device and the sink the last one.
*/ static int _coresight_build_path(struct coresight_device *csdev, struct coresight_device *sink, @@ -824,7 +814,7 @@ static int _coresight_build_path(struct coresight_device *csdev, bool found = false; struct coresight_node *node; - /* An activated sink has been found. Enqueue the element */ + /* The selected sink has been found. Enqueue the element */
Similarly here.
Same as above.
Rest looks fine to me.
Suzuki
if (csdev == sink) goto out; @@ -1145,7 +1135,7 @@ int coresight_enable(struct coresight_device *csdev) goto out; } - sink = coresight_get_enabled_sink(csdev); + sink = coresight_find_activated_sysfs_sink(csdev); if (!sink) { ret = -EINVAL; goto out; @@ -1259,7 +1249,7 @@ static ssize_t enable_sink_show(struct device *dev, { struct coresight_device *csdev = to_coresight_device(dev); - return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->activated); + return scnprintf(buf, PAGE_SIZE, "%u\n", csdev->sysfs_sink_activated); } static ssize_t enable_sink_store(struct device *dev, @@ -1274,10 +1264,7 @@ static ssize_t enable_sink_store(struct device *dev, if (ret) return ret; - if (val) - csdev->activated = true; - else - csdev->activated = false; + csdev->sysfs_sink_activated = !!val; return size; diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index 30c051055e54..ced5be05a527 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -130,8 +130,6 @@ void coresight_disable_path(struct list_head *path); int coresight_enable_path(struct list_head *path, enum cs_mode mode, void *sink_data); struct coresight_device *coresight_get_sink(struct list_head *path); -struct coresight_device * -coresight_get_enabled_sink(struct coresight_device *source); struct coresight_device *coresight_get_sink_by_id(u32 id); struct coresight_device * coresight_find_default_sink(struct coresight_device *csdev); diff --git a/include/linux/coresight.h b/include/linux/coresight.h index a4cb7dd6ca23..65131bfbd904 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -229,10 +229,12 @@ struct coresight_sysfs_link { * @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.
- @activated: 'true' only if a _sink_ has been activated. A sink
can be
- * activated but not yet enabled. Enabling for a _sink_
- * happens when a source has been selected and a path is enabled
- * from source to that sink.
- @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
- * when a source has been selected and a path is enabled from
- * source to that sink. A sink can also become enabled but not
- * activated if it's used via Perf.
* @ea: Device attribute for sink representation under PMU directory. * @def_sink: cached reference to default sink found for this device. * @nr_links: number of sysfs links created to other components from this @@ -252,9 +254,9 @@ struct coresight_device { struct device dev; atomic_t refcnt; bool orphan; - bool enable; /* true only if configured as part of a path */ + bool enable; /* sink specific fields */ - bool activated; /* true only if a sink is part of a path */ + bool sysfs_sink_activated; struct dev_ext_attribute *ea; struct coresight_device *def_sink; /* sysfs links between components */