[PATCH 3/3] perf/imx_ddr: don't enable counter0 if none of 4 counters are used

[Xu Yang]

> -----Original Message-----
> 
> > In current driver, counter0 will be enabled after ddr_perf_pmu_enable()
> > is called even though none of the 4 counters are used. This will cause
> > counter0 continue to count until ddr_perf_pmu_disabled() is called. If
> > pmu is not disabled all the time, the pmu interrupt will be asserted
> > from time to time due to counter0 will overflow and irq handler will
> > clear it. It's not an expected behavior. This patch will not enable
> > counter0 if none of 4 counters are used.
> >
> > Fixes: 9a66d36cc7ac ("drivers/perf: imx_ddr: Add DDR performance counter
> > support to perf")
> > Signed-off-by: Xu Yang <xu.yang_2 at nxp.com>
> >
> > ---
> > Changes in v2:
> >  - add active events count as suggested from Frank
> > ---
> >  drivers/perf/fsl_imx8_ddr_perf.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c
> > b/drivers/perf/fsl_imx8_ddr_perf.c
> > index d65200d4e96e..761e45f21092 100644
> > --- a/drivers/perf/fsl_imx8_ddr_perf.c
> > +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> > @@ -103,6 +103,7 @@ struct ddr_pmu {
> >  	const struct fsl_ddr_devtype_data *devtype_data;
> >  	int irq;
> >  	int id;
> > +	int active_count;
> >  };
> >
> >  static ssize_t ddr_perf_identifier_show(struct device *dev,
> > @@ -516,6 +517,9 @@ static void ddr_perf_event_start(struct perf_event
> > *event, int flags)
> >
> >  	ddr_perf_counter_enable(pmu, event->attr.config, counter, true);
> >
> > +	if (counter != EVENT_CYCLES_COUNTER)
> > +		pmu->active_count++;
> > +
> 
> [Frank Li] why need check EVENT_CYCLES_COUNTER?
> EVENT_CYCLES_COUNTER should treat as normal event.

According to previous driver design, I think the function of
ddr_perf_pmu_enable() is going to enable EVENT_CYCLES_COUNTER when
other 3 counters are enabled. After your question, I agree with you
EVENT_CYCLES_COUNTER should be treated as a normal counter.

> 
> >  	hwc->state = 0;
> >  }
> >
> > @@ -569,6 +573,9 @@ static void ddr_perf_event_stop(struct perf_event
> > *event, int flags)
> >  	ddr_perf_counter_enable(pmu, event->attr.config, counter, false);
> >  	ddr_perf_event_update(event);
> >
> > +	if (counter != EVENT_CYCLES_COUNTER)
> > +		pmu->active_count--;
> > +
> >  	hwc->state |= PERF_HES_STOPPED;
> >  }
> >
> > @@ -589,7 +596,8 @@ static void ddr_perf_pmu_enable(struct pmu *pmu)
> >  	struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> >
> >  	/* enable cycle counter if cycle is not active event list */
> > -	if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > +	if ((ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > +		&& ddr_pmu->active_count > 0)
> 
> [Frank Li] suppose it should
> 		If (ddr_pmu->active_count > 0)
> 			enable()

After EVENT_CYCLES_COUNTER is treated as a normal counter, this if 
condition can be changed to yours.

> 
> >  		ddr_perf_counter_enable(ddr_pmu,
> >  				      EVENT_CYCLES_ID,
> >  				      EVENT_CYCLES_COUNTER,
> > @@ -600,7 +608,8 @@ static void ddr_perf_pmu_disable(struct pmu *pmu)
> >  {
> >  	struct ddr_pmu *ddr_pmu = to_ddr_pmu(pmu);
> >
> > -	if (ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > +	if ((ddr_pmu->events[EVENT_CYCLES_COUNTER] == NULL)
> > +		&& ddr_pmu->active_count > 0)
> 
> Supposed check condition should be
> 		If (ddr_pmu->active_count == 0)
> 			Disable() ..

But this if condition can't be changed to above form.
The kernel/event/core.c driver contains a lot of below sequence:

perf_pmu_disable(event->pmu);
do something... (such as add/delete/enable/stop/... events)
perf_pmu_enable(event->pmu);

If we perf stat any non-cycle counter and stop it, perf_pmu_disable()
will not disable cycle counter since the active_count is at least 1 at
this point. Finally, cycle counter is still running.

For this reason, I would suggest remove if condition in ddr_perf_pmu_disable().
Do you have other advices?

Thanks,
Xu Yang

> 
> >  		ddr_perf_counter_enable(ddr_pmu,
> >  				      EVENT_CYCLES_ID,
> >  				      EVENT_CYCLES_COUNTER,
> > --
> > 2.34.1