Re: [PATCH RFC 1/3] coresight: tmc: add the handle of the event to the path

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

On Thu, Sep 25, 2025 at 11:10:51AM +0100, James Clark wrote:

[...]

In short, I prefer to store perf handle in coresight path, as Jie has
done in this series.

I will give details below, sorry for a long replying.

[...]

> This one is just a pointer to the perf handle which really does belong to
> the session rather than the device. This makes it more of a path thing than
> a csdev thing. Maybe we can rename path to be more like "session", which
> also happens to contain a path. But I think path is fine for now.

Yes, renaming 'coresight_path' (to like 'coresight_runtime_ctx') better
reflects the structure’s purpose.

The point is we can take chance to separate runtime parameters from
device and driver instances.

  - 'coresight_path': runtime parameters for an active session
  - 'coresight_device': a device instance registered on the bus
  - driver data: after probe, the driver maintains driver-specific
    attributes (e.g., the ETMv4 driver keeps mode and filters in struct
    etmv4_drvdata)

These structures have different lifetimes. For example, coresight_path
is valid only during a session; otherwise, it should be cleared.

From a lifecycle perspective, storing the perf handle in coresight_path
makes sense, since both are valid only for the duration of a session
(the perf handle isn't used in sysfs mode, in which case we can simply
leave it unset).

Furthermore, the perf handle is not just a handle; it lets us easily
retrieve private event data (see perf_get_aux()).

> However in this case handle is per-cpu data that is only accessed on the
> same cpu in tmc_etr_get_buffer(). Assigning it in etm_event_start() just
> copies the same per-cpu variable into a non per-cpu place that eventually
> gets accessed on the same cpu anyway.
> 
> If we exported it then it can be used directly without worrying where to
> store it:

We need to be careful for exporting data structures across modules, as
it makes them harder to manage.

In fact, we already share 'etm_event_data' for ETR driver for the same
purpose, and seems to me, it is redendant to export another structure
'etm_ctxt' to just make it convenient for obtaining a buffer config
pointer.

> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c
> b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 17afa0f4cdee..4c33f442c80b 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -42,12 +42,8 @@ static bool etm_perf_up;
>   * the ETM. Thus the event_data for the session must be part of the ETM
> context
>   * to make sure we can disable the trace path.
>   */
> -struct etm_ctxt {
> -       struct perf_output_handle handle;
> -       struct etm_event_data *event_data;
> -};
> -
> -static DEFINE_PER_CPU(struct etm_ctxt, etm_ctxt);
> +DEFINE_PER_CPU(struct etm_ctxt, etm_ctxt);
> +EXPORT_SYMBOL_GPL(etm_ctxt);

I'd suggest a different approach: drop the etm_ctxt structure and
instead define a per-CPU pointer to etm_event_data:

  static DEFINE_PER_CPU(struct etm_event_data *, etm_ev_ctx);

As mentioned above, if perf handle is maintained in coresight_path, we
can easily retrieve the etm_event_data via the perf handle.

A more aggressive refactoring would remove etm_ctxt from
coresight-etm-perf.c entirely, relying on the perf event to manage
private context data. For now, we keep it only to validate whether
ETM is enabled (see multiple place to validate ctxt->event_data).

Thanks,
Leo