| Message ID | 20240429152207.479221-2-james.clark@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: Use per-sink trace ID maps for Perf sessions | expand |
On 4/29/24 20:51, James Clark wrote: > The likely fix for this is to update Perf so print a helpful message. > > Signed-off-by: James Clark <james.clark@arm.com> > --- > tools/perf/util/cs-etm.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > index d65d7485886c..32818bd7cd17 100644 > --- a/tools/perf/util/cs-etm.c > +++ b/tools/perf/util/cs-etm.c > @@ -335,8 +335,11 @@ static int cs_etm__process_aux_output_hw_id(struct perf_session *session, > trace_chan_id = FIELD_GET(CS_AUX_HW_ID_TRACE_ID_MASK, hw_id); > > /* check that we can handle this version */ > - if (version > CS_AUX_HW_ID_CURR_VERSION) > + if (version > CS_AUX_HW_ID_CURR_VERSION) { > + pr_err("CS ETM Trace: PERF_RECORD_AUX_OUTPUT_HW_ID version %d not supported. Please update Perf.\n", Is not this bit misleading ? PERF_RECORD_AUX_OUTPUT_HW_ID is just the perf record format identifier. The record version here, is derived from the platform specific hardware ID information embedded in this perf record. Should not this be just s/PERF_RECORD_AUX_OUTPUT_HW_ID/hardware ID/ instead ? > + version); > return -EINVAL; > + } > > /* get access to the etm metadata */ > etm = container_of(session->auxtrace, struct cs_etm_auxtrace, auxtrace);
On 07/05/2024 04:47, Anshuman Khandual wrote: > > > On 4/29/24 20:51, James Clark wrote: >> The likely fix for this is to update Perf so print a helpful message. >> >> Signed-off-by: James Clark <james.clark@arm.com> >> --- >> tools/perf/util/cs-etm.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c >> index d65d7485886c..32818bd7cd17 100644 >> --- a/tools/perf/util/cs-etm.c >> +++ b/tools/perf/util/cs-etm.c >> @@ -335,8 +335,11 @@ static int cs_etm__process_aux_output_hw_id(struct perf_session *session, >> trace_chan_id = FIELD_GET(CS_AUX_HW_ID_TRACE_ID_MASK, hw_id); >> >> /* check that we can handle this version */ >> - if (version > CS_AUX_HW_ID_CURR_VERSION) >> + if (version > CS_AUX_HW_ID_CURR_VERSION) { >> + pr_err("CS ETM Trace: PERF_RECORD_AUX_OUTPUT_HW_ID version %d not supported. Please update Perf.\n", > > Is not this bit misleading ? PERF_RECORD_AUX_OUTPUT_HW_ID is just the perf record > format identifier. The record version here, is derived from the platform specific > hardware ID information embedded in this perf record. Not sure I follow what you mean here. 'version' is something that's output by the kernel. It's saved into a perf.data file, and if Perf can't handle version 2 for example, you need to update Perf. > > Should not this be just s/PERF_RECORD_AUX_OUTPUT_HW_ID/hardware ID/ instead ? > It's just a way to go from the error message to the part of the code or docs that you need to look at. "hardware ID" wouldn't lead you anywhere so I don't think it would be useful. >> + version); >> return -EINVAL; >> + } >> >> /* get access to the etm metadata */ >> etm = container_of(session->auxtrace, struct cs_etm_auxtrace, auxtrace); >
On 5/7/24 15:36, James Clark wrote: > > > On 07/05/2024 04:47, Anshuman Khandual wrote: >> >> >> On 4/29/24 20:51, James Clark wrote: >>> The likely fix for this is to update Perf so print a helpful message. >>> >>> Signed-off-by: James Clark <james.clark@arm.com> >>> --- >>> tools/perf/util/cs-etm.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c >>> index d65d7485886c..32818bd7cd17 100644 >>> --- a/tools/perf/util/cs-etm.c >>> +++ b/tools/perf/util/cs-etm.c >>> @@ -335,8 +335,11 @@ static int cs_etm__process_aux_output_hw_id(struct perf_session *session, >>> trace_chan_id = FIELD_GET(CS_AUX_HW_ID_TRACE_ID_MASK, hw_id); >>> >>> /* check that we can handle this version */ >>> - if (version > CS_AUX_HW_ID_CURR_VERSION) >>> + if (version > CS_AUX_HW_ID_CURR_VERSION) { >>> + pr_err("CS ETM Trace: PERF_RECORD_AUX_OUTPUT_HW_ID version %d not supported. Please update Perf.\n", >> >> Is not this bit misleading ? PERF_RECORD_AUX_OUTPUT_HW_ID is just the perf record >> format identifier. The record version here, is derived from the platform specific >> hardware ID information embedded in this perf record. > > Not sure I follow what you mean here. 'version' is something that's > output by the kernel. It's saved into a perf.data file, and if Perf > can't handle version 2 for example, you need to update Perf. Got it. > >> >> Should not this be just s/PERF_RECORD_AUX_OUTPUT_HW_ID/hardware ID/ instead ? >> > > It's just a way to go from the error message to the part of the code or > docs that you need to look at. "hardware ID" wouldn't lead you anywhere > so I don't think it would be useful. Sure, fair enough. > >>> + version); >>> return -EINVAL; >>> + } >>> >>> /* get access to the etm metadata */ >>> etm = container_of(session->auxtrace, struct cs_etm_auxtrace, auxtrace); >> > _______________________________________________ > CoreSight mailing list -- coresight@lists.linaro.org > To unsubscribe send an email to coresight-leave@lists.linaro.org
On Tue, May 07, 2024 at 04:27:25PM +0530, Anshuman Khandual wrote: > On 5/7/24 15:36, James Clark wrote: > > On 07/05/2024 04:47, Anshuman Khandual wrote: > >> On 4/29/24 20:51, James Clark wrote: > >>> The likely fix for this is to update Perf so print a helpful message. > >>> > >>> Signed-off-by: James Clark <james.clark@arm.com> > >>> --- > >>> tools/perf/util/cs-etm.c | 5 ++++- > >>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > >>> index d65d7485886c..32818bd7cd17 100644 > >>> --- a/tools/perf/util/cs-etm.c > >>> +++ b/tools/perf/util/cs-etm.c > >>> @@ -335,8 +335,11 @@ static int cs_etm__process_aux_output_hw_id(struct perf_session *session, > >>> trace_chan_id = FIELD_GET(CS_AUX_HW_ID_TRACE_ID_MASK, hw_id); > >>> > >>> /* check that we can handle this version */ > >>> - if (version > CS_AUX_HW_ID_CURR_VERSION) > >>> + if (version > CS_AUX_HW_ID_CURR_VERSION) { > >>> + pr_err("CS ETM Trace: PERF_RECORD_AUX_OUTPUT_HW_ID version %d not supported. Please update Perf.\n", > >> > >> Is not this bit misleading ? PERF_RECORD_AUX_OUTPUT_HW_ID is just the perf record > >> format identifier. The record version here, is derived from the platform specific > >> hardware ID information embedded in this perf record. > > > > Not sure I follow what you mean here. 'version' is something that's > > output by the kernel. It's saved into a perf.data file, and if Perf > > can't handle version 2 for example, you need to update Perf. > Got it. > >> Should not this be just s/PERF_RECORD_AUX_OUTPUT_HW_ID/hardware ID/ instead ? > >> > > > > It's just a way to go from the error message to the part of the code or > > docs that you need to look at. "hardware ID" wouldn't lead you anywhere > > so I don't think it would be useful. > > Sure, fair enough. I'm taking this as an Acked-by, ok? - Arnaldo
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index d65d7485886c..32818bd7cd17 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -335,8 +335,11 @@ static int cs_etm__process_aux_output_hw_id(struct perf_session *session, trace_chan_id = FIELD_GET(CS_AUX_HW_ID_TRACE_ID_MASK, hw_id); /* check that we can handle this version */ - if (version > CS_AUX_HW_ID_CURR_VERSION) + if (version > CS_AUX_HW_ID_CURR_VERSION) { + pr_err("CS ETM Trace: PERF_RECORD_AUX_OUTPUT_HW_ID version %d not supported. Please update Perf.\n", + version); return -EINVAL; + } /* get access to the etm metadata */ etm = container_of(session->auxtrace, struct cs_etm_auxtrace, auxtrace);
The likely fix for this is to update Perf so print a helpful message. Signed-off-by: James Clark <james.clark@arm.com> --- tools/perf/util/cs-etm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)