Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events

From: Petr Mladek <pmladek@suse.com>
To: Kees Cook <keescook@chromium.org>
Cc: Pavel Tatashin <pasha.tatashin@soleen.com>,
	Anton Vorontsov <anton@enomsg.org>,
	Colin Cross <ccross@android.com>, Tony Luck <tony.luck@intel.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Rob Herring <robh+dt@kernel.org>,
	Benson Leung <bleung@chromium.org>,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	jmorris@namei.org, sashal@kernel.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v3 0/6] allow ramoops to collect all kmesg_dump events
Date: Tue, 12 May 2020 15:16:55 +0200	[thread overview]
Message-ID: <20200512131655.GE17734@linux-b0ei> (raw)
In-Reply-To: <20200506211523.15077-1-keescook@chromium.org>

On Wed 2020-05-06 14:15:17, Kees Cook wrote:
> Hi!
> 
> This is my stab at rearranging a few things based on Pavel's series. Most
> things remain the same; I just tweaked how defaults are arranged and
> detected and expanded the wording in a few places. Pavel, how does this
> v3 look to you?
> 
> Pavel's original cover letter:
> 
> pstore /mnt/console-ramoops-0 outputs only messages below the console
> loglevel, and our console loglevel is set to 3 due to slowness of
> serial console. Which means only errors and worse types of messages
> are recorded. There is no way to have different log levels for
> different consoles.
> 
> This patch series adds a new option to ramoops: max_reason that enables
> it to collect kmdesg dumps for other reasons beside oops and panics.

I was a bit confused by the above explanation. It talks about two
different numbering schemes:

   + console loglevels: emerg, alert, crit, err, warning, ...
   + dump reason: panic, oops, emerg, restart, halt, poweroff

This difference and also the jump from consoles to ramoops is far from
obvious.

My understanding is the following:

It is not possible to set loglevel per console. The global value must
be set by the slowest one. This prevents seeing all messages even
on fast consoles.

Alternative solution is to dump all messages using ramoops. The
problem is that it currently works only during Oops and panic
situation. This is solved by this patchset.

OK, I personally see this as two separate problems:

   1. Missing support to set loglevel per console.
   2. Missing support to dump messages for other reasons.

I would remove the paragraph about console log levels completely.
It is your reason to use ramoops. But it is not reason to modify
the logic about max_reason.

Now, the max_reason logic makes sense only when all the values
have some ordering. Is this the case?

I see it as two distinct sets:

   + panic, oops, emerg: describe how critical is an error situation
   + restart, halt, poweroff: describe behavior when the system goes down

Let's say that panic is more critical than oops. Is restart more
critical than halt?

If you want the dump during restart. Does it mean that you want it
also during emergency situation?

My fear is that this patchset is going to introduce user interface
(max_reason) with a weird logic. IMHO, max_reason is confusing even
in the code and we should not spread this to users.

Is there any reason why the existing printk.always_kmsg_dump option
is not enough for you?

Best Regards,
Petr