diff mbox series

[v2,03/10] coresight: etm-pmu: Ensure the AUX handle is valid

Message ID 20210723124611.3828908-4-suzuki.poulose@arm.com (mailing list archive)
State New, archived
Headers show
Series coresight: TRBE and Self-Hosted trace fixes | expand

Commit Message

Suzuki K Poulose July 23, 2021, 12:46 p.m. UTC
The ETM perf infrastructure closes out a handle during event_stop
or on an error in starting the event. In either case, it is possible
for a "sink" to update/close the handle, under certain circumstances.
(e.g no space in ring buffer.). So, ensure that we handle this
gracefully in the PMU driver by verifying the handle is still valid.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Leo Yan <leo.yan@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 .../hwtracing/coresight/coresight-etm-perf.c  | 27 ++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

Comments

Anshuman Khandual July 30, 2021, 4:14 a.m. UTC | #1
On 7/23/21 6:16 PM, Suzuki K Poulose wrote:
> The ETM perf infrastructure closes out a handle during event_stop
> or on an error in starting the event. In either case, it is possible
> for a "sink" to update/close the handle, under certain circumstances.
> (e.g no space in ring buffer.). So, ensure that we handle this
> gracefully in the PMU driver by verifying the handle is still valid.
> 
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Leo Yan <leo.yan@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  .../hwtracing/coresight/coresight-etm-perf.c  | 27 ++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 6f398377fec9..a6ab603afee4 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -450,8 +450,15 @@ static void etm_event_start(struct perf_event *event, int flags)
>  fail_disable_path:
>  	coresight_disable_path(path);
>  fail_end_stop:
> -	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> -	perf_aux_output_end(handle, 0);
> +	/*
> +	 * Check if the handle is still associated with the event,
> +	 * to handle cases where if the sink failed to start the
> +	 * trace and TRUNCATED the handle already.
> +	 */
> +	if (READ_ONCE(handle->event)) {
> +		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +		perf_aux_output_end(handle, 0);
> +	}

Makes sense as perf_aux_output_[flag|end]() helpers do not
validate the 'handle' themselves.

>  fail:
>  	event->hw.state = PERF_HES_STOPPED;
>  	goto out;
> @@ -519,7 +526,21 @@ static void etm_event_stop(struct perf_event *event, int mode)
>  
>  		size = sink_ops(sink)->update_buffer(sink, handle,
>  					      event_data->snk_config);
> -		perf_aux_output_end(handle, size);
> +		/*
> +		 * Make sure the handle is still valid as the
> +		 * sink could have closed it from an IRQ.
> +		 * The sink driver must handle the race with
> +		 * update_buffer() and IRQ. Thus either we
> +		 * should get a valid handle and valid size
> +		 * (which may be 0).
> +		 *
> +		 * But we should never get a non-zero size with
> +		 * an invalid handle.
> +		 */
> +		if (READ_ONCE(handle->event))
> +			perf_aux_output_end(handle, size);
> +		else
> +			WARN_ON(size);

Right.

>  	}
>  
>  	/* Disabling the path make its elements available to other sessions */
> 

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 6f398377fec9..a6ab603afee4 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -450,8 +450,15 @@  static void etm_event_start(struct perf_event *event, int flags)
 fail_disable_path:
 	coresight_disable_path(path);
 fail_end_stop:
-	perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
-	perf_aux_output_end(handle, 0);
+	/*
+	 * Check if the handle is still associated with the event,
+	 * to handle cases where if the sink failed to start the
+	 * trace and TRUNCATED the handle already.
+	 */
+	if (READ_ONCE(handle->event)) {
+		perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
+		perf_aux_output_end(handle, 0);
+	}
 fail:
 	event->hw.state = PERF_HES_STOPPED;
 	goto out;
@@ -519,7 +526,21 @@  static void etm_event_stop(struct perf_event *event, int mode)
 
 		size = sink_ops(sink)->update_buffer(sink, handle,
 					      event_data->snk_config);
-		perf_aux_output_end(handle, size);
+		/*
+		 * Make sure the handle is still valid as the
+		 * sink could have closed it from an IRQ.
+		 * The sink driver must handle the race with
+		 * update_buffer() and IRQ. Thus either we
+		 * should get a valid handle and valid size
+		 * (which may be 0).
+		 *
+		 * But we should never get a non-zero size with
+		 * an invalid handle.
+		 */
+		if (READ_ONCE(handle->event))
+			perf_aux_output_end(handle, size);
+		else
+			WARN_ON(size);
 	}
 
 	/* Disabling the path make its elements available to other sessions */