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

[Andy Shevchenko]

On Wed, Jul 01, 2026 at 09:15:34AM -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

strscpy()

> 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().

Thanks, this is useful! See my comments below.

> Fixes: ad85094b293e ("Revert "media: staging: atomisp: Remove driver"")

Nope. Find the actual commit in the history.

...

> +static void ia_css_debug_build_info(char *info, size_t info_size, int *offset, bool flag,
> +				    const char *flag_str, size_t flag_str_size, int *line_len)
> +{
> +	if (flag) {

So, this makes code much better if

	if (!flag)
		return;

BUT, the usual way of such functions is to make check in the caller(s) and drop
this "flag" completely.

> +		/* If new line length exceeds max line length, replace the last ',' with a "\\n" */

The media subsystem is quite strict about 80 character limit, this line way
too long.

> +		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;
> +	}
> +}

...

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


Use logical split and make this all to be shorter.

> +		/* 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

Instead of doing this way, consider making an string literal array and just
loop over it. This might need to reconsider representation of those flags
as well. Yet, don't come to the conclusion, you need to try and see which
one is better. The current approach is okay if my suggestion will look less
readable.

-- 
With Best Regards,
Andy Shevchenko