Re: [PATCH v9 3/6] Coresight: Introduce a new struct coresight_path

[Jie Gan <quic_jiegan@quicinc.com>]

On 1/28/2025 7:54 PM, James Clark wrote:
> 
> 
> On 24/01/2025 7:25 am, Jie Gan wrote:
>> Add 'struct coresight_path' to store the data that is needed by
>> coresight_enable_path/coresight_disable_path. The structure
>> will be transmitted to the helper and sink device to enable
>> related funcationalities.
>>
>> Signed-off-by: Jie Gan <quic_jiegan@quicinc.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-core.c  | 87 ++++++++++++++-----
>>   drivers/hwtracing/coresight/coresight-etb10.c |  3 +-
>>   .../hwtracing/coresight/coresight-etm-perf.c  | 52 ++++++-----
>>   .../hwtracing/coresight/coresight-etm-perf.h  |  2 +-
>>   drivers/hwtracing/coresight/coresight-priv.h  | 21 +++--
>>   drivers/hwtracing/coresight/coresight-sysfs.c | 32 +++----
>>   .../hwtracing/coresight/coresight-tmc-etf.c   |  3 +-
>>   .../hwtracing/coresight/coresight-tmc-etr.c   |  6 +-
>>   drivers/hwtracing/coresight/coresight-trbe.c  |  4 +-
>>   drivers/hwtracing/coresight/ultrasoc-smb.c    |  3 +-
>>   10 files changed, 137 insertions(+), 76 deletions(-)
>>
> 
> [...]
> 
> 
>>       INIT_LIST_HEAD(path);
>> +    cs_path->path = path;
>> +    /*
>> +     * Since not all source devices have a defined trace_id function,
>> +     * make sure to check for it before use.
>> +     *
>> +     * Assert the mode is CS_MODE_SYSFS, the trace_id will be assigned
>> +     * again later if the mode is CS_MODE_PERF.
>> +     */
>> +    if (source_ops(source)->trace_id != NULL) {
>> +        rc = source_ops(source)->trace_id(source, CS_MODE_SYSFS, NULL);
> 
> I don't think we should do this. Doesn't this consume two trace IDs for 
> each session? And I'm not even sure if it's released properly if it's 
> overwritten.
Yes, you are right, we may waste our trace ID here.
> 
> It should be possible to consolidate the all the trace ID allocation to 
> a single step when building the path, or another function that gets 
> called just after the path is built. At the moment the ID can be 
> allocated from about 5 different places and it's quite hard to 
> understand, especially with these new changes. I have some of it coded 
> up, let me finish it off and I can share it.
Waiting for your update. I am also looking forward to another solution.

> 
>> +        if(IS_VALID_CS_TRACE_ID(rc))
>> +            cs_path->trace_id = rc;
>> +        else
>> +            cs_path->trace_id = 0;
>> +    }
>> +    else
>> +        cs_path->trace_id = 0;
> 
> [...]
> 
>> +/**
>> + * struct coresight_path - data needed by enable/disable path
>> + * @handle:        perf aux handle for ETM.
>> + * @path:        path from source to sink.
>> + * @trace_id:        trace_id of the whole path.
>> + */
>> +struct coresight_path {
>> +    struct perf_output_handle    *handle;
> 
> This is only needed to avoid adding *handle to the enable function call 
> signature, but having it here implies it needs to be stored. And then we 
> need to manage the lifecycle of it by nulling it on deletion. All of 
> this can be avoided by just adding handle to enable().
> 
> Unrelated to this patch, but I'm not sure why we were passing around 
> void* for handle either. It just makes the code hard to read and implies 
> some flexibility that doesn't exist. It's always "struct 
> perf_output_handle", so we can change void* to that in the enable 
> functions. I also have a patch for this that I'll share in a bit.
> 
Thanks for support. I am totally agree with you. It's not related to the 
patch series and it looks like a hack here.

Waiting for your update.

>> +    struct list_head        *path;
>> +    u8                trace_id;
>> +};
>> +
>>   static inline void coresight_insert_barrier_packet(void *buf)
>>   {
>>       if (buf)
>> @@ -132,16 +144,15 @@ static inline void CS_UNLOCK(void __iomem *addr)
>>       } while (0);
>>   }
>> -void coresight_disable_path(struct list_head *path);
>> -int coresight_enable_path(struct list_head *path, enum cs_mode mode,
>> -              void *sink_data);
>> +void coresight_disable_path(struct coresight_path *cs_path);
>> +int coresight_enable_path(struct coresight_path *cs_path, enum 
>> cs_mode mode);
>>   struct coresight_device *coresight_get_sink(struct list_head *path);
> 
> This needs to be exported otherwise the build fails because you use it 
> in a module in another commit. I assume you are building as static?
> 
> 
Yes, you are right. I made a mistake here. I did not test it with build 
as module. Sorry about the mistake.

Thanks,
Jie