[PATCH v2 0/3] staging: media: atomisp: clean up ia_css_stream_destroy

[PATCH v2 0/3] staging: media: atomisp: clean up ia_css_stream_destroy

[Jose A. Perez de Azpillaga]

This series fixes a loop shadowing bug and refactors the ISP2401 cleanup
logic inside ia_css_stream_destroy().

Changes in v2:
- Split the original patch into a 3-patch series as requested by
  Dan Carpenter.
- Added a Fixes tag for the loop shadowing bug.
- Separated the extraction from the logical improvements.
- Added clarification that assert() is a wrapper around BUG().

Jose A. Perez de Azpillaga (3):
  staging: media: atomisp: fix loop shadowing in ia_css_stream_destroy()
  staging: media: atomisp: extract ISP2401 cleanup into helper function
  staging: media: atomisp: improve ia_css_stream_destroy_isp2401

 drivers/staging/media/atomisp/pci/sh_css.c | 79 ++++++++++------------
 1 file changed, 37 insertions(+), 42 deletions(-)

--
2.53.0

[PATCH v2 1/3] staging: media: atomisp: fix loop shadowing in ia_css_stream_destroy()

[Jose A. Perez de Azpillaga]

The nested loop inside the IS_ISP2401 block incorrectly uses the same
variable 'i' as the outer loop. This shadows the outer loop variable
and causes premature termination or skipped array elements.

Change the inner loop to use a new variable 'j' to prevent this.

Fixes: 113401c67386 ("media: atomisp: sh_css: Removed #ifdef ISP2401 to make code generic")
Signed-off-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
---
 drivers/staging/media/atomisp/pci/sh_css.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
index 6cda5925fa45..8d8b82dc59f1 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -8192,7 +8192,7 @@ ia_css_stream_create(const struct ia_css_stream_config *stream_config,
 int
 ia_css_stream_destroy(struct ia_css_stream *stream)
 {
-	int i;
+	int i, j;
 	int err = 0;
 
 	IA_CSS_ENTER_PRIVATE("stream = %p", stream);
@@ -8223,10 +8223,10 @@ ia_css_stream_destroy(struct ia_css_stream *stream)
 					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++) {
+					for (j = 0; j < IA_CSS_STREAM_MAX_ISYS_STREAM_PER_CH; j++) {
 						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)
+							&sp_pipeline_input_terminal->context.virtual_input_system_stream[j];
+						if (stream->config.isys_config[j].valid && isys_stream->valid)
 							ia_css_isys_stream_destroy(isys_stream);
 					}
 				}
-- 
2.53.0

[PATCH v2 2/3] staging: media: atomisp: extract ISP2401 cleanup into helper function

[Jose A. Perez de Azpillaga]

To reduce indentation and improve the readability of
ia_css_stream_destroy(), extract the ISP2401-specific cleanup block into
a new static helper function, ia_css_stream_destroy_isp2401().

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

diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
index 8d8b82dc59f1..0e848758723f 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -8189,10 +8189,53 @@ 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;
+
+		assert(entry);
+		if (entry) {
+			if (!ia_css_pipeline_get_sp_thread_id(
+				ia_css_pipe_get_pipe_num(entry), &sp_thread_id))
+				return;
+
+			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++) {
+			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);
+}
+
 int
 ia_css_stream_destroy(struct ia_css_stream *stream)
 {
-	int i, j;
+	int i;
 	int err = 0;
 
 	IA_CSS_ENTER_PRIVATE("stream = %p", stream);
@@ -8206,48 +8249,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 (j = 0; j < IA_CSS_STREAM_MAX_ISYS_STREAM_PER_CH; j++) {
-						ia_css_isys_stream_h isys_stream =
-							&sp_pipeline_input_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++) {
-					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

[PATCH v2 3/3] staging: media: atomisp: improve cleanup robustness in ia_css_stream_destroy_isp2401()

[Jose A. Perez de Azpillaga]

Remove the 'assert(entry)' call. In the atomisp driver, assert() is a
wrapper around BUG(), which intentionally crashes the entire kernel.
This is dangerous and inappropriate for a simple null pointer check
during stream teardown. Replace it with a safe 'if (!entry) continue;'
check.

Change the early 'return' to a 'continue'. 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.

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

diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
index 0e848758723f..8c368940acd8 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -8198,26 +8198,25 @@ static void ia_css_stream_destroy_isp2401(struct ia_css_stream *stream)
 		unsigned int sp_thread_id;
 		struct sh_css_sp_pipeline_terminal *terminal;
 
-		assert(entry);
-		if (entry) {
-			if (!ia_css_pipeline_get_sp_thread_id(
+		if (!entry)
+			continue;
+
+		if (!ia_css_pipeline_get_sp_thread_id(
 				ia_css_pipe_get_pipe_num(entry), &sp_thread_id))
-				return;
+			continue;
 
-			terminal = &sh_css_sp_group.pipe_io[sp_thread_id].input;
+		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);
-			}
+		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++) {
-			struct ia_css_pipe *entry = stream->pipes[i];
 			/*
 			 * free any mipi frames that are remaining:
 			 * some test stream create-destroy cycles do
@@ -8225,8 +8224,8 @@ static void ia_css_stream_destroy_isp2401(struct ia_css_stream *stream)
 			 * and the mipi buffer is not freed in the
 			 * deque function
 			 */
-			if (entry)
-				free_mipi_frames(entry);
+			if (stream->pipes[i])
+				free_mipi_frames(stream->pipes[i]);
 		}
 	}
 	stream_unregister_with_csi_rx(stream);
-- 
2.53.0

Re: [PATCH v2 0/3] staging: media: atomisp: clean up ia_css_stream_destroy

[Dan Carpenter]

On Thu, Mar 26, 2026 at 10:34:06PM +0100, Jose A. Perez de Azpillaga wrote:
> This series fixes a loop shadowing bug and refactors the ISP2401 cleanup
> logic inside ia_css_stream_destroy().
> 
> Changes in v2:
> - Split the original patch into a 3-patch series as requested by
>   Dan Carpenter.
> - Added a Fixes tag for the loop shadowing bug.
> - Separated the extraction from the logical improvements.
> - Added clarification that assert() is a wrapper around BUG().

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter