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@xxxxxxxxx> 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