[PATCH v2] staging: media: atomisp: refactor pipe graph dump stage formatting

[PATCH v2] staging: media: atomisp: refactor pipe graph dump stage formatting

[Neal Patalay]

The original implementation of ia_css_debug_pipe_graph_dump_stage()
includes an off-by-one error where the original strscpy() size dropped
characters immediately before newlines. It also allocates over 600
bytes across multiple buffers on the stack. Address these shortcomings
and reduce stack usage with a single 256 byte buffer via a new helper
function, ia_css_debug_build_info().

Fixes: 662fb4fceb1a ("media: atomisp: get rid of a string_support.h abstraction layer")
Signed-off-by: Neal Patalay <nealpatalay0@gmail.com>
---
v2:
- Fixed strscpy typo in commit message
- Changed Fixes tag to point to the actual buggy commit
- Dropped "flag" parameter from helper function and moved check to the macro
- Split long lines to conform with subsystem character limit
---
 .../pci/runtime/debug/src/ia_css_debug.c      | 186 +++++++-----------
 1 file changed, 71 insertions(+), 115 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
index 5113aa5973f3..53d47144d8c9 100644
--- a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
+++ b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
@@ -1162,6 +1162,29 @@ void ia_css_debug_pipe_graph_dump_epilogue(void)
 	pg_inst.stream_format = N_ATOMISP_INPUT_FORMAT;
 }
 
+static void ia_css_debug_build_info(char *info, size_t info_size,
+				    int *offset,
+				    const char *flag_str, size_t flag_str_size,
+				    int *line_len)
+{
+	/*
+	 * If new line length exceeds max line length,
+	 * replace the last ',' with a "\\n"
+	 */
+	if (*line_len > 0 && info_size - *offset >= 2 &&
+	    *line_len + flag_str_size > ENABLE_LINE_MAX_LENGTH) {
+		info[*offset - 1] = '\\';
+		info[*offset] = 'n';
+		*offset += 1;
+		*line_len = 0;
+	}
+
+	int len_written = scnprintf(info + *offset, info_size - *offset,
+				    "%s,", flag_str);
+	*offset += len_written;
+	*line_len += len_written;
+}
+
 void
 ia_css_debug_pipe_graph_dump_stage(
     struct ia_css_pipeline_stage *stage,
@@ -1194,123 +1217,56 @@ ia_css_debug_pipe_graph_dump_stage(
 
 	/* Guard in case of binaries that don't have any binary_info */
 	if (stage->binary_info) {
-		char enable_info1[100];
-		char enable_info2[100];
-		char enable_info3[100];
-		char enable_info[302];
+		char enable_info[256];
+		int offset = 0;
+		int line_len = 0;
 		struct ia_css_binary_info *bi = stage->binary_info;
 
-		/* Split it in 2 function-calls to keep the amount of
-		 * parameters per call "reasonable"
-		 */
-		snprintf(enable_info1, sizeof(enable_info1),
-			 "%s%s%s%s%s%s%s%s%s%s%s%s%s%s",
-			 bi->enable.reduced_pipe ?	"rp," : "",
-			 bi->enable.vf_veceven ?		"vfve," : "",
-			 bi->enable.dis ?		"dis," : "",
-			 bi->enable.dvs_envelope ?	"dvse," : "",
-			 bi->enable.uds ?		"uds," : "",
-			 bi->enable.dvs_6axis ?		"dvs6," : "",
-			 bi->enable.block_output ?	"bo," : "",
-			 bi->enable.ds ?			"ds," : "",
-			 bi->enable.bayer_fir_6db ?	"bf6," : "",
-			 bi->enable.raw_binning ?	"rawb," : "",
-			 bi->enable.continuous ?		"cont," : "",
-			 bi->enable.s3a ?		"s3a," : "",
-			 bi->enable.fpnr ?		"fpnr," : "",
-			 bi->enable.sc ?			"sc," : ""
-			);
-
-		snprintf(enable_info2, sizeof(enable_info2),
-			 "%s%s%s%s%s%s%s%s%s%s%s",
-			 bi->enable.macc ?		"macc," : "",
-			 bi->enable.output ?		"outp," : "",
-			 bi->enable.ref_frame ?		"reff," : "",
-			 bi->enable.tnr ?		"tnr," : "",
-			 bi->enable.xnr ?		"xnr," : "",
-			 bi->enable.params ?		"par," : "",
-			 bi->enable.ca_gdc ?		"cagdc," : "",
-			 bi->enable.isp_addresses ?	"ispa," : "",
-			 bi->enable.in_frame ?		"inf," : "",
-			 bi->enable.out_frame ?		"outf," : "",
-			 bi->enable.high_speed ?		"hs," : ""
-			);
-
-		/* And merge them into one string */
-		snprintf(enable_info, sizeof(enable_info), "%s%s",
-			 enable_info1, enable_info2);
-		{
-			int l, p;
-			char *ei = enable_info;
-
-			l = strlen(ei);
-
-			/* Replace last ',' with \0 if present */
-			if (l && enable_info[l - 1] == ',')
-				enable_info[--l] = '\0';
-
-			if (l > ENABLE_LINE_MAX_LENGTH) {
-				/* Too big for one line, find last comma */
-				p = ENABLE_LINE_MAX_LENGTH;
-				while (ei[p] != ',')
-					p--;
-				/* Last comma found, copy till that comma */
-				strscpy(enable_info1, ei, umin(p, sizeof(enable_info1)));
-
-				ei += p + 1;
-				l = strlen(ei);
-
-				if (l <= ENABLE_LINE_MAX_LENGTH) {
-					/* The 2nd line fits */
-					/* we cannot use ei as argument because
-					 * it is not guaranteed dword aligned
-					 */
-
-					strscpy(enable_info2, ei, umin(l, sizeof(enable_info2)));
-
-					snprintf(enable_info, sizeof(enable_info), "%s\\n%s",
-						 enable_info1, enable_info2);
-
-				} else {
-					/* 2nd line is still too long */
-					p = ENABLE_LINE_MAX_LENGTH;
-					while (ei[p] != ',')
-						p--;
-
-					strscpy(enable_info2, ei, umin(p, sizeof(enable_info2)));
-
-					ei += p + 1;
-					l = strlen(ei);
-
-					if (l <= ENABLE_LINE_MAX_LENGTH) {
-						/* The 3rd line fits */
-						/* we cannot use ei as argument because
-						* it is not guaranteed dword aligned
-						*/
-						strscpy(enable_info3, ei,
-							sizeof(enable_info3));
-						snprintf(enable_info, sizeof(enable_info),
-							 "%s\\n%s\\n%s",
-							 enable_info1, enable_info2,
-							 enable_info3);
-					} else {
-						/* 3rd line is still too long */
-						p = ENABLE_LINE_MAX_LENGTH;
-						while (ei[p] != ',')
-							p--;
-						strscpy(enable_info3, ei,
-							umin(p, sizeof(enable_info3)));
-						ei += p + 1;
-						strscpy(enable_info3, ei,
-							sizeof(enable_info3));
-						snprintf(enable_info, sizeof(enable_info),
-							 "%s\\n%s\\n%s",
-							 enable_info1, enable_info2,
-							 enable_info3);
-					}
-				}
-			}
-		}
+#define ADD_INFO(flag, flag_str)						\
+		do {								\
+			if (bi->enable.flag)					\
+				ia_css_debug_build_info(enable_info,		\
+							sizeof(enable_info),	\
+							&offset,		\
+							flag_str,		\
+							sizeof(flag_str),	\
+							&line_len);		\
+		} while (0)
+
+		/* Build string in enable_info buffer */
+		ADD_INFO(reduced_pipe, "rp");
+		ADD_INFO(vf_veceven, "vfve");
+		ADD_INFO(dis, "dis");
+		ADD_INFO(dvs_envelope, "dvse");
+		ADD_INFO(uds, "uds");
+		ADD_INFO(dvs_6axis, "dvs6");
+		ADD_INFO(block_output, "bo");
+		ADD_INFO(ds, "ds");
+		ADD_INFO(bayer_fir_6db, "bf6");
+		ADD_INFO(raw_binning, "rawb");
+		ADD_INFO(continuous, "cont");
+		ADD_INFO(s3a, "s3a");
+		ADD_INFO(fpnr, "fpnr");
+		ADD_INFO(sc, "sc");
+		ADD_INFO(macc, "macc");
+		ADD_INFO(output, "outp");
+		ADD_INFO(ref_frame, "reff");
+		ADD_INFO(tnr, "tnr");
+		ADD_INFO(xnr, "xnr");
+		ADD_INFO(params, "par");
+		ADD_INFO(ca_gdc, "cagdc");
+		ADD_INFO(isp_addresses, "ispa");
+		ADD_INFO(in_frame, "inf");
+		ADD_INFO(out_frame, "outf");
+		ADD_INFO(high_speed, "hs");
+
+#undef ADD_INFO
+
+		/* Replace last ',' with '\0' */
+		if (offset > 0)
+			enable_info[offset - 1] = '\0';
+		else
+			enable_info[0] = '\0';
 
 		dtrace_dot("node [shape = circle, fixedsize=true, width=2.5, label=\"%s\\n%s\\n\\n%s\"]; \"%s(pipe%d)\"",
 			   bin_type, blob_name, enable_info, blob_name, id);
-- 
2.55.0

Re: [PATCH v2] staging: media: atomisp: refactor pipe graph dump stage formatting

[Andy Shevchenko]

On Sat, Jul 04, 2026 at 09:39:20PM -0700, Neal Patalay wrote:
> The original implementation of ia_css_debug_pipe_graph_dump_stage()
> includes an off-by-one error where the original strscpy() size dropped
> characters immediately before newlines. It also allocates over 600
> bytes across multiple buffers on the stack. Address these shortcomings
> and reduce stack usage with a single 256 byte buffer via a new helper
> function, ia_css_debug_build_info().

...

> +static void ia_css_debug_build_info(char *info, size_t info_size,
> +				    int *offset,
> +				    const char *flag_str, size_t flag_str_size,
> +				    int *line_len)
> +{
> +	/*
> +	 * If new line length exceeds max line length,
> +	 * replace the last ',' with a "\\n"
> +	 */
> +	if (*line_len > 0 && info_size - *offset >= 2 &&
> +	    *line_len + flag_str_size > ENABLE_LINE_MAX_LENGTH) {
> +		info[*offset - 1] = '\\';
> +		info[*offset] = 'n';
> +		*offset += 1;
> +		*line_len = 0;
> +	}

> +	int len_written = scnprintf(info + *offset, info_size - *offset,
> +				    "%s,", flag_str);

The definition should go to the top of the function, there are only a few
exceptions and none is for this case.

Also for the sake of better layering, introduce local variables to track values
of offset and line_len and only assign them at the end.

> +	*offset += len_written;
> +	*line_len += len_written;

So this will become

	int len = *line_len;
	int off = *offset;
	int len_written;
	... // here use only off and len
	*offset = off + len_written;
	*line_len = len + len_written;

> +}

...

> +#define ADD_INFO(flag, flag_str)						\
> +		do {								\
> +			if (bi->enable.flag)					\
> +				ia_css_debug_build_info(enable_info,		\
> +							sizeof(enable_info),	\
> +							&offset,		\
> +							flag_str,		\
> +							sizeof(flag_str),	\
> +							&line_len);		\
> +		} while (0)

There is no need to have do {} while (0) as there is conditional already and the
macro doesn't return anything. Also it's wrongly indented.

The below should work.

#define ADD_INFO(flag, flag_str)						\
	if (bi->enable.flag)							\
		ia_css_debug_build_info(enable_info, sizeof(enable_info),	\
					&offset,				\
					flag_str, sizeof(flag_str),		\
					&line_len);				\

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] staging: media: atomisp: refactor pipe graph dump stage formatting

[Neal Patalay]

On Sat, Jul 4, 2026 at 11:37 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> The definition should go to the top of the function, there are only a few
> exceptions and none is for this case.
>
> Also for the sake of better layering, introduce local variables to track values
> of offset and line_len and only assign them at the end.

Thank you, I'll implement this.

> There is no need to have do {} while (0) as there is conditional already and the
> macro doesn't return anything. Also it's wrongly indented.
>
> The below should work.
>
> #define ADD_INFO(flag, flag_str)                                                \
>         if (bi->enable.flag)                                                    \
>                 ia_css_debug_build_info(enable_info, sizeof(enable_info),       \
>                                         &offset,                                \
>                                         flag_str, sizeof(flag_str),             \
>                                         &line_len);                             \
>

Great, I'll fix the indentation and remove the loop. However, please be
aware that the loop's purpose was silencing a spurious checkpatch error,
so that will reappear. Also, I'll take the final "\" off the end of
the macro you
provided and also remove the ";" since that raises a checkpatch warning.

-- 
Thank you,
Neal Patalay