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.

 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);
 	}
 
 	return ret;
-- 
2.52.0.rc2.455.g230fcf2819-goog

On Fri, Nov 21, 2025 at 12:23:50AM +0000, Kuan-Wei Chiu wrote:

[...]

> 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.

I have a question for maintainers.

The ETMv4 architecture specification shows that ETMv4 was released as
a non-confidential module in May 2013 (with the confidential release
even a year earlier). So ETMv4 has been a public IP for more than 12+
years, and ETMv3 has been gradually retired since then.

This fix can still be applied to older kernels, but seems to me that
now might be an appropriate time to consider removing the ETMv3 driver
from the mainline kernel?

Thanks,
Leo

On 26/11/2025 12:09 pm, Leo Yan wrote:
> On Fri, Nov 21, 2025 at 12:23:50AM +0000, Kuan-Wei Chiu wrote:
> 
> [...]
> 
>> 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.
> 
> I have a question for maintainers.
> 
> The ETMv4 architecture specification shows that ETMv4 was released as
> a non-confidential module in May 2013 (with the confidential release
> even a year earlier). So ETMv4 has been a public IP for more than 12+
> years, and ETMv3 has been gradually retired since then.
> 
> This fix can still be applied to older kernels, but seems to me that
> now might be an appropriate time to consider removing the ETMv3 driver
> from the mainline kernel?
> 
> Thanks,
> Leo

Yeah, if anyone is using it it would be on an old kernel surely?

On Wed, Nov 26, 2025 at 12:11:05PM +0000, James Clark wrote:

[...]

> > This fix can still be applied to older kernels, but seems to me that
> > now might be an appropriate time to consider removing the ETMv3 driver
> > from the mainline kernel?
> 
> Yeah, if anyone is using it it would be on an old kernel surely?

We can confirm this in another way: if I don't miss anything, over the
past several years (since 2017), we have not received any questions or
bug reports based on hands-on practice regarding ETMv3 on the Coresight
or perf mailing lists.

Thanks,
Leo

Hi,

On Wed, 26 Nov 2025 at 12:31, Leo Yan <leo.yan@arm.com> wrote:
>
> On Wed, Nov 26, 2025 at 12:11:05PM +0000, James Clark wrote:
>
> [...]
>
> > > This fix can still be applied to older kernels, but seems to me that
> > > now might be an appropriate time to consider removing the ETMv3 driver
> > > from the mainline kernel?
> >
> > Yeah, if anyone is using it it would be on an old kernel surely?
>
> We can confirm this in another way: if I don't miss anything, over the
> past several years (since 2017), we have not received any questions or
> bug reports based on hands-on practice regarding ETMv3 on the Coresight
> or perf mailing lists.
>
> Thanks,
> Leo

The key to this is not the questions we are asked, but which platforms
are still supported by the linux kernel.

The ETMv3 driver supports both ETMv3 and PTM trace (the programming
model is the same, even if the trace decode is vastly different).

So as long as there are  platforms supported that use either of those,
we need to keep the driver in.

Mike

-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

On 26/11/2025 1:42 pm, Mike Leach wrote:
> Hi,
> 
> On Wed, 26 Nov 2025 at 12:31, Leo Yan <leo.yan@arm.com> wrote:
>>
>> On Wed, Nov 26, 2025 at 12:11:05PM +0000, James Clark wrote:
>>
>> [...]
>>
>>>> This fix can still be applied to older kernels, but seems to me that
>>>> now might be an appropriate time to consider removing the ETMv3 driver
>>>> from the mainline kernel?
>>>
>>> Yeah, if anyone is using it it would be on an old kernel surely?
>>
>> We can confirm this in another way: if I don't miss anything, over the
>> past several years (since 2017), we have not received any questions or
>> bug reports based on hands-on practice regarding ETMv3 on the Coresight
>> or perf mailing lists.
>>
>> Thanks,
>> Leo
> 
> The key to this is not the questions we are asked, but which platforms
> are still supported by the linux kernel.
> 
> The ETMv3 driver supports both ETMv3 and PTM trace (the programming
> model is the same, even if the trace decode is vastly different).
> 
> So as long as there are  platforms supported that use either of those,
> we need to keep the driver in.
> 
> Mike
> 

We're not running tests though, so if we find out it's fundamentally 
broken somehow it could be another justification to remove it, even if 
the kernel supports the devices. Do you have a board that you can test 
on Mike?

Maintenance hasn't been zero cost either, even build testing for arm32 
is annoying enough.

On Wed, 26 Nov 2025 at 15:33, James Clark <james.clark@linaro.org> wrote:
>
>
>
> On 26/11/2025 1:42 pm, Mike Leach wrote:
> > Hi,
> >
> > On Wed, 26 Nov 2025 at 12:31, Leo Yan <leo.yan@arm.com> wrote:
> >>
> >> On Wed, Nov 26, 2025 at 12:11:05PM +0000, James Clark wrote:
> >>
> >> [...]
> >>
> >>>> This fix can still be applied to older kernels, but seems to me that
> >>>> now might be an appropriate time to consider removing the ETMv3 driver
> >>>> from the mainline kernel?
> >>>
> >>> Yeah, if anyone is using it it would be on an old kernel surely?
> >>
> >> We can confirm this in another way: if I don't miss anything, over the
> >> past several years (since 2017), we have not received any questions or
> >> bug reports based on hands-on practice regarding ETMv3 on the Coresight
> >> or perf mailing lists.
> >>
> >> Thanks,
> >> Leo
> >
> > The key to this is not the questions we are asked, but which platforms
> > are still supported by the linux kernel.
> >
> > The ETMv3 driver supports both ETMv3 and PTM trace (the programming
> > model is the same, even if the trace decode is vastly different).
> >
> > So as long as there are  platforms supported that use either of those,
> > we need to keep the driver in.
> >
> > Mike
> >
>
> We're not running tests though, so if we find out it's fundamentally
> broken somehow it could be another justification to remove it, even if
> the kernel supports the devices. Do you have a board that you can test
> on Mike?
>

Don't have one myself, but I believe the TC2 was used in development,
(that's the A15/A7 32 bit part - not total compute!) which somewhat
conveniently had both etmv3 and ptm trace.



> Maintenance hasn't been zero cost either, even build testing for arm32
> is annoying enough.
>

Pretty much the seem for any of the arm32 drivers I guess.

Until arm32 kernels are no longer supported, not sure it is safe to
drop the etm3 driver as we do not know who out there might be using
it.

Mike




--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

On Wed, Nov 26, 2025 at 04:14:19PM +0000, Mike Leach wrote:

[...]

> > > The key to this is not the questions we are asked, but which platforms
> > > are still supported by the linux kernel.
> > >
> > > The ETMv3 driver supports both ETMv3 and PTM trace (the programming
> > > model is the same, even if the trace decode is vastly different).
> > >
> > > So as long as there are  platforms supported that use either of those,
> > > we need to keep the driver in.
> > >
> >
> > We're not running tests though, so if we find out it's fundamentally
> > broken somehow it could be another justification to remove it, even if
> > the kernel supports the devices. Do you have a board that you can test
> > on Mike?
> 
> Don't have one myself, but I believe the TC2 was used in development,
> (that's the A15/A7 32 bit part - not total compute!) which somewhat
> conveniently had both etmv3 and ptm trace.

If ETMv4 can be used by Armv7 (arm32) CPUs, and nowdays if Armv7 + ETMv4
is a popular design, it makes sense for me to remove ETMv3 driver.

If Armv7 CPUs are always bound to ETMv3 / PTM, then we should keep the
driver.

Thanks,
Leo

On 27/11/2025 9:29 am, Leo Yan wrote:
> On Wed, Nov 26, 2025 at 04:14:19PM +0000, Mike Leach wrote:
> 
> [...]
> 
>>>> The key to this is not the questions we are asked, but which platforms
>>>> are still supported by the linux kernel.
>>>>
>>>> The ETMv3 driver supports both ETMv3 and PTM trace (the programming
>>>> model is the same, even if the trace decode is vastly different).
>>>>
>>>> So as long as there are  platforms supported that use either of those,
>>>> we need to keep the driver in.
>>>>
>>>
>>> We're not running tests though, so if we find out it's fundamentally
>>> broken somehow it could be another justification to remove it, even if
>>> the kernel supports the devices. Do you have a board that you can test
>>> on Mike?
>>
>> Don't have one myself, but I believe the TC2 was used in development,
>> (that's the A15/A7 32 bit part - not total compute!) which somewhat
>> conveniently had both etmv3 and ptm trace.
> 
> If ETMv4 can be used by Armv7 (arm32) CPUs, and nowdays if Armv7 + ETMv4
> is a popular design, it makes sense for me to remove ETMv3 driver.
> 
> If Armv7 CPUs are always bound to ETMv3 / PTM, then we should keep the
> driver.
> 
> Thanks,
> Leo

Based on the the build system I don't think what the hardware supports 
matters that much. We've always only built the ETM4 driver on Arm64 and 
only build the ETM3 driver on Arm32.

I just found another bug which I think makes sense to log here. We've 
pretty much always turned on timestamps automatically in Perf, but for 
some reason the option validator in Perf thinks they're not supported in 
ETM3. The driver has always accepted it as an option so I'm not sure 
why. The end result is that the command will always fail, and you 
couldn't even undo it with "timestamp=0" until more recently when I 
fixed that.

Sysfs or per-thread Perf mode would be working, but it's one more bit of 
evidence to take in.

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.

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);
>   	}
>   
>   	return ret;

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

> 
> 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);
> >   	}
> >   	return ret;
>

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);
>>>    	}
>>>    	return ret;
>>

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);
> >>>     }
> >>>     return ret;
> >>
>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

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);
>>>>>      }
>>>>>      return ret;
>>>>
>>
> 
>

On Wed, Nov 26, 2025 at 10:57:23AM +0000, James Clark wrote:
> 
> 
> 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.
> 

I don't feel it is my place to say whether the etm3x driver should be
removed entirely.

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);
 	}
 
-	return ret;
+	val = etm_readl(drvdata, ETMCNTVRn(config->cntr_idx));
+	return sprintf(buf, "%x\n", val);
 }
 
 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.

Regards,
Kuan-Wei


> > 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);
> > > > > >      }
> > > > > >      return ret;
> > > > > 
> > > 
> > 
> > 
>

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);
>  	}
>  
> -	return ret;
> +	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.

> +	return sprintf(buf, "%x\n", val);
>  }
>  
>  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

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);
>>   	}
>>   
>> -	return ret;
>> +	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);
>>   }
>>   
>>   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

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
>


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

On Thu, Nov 27, 2025 at 09:30:28AM +0000, James Clark wrote:

[...]

> > > -	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);
> > >   	}
> > > -	return ret;
> > > +	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.

Makes sense. So the pasted change is fine for me.

Thanks for correcting!

On 27/11/2025 8:44 am, Kuan-Wei Chiu wrote:
> On Wed, Nov 26, 2025 at 10:57:23AM +0000, James Clark wrote:
>>
>>
>> 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.
>>
> 
> I don't feel it is my place to say whether the etm3x driver should be
> removed entirely.
> 
> 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);
>   	}
>   
> -	return ret;
> +	val = etm_readl(drvdata, ETMCNTVRn(config->cntr_idx));
> +	return sprintf(buf, "%x\n", val);
>   }
>   
>   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.
> 
> Regards,
> Kuan-Wei
> 
> 

The change looks good. I wouldn't wait for rc1, nobody else is touching 
these files so nothing will change, so makes sense to send it now.

>>> 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);
>>>>>>>       }
>>>>>>>       return ret;
>>>>>>
>>>>
>>>
>>>
>>