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

[PATCH v4] 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>
---
v4:
- Changed logic to truncate output after the third line
- Added offset check to avoid writing beyond bounds
- Fixed indentation inside helper function
- Separated variable definitions from code in helper function
- Fixed comment in helper function

v3:
- Moved "len_written" variable declaration to top of helper function
- Added local variables to track values inside helper function
- Fixed macro indentation
- Removed loop out of macro definition

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      | 198 ++++++++----------
 1 file changed, 83 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..19c4e0246c34 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,43 @@ 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,
+				    int *num_lines)
+{
+	int len = *line_len;
+	int off = *offset;
+	int lines = *num_lines;
+	int len_written;
+
+	if (lines > 3)
+		return;
+
+	/*
+	 * If new line length exceeds max line length,
+	 * replace the last ',' with a "\\n".
+	 */
+	if (len > 0 && off > 0 && info_size - off >= 2 &&
+	    len + flag_str_size > ENABLE_LINE_MAX_LENGTH) {
+		if (lines >= 3) {
+			*num_lines = lines + 1;
+			return;
+		}
+		info[off - 1] = '\\';
+		info[off] = 'n';
+		off += 1;
+		len = 0;
+		lines += 1;
+	}
+
+	len_written = scnprintf(info + off, info_size - off, "%s,", flag_str);
+	*offset = off + len_written;
+	*line_len = len + len_written;
+	*num_lines = lines;
+}
+
 void
 ia_css_debug_pipe_graph_dump_stage(
     struct ia_css_pipeline_stage *stage,
@@ -1194,123 +1231,54 @@ 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;
+		int num_lines = 1;
 		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)						\
+	if (bi->enable.flag)							\
+		ia_css_debug_build_info(enable_info, sizeof(enable_info),	\
+					&offset,				\
+					flag_str, sizeof(flag_str),		\
+					&line_len,				\
+					&num_lines)
+
+		/* 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 v4] staging: media: atomisp: refactor pipe graph dump stage formatting

[Andy Shevchenko]

On Mon, Jul 6, 2026 at 3:10 AM Neal Patalay <nealpatalay0@gmail.com> 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().

Please, slow down! No need to send a new version immediately after
replying to the previous one without settling down all aspects of the
change.

This version is no go, sorry.

...

> +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,
> +                                   int *num_lines)
> +{
> +       int len = *line_len;
> +       int off = *offset;
> +       int lines = *num_lines;
> +       int len_written;
> +
> +       if (lines > 3)
> +               return;
> +
> +       /*
> +        * If new line length exceeds max line length,
> +        * replace the last ',' with a "\\n".
> +        */
> +       if (len > 0 && off > 0 && info_size - off >= 2 &&
> +           len + flag_str_size > ENABLE_LINE_MAX_LENGTH) {
> +               if (lines >= 3) {
> +                       *num_lines = lines + 1;
> +                       return;
> +               }
> +               info[off - 1] = '\\';
> +               info[off] = 'n';
> +               off += 1;
> +               len = 0;
> +               lines += 1;
> +       }
> +
> +       len_written = scnprintf(info + off, info_size - off, "%s,", flag_str);
> +       *offset = off + len_written;
> +       *line_len = len + len_written;
> +       *num_lines = lines;
> +}

This makes helper too ugly and unreadable. Again, study the case
first, when this can be true. Do we really need to cut it? This whole
thing AFAICS depends on the dtrace facility in the driver. Is that
HW-related? Is it pure SW? You need to perform some homework.

>                 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);

Yes, I can understand the frustration, but this driver is not an easy
low-hanging fruit even for (really useful) cleanups like this.


-- 
With Best Regards,
Andy Shevchenko

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

[Neal Patalay]

On Sun, Jul 5, 2026 at 10:37 PM Andy Shevchenko wrote:

> Please, slow down! No need to send a new version immediately after
> replying to the previous one without settling down all aspects of the
> change.
>
> This version is no go, sorry.

Understood, no problem.

> > +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,
> > +                                   int *num_lines)
> > +{
> > +       int len = *line_len;
> > +       int off = *offset;
> > +       int lines = *num_lines;
> > +       int len_written;
> > +
> > +       if (lines > 3)
> > +               return;
> > +
> > +       /*
> > +        * If new line length exceeds max line length,
> > +        * replace the last ',' with a "\\n".
> > +        */
> > +       if (len > 0 && off > 0 && info_size - off >= 2 &&
> > +           len + flag_str_size > ENABLE_LINE_MAX_LENGTH) {
> > +               if (lines >= 3) {
> > +                       *num_lines = lines + 1;
> > +                       return;
> > +               }
> > +               info[off - 1] = '\\';
> > +               info[off] = 'n';
> > +               off += 1;
> > +               len = 0;
> > +               lines += 1;
> > +       }
> > +
> > +       len_written = scnprintf(info + off, info_size - off, "%s,", flag_str);
> > +       *offset = off + len_written;
> > +       *line_len = len + len_written;
> > +       *num_lines = lines;
> > +}
>
> This makes helper too ugly and unreadable. Again, study the case
> first, when this can be true. Do we really need to cut it? This whole
> thing AFAICS depends on the dtrace facility in the driver. Is that
> HW-related? Is it pure SW? You need to perform some homework.

I've taken a look at the driver in greater depth. The flag wrapping
functionality being refactored here is used to construct a graph
visualization for debugging.

> >                 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);

Each one of these calls generates a node in that graph, and the contents
of the node are decided by the flag string we build in enable_info. Seeing
as we are just collecting debug info and populating a visualization with it,
this refactor seems to be software.

Seeing as it's unlikely that every flag is enabled at once, and even if they
are this only causes a slightly more unreadable graph, avoiding 3 line
truncation seems to be the best option since it leads to more readable code
and avoids omitting information.

Additionally, len > 0 && offset == 0 is an impossible condition under any
feasible helper function use condition. This can be documented in a comment.

Given this information, to me it seems like the best course of action is to
revert the helper function to its simpler state without line count truncation
or the offset > 0 comparison. What do you think?

-- 
Thank you,
Neal Patalay

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

[Andy Shevchenko]

On Mon, Jul 06, 2026 at 02:59:16PM -0700, Neal Patalay wrote:
> On Sun, Jul 5, 2026 at 10:37 PM Andy Shevchenko wrote:
> 
> > Please, slow down! No need to send a new version immediately after
> > replying to the previous one without settling down all aspects of the
> > change.
> >
> > This version is no go, sorry.
> 
> Understood, no problem.
> 
> > > +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,
> > > +                                   int *num_lines)
> > > +{
> > > +       int len = *line_len;
> > > +       int off = *offset;
> > > +       int lines = *num_lines;
> > > +       int len_written;
> > > +
> > > +       if (lines > 3)
> > > +               return;
> > > +
> > > +       /*
> > > +        * If new line length exceeds max line length,
> > > +        * replace the last ',' with a "\\n".
> > > +        */
> > > +       if (len > 0 && off > 0 && info_size - off >= 2 &&
> > > +           len + flag_str_size > ENABLE_LINE_MAX_LENGTH) {
> > > +               if (lines >= 3) {
> > > +                       *num_lines = lines + 1;
> > > +                       return;
> > > +               }
> > > +               info[off - 1] = '\\';
> > > +               info[off] = 'n';
> > > +               off += 1;
> > > +               len = 0;
> > > +               lines += 1;
> > > +       }
> > > +
> > > +       len_written = scnprintf(info + off, info_size - off, "%s,", flag_str);
> > > +       *offset = off + len_written;
> > > +       *line_len = len + len_written;
> > > +       *num_lines = lines;
> > > +}
> >
> > This makes helper too ugly and unreadable. Again, study the case
> > first, when this can be true. Do we really need to cut it? This whole
> > thing AFAICS depends on the dtrace facility in the driver. Is that
> > HW-related? Is it pure SW? You need to perform some homework.
> 
> I've taken a look at the driver in greater depth. The flag wrapping
> functionality being refactored here is used to construct a graph
> visualization for debugging.
> 
> > >                 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);
> 
> Each one of these calls generates a node in that graph, and the contents
> of the node are decided by the flag string we build in enable_info. Seeing
> as we are just collecting debug info and populating a visualization with it,
> this refactor seems to be software.
> 
> Seeing as it's unlikely that every flag is enabled at once, and even if they
> are this only causes a slightly more unreadable graph, avoiding 3 line
> truncation seems to be the best option since it leads to more readable code
> and avoids omitting information.
> 
> Additionally, len > 0 && offset == 0 is an impossible condition under any
> feasible helper function use condition. This can be documented in a comment.
> 
> Given this information, to me it seems like the best course of action is to
> revert the helper function to its simpler state without line count truncation
> or the offset > 0 comparison. What do you think?

I agree, just add more comments to the code and elaborate in the commit message
the (potential) difference in the behaviour.

-- 
With Best Regards,
Andy Shevchenko

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

[Neal Patalay]

On Tue, Jul 7, 2026 at 12:18 AM Andy Shevchenko wrote:
>
> On Mon, Jul 06, 2026 at 02:59:16PM -0700, Neal Patalay wrote:
> > On Sun, Jul 5, 2026 at 10:37 PM Andy Shevchenko wrote:
> >
> > > Please, slow down! No need to send a new version immediately after
> > > replying to the previous one without settling down all aspects of the
> > > change.
> > >
> > > This version is no go, sorry.
> >
> > Understood, no problem.
> >
> > > > +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,
> > > > +                                   int *num_lines)
> > > > +{
> > > > +       int len = *line_len;
> > > > +       int off = *offset;
> > > > +       int lines = *num_lines;
> > > > +       int len_written;
> > > > +
> > > > +       if (lines > 3)
> > > > +               return;
> > > > +
> > > > +       /*
> > > > +        * If new line length exceeds max line length,
> > > > +        * replace the last ',' with a "\\n".
> > > > +        */
> > > > +       if (len > 0 && off > 0 && info_size - off >= 2 &&
> > > > +           len + flag_str_size > ENABLE_LINE_MAX_LENGTH) {
> > > > +               if (lines >= 3) {
> > > > +                       *num_lines = lines + 1;
> > > > +                       return;
> > > > +               }
> > > > +               info[off - 1] = '\\';
> > > > +               info[off] = 'n';
> > > > +               off += 1;
> > > > +               len = 0;
> > > > +               lines += 1;
> > > > +       }
> > > > +
> > > > +       len_written = scnprintf(info + off, info_size - off, "%s,", flag_str);
> > > > +       *offset = off + len_written;
> > > > +       *line_len = len + len_written;
> > > > +       *num_lines = lines;
> > > > +}
> > >
> > > This makes helper too ugly and unreadable. Again, study the case
> > > first, when this can be true. Do we really need to cut it? This whole
> > > thing AFAICS depends on the dtrace facility in the driver. Is that
> > > HW-related? Is it pure SW? You need to perform some homework.
> >
> > I've taken a look at the driver in greater depth. The flag wrapping
> > functionality being refactored here is used to construct a graph
> > visualization for debugging.
> >
> > > >                 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);
> >
> > Each one of these calls generates a node in that graph, and the contents
> > of the node are decided by the flag string we build in enable_info. Seeing
> > as we are just collecting debug info and populating a visualization with it,
> > this refactor seems to be software.
> >
> > Seeing as it's unlikely that every flag is enabled at once, and even if they
> > are this only causes a slightly more unreadable graph, avoiding 3 line
> > truncation seems to be the best option since it leads to more readable code
> > and avoids omitting information.
> >
> > Additionally, len > 0 && offset == 0 is an impossible condition under any
> > feasible helper function use condition. This can be documented in a comment.
> >
> > Given this information, to me it seems like the best course of action is to
> > revert the helper function to its simpler state without line count truncation
> > or the offset > 0 comparison. What do you think?
>
> I agree, just add more comments to the code and elaborate in the commit message
> the (potential) difference in the behaviour.

Great, I'll send in v5.

-- 
Thank you,
Neal Patalay