Re: [PATCH v3 4/6] coresight: perf: Support AUX trace pause and resume

[Leo Yan <leo.yan@arm.com>]

Hi Mike,

On Tue, Apr 01, 2025 at 01:50:52PM +0100, Mike Leach wrote:

[...]

> >  static void etm_event_start(struct perf_event *event, int flags)
> >  {
> >         int cpu = smp_processor_id();
> > @@ -463,6 +484,14 @@ static void etm_event_start(struct perf_event *event, int flags)
> >         if (!csdev)
> >                 goto fail;
> >
> 
> Is it possible here that the first call to etm_event_start() also has
> the PERF_EF_RESUME flag set?

The first call has a flow below, using flag 0 but not PERF_EF_RESUME.

  etm_event_add()
    `>  etm_event_start(event, 0);

Note: for the first call, the tracer should be disabled if
'event->hw.aux_paused' is 1.  This is ensured by patch 03.

Thanks,
Leo

> If so it looks like we need to fall through and do a "normal" start to
> get all the ctxt->event_data set up.
> 
> > +       if (flags & PERF_EF_RESUME) {
> > +               if (etm_event_resume(csdev, ctxt) < 0) {
> > +                       dev_err(&csdev->dev, "Failed to resume ETM event.\n");
> > +                       goto fail;
> > +               }
> > +               return;
> > +       }
> > +
> >         /* Have we messed up our tracking ? */
> >         if (WARN_ON(ctxt->event_data))
> >                 goto fail;
> > @@ -545,6 +574,16 @@ static void etm_event_start(struct perf_event *event, int flags)
> >         return;
> >  }
> >
> > +static void etm_event_pause(struct coresight_device *csdev,
> > +                           struct etm_ctxt *ctxt)
> > +{
> > +       if (!ctxt->event_data)
> > +               return;
> > +
> > +       /* Stop tracer */
> > +       coresight_pause_source(csdev);
> > +}
> > +
> >  static void etm_event_stop(struct perf_event *event, int mode)
> >  {
> >         int cpu = smp_processor_id();
> > @@ -555,6 +594,9 @@ static void etm_event_stop(struct perf_event *event, int mode)
> >         struct etm_event_data *event_data;
> >         struct coresight_path *path;
> >
> > +       if (mode & PERF_EF_PAUSE)
> > +               return etm_event_pause(csdev, ctxt);
> > +
> >         /*
> >          * If we still have access to the event_data via handle,
> >          * confirm that we haven't messed up the tracking.
> > @@ -899,7 +941,8 @@ int __init etm_perf_init(void)
> >         int ret;
> >
> >         etm_pmu.capabilities            = (PERF_PMU_CAP_EXCLUSIVE |
> > -                                          PERF_PMU_CAP_ITRACE);
> > +                                          PERF_PMU_CAP_ITRACE |
> > +                                          PERF_PMU_CAP_AUX_PAUSE);
> >
> >         etm_pmu.attr_groups             = etm_pmu_attr_groups;
> >         etm_pmu.task_ctx_nr             = perf_sw_context;
> > --
> > 2.34.1
> >
> 
> If the possible issue above is prevented by perf internals
> 
> Reviewed-by: Mike Leach <mike.leach@linaro.org>
> 
> 
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK