Re: [PATCH] coresight: docs: Remove target sink from examples

[Leo Yan]

On Tue, Mar 11, 2025 at 10:51:39AM +0000, James Clark wrote:

[...]

> > > +Sink selection
> > > +~~~~~~~~~~~~~~
> > > +
> > > +An appropriate sink will be selected automatically for use with Perf, but since
> > > +there will typically be more than one sink, the name of the sink to use may be
> > > +specified as a special config option prefixed with '@'.
> > > +
> > > +The available sinks are listed in sysFS under
> > >   ($SYSFS)/bus/event_source/devices/cs_etm/sinks/::
> > > 
> > >          root@localhost:/sys/bus/event_source/devices/cs_etm/sinks# ls
> > >          tmc_etf0  tmc_etr0  tpiu0
> > 
> > Just a minor comment.  To reflect the latest hardware, it is good to
> > mention the TRBE case, users should not and cannot specify TRBE as the
> 
> Is that strictly true? From looking at the code I think you could pick one
> TRBE sink as long as you are only tracing from a single ETM. Although yeah
> it would be pointless.

My testing result shows perf reports error when specifying trbe as sink:

  # perf record -e cs_etm/@trbe0/ -- ls
  failed to mmap with 12 (Cannot allocate memory)

But I can make success for the command:

  # perf record -C 0 -e cs_etm/@trbe0/ -- ls
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.062 MB perf.data ]

This makes sense to me that if a perf session tries to enable trace for
multiple CPUs but only a CPU sink is supported, the driver should
report error to remind users the command does not work.

> > sink name.  The driver will give priority for TRBE by default unless
> > users specify other sink types.
> > 
> 
> IMO mentioning TRBE would be overly wordy and not really add anything.
> Removing the sink from all the base examples is exactly to make TRBE work
> without going into detail about why. And the advanced section doesn't
> mention TRBE because manually picking it is never right.

The section "Sink selection" applies _only_ to traditional sinks, not
to TRBE.  I am not sure if we should clarify a bit for this.  I don't
have strong opinion on it, as it is a trade-off between providing
necessary info and avoiding overstatement.

So this patch is fine for me:

Reviewed-by: Leo Yan <leo.yan@xxxxxxx>