Slight simplification...
On Thu, 27 Nov 2025 at 09:30, James Clark james.clark@linaro.org wrote:
On 27/11/2025 9:22 am, Leo Yan wrote:
On Thu, Nov 27, 2025 at 04:44:53PM +0800, Kuan-Wei Chiu wrote:
[...]
I don't feel it is my place to say whether the etm3x driver should be removed entirely.
Sorry for confusion. Your fix patch is welcome, this is useful no matter if remove the ETMv3 driver or not.
However, if we decide to keep it, I agree that aligning cntr_val_show with the cntr_val_store behavior (using cntr_idx) makes more sense.
Here is my plan for v2:
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c index 762109307b86..77578885e8f3 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c @@ -717,26 +717,19 @@ static DEVICE_ATTR_RW(cntr_rld_event); static ssize_t cntr_val_show(struct device *dev, struct device_attribute *attr, char *buf) {
int i, ret = 0; u32 val; struct etm_drvdata *drvdata = dev_get_drvdata(dev->parent); struct etm_config *config = &drvdata->config;
if (!coresight_get_mode(drvdata->csdev)) { spin_lock(&drvdata->spinlock);
for (i = 0; i < drvdata->nr_cntr; i++)ret += sprintf(buf, "counter %d: %x\n",i, config->cntr_val[i]);
val = config->cntr_val[config->cntr_idx]; spin_unlock(&drvdata->spinlock);
return ret;- }
- for (i = 0; i < drvdata->nr_cntr; i++) {
val = etm_readl(drvdata, ETMCNTVRn(i));ret += sprintf(buf, "counter %d: %x\n", i, val);
return sprintf(buf, "%x\n", val);
remove this return...
}
- return ret;
else here to set val.
- val = etm_readl(drvdata, ETMCNTVRn(config->cntr_idx));
It is not right to read register at here (it cannot promise to read the CPU (cp14) register on the target CPU).
Please refer to the same function in coresight-etm4x-sysfs.c. I think we can do the same thing at here.
That's a different fix than the display bug though. This change doesn't change that if it's already wrong. The display fix should go in alone and then if there is an issue with not running things on the right CPU that should go in separately.
- return sprintf(buf, "%x\n", val);
just this return point - but emit_sysfs is now the standard way to do this, since we are making a change.
Mike
}
static ssize_t cntr_val_store(struct device *dev,
Given the upcoming merge window, I plan to submit this v2 after -rc1 is released.
Alternatively, if the consensus is to drop the driver, I am happy to submit a patch for that instead.
Please continue this patch. Thanks a lot!
Leo