Re: [PATCH 3/3] media: atomisp: Improve binary finding debug logging

[Andy Shevchenko <andy@kernel.org>]

On Mon, Sep 02, 2024 at 11:52:29AM +0200, Hans de Goede wrote:
> The atomisp firmware contains a number of different pipeline binaries
> inside its firmware file and the driver selects the right one depending
> on the selected pipeline configuration.
> 
> Sometimes (e.g. when the selected output resolution is too big) it fails
> to find a binary. This happens especially when adding support for new
> sensors.
> 
> Improve the logging when this happens to make debugging easier:
> 
> 1. Replace ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, ...) with standard
>    dev_dbg() calls so that the logs can be enabled with dyndbg
> 
> 2. Do not dump_stack() when this fails, doing so adds no useful extra
>    info
> 
> 3. With the dump_stack() call gone, remove the wrapper and rename
>    __ia_css_binary_find() to ia_css_binary_find()
> 
> 4. On error use dev_err() instead of dev_dbg() so that when things
>    fail it is clear why they fail without needing to enable dyndbg
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note the log messages could also do with a bit of rewording and
> reflowing them to put more arguments on a single line to use less
> lines. I consider that out of scope for this patch which already
> enough (see the 1-4 enough) something like this should be done
> in a follow-up patch IMHO.

Yes, but...

...

> -static int __ia_css_binary_find(struct ia_css_binary_descr *descr,
> -				struct ia_css_binary *binary) {
> +int ia_css_binary_find(struct ia_css_binary_descr *descr,
> +		       struct ia_css_binary *binary)

...for example, in this case you touched both lines, so making them a single
line just reduces the future churn.

...

> -	ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
> -			    "ia_css_binary_find() enter: descr=%p, (mode=%d), binary=%p\n",
> -			    descr, descr->mode,
> -			    binary);
> +	dev_dbg(atomisp_dev,
> +		"ia_css_binary_find() enter: descr=%p, (mode=%d), binary=%p\n",

> +		descr, descr->mode,
> +		binary);

So does this.

...

>  	/* print a map of the binary file */
> -	ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,	"BINARY INFO:\n");
> +	dev_dbg(atomisp_dev,	"BINARY INFO:\n");

TAB instead of space.

...

> -			ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
> -					    "ia_css_binary_find() [%d] continue: !%d && %d && (%d != %d)\n",
> -					    __LINE__, candidate->enable.continuous,
> -					    continuous, mode,
> -					    IA_CSS_BINARY_MODE_COPY);
> +			dev_dbg(atomisp_dev,
> +				"ia_css_binary_find() [%d] continue: !%d && %d && (%d != %d)\n",
> +				__LINE__, candidate->enable.continuous,

> +				continuous, mode,
> +				IA_CSS_BINARY_MODE_COPY);

Now one line?

...

> -			ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
> -					    "ia_css_binary_find() [%d] continue: binary is not striped\n",
> -					    __LINE__);
> +			dev_dbg(atomisp_dev,
> +				"ia_css_binary_find() [%d] continue: binary is not striped\n",
> +				__LINE__);

Now the __LINE__ argument is redundant as one may run-time turn it on and off.
So, trimming it while converting to dev_dbg() makes sense to me as a one
logical change. Ditto for other __LINE__ cases.

...

Otherwise it's a good clean up!

-- 
With Best Regards,
Andy Shevchenko