Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <7399cb84e16538d6ff7c8b4f7718b692c831476b.camel@perches.com>
Subject: Re: [PATCH v1 0/19] drm/panel: drmP.h removal and DRM_DEV*
From:   Joe Perches <joe@perches.com>
To:     Sam Ravnborg <sam@ravnborg.org>,
        Thierry Reding <thierry.reding@gmail.com>
Cc:     Sean Paul <sean@poorly.run>, dri-devel@lists.freedesktop.org,
        Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@linux.ie>,
        Linus Walleij <linus.walleij@linaro.org>,
        Stefan Mavrodiev <stefan@olimex.com>,
        linux-kernel@vger.kernel.org
Date:   Fri, 01 Feb 2019 17:31:51 -0800
In-Reply-To: <20190201133704.GA25680@ravnborg.org>
References: <20190131192619.9763-1-sam@ravnborg.org>
         <20190131200723.GZ114153@art_vandelay> <20190131210312.GA8947@ravnborg.org>
         <20190131215417.GA13156@mithrandir> <20190201133704.GA25680@ravnborg.org>
Content-Type: text/plain; charset="ISO-8859-1"
User-Agent: Evolution 3.30.1-1build1 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 2019-02-01 at 14:37 +0100, Sam Ravnborg wrote:
> Hi Thierry.
> 
> > I'm slightly on the fence about this patch.
> 
> Please ignore patch 3-19, there is no consensus on the logging changes.
> We do not want to apply these and then have to redo parts/all of
> it later.
> 
> But the first two patches has not seen any feedback yet:
> 
>     [PATCH v1 01/19] drm/panel: drop drmP.h usage
>     [PATCH v1 02/19] drm/panel: panel-innolux: drop unused variable
> 
> Please consider these, or maybe wait a little to see if someone
> find time to review.
> 
> I can resend the patches if this is preferred.

My preference would also convert all the
DRM_DEV_<level> uses to drm_dev_<level> eventually.

Also, the macros themselves could change to use a
more consistent mechanism.

This would make the drm logging mechanisms more like
other logging mechanisms used in the kernel.

Something like:
---
Subject: [PATCH] drm: Improve and standardize logging functions

Use a more typical logging style.

Add and use drm_printk and drm_dev_printk functions that include the
test for KERN_ERR use where '*ERROR*' is added to logging output.

Remove the slightly unusual _DRM_PRINTK macro and use the new drm_printk
function instead.
---
 drivers/gpu/drm/drm_print.c | 67 +++++++++++++++++++++++++++++----------------
 include/drm/drm_print.h     | 51 ++++++++++++++++++++--------------
 2 files changed, 74 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 0e7fc3e7dfb4..4e3ae7b5cce1 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -174,22 +174,59 @@ void drm_printf(struct drm_printer *p, const char *f, ...)
 }
 EXPORT_SYMBOL(drm_printf);
 
-void drm_dev_printk(const struct device *dev, const char *level,
-		    const char *format, ...)
+void drm_printk(const char *format, ...)
 {
 	struct va_format vaf;
 	va_list args;
+	char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = KERN_DEFAULT;
+	int level = printk_get_level(format);
+	size_t size = printk_skip_level(format) - format;
+
+	if (size) {
+		memcpy(lvl, format, size);
+		lvl[size] = '\0';
+	}
 
 	va_start(args, format);
-	vaf.fmt = format;
+	vaf.fmt = format + size;
+	vaf.va = &args;
+
+	printk("%s" "[" DRM_NAME ":%ps] %s%pV",
+	       lvl, __builtin_return_address(0),
+	       level == LOGLEVEL_ERR ? "*ERROR* " : "",
+	       &vaf);
+
+	va_end(args);
+}
+EXPORT_SYMBOL(drm_printk);
+
+void drm_dev_printk(const struct device *dev, const char *format, ...)
+{
+	struct va_format vaf;
+	va_list args;
+	char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = KERN_DEFAULT;
+	int level = printk_get_level(format);
+	size_t size = printk_skip_level(format) - format;
+
+	if (size) {
+		memcpy(lvl, format, size);
+		lvl[size] = '\0';
+	}
+
+	va_start(args, format);
+	vaf.fmt = format + size;
 	vaf.va = &args;
 
 	if (dev)
-		dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
-			   __builtin_return_address(0), &vaf);
+		dev_printk(lvl, dev, "[" DRM_NAME ":%ps] %s%pV",
+			   __builtin_return_address(0),
+			   level == LOGLEVEL_ERR ? "*ERROR* " : "",
+			   &vaf);
 	else
-		printk("%s" "[" DRM_NAME ":%ps] %pV",
-		       level, __builtin_return_address(0), &vaf);
+		printk("%s" "[" DRM_NAME ":%ps] %s%pV",
+		       lvl, __builtin_return_address(0),
+		       level == LOGLEVEL_ERR ? "*ERROR* " : "",
+		       &vaf);
 
 	va_end(args);
 }
@@ -237,19 +274,3 @@ void drm_dbg(unsigned int category, const char *format, ...)
 	va_end(args);
 }
 EXPORT_SYMBOL(drm_dbg);
-
-void drm_err(const char *format, ...)
-{
-	struct va_format vaf;
-	va_list args;
-
-	va_start(args, format);
-	vaf.fmt = format;
-	vaf.va = &args;
-
-	printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV",
-	       __builtin_return_address(0), &vaf);
-
-	va_end(args);
-}
-EXPORT_SYMBOL(drm_err);
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index afbc3beef089..9d9fd10e6ff8 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -268,9 +268,8 @@ static inline struct drm_printer drm_debug_printer(const char *prefix)
 #define DRM_UT_LEASE		0x80
 #define DRM_UT_DP		0x100
 
-__printf(3, 4)
-void drm_dev_printk(const struct device *dev, const char *level,
-		    const char *format, ...);
+__printf(2, 3)
+void drm_dev_printk(const struct device *dev, const char *format, ...);
 __printf(3, 4)
 void drm_dev_dbg(const struct device *dev, unsigned int category,
 		 const char *format, ...);
@@ -278,26 +277,38 @@ void drm_dev_dbg(const struct device *dev, unsigned int category,
 __printf(2, 3)
 void drm_dbg(unsigned int category, const char *format, ...);
 __printf(1, 2)
-void drm_err(const char *format, ...);
+void drm_printk(const char *format, ...);
 
 /* Macros to make printk easier */
 
-#define _DRM_PRINTK(once, level, fmt, ...)				\
-	printk##once(KERN_##level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
-
-#define DRM_INFO(fmt, ...)						\
-	_DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__)
-#define DRM_NOTE(fmt, ...)						\
-	_DRM_PRINTK(, NOTICE, fmt, ##__VA_ARGS__)
+#define DRM_ERROR(fmt, ...)						\
+	drm_printk(KERN_ERR fmt, ##__VA_ARGS__)
+#define drm_err DRM_ERROR
 #define DRM_WARN(fmt, ...)						\
-	_DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__)
+	drm_printk(KERN_WARNING fmt, ##__VA_ARGS__)
+#define DRM_NOTE(fmt, ...)						\
+	drm_printk(KERN_NOTICE fmt, ##__VA_ARGS__)
+#define DRM_INFO(fmt, ...)						\
+	drm_printk(KERN_INFO fmt, ##__VA_ARGS__)
+
+#define drm_printk_once(fmt, ...)					\
+({									\
+	static bool __print_once __read_mostly;				\
+	bool __ret_print_once = !__print_once;				\
+									\
+	if (!__print_once) {						\
+		__print_once = true;					\
+		drm_printk(fmt, ##__VA_ARGS__);				\
+	}								\
+	unlikely(__ret_print_once);					\
+})
 
-#define DRM_INFO_ONCE(fmt, ...)						\
-	_DRM_PRINTK(_once, INFO, fmt, ##__VA_ARGS__)
-#define DRM_NOTE_ONCE(fmt, ...)						\
-	_DRM_PRINTK(_once, NOTICE, fmt, ##__VA_ARGS__)
 #define DRM_WARN_ONCE(fmt, ...)						\
-	_DRM_PRINTK(_once, WARNING, fmt, ##__VA_ARGS__)
+	drm_printk_once(KERN_WARNING fmt, ##__VA_ARGS__)
+#define DRM_NOTE_ONCE(fmt, ...)						\
+	drm_printk_once(KERN_NOTICE fmt, ##__VA_ARGS__)
+#define DRM_INFO_ONCE(fmt, ...)						\
+	drm_printk_once(KERN_INFO fmt, ##__VA_ARGS__)
 
 /**
  * Error output.
@@ -306,9 +317,7 @@ void drm_err(const char *format, ...);
  * @fmt: printf() like format string.
  */
 #define DRM_DEV_ERROR(dev, fmt, ...)					\
-	drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__)
-#define DRM_ERROR(fmt, ...)						\
-	drm_err(fmt, ##__VA_ARGS__)
+	drm_dev_printk(dev, KERN_ERR fmt, ##__VA_ARGS__)
 
 /**
  * Rate limited error output.  Like DRM_ERROR() but won't flood the log.
@@ -329,7 +338,7 @@ void drm_err(const char *format, ...);
 	DRM_DEV_ERROR_RATELIMITED(NULL, fmt, ##__VA_ARGS__)
 
 #define DRM_DEV_INFO(dev, fmt, ...)					\
-	drm_dev_printk(dev, KERN_INFO, fmt, ##__VA_ARGS__)
+	drm_dev_printk(dev, KERN_INFO fmt, ##__VA_ARGS__)
 
 #define DRM_DEV_INFO_ONCE(dev, fmt, ...)				\
 ({									\