Re: [PATCH v3 15/19] perf test cs-etm: Remove unused Coresight workloads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The function `cs_etm__synth_instruction_sample` takes a `packet` argument but ignores it in favor of `tidq->packet` for critical fields, leading to incorrect instruction synthesis.
- [Medium] Incorrect validation of the double pointer `thread` in `cs_etm__etmq_update_thread` breaks the fallback logic and creates a potential segmentation fault hazard.
--

commit b1b29e2b51869a73781c3576698bb30e8e7b0cb9
Author: James Clark <james.clark@linaro.org>

perf test cs-etm: Remove unused Coresight workloads

This commit removes unused CoreSight workloads from the perf tests. These
workloads had issues with out-of-source builds and slow compilation times.

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 40c6ddfa8c8d9..ce570913669c1 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -1490,34 +1508,51 @@ cs_etm__get_trace(struct cs_etm_queue *etmq)
[ ... ]
> +/*
> + * Convert a raw thread number to a thread struct and assign it to **thread.
> + */
> +static int cs_etm__etmq_update_thread(struct cs_etm_queue *etmq,
> +				      ocsd_ex_level el, pid_t tid,
> +				      struct thread **thread)
>  {
>  	struct machine *machine = cs_etm__get_machine(etmq, el);
>  
> +	if (!machine || !*thread)
> +		return -EINVAL;

[Severity: Medium]
Will this early return prevent the fallback logic from executing?

If *thread is initially NULL (e.g. from an allocation failure), this check
returns -EINVAL immediately instead of reaching the fallback assignment
*thread = machine__idle_thread(machine) at the bottom of the function.
Additionally, checking !*thread before checking if the pointer thread
itself is valid could lead to a segmentation fault if thread is NULL. Should
this verify !thread instead, or should the !*thread check be removed to
allow the idle thread fallback?

> +
>  	if (tid != -1) {
> -		thread__zput(tidq->thread);
> -		tidq->thread = machine__find_thread(machine, -1, tid);
> +		thread__zput(*thread);
> +		*thread = machine__find_thread(machine, -1, tid);
>  	}
>  
>  	/* Couldn't find a known thread */
> -	if (!tidq->thread)
> -		tidq->thread = machine__idle_thread(machine);
> +	if (!*thread)
> +		*thread = machine__idle_thread(machine);
>  
> -	tidq->el = el;
> +	return 0;
>  }
[ ... ]
> @@ -1579,6 +1614,7 @@ static inline u64 cs_etm__resolve_sample_time(struct cs_etm_queue *etmq,
>  
>  static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
>  					    struct cs_etm_traceid_queue *tidq,
> +					    struct cs_etm_packet *packet,
>  					    u64 addr, u64 period)
>  {
>  	int ret = 0;
[ ... ]
>  	sample.flags = tidq->prev_packet->flags;
>  	sample.cpumode = event->sample.header.misc;
>  
> -	cs_etm__copy_insn(etmq, tidq->trace_chan_id, tidq->packet, &sample);
> +	cs_etm__copy_insn(etmq, tidq, tidq->packet, &sample);

[Severity: High]
Is the correct packet being used for instruction synthesis here?

The function signature was updated to take a struct cs_etm_packet *packet
argument, but this line explicitly references tidq->packet instead of the
passed packet. Since callers like cs_etm__flush() and cs_etm__end_block()
pass tidq->prev_packet as the argument, referencing tidq->packet here will
use stale or incorrect packet data, potentially corrupting synthesized trace
data.

>  
>  	if (etm->synth_opts.last_branch)
>  		sample.branch_stack = tidq->last_branch;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-james-cs-context-tracking-fix-v3-0-c392945d9ed5@linaro.org?part=15