On Sat, Feb 06, 2021 at 11:08:29PM +0800, Leo Yan wrote: > From: Suzuki K Poulose <[email protected]> > > 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 <[email protected]> > Signed-off-by: Leo Yan <[email protected]> > Reviewed-by: Mike Leach <[email protected]> Signed-off-by: Suzuki K Poulose <[email protected]> [Extract the change as a separate patch for easier review] Signed-off-by: Leo Yan <[email protected]> Reviewed-by: Mike Leach <[email protected]> > --- > 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 <[email protected]> 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); > if (err) > goto out; > } > -- > 2.25.1 >
