On 19/01/2023 15:56, Suzuki K Poulose wrote:
On 19/01/2023 15:43, James Clark wrote:
From: German Gomez german.gomez@arm.com
Read the value of ts_source exposed by the driver and store it in the ETMv4 and ETE header. If the interface doesn't exist (such as in older Kernels), defaults to a safe value of -1.
Super minor nits feel free to ignore.
Signed-off-by: German Gomez german.gomez@arm.com Signed-off-by: James Clark james.clark@arm.com
tools/perf/arch/arm/util/cs-etm.c | 48 +++++++++++++++++++++++++++++++ tools/perf/util/cs-etm-base.c | 2 ++ tools/perf/util/cs-etm.h | 2 ++ 3 files changed, 52 insertions(+)
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c index b526ffe550a5..481e170cd3f1 100644 --- a/tools/perf/arch/arm/util/cs-etm.c +++ b/tools/perf/arch/arm/util/cs-etm.c @@ -53,6 +53,7 @@ static const char * const metadata_etmv4_ro[] = { [CS_ETMV4_TRCIDR2] = "trcidr/trcidr2", [CS_ETMV4_TRCIDR8] = "trcidr/trcidr8", [CS_ETMV4_TRCAUTHSTATUS] = "mgmt/trcauthstatus", + [CS_ETMV4_TS_SOURCE] = "ts_source", }; static const char * const metadata_ete_ro[] = { @@ -62,6 +63,7 @@ static const char * const metadata_ete_ro[] = { [CS_ETE_TRCIDR8] = "trcidr/trcidr8", [CS_ETE_TRCAUTHSTATUS] = "mgmt/trcauthstatus", [CS_ETE_TRCDEVARCH] = "mgmt/trcdevarch", + [CS_ETE_TS_SOURCE] = "ts_source", }; static bool cs_etm_is_etmv4(struct auxtrace_record *itr, int cpu); @@ -613,6 +615,32 @@ static int cs_etm_get_ro(struct perf_pmu *pmu, int cpu, const char *path) return val; } +static int cs_etm_get_ro_signed(struct perf_pmu *pmu, int cpu, const char *path)
minor nit: This doesn't necessarily care if it is RO ? Also, does it make sense to rename to include cpu relation :
say, cs_etm_pmu_cpu_get_signed() ?
+{ + char pmu_path[PATH_MAX]; + int scan; + int val = 0;
+ /* Get RO metadata from sysfs */ + snprintf(pmu_path, PATH_MAX, "cpu%d/%s", cpu, path);
+ scan = perf_pmu__scan_file(pmu, pmu_path, "%d", &val); + if (scan != 1) + pr_err("%s: error reading: %s\n", __func__, pmu_path);
+ return val; +}
+static bool cs_etm_pmu_path_exists(struct perf_pmu *pmu, int cpu, const char *path)
nit: cs_etm_pmu_cpu_path_exists() ? To make the "cpu" relation explicit ?
For both of these points, I think it was just trying to be consistent with what is already there.
There is already cs_etm_is_etmv4() and cs_etm_get_ro() which don't mention the cpu part, and also the metadata_etmv4_ro variable which has _ro. You're right that it doesn't matter that they're read only, but at the moment everything is so it's probably easiest to leave it for now rather than go and update everything.
Otherwise looks good to me.
Suzuki
Thanks for the review.