On 21/11/2025 5:02 pm, Kuan-Wei Chiu wrote:
Hi James,
On Fri, Nov 21, 2025 at 09:50:03AM +0000, James Clark wrote:
On 21/11/2025 12:23 am, Kuan-Wei Chiu wrote:
The cntr_val_show() function is meant to display the values of all available counters. However, the sprintf() call inside the loop was always writing to the beginning of the buffer, causing the output of previous iterations to be overwritten. As a result, only the value of the last counter was actually returned to the user.
Fix this by using the return value of sprintf() to calculate the correct offset into the buffer for the next write, ensuring that all counter values are appended sequentially.
Fixes: a939fc5a71ad ("coresight-etm: add CoreSight ETM/PTM driver") Signed-off-by: Kuan-Wei Chiu visitorckw@gmail.com
Build tested only. I do not have the hardware to run the etm3x driver, so I would be grateful if someone could verify this on actual hardware.
I noticed this issue while browsing the coresight code after attending a technical talk on the subject. This code dates back to the initial driver submission over 10 years ago, so I was surprised it hadn't been caught earlier. Although I cannot perform runtime testing, the logic error seems obvious to me, so I still decided to submit this patch.
Nice find. I think the point that it wasn't caught changes how we fix it. Either nobody used it ever - so we can just delete it. Or someone was using it and they expect it to always return a single entry with the value of the last counter and this is a potentially breaking change. So maybe instead of fixing this we should add a new cntr_vals_show() which works correctly. But then again if nobody is using it we shouldn't do that either.
Thanks for your feedback.
I agree that if any tool relies on the current behavior, this patch would break the ABI and violate the hard rule that we must never break userspace.
However, I am not sure how to determine if any userspace tools are actually using this sysfs interface. Is there a recommended way to verify this, or a standard procedure/convention to follow in this situation?
Regards, Kuan-Wei
There's no way of knowing apart from deducing that there are 0 users of the 'correct' version of the API because it never existed. This isn't even a regression, it was broken from the beginning.
I suppose it does work when hardware only has one counter, but I don't know how likely that is?
Looking at cntr_val_store() I think I was wrong before when I said it should be a separate file for each counter. Writing to it already writes to the counter currently selected by cntr_idx and splitting it out to separate files would have to break that too. So we can fix the display bug by making show operate on the currently selected counter in the same way as store. Then it makes a bit more sense and we can delete the "counter %d: " prefix.
ETM4 already does it this way too.
The interface isn't even that great, it should be a separate file per counter. You don't want to be parsing strings and colons to try to read a single value, especially in C. Separate files allows you to read it directly without any hassle.
drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c index 762109307b86..312033e74b7a 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c @@ -725,7 +725,7 @@ static ssize_t cntr_val_show(struct device *dev, 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",
ret += sprintf(buf + ret, "counter %d: %x\n", i, config->cntr_val[i]); spin_unlock(&drvdata->spinlock); return ret;@@ -733,7 +733,7 @@ static ssize_t cntr_val_show(struct device *dev, for (i = 0; i < drvdata->nr_cntr; i++) { val = etm_readl(drvdata, ETMCNTVRn(i));
ret += sprintf(buf, "counter %d: %x\n", i, val);
ret += sprintf(buf + ret, "counter %d: %x\n", i, val);
Hi,
On Mon, 24 Nov 2025 at 16:12, James Clark james.clark@linaro.org wrote:
On 21/11/2025 5:02 pm, Kuan-Wei Chiu wrote:
Hi James,
On Fri, Nov 21, 2025 at 09:50:03AM +0000, James Clark wrote:
On 21/11/2025 12:23 am, Kuan-Wei Chiu wrote:
The cntr_val_show() function is meant to display the values of all available counters. However, the sprintf() call inside the loop was always writing to the beginning of the buffer, causing the output of previous iterations to be overwritten. As a result, only the value of the last counter was actually returned to the user.
Fix this by using the return value of sprintf() to calculate the correct offset into the buffer for the next write, ensuring that all counter values are appended sequentially.
Fixes: a939fc5a71ad ("coresight-etm: add CoreSight ETM/PTM driver") Signed-off-by: Kuan-Wei Chiu visitorckw@gmail.com
Build tested only. I do not have the hardware to run the etm3x driver, so I would be grateful if someone could verify this on actual hardware.
I noticed this issue while browsing the coresight code after attending a technical talk on the subject. This code dates back to the initial driver submission over 10 years ago, so I was surprised it hadn't been caught earlier. Although I cannot perform runtime testing, the logic error seems obvious to me, so I still decided to submit this patch.
Nice find. I think the point that it wasn't caught changes how we fix it. Either nobody used it ever - so we can just delete it. Or someone was using it and they expect it to always return a single entry with the value of the last counter and this is a potentially breaking change. So maybe instead of fixing this we should add a new cntr_vals_show() which works correctly. But then again if nobody is using it we shouldn't do that either.
Thanks for your feedback.
I agree that if any tool relies on the current behavior, this patch would break the ABI and violate the hard rule that we must never break userspace.
However, I am not sure how to determine if any userspace tools are actually using this sysfs interface. Is there a recommended way to verify this, or a standard procedure/convention to follow in this situation?
Regards, Kuan-Wei
There's no way of knowing apart from deducing that there are 0 users of the 'correct' version of the API because it never existed. This isn't even a regression, it was broken from the beginning.
I suppose it does work when hardware only has one counter, but I don't know how likely that is?
Looking at cntr_val_store() I think I was wrong before when I said it should be a separate file for each counter. Writing to it already writes to the counter currently selected by cntr_idx and splitting it out to separate files would have to break that too. So we can fix the display bug by making show operate on the currently selected counter in the same way as store. Then it makes a bit more sense and we can delete the "counter %d: " prefix.
ETM4 already does it this way too.
I agree with this.
The difficulty in having a file per counter is that different implementations have different numbers of counters. Rather than complicate the driver by dynamically generating the sysfs files, the single file + selector paradigm makes things easier. The index is validated against the actual number of counters for this instance, and read/write occurs for the selected one.
Regards
Mike
The interface isn't even that great, it should be a separate file per counter. You don't want to be parsing strings and colons to try to read a single value, especially in C. Separate files allows you to read it directly without any hassle.
drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c index 762109307b86..312033e74b7a 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c @@ -725,7 +725,7 @@ static ssize_t cntr_val_show(struct device *dev, 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",
ret += sprintf(buf + ret, "counter %d: %x\n", i, config->cntr_val[i]); spin_unlock(&drvdata->spinlock); return ret;@@ -733,7 +733,7 @@ static ssize_t cntr_val_show(struct device *dev, for (i = 0; i < drvdata->nr_cntr; i++) { val = etm_readl(drvdata, ETMCNTVRn(i));
ret += sprintf(buf, "counter %d: %x\n", i, val);
ret += sprintf(buf + ret, "counter %d: %x\n", i, val);
On 26/11/2025 10:49 am, Mike Leach wrote:
Hi,
On Mon, 24 Nov 2025 at 16:12, James Clark james.clark@linaro.org wrote:
On 21/11/2025 5:02 pm, Kuan-Wei Chiu wrote:
Hi James,
On Fri, Nov 21, 2025 at 09:50:03AM +0000, James Clark wrote:
On 21/11/2025 12:23 am, Kuan-Wei Chiu wrote:
The cntr_val_show() function is meant to display the values of all available counters. However, the sprintf() call inside the loop was always writing to the beginning of the buffer, causing the output of previous iterations to be overwritten. As a result, only the value of the last counter was actually returned to the user.
Fix this by using the return value of sprintf() to calculate the correct offset into the buffer for the next write, ensuring that all counter values are appended sequentially.
Fixes: a939fc5a71ad ("coresight-etm: add CoreSight ETM/PTM driver") Signed-off-by: Kuan-Wei Chiu visitorckw@gmail.com
Build tested only. I do not have the hardware to run the etm3x driver, so I would be grateful if someone could verify this on actual hardware.
I noticed this issue while browsing the coresight code after attending a technical talk on the subject. This code dates back to the initial driver submission over 10 years ago, so I was surprised it hadn't been caught earlier. Although I cannot perform runtime testing, the logic error seems obvious to me, so I still decided to submit this patch.
Nice find. I think the point that it wasn't caught changes how we fix it. Either nobody used it ever - so we can just delete it. Or someone was using it and they expect it to always return a single entry with the value of the last counter and this is a potentially breaking change. So maybe instead of fixing this we should add a new cntr_vals_show() which works correctly. But then again if nobody is using it we shouldn't do that either.
Thanks for your feedback.
I agree that if any tool relies on the current behavior, this patch would break the ABI and violate the hard rule that we must never break userspace.
However, I am not sure how to determine if any userspace tools are actually using this sysfs interface. Is there a recommended way to verify this, or a standard procedure/convention to follow in this situation?
Regards, Kuan-Wei
There's no way of knowing apart from deducing that there are 0 users of the 'correct' version of the API because it never existed. This isn't even a regression, it was broken from the beginning.
I suppose it does work when hardware only has one counter, but I don't know how likely that is?
Looking at cntr_val_store() I think I was wrong before when I said it should be a separate file for each counter. Writing to it already writes to the counter currently selected by cntr_idx and splitting it out to separate files would have to break that too. So we can fix the display bug by making show operate on the currently selected counter in the same way as store. Then it makes a bit more sense and we can delete the "counter %d: " prefix.
ETM4 already does it this way too.
I agree with this.
The difficulty in having a file per counter is that different implementations have different numbers of counters. Rather than complicate the driver by dynamically generating the sysfs files, the
It wouldn't be that hard because there is a maximum number of counters. You'd generate them all and then use the is_visible() thing to hide ones that didn't exist on that device.
But it looks like we don't want to do it that way for other reasons anyway.
single file + selector paradigm makes things easier. The index is validated against the actual number of counters for this instance, and read/write occurs for the selected one.
Regards
Mike
The interface isn't even that great, it should be a separate file per counter. You don't want to be parsing strings and colons to try to read a single value, especially in C. Separate files allows you to read it directly without any hassle.
drivers/hwtracing/coresight/coresight-etm3x-sysfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)diff --git a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c index 762109307b86..312033e74b7a 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm3x-sysfs.c @@ -725,7 +725,7 @@ static ssize_t cntr_val_show(struct device *dev, 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",
ret += sprintf(buf + ret, "counter %d: %x\n", i, config->cntr_val[i]); spin_unlock(&drvdata->spinlock); return ret;@@ -733,7 +733,7 @@ static ssize_t cntr_val_show(struct device *dev, for (i = 0; i < drvdata->nr_cntr; i++) { val = etm_readl(drvdata, ETMCNTVRn(i));
ret += sprintf(buf, "counter %d: %x\n", i, val);
ret += sprintf(buf + ret, "counter %d: %x\n", i, val);
-
James Clark -
Mike Leach