Re: [PATCH] drm/sched: Define pr_fmt() for DRM using pr_*()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 09 Nov 2023, Luben Tuikov <ltuikov89@gmail.com> wrote:
> Define pr_fmt() as "[drm] " for DRM code using pr_*() facilities, especially
> when no devices are available. This makes it easier to browse kernel logs.

Please do not merge patches before people have actually had a chance to
look at them. This was merged *way* too quickly.

This does not do what you think it does, and it's not robust enough.

The drm_print.[ch] facilities use very few pr_*() calls directly. The
users of pr_*() calls do not necessarily include <drm/drm_print.h> at
all, and really don't have to.

Even the ones that do include it, usually have <linux/...> includes
first, and <drm/...> includes next. Notably, <linux/kernel.h> includes
<linux/printk.h>.

And, of course, <linux/printk.h> defines pr_fmt() itself if not already
defined.

> Signed-off-by: Luben Tuikov <ltuikov89@gmail.com>
> ---
>  include/drm/drm_print.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index a93a387f8a1a15..e8fe60d0eb8783 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -26,6 +26,20 @@
>  #ifndef DRM_PRINT_H_
>  #define DRM_PRINT_H_
>  
> +/* Define this before including linux/printk.h, so that the format
> + * string in pr_*() macros is correctly set for DRM. If a file wants
> + * to define this to something else, it should do so before including
> + * this header file.

The only way this would work is by including <drm/drm_print.h> as the
very first header, and that's fragile at best.

> + *
> + * It is encouraged code using pr_err() to prefix their format with
> + * the string "*ERROR* ", to make it easier to scan kernel logs. For
> + * instance,
> + *   pr_err("*ERROR* <the rest of your format string here>", args).

No, it's encouraged not to use pr_*() at all, and prefer drm device
based logging, or device based logging.

I'd rather this whole thing was just reverted.

BR,
Jani.

> + */
> +#ifndef pr_fmt
> +#define pr_fmt(fmt) "[drm] " fmt
> +#endif
> +
>  #include <linux/compiler.h>
>  #include <linux/printk.h>
>  #include <linux/seq_file.h>
>
> base-commit: f3123c2590005c5ff631653d31428e40cd10c618

-- 
Jani Nikula, Intel