Hi,
On 9/15/26 12:34, Yeoreum Yun wrote:
The current ETM3x configuration via sysfs can lead to the following inconsistencies:
- If a configuration is modified via sysfs while a perf session is active, the running configuration may differ between before a sched-out and after a subsequent sched-in.
To resolve these issues, separate the configuration into:
- active_config: the configuration applied to the current session
- config: the configuration set via sysfs
Same comment as for the etm4 config naming
Additionally:
Since active_config and related fields are accessed only by the local CPU in etm_enable/disable_sysfs_smp_call() (similar to perf enable/disable), remove the lock/unlock from the sysfs enable/disable path and starting/dying_cpu path except when to access config fields only.
Some of sysfs interface read etm register directly. To reduce lock scope while the etm_enable_hw()/etm_disable_hw(), handle it via IPI so that registers could be read from on proper CPU.
Fixes: 1925a470ce69 ("coresight: etm3x: splitting struct etm_drvdata") Signed-off-by: Yeoreum Yun yeoreum.yun@arm.com
drivers/hwtracing/coresight/coresight-etm.h | 4 +- drivers/hwtracing/coresight/coresight-etm3x-core.c | 68 ++++++++++-------- .../hwtracing/coresight/coresight-etm3x-sysfs.c | 80 +++++++++++++++------- 3 files changed, 100 insertions(+), 52 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm.h b/drivers/hwtracing/coresight/coresight-etm.h index 1d753cca29439..f3796162168d4 100644 --- a/drivers/hwtracing/coresight/coresight-etm.h +++ b/drivers/hwtracing/coresight/coresight-etm.h @@ -226,7 +226,8 @@ struct etm_config {
- @etmccr: value of register ETMCCR.
- @etmccer: value of register ETMCCER.
- @traceid: value of the current ID for this component.
- @config: structure holding configuration parameters.
- @active_config: structure holding current running configuration.
*/ struct etm_drvdata { struct csdev_access csa;
- @config: structure holding sysfs mode configuration.
@@ -248,6 +249,7 @@ struct etm_drvdata { u32 etmccr; u32 etmccer; u32 traceid;
- struct etm_config active_config; struct etm_config config; };
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c index 862ad0786699c..fd76a57e5f861 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c @@ -308,7 +308,7 @@ void etm_config_trace_mode(struct etm_config *config) static int etm_parse_event_config(struct etm_drvdata *drvdata, struct perf_event *event) {
- struct etm_config *config = &drvdata->config;
- struct etm_config *config = &drvdata->active_config; struct perf_event_attr *attr = &event->attr; u8 ts_level;
@@ -367,7 +367,7 @@ static int etm_enable_hw(struct etm_drvdata *drvdata) { int i, rc; u32 etmcr;
- struct etm_config *config = &drvdata->config;
- struct etm_config *config = &drvdata->active_config; struct coresight_device *csdev = drvdata->csdev;
CS_UNLOCK(drvdata->csa.base); @@ -442,32 +442,30 @@ static int etm_enable_hw(struct etm_drvdata *drvdata) struct etm_enable_arg { struct etm_drvdata *drvdata; struct coresight_path *path;
- struct etm_config config; int rc; };
static void etm_enable_sysfs_smp_call(void *info) { struct etm_enable_arg *arg = info;
- struct etm_drvdata *drvdata; struct coresight_device *csdev;
if (WARN_ON(!arg)) return;
- csdev = arg->drvdata->csdev;
- if (!coresight_take_mode(csdev, CS_MODE_SYSFS)) {
/* Someone is already using the tracer */arg->rc = -EBUSY;return;- }
- drvdata = arg->drvdata;
- csdev = drvdata->csdev;
- arg->rc = etm_enable_hw(arg->drvdata);
- drvdata->active_config = arg->config;
- drvdata->traceid = arg->path->trace_id;
- /* The tracer didn't start */
- if (arg->rc) {
coresight_set_mode(csdev, CS_MODE_DISABLED);
- arg->rc = etm_enable_hw(arg->drvdata);
- if (arg->rc) return;
- }
- drvdata->sticky_enable = true; csdev->path = arg->path; }
@@ -512,9 +510,10 @@ static int etm_enable_sysfs(struct coresight_device *csdev, struct coresight_pat struct etm_enable_arg arg = { }; int ret;
- spin_lock(&drvdata->spinlock);
- drvdata->traceid = path->trace_id;
- if (!coresight_take_mode(csdev, CS_MODE_SYSFS)) {
/* Someone is already using the tracer */return -EBUSY;- }
/* * Configure the ETM only if the CPU is online. If it isn't online @@ -523,23 +522,27 @@ static int etm_enable_sysfs(struct coresight_device *csdev, struct coresight_pat if (cpu_online(drvdata->cpu)) { arg.drvdata = drvdata; arg.path = path;
scoped_guard(spinlock, &drvdata->spinlock) {arg.config = drvdata->config;}
Again I think the arg.config is unnecessary
Regards
Mike
ret = smp_call_function_single(drvdata->cpu, etm_enable_sysfs_smp_call, &arg, 1); if (!ret) ret = arg.rc;
if (!ret)drvdata->sticky_enable = true;
- if (ret)
- if (!ret) {
dev_dbg(&csdev->dev, "ETM tracing enabled\n");- } else { etm_release_trace_id(drvdata);
/* The tracer didn't start */coresight_set_mode(csdev, CS_MODE_DISABLED);- }
- spin_unlock(&drvdata->spinlock);
- if (!ret)
dev_dbg(&csdev->dev, "ETM tracing enabled\n");@@ -565,7 +568,7 @@ static int etm_enable(struct coresight_device *csdev, struct perf_event *event, static void etm_disable_hw(struct etm_drvdata *drvdata) { int i;
- struct etm_config *config = &drvdata->config;
- struct etm_config *config = &drvdata->active_config; struct coresight_device *csdev = drvdata->csdev;
CS_UNLOCK(drvdata->csa.base); @@ -593,7 +596,6 @@ static void etm_disable_sysfs_smp_call(void *info) etm_disable_hw(drvdata); drvdata->csdev->path = NULL;
- coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED); }
static void etm_disable_perf(struct coresight_device *csdev) @@ -630,8 +632,9 @@ static void etm_disable_perf(struct coresight_device *csdev) static void etm_disable_sysfs(struct coresight_device *csdev) { struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
- spin_lock(&drvdata->spinlock);
- struct etm_config *config = &drvdata->config;
- const struct etm_config *active_config = &drvdata->active_config;
- int i;
/* * Executing etm_disable_hw on the cpu whose ETM is being disabled @@ -640,7 +643,14 @@ static void etm_disable_sysfs(struct coresight_device *csdev) smp_call_function_single(drvdata->cpu, etm_disable_sysfs_smp_call, drvdata, 1);
- spin_unlock(&drvdata->spinlock);
- /*
* Userspace may read seq_curr_state and cntr_val through sysfs* after the sysfs-session has been disabled.*/- for (i = 0; i < drvdata->nr_cntr; i++)
config->cntr_val[i] = active_config->cntr_val[i];- config->seq_curr_state = active_config->seq_curr_state;
/* * we only release trace IDs when resetting sysfs. @@ -649,6 +659,8 @@ static void etm_disable_sysfs(struct coresight_device *csdev) * prior trace id allocation method */
- coresight_set_mode(drvdata->csdev, CS_MODE_DISABLED);
- dev_dbg(&csdev->dev, "ETM tracing disabled\n"); }
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c index d4846d6364945..2c8c129228589 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c @@ -6,10 +6,48 @@ #include <linux/pid_namespace.h> #include <linux/pm_runtime.h> +#include <linux/smp.h> #include <linux/sysfs.h> #include "coresight-etm.h" #include "coresight-priv.h" +struct etm_readl_cslocked_arg {
- struct etm_drvdata *drvdata;
- u32 off;
- unsigned long *val;
+};
+static void etm_readl_cslocked_smp_call(void *info) +{
- struct etm_readl_cslocked_arg *arg = info;
- CS_UNLOCK(arg->drvdata->csa.base);
- *arg->val = etm_readl(arg->drvdata, arg->off);
- CS_LOCK(arg->drvdata->csa.base);
+}
+static int etm_readl_cslocked(struct etm_drvdata *drvdata, u32 off,
unsigned long *val)+{
- int ret;
- struct etm_readl_cslocked_arg arg;
- ret = pm_runtime_get_sync(drvdata->csdev->dev.parent);
- if (ret < 0)
goto out;- arg.drvdata = drvdata;
- arg.off = off;
- arg.val = val;
- ret = smp_call_function_single(drvdata->cpu,
etm_readl_cslocked_smp_call,(void *)&arg, 1);+out:
- pm_runtime_put(drvdata->csdev->dev.parent);
- return ret;
+}
- static ssize_t nr_addr_cmp_show(struct device *dev, struct device_attribute *attr, char *buf) {
@@ -45,18 +83,13 @@ static DEVICE_ATTR_RO(nr_ctxid_cmp); static ssize_t etmsr_show(struct device *dev, struct device_attribute *attr, char *buf) {
- unsigned long flags, val;
- int ret;
- unsigned long val; struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent);
- pm_runtime_get_sync(dev->parent);
- spin_lock_irqsave(&drvdata->spinlock, flags);
- CS_UNLOCK(drvdata->csa.base);
- val = etm_readl(drvdata, ETMSR);
- CS_LOCK(drvdata->csa.base);
- spin_unlock_irqrestore(&drvdata->spinlock, flags);
- pm_runtime_put(dev->parent);
- ret = etm_readl_cslocked(drvdata, ETMSR, &val);
- if (ret)
return ret;return sprintf(buf, "%#lx\n", val); } @@ -74,7 +107,7 @@ static ssize_t reset_store(struct device *dev, ret = kstrtoul(buf, 16, &val); if (ret) return ret;
- if (coresight_get_mode(drvdata->csdev))
- if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS) return -EBUSY;
if (val) { @@ -719,7 +752,9 @@ static DEVICE_ATTR_RW(cntr_rld_event); static ssize_t cntr_val_show(struct device *dev, struct device_attribute *attr, char *buf) {
- int ret; u32 val;
- unsigned long val2; struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent); struct etm_config *config = &drvdata->config;
@@ -728,7 +763,10 @@ static ssize_t cntr_val_show(struct device *dev, val = config->cntr_val[config->cntr_idx]; spin_unlock(&drvdata->spinlock); } else {
val = etm_readl(drvdata, ETMCNTVRn(config->cntr_idx));
ret = etm_readl_cslocked(drvdata, ETMCNTVRn(config->cntr_idx), &val2);if (ret)return ret;val = val2;return sysfs_emit(buf, "%#x\n", val); @@ -746,7 +784,7 @@ static ssize_t cntr_val_store(struct device *dev, ret = kstrtoul(buf, 16, &val); if (ret) return ret;
- if (coresight_get_mode(drvdata->csdev))
- if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS) return -EBUSY;
spin_lock(&drvdata->spinlock); @@ -934,7 +972,8 @@ static DEVICE_ATTR_RW(seq_13_event); static ssize_t seq_curr_state_show(struct device *dev, struct device_attribute *attr, char *buf) {
- unsigned long val, flags;
- int ret;
- unsigned long val; struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent); struct etm_config *config = &drvdata->config;
@@ -943,15 +982,10 @@ static ssize_t seq_curr_state_show(struct device *dev, goto out; }
- pm_runtime_get_sync(dev->parent);
- spin_lock_irqsave(&drvdata->spinlock, flags);
- CS_UNLOCK(drvdata->csa.base);
- val = (etm_readl(drvdata, ETMSQR) & ETM_SQR_MASK);
- CS_LOCK(drvdata->csa.base);
- spin_unlock_irqrestore(&drvdata->spinlock, flags);
- pm_runtime_put(dev->parent);
- ret = etm_readl_cslocked(drvdata, ETMSQR, &val);
- if (ret)
return ret;- val &= ETM_SQR_MASK; out: return sprintf(buf, "%#lx\n", val); }