Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 7 Oct 2022 17:15:32 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH printk 15/18] printk: Add struct cons_text_buf
Message-ID: <Y0BClAlwfWfWcsDD@alley>
References: <20220924000454.3319186-1-john.ogness@linutronix.de>
 <20220924000454.3319186-16-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220924000454.3319186-16-john.ogness@linutronix.de>
Precedence: bulk

On Sat 2022-09-24 02:10:51, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Create a data structure to replace the open coded separate buffers for
> regular and extended formatting.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  include/linux/console.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 8ec24fe097d3..05c7325e98f9 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -173,6 +173,20 @@ enum cons_flags {
>  	CON_EXTENDED		= BIT(6),
>  };
>  
> +/**
> + * struct cons_text_buf - console output text buffer
> + * @ext_text:		Buffer for extended log format text
> + * @dropped_text:	Buffer for dropped text
> + * @text:		Buffer for ringbuffer text
> + */
> +struct cons_text_buf {

Sigh, I feel bad to nit-pick about this. It seems that you have used
"cons" everywhere in the new API so any change might be painful.
But I personally find really handful when an API is predictable
and consistent.

I see that "cons" has already been used few times in tty subsystem,
especially tty/vt and tty/hvc.

But I do not see any single "cons_" under kernel/printk/ before
this patchset. Either "console_" or "con_" prefix was
used everywhere, including CON_XXX flags.

Is there any change to change this to either "console_"
or "con_", please? Or is there any particular reason why
this new API should be distinguished by the new prefix?

> +	union {
> +		char	ext_text[CONSOLE_EXT_LOG_MAX];
> +		char	dropped_text[DROPPED_TEXT_MAX];
> +	};
> +	char		text[CONSOLE_LOG_MAX];

We should explain in the commit message why we need
the separate ext_text buffer and why it can be shared
with dropped_text buffer. Something like:

<proposal>
Create a data structure to replace the open coded separate buffers for
regular and extended formatting.

Separate @ext_text buffer is needed because info_print_ext_header()
and msg_print_ext_body() are not able to add the needed extra
information inplace.

@ext_text and @dropped_text buffer can be shared because
they are never used at the same time.
</proposal>


Also I think about using pointers instead of the hard-coded
buffer size. For example, there is no need to have
the big ext_text buffer in the kthread when the related
console does not allow to allocated the extended text.
There is actually only one console that has this enabled.

I mean something like:

struct cons_text_buf {
	char *text;
	char *ext_text;
	char *dropped_text;

	unsigned int text_size;
	unsigned int ext_text_size;
	unsigned int dropped_text_size;
}

We might create a helper to define static buffer:

#define DEFINE_CONS_TEXT_BUF(name) \
	static char _##name##_text[CONSOLE_LOG_MAX];			\
	static char _##name##_ext_text[CONSOLE_EXT_LOG_MAX];		\
	static struct const_text_buf name = {				\
		.text = _##name##_text,					\
		.ext_text = _##name##_ext_text,				\
		.dropped_text = _##name##_ext_text,			\
									\
		.text_size = CONSOLE_LOG_MAX;				\
		.ext_text_size = CONSOLE_LOG_MAX;			\
		.dropped_text_size = DROPPED_TEXT_MAX;			\
	};

Another advantage would be that it looks like a more safe way to
pass the buffer size. The existing code hardcodes CONSOLE_LOG_MAX
and CONSOLE_EXT_LOG_MAX everywhere. And it is less obvious that
the buffer and size fits together. Especially that the names
do not match (text vs. LOG_MAX and ext_text vs. EXT_LOG_MAX).

Well, this might be out of scope of this patchset. I do not resist
on it. We might do this later.

Best Regards,
Petr