On Sat, Feb 06, 2021 at 11:08:29PM +0800, Leo Yan wrote:
From: Suzuki K Poulose suzuki.poulose@arm.com
When set option with macros ETM_OPT_CTXTID and ETM_OPT_TS, it wrongly takes these two values (14 and 28 prespectively) as bit masks, but actually both are the offset for bits. But this doesn't lead to further failure due to the AND logic operation will be always true for ETM_OPT_CTXTID / ETM_OPT_TS.
This patch defines new independent macros (rather than using the "config" bits) for requesting the "contextid" and "timestamp" for cs_etm_set_option().
[leoy: Extract the change as a separate patch for easier review]
This should go just above your name - see below.
Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com Signed-off-by: Leo Yan leo.yan@linaro.org Reviewed-by: Mike Leach mike.leach@linaro.org
Signed-off-by: Suzuki K Poulose suzuki.poulose@arm.com [Extract the change as a separate patch for easier review] Signed-off-by: Leo Yan leo.yan@linaro.org Reviewed-by: Mike Leach mike.leach@linaro.org
tools/perf/arch/arm/util/cs-etm.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index bd446aba64f7..c25c878fd06c 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -156,6 +156,10 @@ static int cs_etm_set_timestamp(struct auxtrace_record *itr, return err; } +#define ETM_SET_OPT_CTXTID (1 << 0) +#define ETM_SET_OPT_TS (1 << 1) +#define ETM_SET_OPT_MASK (ETM_SET_OPT_CTXTID | ETM_SET_OPT_TS)
I would much rather see this fixed with the BIT() macro as it is done in the rest of this set than defining new constant.
With the above:
Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org
I have picked up the kernel portion of this set. I suggest you fix the above and send another revision to Arnaldo with my RBs.
static int cs_etm_set_option(struct auxtrace_record *itr, struct evsel *evsel, u32 option) { @@ -169,17 +173,17 @@ static int cs_etm_set_option(struct auxtrace_record *itr, !cpu_map__has(online_cpus, i)) continue;
if (option & ETM_OPT_CTXTID) {
if (option & ETM_SET_OPT_CTXTID) { err = cs_etm_set_context_id(itr, evsel, i); if (err) goto out;
if (option & ETM_OPT_TS) {
if (option & ETM_SET_OPT_TS) { err = cs_etm_set_timestamp(itr, evsel, i); if (err) goto out;
if (option & ~(ETM_OPT_CTXTID | ETM_OPT_TS))
if (option & ~(ETM_SET_OPT_MASK)) /* Nothing else is currently supported */ goto out;@@ -406,7 +410,7 @@ static int cs_etm_recording_options(struct auxtrace_record *itr, evsel__set_sample_bit(cs_etm_evsel, CPU); err = cs_etm_set_option(itr, cs_etm_evsel,
ETM_OPT_CTXTID | ETM_OPT_TS);
ETM_SET_OPT_CTXTID | ETM_SET_OPT_TS);-- 2.25.1