linux-kernel - Re: [PATCH v2 5/6] coresight: Add format attribute for setting the timestamp interval

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20251001135546.GP7985@e132581.arm.com>
Date: Wed, 1 Oct 2025 14:55:46 +0100
From: Leo Yan <leo.yan@....com>
To: James Clark <james.clark@...aro.org>
Cc: Suzuki K Poulose <suzuki.poulose@....com>,
	Mike Leach <mike.leach@...aro.org>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jonathan Corbet <corbet@....net>, coresight@...ts.linaro.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 5/6] coresight: Add format attribute for setting the
 timestamp interval

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