Re: [PATCH printk v4 6/8] printk: introduce console_prepend_dropped() for dropped messages

From: John Ogness <john.ogness@linutronix.de>
To: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH printk v4 6/8] printk: introduce console_prepend_dropped() for dropped messages
Date: Thu, 05 Jan 2023 17:41:59 +0106	[thread overview]
Message-ID: <87y1qhaqhc.fsf@jogness.linutronix.de> (raw)
In-Reply-To: <Y7b4AND0DRs7jPW9@alley>

On 2023-01-05, Petr Mladek <pmladek@suse.com> wrote:
>> +	if (WARN_ON_ONCE(len + PREFIX_MAX >= outbuf_sz))
>> +		return;
>
> I guess that this will always trigger the compiler warning
> when CONFIG_PRINTK is disabled. See the report for v3 at
> https://lore.kernel.org/r/202301052114.vvN3wQoH-lkp@intel.com

That report is actually complaining about the check after this one. It
is the same "problem".

> Hmm, we might want to fix this warning so that it does not break
> build with -Werror.
>
> IMHO, the proper solution would be to define this function only when
> CONFIG_PRINTK is defined. But it might require bigger changes
> and define many more console functions only when CONFIG_PRINTK
> is defined. This is out-of-scope of this patchset.
>
> I wonder if the following would work as an "intermediate" workaround:
>
> 	if (!IS_ENABLED(CONFIG_PRINTK) ||
> 	    WARN_ON_ONCE(len + PREFIX_MAX >= outbuf_sz))
> 		return;

The whole CONFIG_PRINTK stuff is a total mess right now. We should
definitely get that cleaned up at some point. As an intermediate
workaround, it might make more sense to just put the whole function
inside an "#ifdef CONFIG_PRINTK". It doesn't return anything anyway.

#ifdef CONFIG_PRINTK
static void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
{
...
}
#else
#define console_prepend_dropped(pmsg, dropped)
#endif

There are already places in the code that look like this (for example,
print_caller()).

Otherwise, if you want to use IS_ENABLED(), you will need it for both
checks.

John