On Wed, Oct 01, 2025 at 02:44:06PM +0100, James Clark wrote:
[...]
ATTR_CFG_FLD_ts_level_* is only used in coresight-etm4x-core.c, it is not used in coresight-etm-perf.c. Thus, we don't need to include coresight-etm4x.h in coresight-etm-perf.c. Do I miss anything?
Yes, GEN_PMU_FORMAT_ATTR() uses them but it makes it hard to see.
I did a quick test, it is feasible to move ATTR_CFG_* macros in coresight-etm-perf.h. This is a more suitable ?
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h index 5febbcdb8696..2679d5b2dd9a 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.h +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h @@ -8,6 +8,7 @@ #define _CORESIGHT_ETM_PERF_H
#include <linux/percpu-defs.h> +#include <linux/perf/arm_pmu.h> #include "coresight-priv.h"
struct coresight_device; @@ -20,6 +21,12 @@ struct cscfg_config_desc; */ #define ETM_ADDR_CMP_MAX 8
+#define ATTR_CFG_FLD_ts_level_CFG config3 +#define ATTR_CFG_FLD_ts_level_LO 12 +#define ATTR_CFG_FLD_ts_level_HI 15 +#define ATTR_CFG_FLD_ts_level_MASK GENMASK(ATTR_CFG_FLD_ts_level_HI, \ + ATTR_CFG_FLD_ts_level_LO) +
A similiar case is the attr 'cc_threshold' is only used by ETMv4, it is exported always. It is not bad for me to always expose these attrs but in the are ignored in the ETMv3 driver - so we even don't need to bother adding .visible() callback.
I disagree with always showing them. I think they should be hidden if they're not used, or at least return an error to avoid confusing users. It also wastes config bits if they're allocated but never used.
It is fine for not exposing ETMv4 only attrs for ETMv3.
Either way, this was done because of the header mechanics which can only be avoided by adding more changes than just the #ifdefs. There are also already ETM4 #ifdefs in the file.
Yeah, actually we can remove ETM4 #ifdefs, something like:
/* * contextid always traces the "PID". The PID is in CONTEXTIDR_EL1 @@ -90,9 +83,9 @@ static ssize_t format_attr_contextid_show(struct device *dev, { int pid_fmt = ETM_OPT_CTXTID;
-#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X) - pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ETM_OPT_CTXTID; -#endif + if (IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X)) + pid_fmt = is_kernel_in_hyp_mode() ? ETM_OPT_CTXTID2 : ; + return sprintf(page, "config:%d\n", pid_fmt); }
Thanks, Leo