| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <02acf049-90ad-644c-c2aa-381b8b12e944@arm.com>
Date: Wed, 24 Jan 2024 11:10:11 +0000
From: James Clark <james.clark@....com>
To: Suzuki K Poulose <suzuki.poulose@....com>, coresight@...ts.linaro.org
Cc: Mike Leach <mike.leach@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH 2/8] coresight: Make language around "activated" sinks
consistent
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@....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 */
>
Powered by blists - more mailing lists