[PATCH] staging: media: atomisp: refactor ia_css_stream_destroy

[PATCH] staging: media: atomisp: refactor ia_css_stream_destroy

[Jose A. Perez de Azpillaga]

Refactor the ISP2401 cleanup logic into a separate helper
function.

Fix a logic bug where the loop variable 'i' was being shadowed and
overwritten by a nested loop, potentially causing incorrect cleanup
behavior.

Replace an early 'return -EINVAL' with 'continue' within the cleanup
loop. In a destruction path, it is better to proceed with cleaning up as
many resources as possible rather than aborting early, which would
result in memory leaks for the remaining pipes.

Remove 'assert(entry)' in favor of an explicit null pointer check ('if
(!entry) continue;'). This avoids a macro-based assertions and ensuring
the system remains stable even if a pointer is unexpectedly null during
cleanup.

Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 drivers/staging/media/atomisp/pci/sh_css.c | 89 ++++++++++++----------
 1 file changed, 47 insertions(+), 42 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
index 6cda5925fa45..076a75c9d0fb 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -8189,6 +8189,51 @@ ia_css_stream_create(const struct ia_css_stream_config *stream_config,
 	return err;
 }
 
+static void ia_css_stream_destroy_isp2401(struct ia_css_stream *stream)
+{
+	int i, j;
+
+	for (i = 0; i < stream->num_pipes; i++) {
+		struct ia_css_pipe *entry = stream->pipes[i];
+		unsigned int sp_thread_id;
+		struct sh_css_sp_pipeline_terminal *terminal;
+
+		if (!entry)
+			continue;
+
+		/* get the SP thread id */
+		if (!ia_css_pipeline_get_sp_thread_id(ia_css_pipe_get_pipe_num(entry),
+						      &sp_thread_id))
+			continue;
+
+		/* get the target input terminal */
+		terminal = &sh_css_sp_group.pipe_io[sp_thread_id].input;
+
+		for (j = 0; j < IA_CSS_STREAM_MAX_ISYS_STREAM_PER_CH; j++) {
+			ia_css_isys_stream_h isys_stream =
+				&terminal->context.virtual_input_system_stream[j];
+
+			if (stream->config.isys_config[j].valid && isys_stream->valid)
+				ia_css_isys_stream_destroy(isys_stream);
+		}
+	}
+
+	if (stream->config.mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR) {
+		for (i = 0; i < stream->num_pipes; i++) {
+			/*
+			 * free any mipi frames that are remaining:
+			 * some test stream create-destroy cycles do
+			 * not generate output frames
+			 * and the mipi buffer is not freed in the
+			 * deque function
+			 */
+			if (stream->pipes[i])
+				free_mipi_frames(stream->pipes[i]);
+		}
+	}
+	stream_unregister_with_csi_rx(stream);
+}
+
 int
 ia_css_stream_destroy(struct ia_css_stream *stream)
 {
@@ -8206,48 +8251,8 @@ ia_css_stream_destroy(struct ia_css_stream *stream)
 
 	if ((stream->last_pipe) &&
 	    ia_css_pipeline_is_mapped(stream->last_pipe->pipe_num)) {
-		if (IS_ISP2401) {
-			for (i = 0; i < stream->num_pipes; i++) {
-				struct ia_css_pipe *entry = stream->pipes[i];
-				unsigned int sp_thread_id;
-				struct sh_css_sp_pipeline_terminal *sp_pipeline_input_terminal;
-
-				assert(entry);
-				if (entry) {
-					/* get the SP thread id */
-					if (!ia_css_pipeline_get_sp_thread_id(
-							ia_css_pipe_get_pipe_num(entry), &sp_thread_id))
-						return -EINVAL;
-
-					/* get the target input terminal */
-					sp_pipeline_input_terminal =
-						&sh_css_sp_group.pipe_io[sp_thread_id].input;
-
-					for (i = 0; i < IA_CSS_STREAM_MAX_ISYS_STREAM_PER_CH; i++) {
-						ia_css_isys_stream_h isys_stream =
-							&sp_pipeline_input_terminal->context.virtual_input_system_stream[i];
-						if (stream->config.isys_config[i].valid && isys_stream->valid)
-							ia_css_isys_stream_destroy(isys_stream);
-					}
-				}
-			}
-
-			if (stream->config.mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR) {
-				for (i = 0; i < stream->num_pipes; i++) {
-					struct ia_css_pipe *entry = stream->pipes[i];
-					/*
-					 * free any mipi frames that are remaining:
-					 * some test stream create-destroy cycles do
-					 * not generate output frames
-					 * and the mipi buffer is not freed in the
-					 * deque function
-					 */
-					if (entry)
-						free_mipi_frames(entry);
-				}
-			}
-			stream_unregister_with_csi_rx(stream);
-		}
+		if (IS_ISP2401)
+			ia_css_stream_destroy_isp2401(stream);
 
 		for (i = 0; i < stream->num_pipes; i++) {
 			struct ia_css_pipe *curr_pipe = stream->pipes[i];
-- 
2.53.0

Re: [PATCH] staging: media: atomisp: refactor ia_css_stream_destroy

[Dan Carpenter]

On Thu, Mar 26, 2026 at 12:04:48AM +0100, Jose A. Perez de Azpillaga wrote:
> Refactor the ISP2401 cleanup logic into a separate helper
> function.
> 
> Fix a logic bug where the loop variable 'i' was being shadowed and
> overwritten by a nested loop, potentially causing incorrect cleanup
> behavior.

This should be done separately and have a Fixes tag.

> 
> Replace an early 'return -EINVAL' with 'continue' within the cleanup
> loop. In a destruction path, it is better to proceed with cleaning up as
> many resources as possible rather than aborting early, which would
> result in memory leaks for the remaining pipes.
> 
> Remove 'assert(entry)' in favor of an explicit null pointer check ('if
> (!entry) continue;'). This avoids a macro-based assertions and ensuring
> the system remains stable even if a pointer is unexpectedly null during
> cleanup.

The assert in atomisp is a wrapper around BUG().  Add that to the commit
message.

You're going to have to redo this anyway because the i vs j fix needs
to be done separately as the first patch and it needs to have a Fixes
tag.  I would kind of prefer if you did this as three patches:

patch 1: fix i vs j bug
patch 2: pull it into a separate function
patch 3: improve the function

It's easier for me to review that way.

regards,
dan carpenter