lkml.org 
[lkml]   [2021]   [Apr]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] coresight: add support to enable more coresight paths
On 2021-04-15 17:49, Suzuki K Poulose wrote:
> Hi
>
> On 15/04/2021 10:33, Tao Zhang wrote:
>> Current coresight implementation only supports enabling source
>> ETMs or STM. This patch adds support to enable more kinds of
>> coresight source to sink paths. We build a path from source to
>> sink when any source is enabled and store it in a list. When the
>> source is disabled, we fetch the corresponding path from the list
>> and decrement the refcount on each device in the path. The device
>> is disabled if the refcount reaches zero. Don't store path to
>> coresight data structure of source to avoid unnecessary change to
>> ABI.
>> Since some targets may have coresight sources other than STM and
>> ETMs, we need to add this change to support these coresight
>> devices.
>
> While I am not against the patch, I would like to see why this change
> is
> needed. Which compnents are we talking about , other than STM / ETM ?
> Where is the "device" support code ? Without a legitimate user, we
> cannot add this change in.
>
> Some other comments below.
>
We will upload coresight source device driver code in addition to
STM/ETM in the future. This patch is only our first step. e.g, this
patch can support to build a path from TPDM(a kind of coresight source
device on Qualcomm target) to sink. At the same time, this patch also
can support coresight source devices other than STM/ETM on the other
targets.
>>
>> Signed-off-by: Satyajit Desai <sadesai@codeaurora.org>
>> Signed-off-by: Rama Aparna Mallavarapu <aparnam@codeaurora.org>
>> Signed-off-by: Mulu He <muluhe@codeaurora.org>
>> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
>> Signed-off-by: Tao Zhang <taozha@codeaurora.org>
>> ---
>> drivers/hwtracing/coresight/coresight-core.c | 101
>> +++++++++++++++------------
>> 1 file changed, 56 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c
>> b/drivers/hwtracing/coresight/coresight-core.c
>> index 4ba801d..7dfadb6 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -35,18 +35,16 @@ struct coresight_node {
>> };
>> /*
>> - * When operating Coresight drivers from the sysFS interface, only a
>> single
>> - * path can exist from a tracer (associated to a CPU) to a sink.
>> + * struct coresight_path - path from source to sink
>> + * @path: Address of path list.
>> + * @link: hook to the list.
>> */
>> -static DEFINE_PER_CPU(struct list_head *, tracer_path);
>> +struct coresight_path {
>> + struct list_head *path;
>> + struct list_head link;
>> +};
>> -/*
>> - * As of this writing only a single STM can be found in CS
>> topologies. Since
>> - * there is no way to know if we'll ever see more and what kind of
>> - * configuration they will enact, for the time being only define a
>> single path
>> - * for STM.
>> - */
>> -static struct list_head *stm_path;
>> +static LIST_HEAD(cs_active_paths);
>> /*
>> * When losing synchronisation a new barrier packet needs to be
>> inserted at the
>> @@ -326,7 +324,7 @@ static void coresight_disable_sink(struct
>> coresight_device *csdev)
>> if (ret)
>> return;
>> coresight_control_assoc_ectdev(csdev, false);
>> - csdev->enable = false;
>> + csdev->activated = false;
>> }
>> static int coresight_enable_link(struct coresight_device *csdev,
>> @@ -562,6 +560,20 @@ int coresight_enable_path(struct list_head *path,
>> u32 mode, void *sink_data)
>> goto out;
>> }
>> +static struct coresight_device *coresight_get_source(struct
>> list_head *path)
>> +{
>> + struct coresight_device *csdev;
>> +
>> + if (!path)
>> + return NULL;
>> +
>> + csdev = list_first_entry(path, struct coresight_node, link)->csdev;
>> + if (csdev->type != CORESIGHT_DEV_TYPE_SOURCE)
>> + return NULL;
>> +
>> + return csdev;
>> +}
>> +
>> struct coresight_device *coresight_get_sink(struct list_head *path)
>> {
>> struct coresight_device *csdev;
>> @@ -1047,9 +1059,23 @@ static int coresight_validate_source(struct
>> coresight_device *csdev,
>> return 0;
>> }
>> +static int coresight_store_path(struct list_head *path)
>> +{
>> + struct coresight_path *node;
>> +
>> + node = kzalloc(sizeof(struct coresight_path), GFP_KERNEL);
>
> Have you run this with all the "DEBUG" whistles turned on ? This could
> be problematic when called from a context where you are not allowed to
> sleep. e.g, perf enable call back.
>
Only coresight_enable function will call coresight_store_path, and seems
like coresight_enable function should be allowed to sleep since it is
protected by Mutex. And I find in function coresight_build_path also
will call kzalloc. Do you have any other concerns about calling kzalloc
here?
>> + if (!node)
>> + return -ENOMEM;
>> +
>> + node->path = path;
>> + list_add(&node->link, &cs_active_paths);
>> +
>> + return 0;
>> +}
>> +
>> int coresight_enable(struct coresight_device *csdev)
>> {
>> - int cpu, ret = 0;
>> + int ret = 0;
>> struct coresight_device *sink;
>> struct list_head *path;
>> enum coresight_dev_subtype_source subtype;
>> @@ -1094,25 +1120,9 @@ int coresight_enable(struct coresight_device
>> *csdev)
>> if (ret)
>> goto err_source;
>> - switch (subtype) {
>> - case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
>> - /*
>> - * When working from sysFS it is important to keep track
>> - * of the paths that were created so that they can be
>> - * undone in 'coresight_disable()'. Since there can only
>> - * be a single session per tracer (when working from sysFS)
>> - * a per-cpu variable will do just fine.
>> - */
>> - cpu = source_ops(csdev)->cpu_id(csdev);
>> - per_cpu(tracer_path, cpu) = path;
>> - break;
>> - case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
>> - stm_path = path;
>> - break;
>> - default:
>> - /* We can't be here */
>> - break;
>> - }
>> + ret = coresight_store_path(path);
>> + if (ret)
>> + goto err_source;
>> out:
>> mutex_unlock(&coresight_mutex);
>> @@ -1129,8 +1139,11 @@ EXPORT_SYMBOL_GPL(coresight_enable);
>> void coresight_disable(struct coresight_device *csdev)
>> {
>> - int cpu, ret;
>> + int ret;
>> struct list_head *path = NULL;
>> + struct coresight_path *cspath = NULL;
>> + struct coresight_path *cspath_next = NULL;
>> + struct coresight_device *src_csdev = NULL;
>> mutex_lock(&coresight_mutex);
>> @@ -1141,20 +1154,18 @@ void coresight_disable(struct
>> coresight_device *csdev)
>> if (!csdev->enable || !coresight_disable_source(csdev))
>> goto out;
>> - switch (csdev->subtype.source_subtype) {
>> - case CORESIGHT_DEV_SUBTYPE_SOURCE_PROC:
>> - cpu = source_ops(csdev)->cpu_id(csdev);
>> - path = per_cpu(tracer_path, cpu);
>> - per_cpu(tracer_path, cpu) = NULL;
>> - break;
>> - case CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE:
>> - path = stm_path;
>> - stm_path = NULL;
>> - break;
>> - default:
>> - /* We can't be here */
>> - break;
>
> --> Cut here <---
>
>> + list_for_each_entry_safe(cspath, cspath_next, &cs_active_paths,
>> link) {
>> + src_csdev = coresight_get_source(cspath->path);
>> + if (!src_csdev)
>> + continue;
>> + if (src_csdev == csdev) {
>> + path = cspath->path;
>> + list_del(&cspath->link);
>> + kfree(cspath);
>> + }
>> }
>
> Why not add the above to
> coresight_{retrieve/remove/}_active_path(csdev) ?
>
>
>
> Suzuki
I didn't find coresight_{retrieve/remove/}_active_path(csdev) from
coresight next branch. Can you help point out the exact function name
that you think this part of code should be added to?

\
 
 \ /
  Last update: 2021-04-16 13:51    [W:0.058 / U:0.672 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site