On 11/23/20 6:08 AM, Anshuman Khandual wrote:
On 11/12/20 2:57 PM, Suzuki K Poulose wrote:
On 11/10/20 12:45 PM, Anshuman Khandual wrote:
perf handle structure needs to be shared with the TRBE IRQ handler for capturing trace data and restarting the handle. There is a probability of an undefined reference based crash when etm event is being stopped while a TRBE IRQ also getting processed. This happens due the release of perf handle via perf_aux_output_end(). This stops the sinks via the link before releasing the handle, which will ensure that a simultaneous TRBE IRQ could not happen.
Signed-off-by: Anshuman Khandual anshuman.khandual@arm.com
This might cause problem with traditional sink devices which can be operated in both sysfs and perf mode. This needs to be addressed correctly. One option would be to move the update_buffer callback into the respective sink devices. e.g, disable().
drivers/hwtracing/coresight/coresight-etm-perf.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 534e205..1a37991 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -429,7 +429,9 @@ static void etm_event_stop(struct perf_event *event, int mode) size = sink_ops(sink)->update_buffer(sink, handle, event_data->snk_config); + coresight_disable_path(path); perf_aux_output_end(handle, size); + return; }
As you mentioned, this is not ideal where another session could be triggered on the sink from a different ETM (not for per-CPU sink) in a different mode before you collect the buffer. I believe the best option is to leave the update_buffer() to disable_hw. This would need to pass on the "handle" to the disable_path.
Passing 'handle' into coresight_ops_sink->disable() would enable pushing updated trace data into perf aux buffer. But do you propose to drop the update_buffer() call back completely or just move it into disable() call back (along with PERF_EF_UPDATE mode check) for all individual sinks for now. May be, later it can be dropped off completely.
Yes, once we update the buffer from within the sink_ops->disable(), we don't need the update buffer anymore. It is pointless to have a function that is provided to the external user.
That way the races can be handled inside the sinks. Also, this aligns the perf mode of the sinks with that of the sysfs mode.
Did not get that, could you please elaborate ?
In sysfs mode, we already do an action similar to "update buffer" for all the sinks. (e.g, see tmc_etr_sync_sysfs_buf() ). i.e, update the buffer before the sink is disabled. That is the same we propose above.
Suzuki