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;
From:   John Ogness <john.ogness@linutronix.de>
To:     Petr Mladek <pmladek@suse.com>, Chris Down <chris@chrisdown.name>
Cc:     linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>, kernel-team@fb.com
Subject: Re: [PATCH v5 2/2] printk: console: Support console-specific loglevels
In-Reply-To: <ZEp9dXwHCYNPidjC@alley>
References: <cover.1682427812.git.chris@chrisdown.name>
 <4d3846bf2543de20aa071b2a12de924eea3e9574.1682427812.git.chris@chrisdown.name>
 <ZEp9dXwHCYNPidjC@alley>
Date:   Thu, 27 Apr 2023 17:47:26 +0206
Message-ID: <87v8hhs57d.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On 2023-04-27, Petr Mladek <pmladek@suse.com> wrote:
>> @@ -2764,7 +2982,8 @@ static void console_prepend_dropped(struct printk_message *pmsg, unsigned long d
>>   * of @pmsg are valid. (See the documentation of struct printk_message
>>   * for information about the @pmsg fields.)
>>   */
>> -static bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>> +static bool printk_get_next_message(struct printk_message *pmsg,
>> +				    struct console *con, u64 seq,
>>  				    bool is_extended, bool may_suppress)
>
> @is_extended has to match @con->flags & CON_EXTENDED. It would be
> ugly if the API is not used correctly and they do not match.
>
> Also the "may_suppress" value might be guessed from @con.
>
> I see two solutions:
>
> 1. If we pass @con then @is_extended and @may_suppress parameters
>    are not needed. The values might be guessed. The logic is
>    the following:
>
> 	if (con) {
> 		is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
> 		may_suppress = true;
> 	} else {
> 		/* Used only by devkmsg_read(). */
> 		is_extended = true;
> 		may_suppress = false;
> 	}

This is correct, but we also need to keep in mind that the caller of
printk_get_next_message() also needs to know if it is extended, so that
it can know if it should prepend the dropped messages. Although I guess
it doesn't matter if the caller is also reading @con->flags to get the
extended status.

I don't want to move console_prepend_dropped() inside
printk_get_next_message() because then printk_get_next_message() would
be modifying @dropped. The main purpose of printk_get_next_message() was
to locklessly get formatted text, thus simplifying the atomic console
implementation.

>    The drawback is that @con->seq and @seq values might still
>    be inconsistent. But the @seq value must be passed explicitly
>    when called from dev_kmsg() and @con is NULL. Also the explicit
>    @seq parameter will most likely useful also for the
>    con->atomic_write() callback added by John's patchset.
>    The atomic consoles will not use con->seq at all.

Correct. @flags and @loglevel are the _only_ fields that may be read
locklessly.

> 2. We could pass @con_loglevel instead of @con. And pass it to
>    suppress_message_printing() instead of @con as well.
>
>    It is probably cleaner solution but the many parameters suck.

We would need to pass console_effective_loglevel(). But yes, 3 separate
parameters when they could be determined from 1 sucks.

> I would personally use the 1st proposal and live with the fact
> that the function would ignore con->seq and use the passed
> @seq instead.

Whitelist: only @flags and @level may be read

I agree that we should go with your 1st proposal. But I would like to
see some comments added into printk_get_next_message() that only @flags
and @level may be considered valid.

John