Re: [PATCH 01/10] coresight: add support to enable more coresight paths

[Jinlong Mao <quic_jinlmao@quicinc.com>]

Hi Mathieu,

Thanks for the comments.

I double checked the code. Please see my comments below.


On 10/29/2021 2:06 AM, Mathieu Poirier wrote:
> On Thu, Oct 21, 2021 at 03:38:47PM +0800, 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.
>>
>> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-core.c | 100 +++++++++++--------
>>   1 file changed, 56 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
>> index 8a18c71df37a..1e621d61307a 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -37,18 +37,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;
>> +};
> For sources associated with a CPU, like ETMs, having a per-cpu way of storing
> paths is a definite advantage and should be kept that way.

Hi Mathieu,

Could you please share what is the advantage to handle the sources 
associated with a CPU separatly ?

 From the code, cpu id is only used to get the path of the ETM source.

As there will be many tpdm sources, I think it will be easier to only 
maintain one list for all the sources.

>>   
>> -/*
>> - * 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);
> Then there are sources that aren't associated with a CPU like STMs and TPDMs.
> Perhaps using an IDR or the hash of the device name as a key to a hashing
> vector would be better than doing a sequential search, especially as the
> list of devices is bound to increase over time.

Agree with you. I will try to use IDR or  the hash of the device name as 
a key to a hashing vector.

>
>>   
>>   /*
>>    * When losing synchronisation a new barrier packet needs to be inserted at the
>> @@ -354,6 +352,7 @@ static void coresight_disable_sink(struct coresight_device *csdev)
>>   	if (ret)
>>   		return;
>>   	coresight_control_assoc_ectdev(csdev, false);
>> +	csdev->activated = false;
> I don't see why this is needed and without proper documentation there is no way
> for me to guess the logic behind the change.  The ->activated flag should be
> manipulated from the command line interface only.

When source is disabled, but sink is still actived. It will be confused 
for end users.

>
>>   	csdev->enable = false;
>>   }
>>   
>> @@ -590,6 +589,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;
>> @@ -1086,9 +1099,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);
>> +	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;
>> @@ -1133,25 +1160,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);
>> @@ -1168,8 +1179,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);
>>   
>> @@ -1180,20 +1194,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;
>> +	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);
> See my comment above - I agree that sources _not_ associated with a CPU should
> be handled differently.  CPU bound sources should be kept untouched.
>
> That is all the time I had for today, I will continue tomorrow.
>
> Thanks,
> Mathieu
>
>> +		}
>>   	}
>> +	if (path == NULL)
>> +		goto out;
>>   
>>   	coresight_disable_path(path);
>>   	coresight_release_path(path);
>> -- 
>> 2.17.1
>>