Re: [PATCH v2 2/8] coresight: Introduce pause and resume APIs for source

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

On Mon, Mar 10, 2025 at 12:01:25PM +0000, Suzuki Kuruppassery Poulose wrote:
> On 10/03/2025 10:49, Leo Yan wrote:
> > Introduce APIs for pausing and resuming trace source and export as GPL
> > symbols.
> > 
> > Signed-off-by: Leo Yan <leo.yan@arm.com>
> > ---
> >   drivers/hwtracing/coresight/coresight-core.c | 20 ++++++++++++++++++++
> >   drivers/hwtracing/coresight/coresight-priv.h |  2 ++
> >   include/linux/coresight.h                    |  4 ++++
> >   3 files changed, 26 insertions(+)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index 0a9380350fb5..eb7b83a7bfa2 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -365,6 +365,26 @@ void coresight_disable_source(struct coresight_device *csdev, void *data)
> >   }
> >   EXPORT_SYMBOL_GPL(coresight_disable_source);
> > +void coresight_pause_source(struct coresight_device *csdev)
> > +{
> > +	if (!csdev || !coresight_is_percpu_source(csdev))
> 
> You may remove the !csdev check, coresight_is_percpu_source() already checks
> that.

Will do.

> > +		return;
> > +
> > +	if (source_ops(csdev)->pause)
> > +		source_ops(csdev)->pause(csdev);
> > +}
> > +EXPORT_SYMBOL_GPL(coresight_pause_source);
> > +
> > +void coresight_resume_source(struct coresight_device *csdev)
> > +{
> > +	if (!csdev || !coresight_is_percpu_source(csdev))
> > +		return;
> > +
> 
> Same as above.

Will do.

> > +	if (source_ops(csdev)->resume)
> > +		source_ops(csdev)->resume(csdev);
> > +}
> > +EXPORT_SYMBOL_GPL(coresight_resume_source);
> > +
> >   /*
> >    * coresight_disable_path_from : Disable components in the given path beyond
> >    * @nd in the list. If @nd is NULL, all the components, except the SOURCE are
> > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
> > index 76403530f33e..a9f14c075e91 100644
> > --- a/drivers/hwtracing/coresight/coresight-priv.h
> > +++ b/drivers/hwtracing/coresight/coresight-priv.h
> > @@ -247,5 +247,7 @@ void coresight_add_helper(struct coresight_device *csdev,
> >   void coresight_set_percpu_sink(int cpu, struct coresight_device *csdev);
> >   struct coresight_device *coresight_get_percpu_sink(int cpu);
> >   void coresight_disable_source(struct coresight_device *csdev, void *data);
> > +void coresight_pause_source(struct coresight_device *csdev);
> > +void coresight_resume_source(struct coresight_device *csdev);
> >   #endif
> > diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> > index 17276965ff1d..703e1b8dbe22 100644
> > --- a/include/linux/coresight.h
> > +++ b/include/linux/coresight.h
> > @@ -385,6 +385,8 @@ struct coresight_ops_link {
> >    *		is associated to.
> >    * @enable:	enables tracing for a source.
> >    * @disable:	disables tracing for a source.
> > + * @resume:	resumes tracing for a source.
> > + * @pause:	pauses tracing for a source.
> >    */
> >   struct coresight_ops_source {
> >   	int (*cpu_id)(struct coresight_device *csdev);
> > @@ -392,6 +394,8 @@ struct coresight_ops_source {
> >   		      enum cs_mode mode, struct coresight_trace_id_map *id_map);
> >   	void (*disable)(struct coresight_device *csdev,
> >   			struct perf_event *event);
> > +	int (*resume)(struct coresight_device *csdev);
> > +	void (*pause)(struct coresight_device *csdev);
> 
> Given these callbacks are for perf mode, does it make sense to pass the
> parameters similar to enable() ?

It is not necessary to pass perf event handle to resume() and pause()
callbacks.

The main reason is perf related operations have been processed in the
coresight-etm-perf.c layer, the low level's callbacks (pause/resume)
should simply take actions on hardware logic.

> Also, should we rename these as :
> 
> resume_perf
> pause_perf
> 
> To explicitly mark them perf mode specific ?

This makes sense to me.  The naming "resume" might be confused with
CPUIdle's terminology (CPU resume).  pause_perf/resume_perf can
perfectly avoid any confusion.

Thanks,
Leo