Re: [PATCH v7 02/13] coresight: etm4x: fix underflow for nrseqstate

[Yeoreum Yun]

On Mon, Jun 01, 2026 at 10:52:45AM +0100, Suzuki K Poulose wrote:
> On 19/05/2026 16:48, Yeoreum Yun wrote:
> > TCRSEQEVR<n> is implemented only when TCRIDR5.NUMSEQSTATE is 0b100,
> > in which case n ranges from 0 to 2; otherwise, TCRIDR5.NUMSEQSTATE is 0b000.
> > 
> > Therefore, drvdata->nrseqstate should be checked before entering the loop.
> 
> s/TCR/TRC/g ^^^ and every other patch in the series. Seem like you have been
> playing with the MMU code ;-)

Sorry. I'll fix this typo and Thanks!

> 
> 
> > 
> > Link: https://developer.arm.com/documentation/ihi0064/latest/ [0]
> > Fixes: 2e1cdfe184b5 ("coresight-etm4x: Adding CoreSight ETM4x driver")
> > Reviewed-by: Leo Yan <leo.yan@xxxxxxx>
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> > ---
> >   .../hwtracing/coresight/coresight-etm4x-core.c | 18 ++++++++++--------
> >   .../coresight/coresight-etm4x-sysfs.c          |  2 ++
> >   2 files changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index 1e3b0344dc00..94b9385e964a 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -542,9 +542,11 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
> >   	etm4x_relaxed_write32(csa, config->vissctlr, TRCVISSCTLR);
> >   	if (drvdata->nr_pe_cmp)
> >   		etm4x_relaxed_write32(csa, config->vipcssctlr, TRCVIPCSSCTLR);
> > -	for (i = 0; i < drvdata->nrseqstate - 1; i++)
> > -		etm4x_relaxed_write32(csa, config->seq_ctrl[i], TRCSEQEVRn(i));
> > +
> >   	if (drvdata->nrseqstate) {
> > +		for (i = 0; i < drvdata->nrseqstate - 1; i++)
> > +			etm4x_relaxed_write32(csa, config->seq_ctrl[i], TRCSEQEVRn(i));
> 
> Looking at this again, I think we are contemplating two different values
> from a single variable (blame the architecture). Instead, we should:
> 
> 1. Disconnect number of TRCSEQEVRn from drvdata->nrseqstate, and add
> nr_seq_ctrls to drvdata and initialise this at probe.
> 
> 
> for (i = 0; i < drvdata->nr_seq_ctrls; i++)
> 
> Suzuki

Okay. THen I'll add nr_seq_ctrls instead. Thanks!
> 
> 
> > +
> >   		etm4x_relaxed_write32(csa, config->seq_rst, TRCSEQRSTEVR);
> >   		etm4x_relaxed_write32(csa, config->seq_state, TRCSEQSTR);
> >   	}
> > @@ -1896,10 +1898,10 @@ static int etm4_cpu_save(struct coresight_device *csdev)
> >   	if (drvdata->nr_pe_cmp)
> >   		state->trcvipcssctlr = etm4x_read32(csa, TRCVIPCSSCTLR);
> > -	for (i = 0; i < drvdata->nrseqstate - 1; i++)
> > -		state->trcseqevr[i] = etm4x_read32(csa, TRCSEQEVRn(i));
> > -
> >   	if (drvdata->nrseqstate) {
> > +		for (i = 0; i < drvdata->nrseqstate - 1; i++)
> > +			state->trcseqevr[i] = etm4x_read32(csa, TRCSEQEVRn(i));
> > +
> >   		state->trcseqrstevr = etm4x_read32(csa, TRCSEQRSTEVR);
> >   		state->trcseqstr = etm4x_read32(csa, TRCSEQSTR);
> >   	}
> > @@ -2009,10 +2011,10 @@ static void etm4_cpu_restore(struct coresight_device *csdev)
> >   	if (drvdata->nr_pe_cmp)
> >   		etm4x_relaxed_write32(csa, state->trcvipcssctlr, TRCVIPCSSCTLR);
> > -	for (i = 0; i < drvdata->nrseqstate - 1; i++)
> > -		etm4x_relaxed_write32(csa, state->trcseqevr[i], TRCSEQEVRn(i));
> > -
> >   	if (drvdata->nrseqstate) {
> > +		for (i = 0; i < drvdata->nrseqstate - 1; i++)
> > +			etm4x_relaxed_write32(csa, state->trcseqevr[i], TRCSEQEVRn(i));
> > +
> >   		etm4x_relaxed_write32(csa, state->trcseqrstevr, TRCSEQRSTEVR);
> >   		etm4x_relaxed_write32(csa, state->trcseqstr, TRCSEQSTR);
> >   	}
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > index e9eeea6240d5..0e1ad175aa1e 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c
> > @@ -1395,6 +1395,8 @@ static ssize_t seq_idx_store(struct device *dev,
> >   	struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent);
> >   	struct etmv4_config *config = &drvdata->config;
> > +	if (!drvdata->nrseqstate)
> > +		return -ENOTSUPP;
> >   	if (kstrtoul(buf, 16, &val))
> >   		return -EINVAL;
> >   	if (val >= drvdata->nrseqstate - 1)
> 

-- 
Sincerely,
Yeoreum Yun