[PATCH] coresight: etm3x: Fix buffer overwrite in cntr_val_show()

[Mike Leach]

Slight simplification...

On Thu, 27 Nov 2025 at 09:30, James Clark <james.clark at 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