Re: [PATCH 09/25] coresight: perf: Improve hardware state transitions

[James Clark <james.clark@linaro.org>]

On 15/09/2026 16:48, Leo Yan wrote:
> Keep hw.state consistent with event activation and AUX buffer updates.
> Initialize events as stopped and up to date, clear both flags after a
> successful start, and set UPTODATE after committing an AUX buffer
> update. On resume, clear UPTODATE only on success. Preserve state on
> resume failure and ignore duplicate stops.
> 
> Add helpers for setting and clearing state bits. Use READ_ONCE() and
> WRITE_ONCE() to prevent compiler merging or refetching of state
> accesses.
> 
> Assisted-by: Codex:gpt-6
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-etm-perf.c | 58 ++++++++++++++++--------
>   1 file changed, 40 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index e3c9d3cf84c502f357a430bc3bfa854a776618f1..f66c99b26e1862ab04105704276c901b21728207 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -150,6 +150,20 @@ etm_event_cpu_path(struct etm_event_data *data, int cpu)
>   	return *etm_event_cpu_path_ptr(data, cpu);
>   }
>   
> +static inline void etm_event_set_hw_state(struct perf_event *event, int mask)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	WRITE_ONCE(hwc->state, READ_ONCE(hwc->state) | mask);
> +}
> +
> +static inline void etm_event_clear_hw_state(struct perf_event *event, int mask)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	WRITE_ONCE(hwc->state, READ_ONCE(hwc->state) & ~mask);
> +}
> +
>   static void etm_event_read(struct perf_event *event) {}
>   
>   static int etm_addr_filters_alloc(struct perf_event *event)
> @@ -520,6 +534,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>   static int etm_event_resume(struct etm_ctxt *ctxt)
>   {
>   	struct perf_output_handle *handle = &ctxt->handle;
> +	struct perf_event *event = handle->event;
>   	struct coresight_device *source;
>   	struct coresight_path *path;
>   	int ret;
> @@ -536,10 +551,13 @@ static int etm_event_resume(struct etm_ctxt *ctxt)
>   		return 0;
>   
>   	ret = coresight_resume_source(source);
> -	if (ret < 0)
> +	if (ret < 0) {
>   		dev_err(&source->dev, "Failed to resume ETM event.\n");
> +		return ret;
> +	}
>   
> -	return ret;
> +	etm_event_clear_hw_state(event, PERF_HES_UPTODATE);
> +	return 0;
>   }
>   
>   static void etm_event_start(struct perf_event *event, int flags)
> @@ -553,8 +571,7 @@ static void etm_event_start(struct perf_event *event, int flags)
>   	u64 hw_id;
>   
>   	if (flags & PERF_EF_RESUME) {
> -		if (etm_event_resume(ctxt) < 0)
> -			goto fail;
> +		WARN_ON_ONCE(etm_event_resume(ctxt));
>   		return;
>   	}
>   
> @@ -583,7 +600,7 @@ static void etm_event_start(struct perf_event *event, int flags)
>   	 */
>   	if (!cpumask_test_cpu(cpu, &event_data->mask)) {
>   		perf_aux_output_end(handle, 0);
> -		event->hw.state = 0;
> +		etm_event_clear_hw_state(event, PERF_HES_STOPPED);
>   		return;
>   	}
>   
> @@ -621,7 +638,7 @@ static void etm_event_start(struct perf_event *event, int flags)
>   	}
>   
>   	/* Tell the perf core the event is alive */
> -	event->hw.state = 0;
> +	etm_event_clear_hw_state(event, PERF_HES_STOPPED | PERF_HES_UPTODATE);
>   	/* Save the event_data for this ETM */
>   	WRITE_ONCE(ctxt->event_data, event_data);
>   	return;
> @@ -639,11 +656,12 @@ static void etm_event_start(struct perf_event *event, int flags)
>   		perf_aux_output_end(handle, 0);
>   	}
>   fail:
> -	event->hw.state = PERF_HES_STOPPED;
> +	etm_event_set_hw_state(event, PERF_HES_STOPPED);
>   	return;
>   }
>   
> -static void etm_event_update_buffer(struct perf_output_handle *handle,
> +static void etm_event_update_buffer(struct perf_event *event,
> +				    struct perf_output_handle *handle,
>   				    struct etm_event_data *event_data,
>   				    struct coresight_device *sink,
>   				    int mode)
> @@ -668,6 +686,7 @@ static void etm_event_update_buffer(struct perf_output_handle *handle,
>   		size = sink_ops(sink)->update_buffer(sink, handle,
>   						     event_data->snk_config);
>   		perf_aux_output_end(handle, size);
> +		etm_event_set_hw_state(event, PERF_HES_UPTODATE);
>   		return;
>   	}
>   
> @@ -704,7 +723,8 @@ static void etm_event_pause(struct coresight_path *path,
>   		return;
>   
>   	event_data = READ_ONCE(ctxt->event_data);
> -	etm_event_update_buffer(handle, event_data, sink, PERF_EF_UPDATE);
> +	etm_event_update_buffer(event, handle, event_data, sink,
> +				PERF_EF_UPDATE);
>   
>   	/* Prepare the handle for resuming trace */
>   	perf_aux_output_begin(handle, event);
> @@ -716,8 +736,13 @@ static void etm_event_stop(struct perf_event *event, int mode)
>   	struct etm_ctxt *ctxt = this_cpu_ptr(&etm_ctxt);
>   	struct perf_output_handle *handle = &ctxt->handle;
>   	struct coresight_path *path = etm_event_get_ctxt_path(ctxt);
> +	struct hw_perf_event *hwc = &event->hw;
>   	struct etm_event_data *event_data;
>   
> +	/* If we're already stopped, then nothing to do */
> +	if (READ_ONCE(hwc->state) & PERF_HES_STOPPED)
> +		return;
> +
>   	if (mode & PERF_EF_PAUSE)
>   		return etm_event_pause(path, event, ctxt);
>   
> @@ -726,7 +751,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
>   	 * to synchronize. Mark the event stopped and up to date.
>   	 */
>   	if (!path) {
> -		event->hw.state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
> +		etm_event_set_hw_state(event, PERF_HES_STOPPED | PERF_HES_UPTODATE);
>   		return;
>   	}
>   
> @@ -734,9 +759,6 @@ static void etm_event_stop(struct perf_event *event, int mode)
>   	/* Clear the event_data as this ETM is stopping the trace. */
>   	WRITE_ONCE(ctxt->event_data, NULL);
>   
> -	if (event->hw.state == PERF_HES_STOPPED)
> -		return;
> -
>   	source = coresight_get_source(path);
>   	sink = coresight_get_sink(path);
>   	if (!source || !sink)
> @@ -746,9 +768,9 @@ static void etm_event_stop(struct perf_event *event, int mode)
>   	coresight_disable_source(source, event);
>   
>   	/* tell the core */
> -	event->hw.state = PERF_HES_STOPPED;
> +	etm_event_set_hw_state(event, PERF_HES_STOPPED);
>   
> -	etm_event_update_buffer(handle, event_data, sink, mode);
> +	etm_event_update_buffer(event, handle, event_data, sink, mode);
>   
>   	/* Disabling the path make its elements available to other sessions */
>   	coresight_disable_path(path);
> @@ -759,12 +781,12 @@ static int etm_event_add(struct perf_event *event, int mode)
>   	int ret = 0;
>   	struct hw_perf_event *hwc = &event->hw;
>   
> +	etm_event_set_hw_state(event, PERF_HES_STOPPED | PERF_HES_UPTODATE);
> +
>   	if (mode & PERF_EF_START) {
>   		etm_event_start(event, 0);
> -		if (hwc->state & PERF_HES_STOPPED)
> +		if (READ_ONCE(hwc->state) & PERF_HES_STOPPED)
>   			ret = -EINVAL;
> -	} else {
> -		hwc->state = PERF_HES_STOPPED;
>   	}
>   
>   	return ret;
> 

Reviewed-by: James Clark <james.clark@linaro.org>