Re: [PATCH v6 02/11] printk: Use struct console for suppression and extended console state

[Petr Mladek <pmladek@suse.com>]

On Wed 2024-11-20 04:17:51, Chris Down wrote:
> John Ogness writes:
> > On 2024-10-28, Chris Down <chris@chrisdown.name> wrote:
> > > In preparation for supporting per-console loglevels, modify
> > > printk_get_next_message() to accept the console itself instead of
> > > individual arguments that mimic its fields.
> > 
> > Sorry, this is not allowed. printk_get_next_message() was created
> > specifically to locklessly retrieve and format arbitrary records. It
> > must never be tied to a console because it has nothing to do with
> > consoles (as can bee seen with the devkmsg_read() hack you added in the
> > function).
> > 
> > I recommend adding an extra argument specifying the level.
> > 
> > The extra argument would be redundant if may_suppress=false. So perhaps
> > as an alternative change "bool may_suppress" to "u32 supress_level". The
> > loglevels are only 3 bits. So you could easily define a special value
> > NO_SUPPRESS to represent the may_suppress=false case.
> > 
> > #define NO_SUPPRESS BIT(31)
> > 
> > bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
> >                             bool is_extended, u32 suppress_level);
> > 
> > Then in devkmsg_read():
> > 
> > printk_get_next_message(&pmsg, atomic64_read(&user->seq), true, NO_SUPRRESS)
> 
> Petr, what do you think about this? I remember when we discussed this before
> we talked about either determining state via `struct console` (which seems
> to turn out not to be feasible) or passing another argument.
> 
> Do you prefer to have another argument or do the bit dance?
> 
> Personally I prefer the simpler solution with more arguments instead of bit
> stuffing if we have to go this way, but I'm up for whichever sounds good to
> you.

Ah, I though that John's proposal was reasonable. But it is true that
the meaning of @supress_level is not clear.

I see two possibilities:

  1. printk_get_next_message() and console_emit_next_record()
     could pass con->level.

     But then we would need to create the extra value for devkmsg_read().


 2. printk_get_next_message() and console_emit_next_record()
    could pass console_effective_loglevel().

    devkmsg_read() could pass CONSOLE_LOGLEVEL_MOTORMOUTH.


Sigh, it seems that any solution is hairy, including the one which
passed @con.

I personally think that the 2nd variant, passing the effective
loglevel, is least ugly. I am just not sure about a good name
for the parameter. What about?

    bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
				 bool is_extended, int con_eff_level);

Note that this would require passing the effective loglevel also
to suppress_message_printing() so that we would get:

static bool suppress_message_printing(int level, int con_eff_level)
{
	return level >= con_eff_level;
}

Best Regards,
Petr