From: Carl Worth <carl@os.amperecomputing.com>
The handle is essential for retrieving the AUX_EVENT of each CPU and is
required in perf mode. It has been added to the coresight_path so that
dependent devices can access it from the path when needed.
Fixes: 080ee83cc361 ("Coresight: Change functions to accept the coresight_path")
Signed-off-by: Carl Worth <carl@os.amperecomputing.com>
Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
---
drivers/hwtracing/coresight/coresight-etm-perf.c | 1 +
drivers/hwtracing/coresight/coresight-tmc-etr.c | 3 ++-
include/linux/coresight.h | 10 ++++++----
3 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index f677c08233ba..5c256af6e54a 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -520,6 +520,7 @@ static void etm_event_start(struct perf_event *event, int flags)
goto out;
path = etm_event_cpu_path(event_data, cpu);
+ path->handle = handle;
/* We need a sink, no need to continue without one */
sink = coresight_get_sink(path);
if (WARN_ON_ONCE(!sink))
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index b07fcdb3fe1a..1040f73f0537 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1327,7 +1327,8 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
enum cs_mode mode, void *data)
{
- struct perf_output_handle *handle = data;
+ struct coresight_path *path = data;
+ struct perf_output_handle *handle = path->handle;
struct etr_perf_buffer *etr_perf;
switch (mode) {
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 6de59ce8ef8c..4591121ae1d4 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -332,12 +332,14 @@ static struct coresight_dev_list (var) = { \
/**
* struct coresight_path - data needed by enable/disable path
- * @path_list: path from source to sink.
- * @trace_id: trace_id of the whole path.
+ * @path_list: path from source to sink.
+ * @trace_id: trace_id of the whole path.
+ * struct perf_output_handle: handle of the aux_event.
*/
struct coresight_path {
- struct list_head path_list;
- u8 trace_id;
+ struct list_head path_list;
+ u8 trace_id;
+ struct perf_output_handle *handle;
};
enum cs_mode {
--
2.34.1Jie Gan <jie.gan@oss.qualcomm.com> writes: > From: Carl Worth <carl@os.amperecomputing.com> > > The handle is essential for retrieving the AUX_EVENT of each CPU and is > required in perf mode. It has been added to the coresight_path so that > dependent devices can access it from the path when needed. I'd still like to have the original command I used to trigger the bug in the commit message. I really like having reproduction steps captured in commit messages when I look back at commits in the future. So, that was: perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null > /** > * struct coresight_path - data needed by enable/disable path > - * @path_list: path from source to sink. > - * @trace_id: trace_id of the whole path. > + * @path_list: path from source to sink. > + * @trace_id: trace_id of the whole path. > + * struct perf_output_handle: handle of the aux_event. > */ Fixing to "@handle" was mentioned in another comment already. Something about the above still feels a little off to me. It feels like we're throwing new data into a structure just because it happens to be conveniently at hand for the code paths we're needing, and not because it really _belongs_ there. Or, maybe it's the right place for it, and the cause of my concern is that "path" is an overly-narrow name in struct coresight_path? But if a renaming of this structure would improve the code, I'd also be fine with that happening in a subsequent commit, so I won't try to hold up the current series based on that. -Carl
On 9/23/2025 1:31 AM, Carl Worth wrote: > Jie Gan <jie.gan@oss.qualcomm.com> writes: >> From: Carl Worth <carl@os.amperecomputing.com> >> >> The handle is essential for retrieving the AUX_EVENT of each CPU and is >> required in perf mode. It has been added to the coresight_path so that >> dependent devices can access it from the path when needed. > > I'd still like to have the original command I used to trigger the bug in > the commit message. I really like having reproduction steps captured in > commit messages when I look back at commits in the future. So, that was: > > perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null > Sure, I’ll include your commit message in the formal patch series, I think it's V3 since you have submitted two versions, if you're okay with me sending it out. >> /** >> * struct coresight_path - data needed by enable/disable path >> - * @path_list: path from source to sink. >> - * @trace_id: trace_id of the whole path. >> + * @path_list: path from source to sink. >> + * @trace_id: trace_id of the whole path. >> + * struct perf_output_handle: handle of the aux_event. >> */ > > Fixing to "@handle" was mentioned in another comment already. > > Something about the above still feels a little off to me. It feels like > we're throwing new data into a structure just because it happens to be > conveniently at hand for the code paths we're needing, and not because > it really _belongs_ there. > The core idea behind coresight_path is that it can hold all the data potentially needed by any device along the path. For example with the path ETM->Link->ETR->CATU: All the mentioned devices operate by forming a path, for which the system constructs a coresight_path. This 'path' is then passed to each device along the route, allowing any device to directly access the required data from coresight_path instead of receiving it as a separate argument. Imagine a device that requires more than two or three arguments, and you want to pass them through coresight_enable_path or similar functions... For certain coresight_path instances, we may not need the handle or other parameters. Since these values are initialized, it's acceptable to leave them as NULL or 0. > Or, maybe it's the right place for it, and the cause of my concern is > that "path" is an overly-narrow name in struct coresight_path? > It defines the direction of data flow—serving as the path for trace data. Thanks, Jie > But if a renaming of this structure would improve the code, I'd also be > fine with that happening in a subsequent commit, so I won't try to hold > up the current series based on that. > > -Carl
Jie Gan <jie.gan@oss.qualcomm.com> writes: > On 9/23/2025 1:31 AM, Carl Worth wrote: >> I'd still like to have the original command I used to trigger the bug in >> the commit message. I really like having reproduction steps captured in >> commit messages when I look back at commits in the future. So, that was: >> >> perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null > > Sure, I’ll include your commit message in the formal patch series, I > think it's V3 since you have submitted two versions, if you're okay with > me sending it out. Yes. Please do. And thank you. > The core idea behind coresight_path is that it can hold all the data > potentially needed by any device along the path. > > For example with the path ETM->Link->ETR->CATU: OK. That makes sense to me. The name of coresight_path definitely threw me off, since I interpreted it as being a description of the path, not a container for data to be consumed along the path. -Carl
Hi, On Tue, 23 Sept 2025 at 02:49, Jie Gan <jie.gan@oss.qualcomm.com> wrote: > > > > On 9/23/2025 1:31 AM, Carl Worth wrote: > > Jie Gan <jie.gan@oss.qualcomm.com> writes: > >> From: Carl Worth <carl@os.amperecomputing.com> > >> > >> The handle is essential for retrieving the AUX_EVENT of each CPU and is > >> required in perf mode. It has been added to the coresight_path so that > >> dependent devices can access it from the path when needed. > > > > I'd still like to have the original command I used to trigger the bug in > > the commit message. I really like having reproduction steps captured in > > commit messages when I look back at commits in the future. So, that was: > > > > perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null > > > > Sure, I’ll include your commit message in the formal patch series, I > think it's V3 since you have submitted two versions, if you're okay with > me sending it out. > > >> /** > >> * struct coresight_path - data needed by enable/disable path > >> - * @path_list: path from source to sink. > >> - * @trace_id: trace_id of the whole path. > >> + * @path_list: path from source to sink. > >> + * @trace_id: trace_id of the whole path. > >> + * struct perf_output_handle: handle of the aux_event. > >> */ > > > > Fixing to "@handle" was mentioned in another comment already. > > > > Something about the above still feels a little off to me. It feels like > > we're throwing new data into a structure just because it happens to be > > conveniently at hand for the code paths we're needing, and not because > > it really _belongs_ there. > > > This data is perf specific - not path generic; so I agree that this structure should go elsewhere. I would suggest in the csdev (coresight_device) structure itself. We already have some sink specific data in here e.g. perf_sink_id_map. This could then be set/clear in the functions coresight-etm-perf.c file, where there is a significant amount of code dealing with the perf handle and ensuring it is valid and in scope. This can then be set only when appropriate - for source / sink devices and only when in perf mode, and avoid the need to pass the handle around as API call parameters. Regards Mike. > The core idea behind coresight_path is that it can hold all the data > potentially needed by any device along the path. > > For example with the path ETM->Link->ETR->CATU: > > All the mentioned devices operate by forming a path, for which the > system constructs a coresight_path. This 'path' is then passed to each > device along the route, allowing any device to directly access the > required data from coresight_path instead of receiving it as a separate > argument. > > Imagine a device that requires more than two or three arguments, and you > want to pass them through coresight_enable_path or similar functions... > > For certain coresight_path instances, we may not need the handle or > other parameters. Since these values are initialized, it's acceptable to > leave them as NULL or 0. > > > > Or, maybe it's the right place for it, and the cause of my concern is > > that "path" is an overly-narrow name in struct coresight_path? > > > > It defines the direction of data flow—serving as the path for trace data. > > Thanks, > Jie > > > But if a renaming of this structure would improve the code, I'd also be > > fine with that happening in a subsequent commit, so I won't try to hold > > up the current series based on that. > > > > -Carl > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
On 24/09/2025 11:21, Mike Leach wrote: > Hi, > > On Tue, 23 Sept 2025 at 02:49, Jie Gan <jie.gan@oss.qualcomm.com> wrote: >> >> >> >> On 9/23/2025 1:31 AM, Carl Worth wrote: >>> Jie Gan <jie.gan@oss.qualcomm.com> writes: >>>> From: Carl Worth <carl@os.amperecomputing.com> >>>> >>>> The handle is essential for retrieving the AUX_EVENT of each CPU and is >>>> required in perf mode. It has been added to the coresight_path so that >>>> dependent devices can access it from the path when needed. >>> >>> I'd still like to have the original command I used to trigger the bug in >>> the commit message. I really like having reproduction steps captured in >>> commit messages when I look back at commits in the future. So, that was: >>> >>> perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null >>> >> >> Sure, I’ll include your commit message in the formal patch series, I >> think it's V3 since you have submitted two versions, if you're okay with >> me sending it out. >> >>>> /** >>>> * struct coresight_path - data needed by enable/disable path >>>> - * @path_list: path from source to sink. >>>> - * @trace_id: trace_id of the whole path. >>>> + * @path_list: path from source to sink. >>>> + * @trace_id: trace_id of the whole path. >>>> + * struct perf_output_handle: handle of the aux_event. >>>> */ >>> >>> Fixing to "@handle" was mentioned in another comment already. >>> >>> Something about the above still feels a little off to me. It feels like >>> we're throwing new data into a structure just because it happens to be >>> conveniently at hand for the code paths we're needing, and not because >>> it really _belongs_ there. >>> >> > This data is perf specific - not path generic; so I agree that this > structure should go elsewhere. > > I would suggest in the csdev (coresight_device) structure itself. We > already have some sink specific data in here e.g. perf_sink_id_map. > > This could then be set/clear in the functions coresight-etm-perf.c > file, where there is a significant amount of code dealing with the > perf handle and ensuring it is valid and in scope. > > This can then be set only when appropriate - for source / sink devices > and only when in perf mode, and avoid the need to pass the handle > around as API call parameters. I think this data is specific to the session we are enabling the device(s) in. e.g., we keep the trace-id in the path. So, I don't mind having this in the path structure. Instead of modifying csdev with additional locking from "etm-perf" it is always cleaner to handle this in the path. Suzuki > > Regards > > Mike. > > > > >> The core idea behind coresight_path is that it can hold all the data >> potentially needed by any device along the path. >> >> For example with the path ETM->Link->ETR->CATU: >> >> All the mentioned devices operate by forming a path, for which the >> system constructs a coresight_path. This 'path' is then passed to each >> device along the route, allowing any device to directly access the >> required data from coresight_path instead of receiving it as a separate >> argument. >> >> Imagine a device that requires more than two or three arguments, and you >> want to pass them through coresight_enable_path or similar functions... >> >> For certain coresight_path instances, we may not need the handle or >> other parameters. Since these values are initialized, it's acceptable to >> leave them as NULL or 0. >> >> >>> Or, maybe it's the right place for it, and the cause of my concern is >>> that "path" is an overly-narrow name in struct coresight_path? >>> >> >> It defines the direction of data flow—serving as the path for trace data. >> >> Thanks, >> Jie >> >>> But if a renaming of this structure would improve the code, I'd also be >>> fine with that happening in a subsequent commit, so I won't try to hold >>> up the current series based on that. >>> >>> -Carl >> > > > -- > Mike Leach > Principal Engineer, ARM Ltd. > Manchester Design Centre. UK
On Mon, Sep 22, 2025 at 03:31:39PM +0800, Jie Gan wrote:
> From: Carl Worth <carl@os.amperecomputing.com>
>
> The handle is essential for retrieving the AUX_EVENT of each CPU and is
> required in perf mode. It has been added to the coresight_path so that
> dependent devices can access it from the path when needed.
>
> Fixes: 080ee83cc361 ("Coresight: Change functions to accept the coresight_path")
> Signed-off-by: Carl Worth <carl@os.amperecomputing.com>
> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
> ---
> drivers/hwtracing/coresight/coresight-etm-perf.c | 1 +
> drivers/hwtracing/coresight/coresight-tmc-etr.c | 3 ++-
> include/linux/coresight.h | 10 ++++++----
> 3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index f677c08233ba..5c256af6e54a 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -520,6 +520,7 @@ static void etm_event_start(struct perf_event *event, int flags)
> goto out;
>
> path = etm_event_cpu_path(event_data, cpu);
> + path->handle = handle;
> /* We need a sink, no need to continue without one */
> sink = coresight_get_sink(path);
> if (WARN_ON_ONCE(!sink))
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index b07fcdb3fe1a..1040f73f0537 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1327,7 +1327,8 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
> struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
> enum cs_mode mode, void *data)
> {
> - struct perf_output_handle *handle = data;
> + struct coresight_path *path = data;
> + struct perf_output_handle *handle = path->handle;
> struct etr_perf_buffer *etr_perf;
>
> switch (mode) {
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 6de59ce8ef8c..4591121ae1d4 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -332,12 +332,14 @@ static struct coresight_dev_list (var) = { \
>
> /**
> * struct coresight_path - data needed by enable/disable path
> - * @path_list: path from source to sink.
> - * @trace_id: trace_id of the whole path.
> + * @path_list: path from source to sink.
> + * @trace_id: trace_id of the whole path.
> + * struct perf_output_handle: handle of the aux_event.
s/struct perf_output_handle/@handle/
Otherwise, LGTM:
Reviewed-by: Leo Yan <leo.yan@arm.com>
> */
> struct coresight_path {
> - struct list_head path_list;
> - u8 trace_id;
> + struct list_head path_list;
> + u8 trace_id;
> + struct perf_output_handle *handle;
> };
>
> enum cs_mode {
>
> --
> 2.34.1
>
>
On 9/22/2025 4:29 PM, Leo Yan wrote:
> On Mon, Sep 22, 2025 at 03:31:39PM +0800, Jie Gan wrote:
>> From: Carl Worth <carl@os.amperecomputing.com>
>>
>> The handle is essential for retrieving the AUX_EVENT of each CPU and is
>> required in perf mode. It has been added to the coresight_path so that
>> dependent devices can access it from the path when needed.
>>
>> Fixes: 080ee83cc361 ("Coresight: Change functions to accept the coresight_path")
>> Signed-off-by: Carl Worth <carl@os.amperecomputing.com>
>> Co-developed-by: Jie Gan <jie.gan@oss.qualcomm.com>
>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>> ---
>> drivers/hwtracing/coresight/coresight-etm-perf.c | 1 +
>> drivers/hwtracing/coresight/coresight-tmc-etr.c | 3 ++-
>> include/linux/coresight.h | 10 ++++++----
>> 3 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> index f677c08233ba..5c256af6e54a 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> @@ -520,6 +520,7 @@ static void etm_event_start(struct perf_event *event, int flags)
>> goto out;
>>
>> path = etm_event_cpu_path(event_data, cpu);
>> + path->handle = handle;
>> /* We need a sink, no need to continue without one */
>> sink = coresight_get_sink(path);
>> if (WARN_ON_ONCE(!sink))
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index b07fcdb3fe1a..1040f73f0537 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> @@ -1327,7 +1327,8 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
>> struct etr_buf *tmc_etr_get_buffer(struct coresight_device *csdev,
>> enum cs_mode mode, void *data)
>> {
>> - struct perf_output_handle *handle = data;
>> + struct coresight_path *path = data;
>> + struct perf_output_handle *handle = path->handle;
>> struct etr_perf_buffer *etr_perf;
>>
>> switch (mode) {
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index 6de59ce8ef8c..4591121ae1d4 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -332,12 +332,14 @@ static struct coresight_dev_list (var) = { \
>>
>> /**
>> * struct coresight_path - data needed by enable/disable path
>> - * @path_list: path from source to sink.
>> - * @trace_id: trace_id of the whole path.
>> + * @path_list: path from source to sink.
>> + * @trace_id: trace_id of the whole path.
>> + * struct perf_output_handle: handle of the aux_event.
>
> s/struct perf_output_handle/@handle/
Hi Leo,
Thanks for pointing out. I actually missed this error during the code
review...
Jie
>
> Otherwise, LGTM:
>
> Reviewed-by: Leo Yan <leo.yan@arm.com>
>
>> */
>> struct coresight_path {
>> - struct list_head path_list;
>> - u8 trace_id;
>> + struct list_head path_list;
>> + u8 trace_id;
>> + struct perf_output_handle *handle;
>> };
>>
>> enum cs_mode {
>>
>> --
>> 2.34.1
>>
>>
© 2016 - 2025 Red Hat, Inc.