Re: [PATCH 2/8] coresight: Make language around "activated" sinks consistent

[James Clark]

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@xxxxxxx>
>> ---
>>   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 */
>