Re: [PATCH 12/17] coresight: Make CPU id map a property of a trace ID map

[James Clark <james.clark@arm.com>]

On 07/05/2024 07:22, Anshuman Khandual wrote:
> 
> 
> On 4/29/24 20:51, James Clark wrote:
>> The global CPU ID mappings won't work for per-sink ID maps so move it to
>> the ID map struct. coresight_trace_id_release_all_pending() is hard
>> coded to operate on the default map, but once Perf sessions use their
>> own maps the pending release mechanism will be deleted. So it doesn't
>> need to be extended to accept a trace ID map argument at this point.
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  .../hwtracing/coresight/coresight-etm-perf.c  |  3 +-
>>  .../coresight/coresight-etm3x-core.c          |  3 +-
>>  .../coresight/coresight-etm4x-core.c          |  3 +-
>>  .../hwtracing/coresight/coresight-trace-id.c  | 28 ++++++++-----------
>>  .../hwtracing/coresight/coresight-trace-id.h  |  2 +-
>>  include/linux/coresight.h                     |  1 +
>>  6 files changed, 20 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> index 4afb9d29f355..25f1f87c90d1 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> @@ -508,7 +508,8 @@ static void etm_event_start(struct perf_event *event, int flags)
>>  		hw_id = FIELD_PREP(CS_AUX_HW_ID_VERSION_MASK,
>>  				   CS_AUX_HW_ID_CURR_VERSION);
>>  		hw_id |= FIELD_PREP(CS_AUX_HW_ID_TRACE_ID_MASK,
>> -				    coresight_trace_id_read_cpu_id(cpu));
>> +				    coresight_trace_id_read_cpu_id(cpu,
>> +						coresight_trace_id_map_default()));
>>  		perf_report_aux_output_id(event, hw_id);
>>  	}
>>  
>> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
>> index 4149e7675ceb..b21f5ad94e63 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
>> @@ -501,7 +501,8 @@ static int etm_enable_perf(struct coresight_device *csdev,
>>  	 * with perf locks - we know the ID cannot change until perf shuts down
>>  	 * the session
>>  	 */
>> -	trace_id = coresight_trace_id_read_cpu_id(drvdata->cpu);
>> +	trace_id = coresight_trace_id_read_cpu_id(drvdata->cpu,
>> +						  coresight_trace_id_map_default());
>>  	if (!IS_VALID_CS_TRACE_ID(trace_id)) {
>>  		dev_err(&drvdata->csdev->dev, "Failed to set trace ID for %s on CPU%d\n",
>>  			dev_name(&drvdata->csdev->dev), drvdata->cpu);
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index f32c8cd7742d..d16d6efb26fa 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -776,7 +776,8 @@ static int etm4_enable_perf(struct coresight_device *csdev,
>>  	 * with perf locks - we know the ID cannot change until perf shuts down
>>  	 * the session
>>  	 */
>> -	trace_id = coresight_trace_id_read_cpu_id(drvdata->cpu);
>> +	trace_id = coresight_trace_id_read_cpu_id(drvdata->cpu,
>> +						  coresight_trace_id_map_default());
>>  	if (!IS_VALID_CS_TRACE_ID(trace_id)) {
>>  		dev_err(&drvdata->csdev->dev, "Failed to set trace ID for %s on CPU%d\n",
>>  			dev_name(&drvdata->csdev->dev), drvdata->cpu);
>> diff --git a/drivers/hwtracing/coresight/coresight-trace-id.c b/drivers/hwtracing/coresight/coresight-trace-id.c
>> index 45ddd50d09a6..b393603dd713 100644
>> --- a/drivers/hwtracing/coresight/coresight-trace-id.c
>> +++ b/drivers/hwtracing/coresight/coresight-trace-id.c
>> @@ -13,10 +13,12 @@
>>  #include "coresight-trace-id.h"
>>  
>>  /* Default trace ID map. Used in sysfs mode and for system sources */
>> -static struct coresight_trace_id_map id_map_default;
>> +static DEFINE_PER_CPU(atomic_t, id_map_default_cpu_ids) = ATOMIC_INIT(0);
>> +static struct coresight_trace_id_map id_map_default = {
>> +	.cpu_map = &id_map_default_cpu_ids
>> +};
>>  
>> -/* maintain a record of the mapping of IDs and pending releases per cpu */
>> -static DEFINE_PER_CPU(atomic_t, cpu_id) = ATOMIC_INIT(0);
>> +/* maintain a record of the pending releases per cpu */
>>  static cpumask_t cpu_id_release_pending;
>>  
>>  /* perf session active counter */
>> @@ -46,12 +48,6 @@ static void coresight_trace_id_dump_table(struct coresight_trace_id_map *id_map,
>>  #define PERF_SESSION(n)
>>  #endif
>>  
>> -/* unlocked read of current trace ID value for given CPU */
>> -static int _coresight_trace_id_read_cpu_id(int cpu)
>> -{
>> -	return atomic_read(&per_cpu(cpu_id, cpu));
>> -}
> 
> Just wondering where this per cpu cpu_id ^^ is being dropped off as well
> OR is it still getting used ?
> 

No it's still needed. It's not dropped in this set.

I just moved the implementation into coresight_trace_id_read_cpu_id()
rather than add a new argument to _coresight_trace_id_read_cpu_id().
Although I might change that because of Mike's comment about keeping
both the map and non map versions of the functions to reduce some of the
churn changing the callers.