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

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

Hi Mohammad,

On Tue Mar 10, 2026 at 10:19 AM 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.
>
> v2:
> -Refactor check condition (Sebastien)
>
> v3:
> -Update assert logic (Jeevan)
>
> Signed-off-by: Mohammed Bilal <mohammed.bilal@intel.com>
> ---
>  tests/chamelium/kms_chamelium_audio.c | 51 ++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/tests/chamelium/kms_chamelium_audio.c b/tests/chamelium/kms_chamelium_audio.c
> index 2967e3c50..252a36274 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,13 +790,18 @@ static void test_display_audio(chamelium_data_t *data,
>  			audio_state_fini(&state);
>  
>  			alsa_close_output(alsa);
> +
> +			/* Fail early if any test fails */
> +			igt_assert_f(success,
> +				     "Audio test failed for format %s, "
> +				     "sampling rate %d Hz and %d channels\n",
> +				     snd_pcm_format_name(format),
> +				     sampling_rate, channels);
Patch looks much better now, but this part seems outside the scope of the
commit message.  Failing fast here is fine, but you don’t mention it
anywhere in the message, and it still bypasses some cleanup. It
would be better to send this change separately.

>  		}
>  	}
>  
>  	/* Make sure we tested at least one frequency and format. */
>  	igt_assert(run);
> -	/* Make sure all runs were successful. */
> -	igt_assert(success);
>  
>  	igt_remove_fb(data->drm_fd, &fb);
>  




-- 
Best regards,
Sebastian