Re: [PATCH v4 2/2] tests/chamelium/audio: Fix resource leaks on assertion failure

[Sebastian Brzezinka <sebastian.brzezinka@intel.com>]

Hi Mohammed,

On Mon Mar 16, 2026 at 12:47 PM CET, Mohammed Bilal wrote:
> Fatal assertions can bypass cleanup paths, causing resources to remain
> unreleased when failures occur during audio tests.
>
> Replace fatal assertions with non-fatal checks and route failures
> through a common cleanup path to ensure proper cleanup. Exit early
> on first failure to avoid testing remaining combinations unnecessarily.
>
> v2:
> -Refactor check condition (Sebastien)
>
> v3:
> -Update assert logic (Jeevan)
>
> v4:
> -Fix memory leak logic and update commit message (Sebastien)
>
> Signed-off-by: Mohammed Bilal <mohammed.bilal@intel.com>
> Reviewed-by: Jeevan B <jeevan.b@intel.com>
> ---
>  tests/chamelium/kms_chamelium_audio.c | 56 ++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/tests/chamelium/kms_chamelium_audio.c b/tests/chamelium/kms_chamelium_audio.c
> index 2967e3c50..2f17ad91a 100644
> --- a/tests/chamelium/kms_chamelium_audio.c
> +++ b/tests/chamelium/kms_chamelium_audio.c
> @@ -389,11 +389,11 @@ static int audio_output_frequencies_callback(void *data, void *buffer,
>  static bool test_audio_frequencies(struct audio_state *state)
>  {
>  	int freq, step;
> -	int32_t *recv, *buf;
> -	double *channel;
> +	int32_t *recv = NULL, *buf = NULL;
> +	double *channel = NULL;
>  	size_t i, j, streak;
> -	size_t recv_len, buf_len, buf_cap, channel_len;
> -	bool success;
> +	size_t recv_len = 0, buf_len, buf_cap, channel_len;
> +	bool success = false;
>  	int capture_chan;
>  
>  	state->signal = audio_signal_init(state->playback.channels,
> @@ -426,9 +426,12 @@ static bool test_audio_frequencies(struct audio_state *state)
>  
>  	audio_state_start(state, "frequencies");
>  
> -	igt_assert_f(state->capture.rate == state->playback.rate,
> -		     "Capture rate (%dHz) doesn't match playback rate (%dHz)\n",
> -		     state->capture.rate, state->playback.rate);
> +	if (state->capture.rate != state->playback.rate) {
> +		igt_critical("Capture rate (%dHz) doesn't match playback "
> +			     "rate (%dHz)\n",
> +			     state->capture.rate, state->playback.rate);
> +		goto out;
> +	}
>  
>  	/* Needs to be a multiple of 128, because that's the number of samples
>  	 * we get per channel each time we receive an audio page from the
> @@ -447,10 +450,6 @@ static bool test_audio_frequencies(struct audio_state *state)
>  	buf = malloc(sizeof(int32_t) * buf_cap);
>  	buf_len = 0;
>  
> -	recv = NULL;
> -	recv_len = 0;
> -
> -	success = false;
>  	streak = 0;
>  	while (!success && state->msec < AUDIO_TIMEOUT) {
>  		audio_state_receive(state, &recv, &recv_len);
> @@ -460,13 +459,21 @@ static bool test_audio_frequencies(struct audio_state *state)
>  
>  		if (buf_len < buf_cap)
>  			continue;
> -		igt_assert(buf_len == buf_cap);
> +		if (buf_len > buf_cap) {
> +			igt_critical("Buffer overflow: %zu > %zu\n",
> +				     buf_len, buf_cap);
> +			goto out;
> +		}
>  
>  		igt_debug("Detecting audio signal, t=%d msec\n", state->msec);
>  
>  		for (j = 0; j < state->playback.channels; j++) {
>  			capture_chan = state->channel_mapping[j];
> -			igt_assert(capture_chan >= 0);
> +			if (capture_chan < 0) {
> +				igt_critical("Invalid channel mapping for "
> +					     "channel %zu\n", j);
> +				goto out;
> +			}
>  			igt_debug("Processing channel %zu (captured as "
>  				  "channel %d)\n",
>  				  j, capture_chan);
> @@ -489,6 +496,7 @@ static bool test_audio_frequencies(struct audio_state *state)
>  		success = streak == MIN_STREAK * state->playback.channels;
>  	}
>  
> +out:
>  	audio_state_stop(state, success);
>  
>  	free(recv);
> @@ -587,6 +595,7 @@ static bool test_audio_flatline(struct audio_state *state)
>  	recv = NULL;
>  	recv_len = 0;
>  	amp_success = false;
> +	align_success = false;
>  	streak = 0;
>  	while (!amp_success && state->msec < AUDIO_TIMEOUT) {
>  		audio_state_receive(state, &recv, &recv_len);
> @@ -595,7 +604,11 @@ static bool test_audio_flatline(struct audio_state *state)
>  
>  		for (i = 0; i < state->playback.channels; i++) {
>  			capture_chan = state->channel_mapping[i];
> -			igt_assert(capture_chan >= 0);
> +			if (capture_chan < 0) {
> +				igt_critical("Invalid channel mapping for "
> +					     "channel %zu\n", i);
> +				goto out;
> +			}
>  			igt_debug("Processing channel %zu (captured as "
>  				  "channel %d)\n",
>  				  i, capture_chan);
> @@ -661,6 +674,7 @@ static bool test_audio_flatline(struct audio_state *state)
>  		}
>  	}
>  
> +out:
>  	success = amp_success && align_success;
>  	audio_state_stop(state, success);
>  
> @@ -776,19 +790,23 @@ static void test_display_audio(chamelium_data_t *data,
>  			audio_state_fini(&state);
>  
>  			alsa_close_output(alsa);
> +
> +			if (!success)
> +				goto out;
You've slightly changed the logic here, and the test now fails fast. Since
you've explained this behavior change in the commit message, I'm fine
with it.

>  		}
>  	}
>  
> -	/* Make sure we tested at least one frequency and format. */
> -	igt_assert(run);
> -	/* Make sure all runs were successful. */
> -	igt_assert(success);
> -
> +out:
>  	igt_remove_fb(data->drm_fd, &fb);
>  
>  	drmModeFreeConnector(connector);
>  
>  	free(alsa);
> +
> +	/* Make sure we tested at least one frequency and format. */
> +	igt_assert(run);
> +	/* Make sure all runs were successful. */
> +	igt_assert(success);
>  }
>  
>  static const char test_display_audio_edid_desc[] =

Reviewed-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>

-- 
Best regards,
Sebastian