[PATCH v6 00/11] printk: console: Per-console loglevels

[PATCH v6 00/11] printk: console: Per-console loglevels

[Chris Down]

v6:

- Add .rst suffix to documentation in do_syslog
- Add loglevel table to per-console-loglevel.rst and serial-console.rst
- Add newlines between multiline bullets in per-console-loglevel.rst
- Make effective_loglevel doc more clear
- Remove ignore_per_console_loglevel doc, it's not shown in sysfs now
- Use READ_ONCE/WRITE_ONCE for con->level
- Ignore/restore per console loglevel in sysrq
- Add new fields to comment above struct console
- Remove now unused flags field on console_cmdline
- Remove WARN_ON_ONCE(!con) in effective loglevel checks
- Avoid underflow in find_and_remove_console_option if val_buf_size == 0
- Better error message on oversize in find_and_remove_loglevel_option
- Reject if clamped in find_and_remove_loglevel_option
- Clarify level setting logic in __add_preferred_console
- Move console emission check to printk_delay itself
- Use console_src_read_flags in enabled_show
- Infer if extended from con in printk_get_next_message, don't pass args
- Remove misleading comment about early consoles in console_init
- Use a goto in loglevel_store to avoid setting level in multiple places
- Mention only @flags and @level are valid in printk_get_next_message
- Use LOGLEVEL_DEBUG for max clamp in sysctls
- Update for class_create interface changes
- Move sysctl functionality out to sysfs.c
- Purge default_console_loglevel
- Update sysctl docs for kernel.printk deprecation

v5:

- Fix syntax in boot_delay

v4:

- Change base to Linus' master
- Use SRCU iterators for console walks
- Override per-console loglevels on magic sysrq
- Fix htmldocs
- Fix mistaken __user annotation in sysctl callbacks
- Consistently use indexed names (eg. ttyS0 instead of ttyS)
- Remove "The loglevel for a console can be set in many places" comment
- Remove CON_LOGLEVEL flag and infer based on >0
- Open code our dev_get_drvdata console stashing
- Split out console_effective_loglevel functions per Petr's suggestion
- Make boot_delay_msec/printk_delay check if it would be emitted
- Simplify warning on SYSLOG_ACTION_CONSOLE_LEVEL
- Save/restore ignore_per_console_loglevel on syslog console actions
- Unify min/max level checks across sysfs/proc/syslog
- Add find_and_remove_console_option to avoid affecting io/mmio options

v3:

- Update to work with John's kthread patches
- Remove force_console_loglevel, now we only have global and local levels
- Remove minimum_console_loglevel control and document how to change it
- The minimum loglevel is now only honoured on setting global/local level
- Add ignore_per_console_loglevel
- Return -EINVAL if trying to set below minimum console level
- Add parser for named console= options
- Fix docs around ignore_loglevel: it can be changed at runtime
- Fix ordering in "in order of authority" docs
- Remove duplicated default_console_loglevel doc
- Only warn once on syslog() use

v2:

- Dynamically allocate struct device*
- Document sysfs attributes in Documentation/ABI/
- Use sysfs_emit() instead of sprintf() in dev sysfs files
- Remove WARN_ON() for device_add/IS_ERR(console_class)
- Remove "soon" comment for kernel.printk
- Fix !CONFIG_PRINTK build
- Fix device_unregister() NULL dereference if called before class setup
- Add new documentation to MAINTAINERS

Chris Down (11):
  printk: Avoid delaying messages that aren't solicited by any console
  printk: Use struct console for suppression and extended console state
  printk: console: Implement core per-console loglevel infrastructure
  printk: Support toggling per-console loglevel via syslog() and cmdline
  MAINTAINERS: Mark printk-basics.rst as owned by printk subsystem
  printk: console: Introduce sysfs interface for per-console loglevels
  printk: Constrain hardware-addressed console checks to name position
  printk: Support setting initial console loglevel via console= on
    cmdline
  printk: Add sysctl interface to set global loglevels
  printk: Deprecate the kernel.printk sysctl interface
  printk: Purge default_console_loglevel

 Documentation/ABI/testing/sysfs-class-console |  47 +++
 Documentation/admin-guide/index.rst           |   1 +
 .../admin-guide/kernel-parameters.txt         |  24 +-
 .../admin-guide/per-console-loglevel.rst      | 111 ++++++++
 Documentation/admin-guide/serial-console.rst  |  37 ++-
 Documentation/admin-guide/sysctl/kernel.rst   |  25 +-
 Documentation/core-api/printk-basics.rst      |  35 +--
 Documentation/networking/netconsole.rst       |  17 ++
 MAINTAINERS                                   |   3 +
 drivers/tty/sysrq.c                           |  20 ++
 include/linux/console.h                       |   5 +
 include/linux/printk.h                        |  10 +-
 kernel/printk/Makefile                        |   2 +-
 kernel/printk/console_cmdline.h               |   1 +
 kernel/printk/internal.h                      |  19 +-
 kernel/printk/nbcon.c                         |   2 +-
 kernel/printk/printk.c                        | 267 ++++++++++++++++--
 kernel/printk/sysctl.c                        |  56 +++-
 kernel/printk/sysfs.c                         | 178 ++++++++++++
 19 files changed, 795 insertions(+), 65 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-console
 create mode 100644 Documentation/admin-guide/per-console-loglevel.rst
 create mode 100644 kernel/printk/sysfs.c


base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e
-- 
2.46.0

[PATCH v6 01/11] printk: Avoid delaying messages that aren't solicited by any console

[Chris Down]

printk_delay() may introduce delays even when no console wants to emit
the message, which is unnecessary and may hold back messages we actually
care about. Add a check in printk_delay() to determine if any console
will print the message to avoid introducing unnecessary delays. This
change aligns with the existing behaviour of boot-delayed printk
messages, which already have a similar check.

Signed-off-by: Chris Down <chris@chrisdown.name>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index beb808f4c367..d6159f1c5b29 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1315,15 +1315,13 @@ static int __init boot_delay_setup(char *str)
 }
 early_param("boot_delay", boot_delay_setup);
 
-static void boot_delay_msec(int level)
+static void boot_delay_msec(void)
 {
 	unsigned long long k;
 	unsigned long timeout;
 
-	if ((boot_delay == 0 || system_state >= SYSTEM_RUNNING)
-		|| suppress_message_printing(level)) {
+	if (boot_delay == 0 || system_state >= SYSTEM_RUNNING)
 		return;
-	}
 
 	k = (unsigned long long)loops_per_msec * boot_delay;
 
@@ -1342,7 +1340,7 @@ static void boot_delay_msec(int level)
 	}
 }
 #else
-static inline void boot_delay_msec(int level)
+static inline void boot_delay_msec(void)
 {
 }
 #endif
@@ -2124,7 +2122,10 @@ int printk_delay_msec __read_mostly;
 
 static inline void printk_delay(int level)
 {
-	boot_delay_msec(level);
+	if (suppress_message_printing(level))
+		return;
+
+	boot_delay_msec();
 
 	if (unlikely(printk_delay_msec)) {
 		int m = printk_delay_msec;
-- 
2.46.0

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

[Chris Down]

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. As part of the upcoming
per-console loglevel support we need the console object here anyway, so
it makes sense to amortise this now.

devkmsg_read() has special behaviour here, but all other consoles follow
the same patterns and can have their extension/suppression states
determined from their struct console.

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 kernel/printk/internal.h |  4 ++--
 kernel/printk/nbcon.c    |  2 +-
 kernel/printk/printk.c   | 33 ++++++++++++++++++++-------------
 3 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 3fcb48502adb..58ad209e0310 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -328,8 +328,8 @@ struct printk_message {
 };
 
 bool other_cpu_in_panic(void);
-bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
-			     bool is_extended, bool may_supress);
+bool printk_get_next_message(struct printk_message *pmsg, struct console *con,
+			     u64 seq);
 
 #ifdef CONFIG_PRINTK
 void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped);
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index fd12efcc4aed..5ae1155c34d3 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -974,7 +974,7 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt, bool use_a
 	if (!nbcon_context_enter_unsafe(ctxt))
 		return false;
 
-	ctxt->backlog = printk_get_next_message(&pmsg, ctxt->seq, is_extended, true);
+	ctxt->backlog = printk_get_next_message(&pmsg, con, ctxt->seq);
 	if (!ctxt->backlog)
 		return nbcon_context_exit_unsafe(ctxt);
 
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index d6159f1c5b29..dfe7011b863a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -833,7 +833,7 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
 	if (ret)
 		return ret;
 
-	if (!printk_get_next_message(&pmsg, atomic64_read(&user->seq), true, false)) {
+	if (!printk_get_next_message(&pmsg, NULL, atomic64_read(&user->seq))) {
 		if (file->f_flags & O_NONBLOCK) {
 			ret = -EAGAIN;
 			goto out;
@@ -850,8 +850,8 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
 		 * This pairs with __wake_up_klogd:A.
 		 */
 		ret = wait_event_interruptible(log_wait,
-				printk_get_next_message(&pmsg, atomic64_read(&user->seq), true,
-							false)); /* LMM(devkmsg_read:A) */
+				printk_get_next_message(&pmsg, NULL,
+					atomic64_read(&user->seq))); /* LMM(devkmsg_read:A) */
 		if (ret)
 			goto out;
 	}
@@ -2925,20 +2925,19 @@ void console_prepend_replay(struct printk_message *pmsg)
  * @pmsg will contain the formatted result. @pmsg->pbufs must point to a
  * struct printk_buffers.
  *
+ * @con is the console in question. Only @con->flags and @con->level are
+ * guaranteed to be valid at this point. Note especially well that con->seq is
+ * not yet guaranteed to be consistent with @seq.
+ *
  * @seq is the record to read and format. If it is not available, the next
  * valid record is read.
  *
- * @is_extended specifies if the message should be formatted for extended
- * console output.
- *
- * @may_supress specifies if records may be skipped based on loglevel.
- *
  * Returns false if no record is available. Otherwise true and all fields
  * of @pmsg are valid. (See the documentation of struct printk_message
  * for information about the @pmsg fields.)
  */
-bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
-			     bool is_extended, bool may_suppress)
+bool printk_get_next_message(struct printk_message *pmsg, struct console *con,
+			     u64 seq)
 {
 	struct printk_buffers *pbufs = pmsg->pbufs;
 	const size_t scratchbuf_sz = sizeof(pbufs->scratchbuf);
@@ -2948,6 +2947,14 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
 	struct printk_info info;
 	struct printk_record r;
 	size_t len = 0;
+	bool is_extended;
+
+	if (con) {
+		is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
+	} else {
+		/* Used only by devkmsg_read(). */
+		is_extended = true;
+	}
 
 	/*
 	 * Formatting extended messages requires a separate buffer, so use the
@@ -2967,8 +2974,8 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
 	pmsg->seq = r.info->seq;
 	pmsg->dropped = r.info->seq - seq;
 
-	/* Skip record that has level above the console loglevel. */
-	if (may_suppress && suppress_message_printing(r.info->level))
+	/* Never suppress when used in devkmsg_read() */
+	if (con && suppress_message_printing(r.info->level))
 		goto out;
 
 	if (is_extended) {
@@ -3044,7 +3051,7 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
 
 	*handover = false;
 
-	if (!printk_get_next_message(&pmsg, con->seq, is_extended, true))
+	if (!printk_get_next_message(&pmsg, con, con->seq))
 		return false;
 
 	con->dropped += pmsg.dropped;
-- 
2.46.0

[PATCH v6 03/11] printk: console: Implement core per-console loglevel infrastructure

[Chris Down]

Consoles can have vastly different latencies and throughputs. For
example, writing a message to the serial console can take on the order
of tens of milliseconds to get the UART to successfully write a message.
While this might be fine for a single, one-off message, this can cause
significant application-level stalls in situations where the kernel
writes large amounts of information to the console.

This means that while you might want to send at least INFO level
messages to (for example) netconsole, which is relatively fast, you may
only want to send at least WARN level messages to the serial console.
Such an implementation would permit debugging using the serial console
in cases that netconsole doesn't receive messages during particularly
bad system issues, while still keeping the noise low enough to avoid
inducing latency in userspace applications. To mitigate this, add such
an interface, extending the existing console loglevel controls to allow
each console to have its own loglevel.

One can't just disable the serial console, because one may actually need
it in situations where the machine is in a bad enough state that nothing
is received on netconsole. One also can't just bump the loglevel at
runtime after the issue, because usually the machine is already so
wedged by this point that it isn't responsive to such requests.

The sysfs and kernel command line interfaces to set the per-console
loglevel will be added later. For now, simply add the necessary internal
infrastructure to be used by later patches.

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 drivers/tty/sysrq.c      | 15 ++++++++
 include/linux/console.h  |  2 ++
 include/linux/printk.h   |  5 +++
 kernel/printk/internal.h | 10 ++++++
 kernel/printk/printk.c   | 78 +++++++++++++++++++++++++++++++++++++---
 5 files changed, 106 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 930b04e3d148..daa9fe7dad54 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -51,6 +51,7 @@
 #include <linux/syscalls.h>
 #include <linux/of.h>
 #include <linux/rcupdate.h>
+#include <linux/console.h>
 
 #include <asm/ptrace.h>
 #include <asm/irq_regs.h>
@@ -101,11 +102,25 @@ __setup("sysrq_always_enabled", sysrq_always_enabled_setup);
 static void sysrq_handle_loglevel(u8 key)
 {
 	u8 loglevel = key - '0';
+	int cookie;
+	int warned = 0;
+	struct console *con;
 
 	console_loglevel = CONSOLE_LOGLEVEL_DEFAULT;
 	pr_info("Loglevel set to %u\n", loglevel);
 	console_loglevel = loglevel;
+
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(con) {
+		if (!warned && per_console_loglevel_is_set(con)) {
+			warned = 1;
+			pr_warn("Overriding per-console loglevel from sysrq\n");
+		}
+		WRITE_ONCE(con->level, -1);
+	}
+	console_srcu_read_unlock(cookie);
 }
+
 static const struct sysrq_key_op sysrq_loglevel_op = {
 	.handler	= sysrq_handle_loglevel,
 	.help_msg	= "loglevel(0-9)",
diff --git a/include/linux/console.h b/include/linux/console.h
index eba367bf605d..3ff22bfeef2a 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -321,6 +321,7 @@ struct nbcon_write_context {
  * @dropped:		Number of unreported dropped ringbuffer records
  * @data:		Driver private data
  * @node:		hlist node for the console list
+ * @level:		Console-specific loglevel
  *
  * @nbcon_state:	State for nbcon consoles
  * @nbcon_seq:		Sequence number of the next record for nbcon to print
@@ -349,6 +350,7 @@ struct console {
 	unsigned long		dropped;
 	void			*data;
 	struct hlist_node	node;
+	int			level;
 
 	/* nbcon console specific members */
 
diff --git a/include/linux/printk.h b/include/linux/printk.h
index eca9bb2ee637..5fbd6b7f1ab4 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -204,6 +204,7 @@ void printk_legacy_allow_panic_sync(void);
 extern bool nbcon_device_try_acquire(struct console *con);
 extern void nbcon_device_release(struct console *con);
 void nbcon_atomic_flush_unsafe(void);
+bool per_console_loglevel_is_set(const struct console *con);
 #else
 static inline __printf(1, 0)
 int vprintk(const char *s, va_list args)
@@ -303,6 +304,10 @@ static inline void nbcon_device_release(struct console *con)
 static inline void nbcon_atomic_flush_unsafe(void)
 {
 }
+static inline bool per_console_loglevel_is_set(const struct console *con)
+{
+	return false;
+}
 
 #endif
 
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 58ad209e0310..9303839d0551 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -15,6 +15,16 @@ int devkmsg_sysctl_set_loglvl(const struct ctl_table *table, int write,
 #define printk_sysctl_init() do { } while (0)
 #endif
 
+enum loglevel_source {
+	LLS_GLOBAL,
+	LLS_LOCAL,
+	LLS_IGNORE_LOGLEVEL,
+};
+
+enum loglevel_source
+console_effective_loglevel_source(const struct console *con);
+int console_effective_loglevel(const struct console *con);
+
 #define con_printk(lvl, con, fmt, ...)				\
 	printk(lvl pr_fmt("%s%sconsole [%s%d] " fmt),		\
 		(con->flags & CON_NBCON) ? "" : "legacy ",	\
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index dfe7011b863a..2e99b63efb46 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1287,9 +1287,62 @@ module_param(ignore_loglevel, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(ignore_loglevel,
 		 "ignore loglevel setting (prints all kernel messages to the console)");
 
-static bool suppress_message_printing(int level)
+bool per_console_loglevel_is_set(const struct console *con)
 {
-	return (level >= console_loglevel && !ignore_loglevel);
+	return con && (READ_ONCE(con->level) > 0);
+}
+
+/*
+ * Hierarchy of loglevel authority:
+ *
+ * 1. con->level. The locally set, console-specific loglevel. Optional, only
+ *    valid if >0.
+ * 2. console_loglevel. The default global console loglevel, always present.
+ */
+enum loglevel_source
+console_effective_loglevel_source(const struct console *con)
+{
+	if (WARN_ON_ONCE(!con))
+		return LLS_GLOBAL;
+
+	if (ignore_loglevel)
+		return LLS_IGNORE_LOGLEVEL;
+
+	if (per_console_loglevel_is_set(con))
+		return LLS_LOCAL;
+
+	return LLS_GLOBAL;
+}
+
+int console_effective_loglevel(const struct console *con)
+{
+	enum loglevel_source source;
+	int level;
+
+	source = console_effective_loglevel_source(con);
+
+	switch (source) {
+	case LLS_IGNORE_LOGLEVEL:
+		level = CONSOLE_LOGLEVEL_MOTORMOUTH;
+		break;
+	case LLS_LOCAL:
+		level = READ_ONCE(con->level);
+		break;
+	case LLS_GLOBAL:
+		level = console_loglevel;
+		break;
+	default:
+		pr_warn("Unhandled console loglevel source: %d", source);
+		level = console_loglevel;
+		break;
+	}
+
+	return level;
+}
+
+static bool suppress_message_printing(int level, struct console *con)
+{
+	return level >= console_effective_loglevel(con);
 }
 
 #ifdef CONFIG_BOOT_PRINTK_DELAY
@@ -2122,7 +2175,21 @@ int printk_delay_msec __read_mostly;
 
 static inline void printk_delay(int level)
 {
-	if (suppress_message_printing(level))
+	bool will_emit = false;
+	int cookie;
+	struct console *con;
+
+	cookie = console_srcu_read_lock();
+
+	for_each_console_srcu(con) {
+		if (!suppress_message_printing(level, con)) {
+			will_emit = true;
+			break;
+		}
+	}
+	console_srcu_read_unlock(cookie);
+
+	if (!will_emit)
 		return;
 
 	boot_delay_msec();
@@ -2975,7 +3042,7 @@ bool printk_get_next_message(struct printk_message *pmsg, struct console *con,
 	pmsg->dropped = r.info->seq - seq;
 
 	/* Never suppress when used in devkmsg_read() */
-	if (con && suppress_message_printing(r.info->level))
+	if (con && suppress_message_printing(r.info->level, con))
 		goto out;
 
 	if (is_extended) {
@@ -3789,6 +3856,9 @@ static int try_enable_preferred_console(struct console *newcon,
 			if (newcon->index < 0)
 				newcon->index = c->index;
 
+			// TODO: Will be configurable in a later patch
+			newcon->level = -1;
+
 			if (_braille_register_console(newcon, c))
 				return 0;
 
-- 
2.46.0

[PATCH v6 04/11] printk: Support toggling per-console loglevel via syslog() and cmdline

[Chris Down]

A new module parameter (ignore_per_console_loglevel) is added, which can
be set via the kernel command line or at runtime through
/sys/module/printk/parameters/ignore_per_console_loglevel. When set, the
per-console loglevels are ignored, and the global console loglevel
(console_loglevel) is used for all consoles.

During sysrq, we temporarily disable per-console loglevels to ensure all
requisite messages are printed to the console. This is necessary because
sysrq is often used in dire circumstances where access to
/sys/class/console may not be trivially possible.

Additionally, the syslog actions SYSLOG_ACTION_CONSOLE_ON and
SYSLOG_ACTION_CONSOLE_OFF are augmented to save and restore the state of
ignore_per_console_loglevel. This allows administrators to enable or
disable per-console loglevels dynamically using the syslog() system
call, as supported in userspace by things like dmesg.

This is useful when debugging issues with message emission, or when
needing to quickly break glass and revert to global loglevel only.

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 Documentation/admin-guide/index.rst           |  1 +
 .../admin-guide/per-console-loglevel.rst      | 70 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 drivers/tty/sysrq.c                           |  5 ++
 include/linux/printk.h                        |  2 +
 kernel/printk/printk.c                        | 39 ++++++++++-
 6 files changed, 116 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/admin-guide/per-console-loglevel.rst

diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
index e85b1adf5908..366a08a1eee2 100644
--- a/Documentation/admin-guide/index.rst
+++ b/Documentation/admin-guide/index.rst
@@ -119,6 +119,7 @@ configure specific aspects of kernel behavior to your liking.
    namespaces/index
    numastat
    parport
+   per-console-loglevel
    perf-security
    pm/index
    pnp
diff --git a/Documentation/admin-guide/per-console-loglevel.rst b/Documentation/admin-guide/per-console-loglevel.rst
new file mode 100644
index 000000000000..1ec7608f94b0
--- /dev/null
+++ b/Documentation/admin-guide/per-console-loglevel.rst
@@ -0,0 +1,70 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+.. _per_console_loglevel:
+
+Per-console loglevel support
+============================
+
+Motivation
+----------
+
+Consoles can have vastly different latencies and throughputs. For example,
+writing a message to the serial console can take on the order of tens of
+milliseconds to get the UART to successfully write a message. While this might
+be fine for a single, one-off message, this can cause significant
+application-level stalls in situations where the kernel writes large amounts of
+information to the console.
+
+This means that while you might want to send at least INFO level messages to
+(for example) netconsole, which is relatively fast, you may only want to send
+at least WARN level messages to the serial console. This permits debugging
+using the serial console in cases that netconsole doesn't receive messages
+during particularly bad system issues, while still keeping the noise low enough
+to avoid inducing latency in userspace applications.
+
+Loglevel
+--------
+
+Kernel loglevels are defined thus:
+
++---+--------------+-----------------------------------+
+| 0 | KERN_EMERG   | system is unusable                |
++---+--------------+-----------------------------------+
+| 1 | KERN_ALERT   | action must be taken immediately  |
++---+--------------+-----------------------------------+
+| 2 | KERN_CRIT    | critical conditions               |
++---+--------------+-----------------------------------+
+| 3 | KERN_ERR     | error conditions                  |
++---+--------------+-----------------------------------+
+| 4 | KERN_WARNING | warning conditions                |
++---+--------------+-----------------------------------+
+| 5 | KERN_NOTICE  | normal but significant condition  |
++---+--------------+-----------------------------------+
+| 6 | KERN_INFO    | informational                     |
++---+--------------+-----------------------------------+
+| 7 | KERN_DEBUG   | debug-level messages              |
++---+--------------+-----------------------------------+
+
+Tunables
+--------
+
+In order to allow tuning per-console loglevels, the following controls exist:
+
+Global
+~~~~~~
+
+The global loglevel is set by the ``kernel.console_loglevel`` sysctl, which can
+also be set as ``loglevel=`` on the kernel command line.
+
+The printk module also takes two parameters which modify this behaviour
+further:
+
+* ``ignore_loglevel`` on the kernel command line or set in printk parameters:
+  Emit all messages. All other controls are ignored if this is present.
+
+* ``ignore_per_console_loglevel`` on the kernel command line or set in printk
+  parameters: Ignore all per-console loglevels and use the global loglevel.
+
+The default value for ``kernel.console_loglevel`` comes from
+``CONFIG_CONSOLE_LOGLEVEL_DEFAULT``, or ``CONFIG_CONSOLE_LOGLEVEL_QUIET`` if
+``quiet`` is passed on the kernel command line.
diff --git a/MAINTAINERS b/MAINTAINERS
index a27407950242..36490a1fc721 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18449,6 +18449,7 @@ R:	John Ogness <john.ogness@linutronix.de>
 R:	Sergey Senozhatsky <senozhatsky@chromium.org>
 S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git
+F:	Documentation/admin-guide/per-console-loglevel.rst
 F:	include/linux/printk.h
 F:	kernel/printk/
 
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index daa9fe7dad54..84befb6d8d82 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -598,6 +598,7 @@ static void __sysrq_put_key_op(u8 key, const struct sysrq_key_op *op_p)
 void __handle_sysrq(u8 key, bool check_mask)
 {
 	const struct sysrq_key_op *op_p;
+	bool orig_ignore_per_console_loglevel;
 	int orig_log_level;
 	int orig_suppress_printk;
 	int i;
@@ -616,6 +617,9 @@ void __handle_sysrq(u8 key, bool check_mask)
 	orig_log_level = console_loglevel;
 	console_loglevel = CONSOLE_LOGLEVEL_DEFAULT;
 
+	orig_ignore_per_console_loglevel = ignore_per_console_loglevel;
+	ignore_per_console_loglevel = true;
+
 	op_p = __sysrq_get_key_op(key);
 	if (op_p) {
 		/*
@@ -651,6 +655,7 @@ void __handle_sysrq(u8 key, bool check_mask)
 	rcu_read_unlock();
 	rcu_sysrq_end();
 
+	ignore_per_console_loglevel = orig_ignore_per_console_loglevel;
 	suppress_printk = orig_suppress_printk;
 }
 
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 5fbd6b7f1ab4..0053533dcfec 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -73,6 +73,8 @@ extern int console_printk[];
 #define minimum_console_loglevel (console_printk[2])
 #define default_console_loglevel (console_printk[3])
 
+extern bool ignore_per_console_loglevel;
+
 extern void console_verbose(void);
 
 /* strlen("ratelimit") + 1 */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 2e99b63efb46..055619c5c7e8 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -103,6 +103,9 @@ DEFINE_STATIC_SRCU(console_srcu);
  */
 int __read_mostly suppress_printk;
 
+/* The sysrq infrastructure needs this even on !CONFIG_PRINTK. */
+bool __read_mostly ignore_per_console_loglevel;
+
 #ifdef CONFIG_LOCKDEP
 static struct lockdep_map console_lock_dep_map = {
 	.name = "console_lock"
@@ -1287,9 +1290,21 @@ module_param(ignore_loglevel, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(ignore_loglevel,
 		 "ignore loglevel setting (prints all kernel messages to the console)");
 
+static int __init ignore_per_console_loglevel_setup(char *str)
+{
+	ignore_per_console_loglevel = true;
+	return 0;
+}
+
+early_param("ignore_per_console_loglevel", ignore_per_console_loglevel_setup);
+module_param(ignore_per_console_loglevel, bool, 0644);
+MODULE_PARM_DESC(
+	ignore_per_console_loglevel,
+	"ignore per-console loglevel setting (only respect global console loglevel)");
+
 bool per_console_loglevel_is_set(const struct console *con)
 {
-	return con && (READ_ONCE(con->level) > 0);
+	return !ignore_per_console_loglevel && con && (READ_ONCE(con->level) > 0);
 }
 
 /*
@@ -1298,6 +1313,16 @@ bool per_console_loglevel_is_set(const struct console *con)
  * 1. con->level. The locally set, console-specific loglevel. Optional, only
  *    valid if >0.
  * 2. console_loglevel. The default global console loglevel, always present.
+ *
+ * The behaviour can be further changed by the following printk module
+ * parameters:
+ *
+ * 1. ignore_loglevel. Can be set at boot or at runtime with
+ *    /sys/module/printk/parameters/ignore_loglevel. Overrides absolutely
+ *    everything since it's used to debug.
+ * 2. ignore_per_console_loglevel. Existing per-console loglevel values are left
+ *    intact, but are ignored in favour of console_loglevel as long as this is
+ *    true. Also manipulated through syslog(SYSLOG_ACTION_CONSOLE_{ON,OFF}).
  */
 enum loglevel_source
 console_effective_loglevel_source(const struct console *con)
@@ -1796,6 +1821,7 @@ int do_syslog(int type, char __user *buf, int len, int source)
 	struct printk_info info;
 	bool clear = false;
 	static int saved_console_loglevel = LOGLEVEL_DEFAULT;
+	static int saved_ignore_per_console_loglevel;
 	int error;
 
 	error = check_syslog_permissions(type, source);
@@ -1836,19 +1862,28 @@ int do_syslog(int type, char __user *buf, int len, int source)
 		break;
 	/* Disable logging to console */
 	case SYSLOG_ACTION_CONSOLE_OFF:
-		if (saved_console_loglevel == LOGLEVEL_DEFAULT)
+		if (saved_console_loglevel == LOGLEVEL_DEFAULT) {
 			saved_console_loglevel = console_loglevel;
+			saved_ignore_per_console_loglevel =
+				ignore_per_console_loglevel;
+		}
 		console_loglevel = minimum_console_loglevel;
+		ignore_per_console_loglevel = true;
 		break;
 	/* Enable logging to console */
 	case SYSLOG_ACTION_CONSOLE_ON:
 		if (saved_console_loglevel != LOGLEVEL_DEFAULT) {
 			console_loglevel = saved_console_loglevel;
+			ignore_per_console_loglevel =
+				saved_ignore_per_console_loglevel;
 			saved_console_loglevel = LOGLEVEL_DEFAULT;
 		}
 		break;
 	/* Set level of messages printed to console */
 	case SYSLOG_ACTION_CONSOLE_LEVEL:
+		if (!ignore_per_console_loglevel)
+			pr_warn_once(
+				"SYSLOG_ACTION_CONSOLE_LEVEL is ignored by consoles with an explicitly set per-console loglevel, see Documentation/admin-guide/per-console-loglevel.rst\n");
 		if (len < 1 || len > 8)
 			return -EINVAL;
 		if (len < minimum_console_loglevel)
-- 
2.46.0

[PATCH v6 05/11] MAINTAINERS: Mark printk-basics.rst as owned by printk subsystem

[Chris Down]

We are going to update this in the next patch, so while we're here, we
might as well mark it as owned by us.

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 36490a1fc721..ea134675669e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18450,6 +18450,7 @@ R:	Sergey Senozhatsky <senozhatsky@chromium.org>
 S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git
 F:	Documentation/admin-guide/per-console-loglevel.rst
+F:	Documentation/core-api/printk-basics.rst
 F:	include/linux/printk.h
 F:	kernel/printk/
 
-- 
2.46.0

[PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Chris Down]

A sysfs interface under /sys/class/console/ is created that permits
viewing and configuring per-console attributes. This is the main
interface with which we expect users to interact with and configure
per-console loglevels.

Each console device now has its own directory (for example,
/sys/class/console/ttyS0/) containing the following attributes:

- effective_loglevel (ro): The effective loglevel for the console after
  considering all loglevel authorities (e.g., global loglevel,
  per-console loglevel).
- effective_loglevel_source (ro): The source of the effective loglevel
  (e.g., local, global, ignore_loglevel).
- enabled (ro): Indicates whether the console is enabled (1) or disabled
  (0).
- loglevel (rw): The per-console loglevel. Writing a value between 0
  (KERN_EMERG) and 8 (KERN_DEBUG + 1) sets the per-console loglevel.
  Writing -1 disables the per-console loglevel.

In terms of technical implementation, we embed a device pointer in the
console struct, and register each console using it so we can expose
attributes in sysfs. We currently expose the following attributes:

    % ls -l /sys/class/console/ttyS0/
    total 0
    lrwxrwxrwx 1 root root    0 Oct 23 13:17 subsystem -> ../../../../class/console/
    -r--r--r-- 1 root root 4096 Oct 23 13:18 effective_loglevel
    -r--r--r-- 1 root root 4096 Oct 23 13:18 effective_loglevel_source
    -r--r--r-- 1 root root 4096 Oct 23 13:18 enabled
    -rw-r--r-- 1 root root 4096 Oct 23 13:18 loglevel
    -rw-r--r-- 1 root root 4096 Oct 23 13:17 uevent

The lifecycle of this classdev looks like this on registration:

    register_console(con)/printk_late_init()
      console_register_device(con)
        device_initialize(con->classdev) # refcount++
        device_add(con->classdev)        # refcount++

At stable state, the refcount is two.

Console unregistration looks like this:

    [con->classdev refcount drops to 0]
      console_classdev_release(con->classdev)
        kfree(con->classdev)

    unregister_console(con)
      device_unregister(con->classdev)
        device_del(con->classdev) # refcount--
          device_remove_class_symlinks()
            kernfs_remove_by_name_ns()
              kernfs_drain()
                kernfs_drain_open_files() # wait for close()
        put_device(con->classdev) # refcount--

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 Documentation/ABI/testing/sysfs-class-console |  47 +++++
 .../admin-guide/per-console-loglevel.rst      |  41 ++++
 Documentation/core-api/printk-basics.rst      |  35 ++--
 Documentation/networking/netconsole.rst       |  13 ++
 MAINTAINERS                                   |   1 +
 include/linux/console.h                       |   3 +
 include/linux/printk.h                        |   2 +
 kernel/printk/Makefile                        |   2 +-
 kernel/printk/internal.h                      |   5 +
 kernel/printk/printk.c                        |  15 ++
 kernel/printk/sysfs.c                         | 178 ++++++++++++++++++
 11 files changed, 324 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-console
 create mode 100644 kernel/printk/sysfs.c

diff --git a/Documentation/ABI/testing/sysfs-class-console b/Documentation/ABI/testing/sysfs-class-console
new file mode 100644
index 000000000000..40b90b190af3
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-console
@@ -0,0 +1,47 @@
+What:		/sys/class/console/
+Date:		October 2024
+Contact:	Chris Down <chris@chrisdown.name>
+Description:	Interface for viewing and setting per-console attributes, like
+		the per-console loglevel. For a high-level document describing
+		the motivations for this interface and related non-sysfs
+		controls, see
+		Documentation/admin-guide/per-console-loglevel.rst.
+
+What:		/sys/class/console/<C>/effective_loglevel
+Date:		October 2024
+Contact:	Chris Down <chris@chrisdown.name>
+Description:	Read only. The currently effective loglevel for this console.
+		All messages emitted with a loglevel below the effective value
+		will be emitted to the console.
+
+What:		/sys/class/console/<C>/effective_loglevel_source
+Date:		October 2024
+Contact:	Chris Down <chris@chrisdown.name>
+Description:	Read only. The currently effective loglevel source for this
+		console -- for example, whether it was set globally, or whether
+		it was set locally for this console.
+
+		Possible values are:
+			=============== ============================================
+			local           The loglevel comes from the per-console
+			                loglevel.
+			global          The loglevel comes from the global loglevel.
+			ignore_loglevel Both the per-console loglevel and global
+			                loglevels are ignored as ignore_loglevel is
+			                present on the kernel command line.
+			=============== ============================================
+
+What:		/sys/class/console/<C>/enabled
+Date:		October 2024
+Contact:	Chris Down <chris@chrisdown.name>
+Description:	Read only. "1" if the console is enabled, "0" otherwise.
+
+What:		/sys/class/console/<C>/loglevel
+Date:		October 2024
+Contact:	Chris Down <chris@chrisdown.name>
+Description:	Read write. The current per-console loglevel, which will take
+		effect if not overridden by other non-sysfs controls (see
+		Documentation/admin-guide/per-console-loglevel.rst). Bounds are
+		0 (LOGLEVEL_EMERG) to 8 (LOGLEVEL_DEBUG + 1) inclusive. Also
+		takes the special value "-1" to indicate that no per-console
+		loglevel is set, and we should defer to the global controls.
diff --git a/Documentation/admin-guide/per-console-loglevel.rst b/Documentation/admin-guide/per-console-loglevel.rst
index 1ec7608f94b0..41bf3befb2f3 100644
--- a/Documentation/admin-guide/per-console-loglevel.rst
+++ b/Documentation/admin-guide/per-console-loglevel.rst
@@ -68,3 +68,44 @@ further:
 The default value for ``kernel.console_loglevel`` comes from
 ``CONFIG_CONSOLE_LOGLEVEL_DEFAULT``, or ``CONFIG_CONSOLE_LOGLEVEL_QUIET`` if
 ``quiet`` is passed on the kernel command line.
+
+Console attributes
+~~~~~~~~~~~~~~~~~~
+
+Registered consoles are exposed at ``/sys/class/console``. For example, if you
+are using ``ttyS0``, the console backing it can be viewed at
+``/sys/class/console/ttyS0/``. The following files are available:
+
+* ``effective_loglevel`` (r): The effective loglevel after considering all
+  loglevel authorities. For example, it shows the value of the console-specific
+  loglevel when a console-specific loglevel is defined, and shows the global
+  console loglevel value when the console-specific one is not defined.
+
+* ``effective_loglevel_source`` (r): The loglevel authority which resulted in
+  the effective loglevel being set. The following values can be present:
+
+    * ``local``: The console-specific loglevel is in effect.
+
+    * ``global``: The global loglevel (``kernel.console_loglevel``) is in
+      effect. Set a console-specific loglevel to override it.
+
+    * ``ignore_loglevel``: ``ignore_loglevel`` was specified on the kernel
+      command line or at ``/sys/module/printk/parameters/ignore_loglevel``.
+      Disable it to use level controls.
+
+* ``enabled`` (r): Whether the console is enabled.
+
+* ``loglevel`` (rw): The local, console-specific loglevel for this console.
+  This will be in effect if no other global control overrides it. Look at
+  ``effective_loglevel`` and ``effective_loglevel_source`` to verify that.
+
+Deprecated
+~~~~~~~~~~
+
+* ``kernel.printk`` sysctl: this takes four values, setting
+  ``kernel.console_loglevel``, ``kernel.default_message_loglevel``, the minimum
+  console loglevel, and a fourth unused value. The interface is generally
+  considered to quite confusing, doesn't perform checks on the values given,
+  and is unaware of per-console loglevel semantics.
+
+Chris Down <chris@chrisdown.name>, 13-October-2024
diff --git a/Documentation/core-api/printk-basics.rst b/Documentation/core-api/printk-basics.rst
index 2dde24ca7d9f..bfad359505bb 100644
--- a/Documentation/core-api/printk-basics.rst
+++ b/Documentation/core-api/printk-basics.rst
@@ -54,32 +54,33 @@ string, the log level is not a separate argument). The available log levels are:
 
 The log level specifies the importance of a message. The kernel decides whether
 to show the message immediately (printing it to the current console) depending
-on its log level and the current *console_loglevel* (a kernel variable). If the
-message priority is higher (lower log level value) than the *console_loglevel*
-the message will be printed to the console.
+on its log level and the current global *console_loglevel* or local per-console
+loglevel (kernel variables). If the message priority is higher (lower log level
+value) than the effective loglevel the message will be printed to the console.
 
 If the log level is omitted, the message is printed with ``KERN_DEFAULT``
 level.
 
-You can check the current *console_loglevel* with::
+You can check the current console's loglevel -- for example if you want to
+check the loglevel for serial consoles:
 
-  $ cat /proc/sys/kernel/printk
-  4        4        1        7
+  $ cat /sys/class/console/ttyS0/effective_loglevel
+  6
+  $ cat /sys/class/console/ttyS0/effective_loglevel_source
+  local
 
-The result shows the *current*, *default*, *minimum* and *boot-time-default* log
-levels.
+To change the default loglevel for all consoles, simply write the desired level
+to ``/proc/sys/kernel/console_loglevel``. For example::
 
-To change the current console_loglevel simply write the desired level to
-``/proc/sys/kernel/printk``. For example, to print all messages to the console::
+  # echo 5 > /proc/sys/kernel/console_loglevel
 
-  # echo 8 > /proc/sys/kernel/printk
+This sets the console_loglevel to print KERN_WARNING (4) or more severe
+messages to console. Consoles with a per-console loglevel set will ignore it
+unless ``ignore_per_console_loglevel`` is set on the kernel command line or at
+``/sys/module/printk/parameters/ignore_per_console_loglevel``.
 
-Another way, using ``dmesg``::
-
-  # dmesg -n 5
-
-sets the console_loglevel to print KERN_WARNING (4) or more severe messages to
-console. See ``dmesg(1)`` for more information.
+For more information on per-console loglevels, see
+Documentation/admin-guide/per-console-loglevel.rst.
 
 As an alternative to printk() you can use the ``pr_*()`` aliases for
 logging. This family of macros embed the log level in the macro names. For
diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
index d55c2a22ec7a..34419e6fe106 100644
--- a/Documentation/networking/netconsole.rst
+++ b/Documentation/networking/netconsole.rst
@@ -72,6 +72,19 @@ Built-in netconsole starts immediately after the TCP stack is
 initialized and attempts to bring up the supplied dev at the supplied
 address.
 
+You can also set a loglevel at runtime::
+
+  $ ls -l /sys/class/console/netcon0/
+  total 0
+  lrwxrwxrwx 1 root root    0 May 18 13:28 subsystem -> ../../../../class/console/
+  -r--r--r-- 1 root root 4096 May 18 13:28 effective_loglevel
+  -r--r--r-- 1 root root 4096 May 18 13:28 effective_loglevel_source
+  -r--r--r-- 1 root root 4096 May 18 13:28 enabled
+  -rw-r--r-- 1 root root 4096 May 18 13:28 loglevel
+  -rw-r--r-- 1 root root 4096 May 18 13:28 uevent
+
+See Documentation/admin-guide/per-console-loglevel.rst for more information.
+
 The remote host has several options to receive the kernel messages,
 for example:
 
diff --git a/MAINTAINERS b/MAINTAINERS
index ea134675669e..003f999e531b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18449,6 +18449,7 @@ R:	John Ogness <john.ogness@linutronix.de>
 R:	Sergey Senozhatsky <senozhatsky@chromium.org>
 S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git
+F:	Documentation/ABI/testing/sysfs-class-console
 F:	Documentation/admin-guide/per-console-loglevel.rst
 F:	Documentation/core-api/printk-basics.rst
 F:	include/linux/printk.h
diff --git a/include/linux/console.h b/include/linux/console.h
index 3ff22bfeef2a..332eef0f95f7 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -16,6 +16,7 @@
 
 #include <linux/atomic.h>
 #include <linux/bits.h>
+#include <linux/device.h>
 #include <linux/irq_work.h>
 #include <linux/rculist.h>
 #include <linux/rcuwait.h>
@@ -322,6 +323,7 @@ struct nbcon_write_context {
  * @data:		Driver private data
  * @node:		hlist node for the console list
  * @level:		Console-specific loglevel
+ * @classdev:		Console class device for /sys/class/console
  *
  * @nbcon_state:	State for nbcon consoles
  * @nbcon_seq:		Sequence number of the next record for nbcon to print
@@ -351,6 +353,7 @@ struct console {
 	void			*data;
 	struct hlist_node	node;
 	int			level;
+	struct device		*classdev;
 
 	/* nbcon console specific members */
 
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 0053533dcfec..b7e8411e033d 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -77,6 +77,8 @@ extern bool ignore_per_console_loglevel;
 
 extern void console_verbose(void);
 
+int clamp_loglevel(int level);
+
 /* strlen("ratelimit") + 1 */
 #define DEVKMSG_STR_MAX_SIZE 10
 extern char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE];
diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile
index 39a2b61c7232..3c0b6e2a8083 100644
--- a/kernel/printk/Makefile
+++ b/kernel/printk/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-y	= printk.o
+obj-y	= printk.o sysfs.o
 obj-$(CONFIG_PRINTK)	+= printk_safe.o nbcon.o
 obj-$(CONFIG_A11Y_BRAILLE_CONSOLE)	+= braille.o
 obj-$(CONFIG_PRINTK_INDEX)	+= index.o
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 9303839d0551..ac607d6907a0 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -21,6 +21,11 @@ enum loglevel_source {
 	LLS_IGNORE_LOGLEVEL,
 };
 
+void console_register_device(struct console *new);
+void console_setup_class(void);
+
+int clamp_loglevel(int level);
+
 enum loglevel_source
 console_effective_loglevel_source(const struct console *con);
 int console_effective_loglevel(const struct console *con);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 055619c5c7e8..bc456f7624d4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -199,6 +199,12 @@ static int __init control_devkmsg(char *str)
 }
 __setup("printk.devkmsg=", control_devkmsg);
 
+int clamp_loglevel(int level)
+{
+	return clamp(level, minimum_console_loglevel,
+		     CONSOLE_LOGLEVEL_MOTORMOUTH);
+}
+
 char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE] = "ratelimit";
 #if defined(CONFIG_PRINTK) && defined(CONFIG_SYSCTL)
 int devkmsg_sysctl_set_loglvl(const struct ctl_table *table, int write,
@@ -3894,6 +3900,8 @@ static int try_enable_preferred_console(struct console *newcon,
 			// TODO: Will be configurable in a later patch
 			newcon->level = -1;
 
+			newcon->classdev = NULL;
+
 			if (_braille_register_console(newcon, c))
 				return 0;
 
@@ -4170,6 +4178,7 @@ void register_console(struct console *newcon)
 	if (use_device_lock)
 		newcon->device_unlock(newcon, flags);
 
+	console_register_device(newcon);
 	console_sysfs_notify();
 
 	/*
@@ -4264,6 +4273,9 @@ static int unregister_console_locked(struct console *console)
 	if (console->flags & CON_NBCON)
 		nbcon_free(console);
 
+	if (console->classdev)
+		device_unregister(console->classdev);
+
 	console_sysfs_notify();
 
 	if (console->exit)
@@ -4428,6 +4440,9 @@ static int __init printk_late_init(void)
 					console_cpu_notify, NULL);
 	WARN_ON(ret < 0);
 	printk_sysctl_init();
+
+	console_setup_class();
+
 	return 0;
 }
 late_initcall(printk_late_init);
diff --git a/kernel/printk/sysfs.c b/kernel/printk/sysfs.c
new file mode 100644
index 000000000000..e24590074861
--- /dev/null
+++ b/kernel/printk/sysfs.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/kernel.h>
+#include <linux/console.h>
+#include <linux/device.h>
+#include <linux/printk.h>
+#include <linux/slab.h>
+#include "internal.h"
+
+#ifdef CONFIG_PRINTK
+static struct class *console_class;
+
+static const char *
+console_effective_loglevel_source_str(const struct console *con)
+{
+	enum loglevel_source source;
+	const char *str;
+
+	source = console_effective_loglevel_source(con);
+
+	switch (source) {
+	case LLS_IGNORE_LOGLEVEL:
+		str = "ignore_loglevel";
+		break;
+	case LLS_LOCAL:
+		str = "local";
+		break;
+	case LLS_GLOBAL:
+		str = "global";
+		break;
+	default:
+		pr_warn("Unhandled console loglevel source: %d", source);
+		str = "unknown";
+		break;
+	}
+
+	return str;
+}
+
+static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct console *con = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", READ_ONCE(con->level));
+}
+
+static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct console *con = dev_get_drvdata(dev);
+	ssize_t ret;
+	int level;
+
+	ret = kstrtoint(buf, 10, &level);
+	if (ret < 0)
+		return ret;
+
+	if (level == -1)
+		goto out;
+
+	if (clamp_loglevel(level) != level)
+		return -ERANGE;
+
+out:
+	WRITE_ONCE(con->level, level);
+
+	return size;
+}
+
+static DEVICE_ATTR_RW(loglevel);
+
+static ssize_t effective_loglevel_source_show(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	struct console *con = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%s\n",
+			  console_effective_loglevel_source_str(con));
+}
+
+static DEVICE_ATTR_RO(effective_loglevel_source);
+
+static ssize_t effective_loglevel_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct console *con = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", console_effective_loglevel(con));
+}
+
+static DEVICE_ATTR_RO(effective_loglevel);
+
+static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	struct console *con = dev_get_drvdata(dev);
+	int cookie;
+	bool enabled;
+
+	cookie = console_srcu_read_lock();
+	enabled = console_srcu_read_flags(con) & CON_ENABLED;
+	console_srcu_read_unlock(cookie);
+	return sysfs_emit(buf, "%d\n", enabled);
+}
+
+static DEVICE_ATTR_RO(enabled);
+
+static struct attribute *console_sysfs_attrs[] = {
+	&dev_attr_loglevel.attr,
+	&dev_attr_effective_loglevel_source.attr,
+	&dev_attr_effective_loglevel.attr,
+	&dev_attr_enabled.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(console_sysfs);
+
+static void console_classdev_release(struct device *dev)
+{
+	kfree(dev);
+}
+
+void console_register_device(struct console *con)
+{
+	/*
+	 * We might be called from register_console() before the class is
+	 * registered. If that happens, we'll take care of it in
+	 * printk_late_init.
+	 */
+	if (IS_ERR_OR_NULL(console_class))
+		return;
+
+	if (WARN_ON(con->classdev))
+		return;
+
+	con->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
+	if (!con->classdev)
+		return;
+
+	device_initialize(con->classdev);
+	dev_set_name(con->classdev, "%s%d", con->name, con->index);
+	dev_set_drvdata(con->classdev, con);
+	con->classdev->release = console_classdev_release;
+	con->classdev->class = console_class;
+	if (device_add(con->classdev))
+		put_device(con->classdev);
+}
+
+void console_setup_class(void)
+{
+	struct console *con;
+	int cookie;
+
+	/*
+	 * printk exists for the lifetime of the kernel, it cannot be unloaded,
+	 * so we should never end up back in here.
+	 */
+	if (WARN_ON(console_class))
+		return;
+
+	console_class = class_create("console");
+	if (!IS_ERR(console_class))
+		console_class->dev_groups = console_sysfs_groups;
+
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(con)
+		console_register_device(con);
+	console_srcu_read_unlock(cookie);
+}
+#else /* CONFIG_PRINTK */
+void console_register_device(struct console *new)
+{
+}
+void console_setup_class(void)
+{
+}
+#endif
-- 
2.46.0

[PATCH v6 07/11] printk: Constrain hardware-addressed console checks to name position

[Chris Down]

Commit 7640f1a44eba ("printk: Add
match_devname_and_update_preferred_console()") adds support for
DEVNAME:n:n style console= registration. It determines whether or not
this is a hardware-addressed console by looking at whether the console=
string has a colon in it. However, this interferes with loglevel:n and
potentially other future named options, which also make use of the
character.

In order to avoid this, only search positions before options.

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 kernel/printk/printk.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index bc456f7624d4..bbd037b84a0d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2670,6 +2670,7 @@ static int __init console_setup(char *str)
 	char *devname = NULL;
 	char *options;
 	char *s;
+	char *first_delim;
 	int idx;
 
 	/*
@@ -2685,8 +2686,13 @@ static int __init console_setup(char *str)
 	if (_braille_console_setup(&str, &brl_options))
 		return 1;
 
-	/* For a DEVNAME:0.0 style console the character device is unknown early */
-	if (strchr(str, ':'))
+	/*
+	 * For a DEVNAME:0.0 style console the character device is unknown
+	 * early. We must restrict this to before any comma to avoid interfering
+	 * with named options, like "loglevel".
+	 */
+	first_delim = strpbrk(str, ":,");
+	if (first_delim && *first_delim == ':')
 		devname = buf;
 	else
 		ttyname = buf;
-- 
2.46.0

[PATCH v6 08/11] printk: Support setting initial console loglevel via console= on cmdline

[Chris Down]

Extend the console= kernel command line parameter to support specifying
per-console loglevels at boot time. This is achieved by introducing a
new loglevel option that can be passed as a loglevel option within a
console= stanza.

For example, this is an example of how one might configure netconsole
devices to print messages with a higher priority than loglevel 3
(KERN_ERR) at startup:

    console=netcon0,loglevel:3

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 .../admin-guide/kernel-parameters.txt         | 24 +++--
 Documentation/admin-guide/serial-console.rst  | 37 +++++++-
 Documentation/networking/netconsole.rst       |  6 +-
 kernel/printk/console_cmdline.h               |  1 +
 kernel/printk/printk.c                        | 87 ++++++++++++++++++-
 5 files changed, 140 insertions(+), 15 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1518343bbe22..d883a851fffc 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -799,13 +799,18 @@
 		ttyS<n>[,options]
 		ttyUSB0[,options]
 			Use the specified serial port.  The options are of
-			the form "bbbbpnf", where "bbbb" is the baud rate,
-			"p" is parity ("n", "o", or "e"), "n" is number of
-			bits, and "f" is flow control ("r" for RTS or
-			omit it).  Default is "9600n8".
+			the form "bbbbpnf,extra", where "bbbb" is the baud
+			rate, "p" is parity ("n", "o", or "e"), "n" is
+			number of bits, and "f" is flow control ("r" for RTS
+			or omit it). Default is "9600n8".
 
-			See Documentation/admin-guide/serial-console.rst for more
-			information.  See
+			At present the only extra option is "loglevel" to
+			set the per-console loglevel. For example:
+
+				console=ttyS0,9600n8,loglevel:3
+
+			See Documentation/admin-guide/serial-console.rst for
+			more information.  See
 			Documentation/networking/netconsole.rst for an
 			alternative.
 
@@ -3167,8 +3172,11 @@
 	loglevel=	[KNL,EARLY]
 			All Kernel Messages with a loglevel smaller than the
 			console loglevel will be printed to the console. It can
-			also be changed with klogd or other programs. The
-			loglevels are defined as follows:
+			also be changed with klogd or other programs. Note that
+			this can be overridden per-console, see
+			Documentation/admin-guide/per-console-loglevel.rst.
+
+			The loglevels are defined as follows:
 
 			0 (KERN_EMERG)		system is unusable
 			1 (KERN_ALERT)		action must be taken immediately
diff --git a/Documentation/admin-guide/serial-console.rst b/Documentation/admin-guide/serial-console.rst
index a3dfc2c66e01..240f7a36379d 100644
--- a/Documentation/admin-guide/serial-console.rst
+++ b/Documentation/admin-guide/serial-console.rst
@@ -32,6 +32,33 @@ The format of this option is::
 			and F is flow control ('r' for RTS). Default is
 			9600n8. The maximum baudrate is 115200.
 
+			One can also specify the per-console loglevel for this
+			console by providing a loglevel parameter, for example
+			"loglevel:4" to set this console's loglevel to 4. The
+			value provided can be from 0 (LOGLEVEL_EMERG) to 8
+			(LOGLEVEL_DEBUG + 1), and messages below that will be
+			emitted onto the console as they become available.
+
+The available loglevels are defined thus:
+
++---+--------------+-----------------------------------+
+| 0 | KERN_EMERG   | system is unusable                |
++---+--------------+-----------------------------------+
+| 1 | KERN_ALERT   | action must be taken immediately  |
++---+--------------+-----------------------------------+
+| 2 | KERN_CRIT    | critical conditions               |
++---+--------------+-----------------------------------+
+| 3 | KERN_ERR     | error conditions                  |
++---+--------------+-----------------------------------+
+| 4 | KERN_WARNING | warning conditions                |
++---+--------------+-----------------------------------+
+| 5 | KERN_NOTICE  | normal but significant condition  |
++---+--------------+-----------------------------------+
+| 6 | KERN_INFO    | informational                     |
++---+--------------+-----------------------------------+
+| 7 | KERN_DEBUG   | debug-level messages              |
++---+--------------+-----------------------------------+
+
 You can specify multiple console= options on the kernel command line.
 
 The behavior is well defined when each device type is mentioned only once.
@@ -39,11 +66,14 @@ In this case, the output will appear on all requested consoles. And
 the last device will be used when you open ``/dev/console``.
 So, for example::
 
-	console=ttyS1,9600 console=tty0
+	console=ttyS1,9600,loglevel:5 console=tty0
 
 defines that opening ``/dev/console`` will get you the current foreground
-virtual console, and kernel messages will appear on both the VGA
-console and the 2nd serial port (ttyS1 or COM2) at 9600 baud.
+virtual console, and kernel messages will appear on both the VGA console and
+the 2nd serial port (ttyS1 or COM2) at 9600 baud. The optional loglevel "5"
+indicates that this console will emit messages more serious than
+LOGLEVEL_NOTICE (that is, LOGLEVEL_WARNING and below, since more serious
+messages have lower ordering).
 
 The behavior is more complicated when the same device type is defined more
 times. In this case, there are the following two rules:
@@ -143,3 +173,4 @@ Replace the sample values as needed.
    the integration of these patches into m68k, ppc and alpha.
 
 Miquel van Smoorenburg <miquels@cistron.nl>, 11-Jun-2000
+Chris Down <chris@chrisdown.name>, 23-October-2024
diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
index 34419e6fe106..a7c7519fe2d0 100644
--- a/Documentation/networking/netconsole.rst
+++ b/Documentation/networking/netconsole.rst
@@ -72,7 +72,11 @@ Built-in netconsole starts immediately after the TCP stack is
 initialized and attempts to bring up the supplied dev at the supplied
 address.
 
-You can also set a loglevel at runtime::
+You can also set a loglevel at boot time on the kernel command line::
+
+  console=netcon0,loglevel:2
+
+This can also be changed at runtime::
 
   $ ls -l /sys/class/console/netcon0/
   total 0
diff --git a/kernel/printk/console_cmdline.h b/kernel/printk/console_cmdline.h
index 0ab573b6d4dc..cb3b99d31d80 100644
--- a/kernel/printk/console_cmdline.h
+++ b/kernel/printk/console_cmdline.h
@@ -7,6 +7,7 @@ struct console_cmdline
 	char	name[16];			/* Name of the driver	    */
 	int	index;				/* Minor dev. to use	    */
 	char	devname[32];			/* DEVNAME:0.0 style device name */
+	int	level;				/* Log level to use */
 	bool	user_specified;			/* Specified by command line vs. platform */
 	char	*options;			/* Options for the driver   */
 #ifdef CONFIG_A11Y_BRAILLE_CONSOLE
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index bbd037b84a0d..c47dda23a7d6 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2597,12 +2597,84 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified)
 	console_set_on_cmdline = 1;
 }
 
+static bool find_and_remove_console_option(char *options, const char *key,
+					   char *val_buf, size_t val_buf_size)
+{
+	bool found = false, first = true;
+	char *option, *next = options;
+
+	while ((option = strsep(&next, ","))) {
+		char *value;
+
+		value = strchr(option, ':');
+		if (value)
+			*(value++) = '\0';
+
+		if (strcmp(option, key) == 0) {
+			found = true;
+			if (value) {
+				if (strlen(value) >= val_buf_size) {
+					pr_warn("Can't copy console option value for %s:%s: not enough space (%zu)\n",
+						option, value, val_buf_size);
+					found = false;
+				} else {
+					strscpy(val_buf, value, val_buf_size);
+				}
+			} else
+				*val_buf = '\0';
+		}
+
+		if (found)
+			break;
+
+		if (next)
+			*(next - 1) = ',';
+		if (value)
+			*(value - 1) = ':';
+
+		first = false;
+	}
+
+	if (found) {
+		if (next)
+			memmove(option, next, strlen(next) + 1);
+		else if (first)
+			*option = '\0';
+		else
+			*--option = '\0';
+	}
+
+	return found;
+}
+
+static int find_and_remove_loglevel_option(char *options)
+{
+	char val[16];
+	int loglevel;
+
+	if (!find_and_remove_console_option(options, "loglevel", val,
+					    sizeof(val)))
+		return -ENOENT;
+
+	if (kstrtoint(val, 10, &loglevel)) {
+		pr_warn("Invalid console loglevel, ignoring: %s\n", val);
+		return -EINVAL;
+	}
+
+	if (clamp_loglevel(loglevel) != loglevel) {
+		pr_warn("Per-console loglevel out of range, ignoring: %d\n", loglevel);
+		return -ERANGE;
+	}
+
+	return loglevel;
+}
+
 static int __add_preferred_console(const char *name, const short idx,
 				   const char *devname, char *options,
 				   char *brl_options, bool user_specified)
 {
 	struct console_cmdline *c;
-	int i;
+	int i, ret;
 
 	if (!name && !devname)
 		return -EINVAL;
@@ -2639,6 +2711,13 @@ static int __add_preferred_console(const char *name, const short idx,
 		strscpy(c->name, name);
 	if (devname)
 		strscpy(c->devname, devname);
+
+	ret = find_and_remove_loglevel_option(options);
+	if (ret >= 0)
+		c->level = ret;
+	else
+		c->level = -1;
+
 	c->options = options;
 	set_user_specified(c, user_specified);
 	braille_set_options(c, brl_options);
@@ -3903,8 +3982,10 @@ static int try_enable_preferred_console(struct console *newcon,
 			if (newcon->index < 0)
 				newcon->index = c->index;
 
-			// TODO: Will be configurable in a later patch
-			newcon->level = -1;
+			if (c->level > 0)
+				newcon->level = c->level;
+			else
+				newcon->level = -1;
 
 			newcon->classdev = NULL;
 
-- 
2.46.0

[PATCH v6 09/11] printk: Add sysctl interface to set global loglevels

[Chris Down]

Introduce two new sysctl interfaces for configuring global loglevels:

- kernel.console_loglevel: Sets the global console loglevel, determining
  the minimum priority of messages printed to consoles. Messages with a
  loglevel lower than this value will be printed.
- kernel.default_message_loglevel: Sets the default loglevel for
  messages that do not specify an explicit loglevel.

The kernel.printk sysctl was previously used to set multiple loglevel
parameters simultaneously, but it was confusing and lacked proper
validation. By introducing these dedicated sysctl interfaces, we provide
a clearer and more granular way to configure the loglevels.

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 Documentation/admin-guide/sysctl/kernel.rst | 17 ++++++++-
 kernel/printk/sysctl.c                      | 42 +++++++++++++++++++++
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index f8bc1630eba0..8019779b27f6 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -1044,6 +1044,20 @@ otherwise the 'doze' mode will be used.
 
 ==============================================================
 
+Some of these settings may be overridden per-console, see
+Documentation/admin-guide/per-console-loglevel.rst. See ``man 2 syslog`` for
+more information on the different loglevels.
+
+console_loglevel
+================
+
+Messages with a higher priority than this will be printed to consoles.
+
+default_message_loglevel
+========================
+
+Messages without an explicit priority will be printed with this priority.
+
 printk
 ======
 
@@ -1052,8 +1066,7 @@ The four values in printk denote: ``console_loglevel``,
 ``default_console_loglevel`` respectively.
 
 These values influence printk() behavior when printing or
-logging error messages. See '``man 2 syslog``' for more info on
-the different loglevels.
+logging error messages.
 
 ======================== =====================================
 console_loglevel         messages with a higher priority than
diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
index f5072dc85f7a..3bce8b89dacc 100644
--- a/kernel/printk/sysctl.c
+++ b/kernel/printk/sysctl.c
@@ -11,6 +11,9 @@
 
 static const int ten_thousand = 10000;
 
+static int min_msg_loglevel = LOGLEVEL_EMERG;
+static int max_msg_loglevel = LOGLEVEL_DEBUG;
+
 static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int write,
 				void *buffer, size_t *lenp, loff_t *ppos)
 {
@@ -20,6 +23,29 @@ static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int writ
 	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 }
 
+static int printk_console_loglevel(const struct ctl_table *table, int write,
+				   void *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table ltable = *table;
+	int ret, level;
+
+	if (!write)
+		return proc_dointvec(table, write, buffer, lenp, ppos);
+
+	ltable.data = &level;
+
+	ret = proc_dointvec(&ltable, write, buffer, lenp, ppos);
+	if (ret)
+		return ret;
+
+	if (level != -1 && level != clamp_loglevel(level))
+		return -ERANGE;
+
+	console_loglevel = level;
+
+	return 0;
+}
+
 static struct ctl_table printk_sysctls[] = {
 	{
 		.procname	= "printk",
@@ -76,6 +102,22 @@ static struct ctl_table printk_sysctls[] = {
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_TWO,
 	},
+	{
+		.procname	= "console_loglevel",
+		.data		= &console_loglevel,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= printk_console_loglevel,
+	},
+	{
+		.procname	= "default_message_loglevel",
+		.data		= &default_message_loglevel,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &min_msg_loglevel,
+		.extra2		= &max_msg_loglevel,
+	},
 };
 
 void __init printk_sysctl_init(void)
-- 
2.46.0

[PATCH v6 10/11] printk: Deprecate the kernel.printk sysctl interface

[Chris Down]

The kernel.printk sysctl interface is deprecated in favour of more
granular and clearer sysctl interfaces for controlling loglevels, namely
kernel.console_loglevel and kernel.default_message_loglevel.

kernel.printk has a number of fairly significant usability problems:

- It takes four values in a specific order, which is not intuitive.
  Speaking from personal experience, even people working on printk
  sometimes need to look up the order of these values, which doesn't
  suggest much in the way of perspicuity.
- There is no validation on the input values, so users can set them to
  invalid or nonsensical values without receiving any errors or
  warnings. This can lead to unpredictable behaviour.
- One of the four values, default_console_loglevel, is not used in the
  kernel (see below), making it redundant and potentially confusing.
- Overall, the interface is complex, hard to understand, and combines
  multiple controls into a single sysctl, which is not ideal. It should
  be separated into distinct controls for clarity.

Setting kernel.printk now calls printk_sysctl_deprecated, which emits a
pr_warn. The warning informs users about the deprecation and suggests
using the new sysctl interfaces instead.

By deprecating kernel.printk, we aim to:

- Encourage users to adopt the new, dedicated sysctl interfaces
  (kernel.console_loglevel and kernel.default_message_loglevel), which
  are more straightforward and easier to understand.
- Improve input validation and error handling, ensuring that users
  receive appropriate feedback when setting invalid values.
- Simplify the configuration of loglevels by exposing only the controls
  that are necessary and relevant.

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 Documentation/admin-guide/sysctl/kernel.rst |  3 +++
 kernel/printk/sysctl.c                      | 14 +++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index 8019779b27f6..6027f0057ea3 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -1061,6 +1061,9 @@ Messages without an explicit priority will be printed with this priority.
 printk
 ======
 
+This sysctl is deprecated and will be removed in future. Please consider using
+``kernel.console_loglevel`` or ``kernel.default_message_loglevel`` instead.
+
 The four values in printk denote: ``console_loglevel``,
 ``default_message_loglevel``, ``minimum_console_loglevel`` and
 ``default_console_loglevel`` respectively.
diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
index 3bce8b89dacc..42f9e94b1e47 100644
--- a/kernel/printk/sysctl.c
+++ b/kernel/printk/sysctl.c
@@ -7,6 +7,7 @@
 #include <linux/printk.h>
 #include <linux/capability.h>
 #include <linux/ratelimit.h>
+#include <linux/console.h>
 #include "internal.h"
 
 static const int ten_thousand = 10000;
@@ -46,13 +47,24 @@ static int printk_console_loglevel(const struct ctl_table *table, int write,
 	return 0;
 }
 
+static int printk_sysctl_deprecated(const struct ctl_table *table, int write,
+				    void *buffer, size_t *lenp, loff_t *ppos)
+{
+	int res = proc_dointvec(table, write, buffer, lenp, ppos);
+
+	if (write)
+		pr_warn_once("printk: The kernel.printk sysctl is deprecated. Consider using kernel.console_loglevel or kernel.default_message_loglevel instead.\n");
+
+	return res;
+}
+
 static struct ctl_table printk_sysctls[] = {
 	{
 		.procname	= "printk",
 		.data		= &console_loglevel,
 		.maxlen		= 4*sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= printk_sysctl_deprecated,
 	},
 	{
 		.procname	= "printk_ratelimit",
-- 
2.46.0

[PATCH v6 11/11] printk: Purge default_console_loglevel

[Chris Down]

default_console_loglevel (then DEFAULT_LOGLEVEL) was created in Linux
0.99.13 (September 1993) to power what we now call
SYSLOG_ACTION_CONSOLE_{ON,OFF}. It was originally hardcoded to 7.

In Linux 2.3.12 (March 1997), Chris Horn made it configurable by sysctl
through kernel.printk.

Its demise came about in July 2009 in commit 1aaad49e856c ("printk:
Restore previous console_loglevel when re-enabling logging"), which
replaces default_console_loglevel with the previously used
console_loglevel. However, the documentation was never updated, and we
still kept on telling users that this sets the default value for
console_loglevel, even though this hasn't been true for over 15 years.

Purge both the documentation and all remaining references to
default_console_loglevel. It will still be initialised in the sysctl
array.

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 Documentation/admin-guide/sysctl/kernel.rst | 5 ++---
 include/linux/printk.h                      | 1 -
 kernel/printk/printk.c                      | 2 +-
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index 6027f0057ea3..e1b5124ab383 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -1065,8 +1065,8 @@ This sysctl is deprecated and will be removed in future. Please consider using
 ``kernel.console_loglevel`` or ``kernel.default_message_loglevel`` instead.
 
 The four values in printk denote: ``console_loglevel``,
-``default_message_loglevel``, ``minimum_console_loglevel`` and
-``default_console_loglevel`` respectively.
+``default_message_loglevel``, ``minimum_console_loglevel`` and an unused legacy
+value respectively.
 
 These values influence printk() behavior when printing or
 logging error messages.
@@ -1078,7 +1078,6 @@ default_message_loglevel messages without an explicit priority
                          will be printed with this priority
 minimum_console_loglevel minimum (highest) value to which
                          console_loglevel can be set
-default_console_loglevel default value for console_loglevel
 ======================== =====================================
 
 
diff --git a/include/linux/printk.h b/include/linux/printk.h
index b7e8411e033d..41ce323b4e77 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -71,7 +71,6 @@ extern int console_printk[];
 #define console_loglevel (console_printk[0])
 #define default_message_loglevel (console_printk[1])
 #define minimum_console_loglevel (console_printk[2])
-#define default_console_loglevel (console_printk[3])
 
 extern bool ignore_per_console_loglevel;
 
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c47dda23a7d6..018087b57031 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -65,7 +65,7 @@ int console_printk[4] = {
 	CONSOLE_LOGLEVEL_DEFAULT,	/* console_loglevel */
 	MESSAGE_LOGLEVEL_DEFAULT,	/* default_message_loglevel */
 	CONSOLE_LOGLEVEL_MIN,		/* minimum_console_loglevel */
-	CONSOLE_LOGLEVEL_DEFAULT,	/* default_console_loglevel */
+	/* legacy default console loglevel, unused */
 };
 EXPORT_SYMBOL_GPL(console_printk);
 
-- 
2.46.0

Re: [PATCH v6 05/11] MAINTAINERS: Mark printk-basics.rst as owned by printk subsystem

[Thomas Gleixner]

On Mon, Oct 28 2024 at 16:45, Chris Down wrote:
> We are going to update this in the next patch, so while we're here, we
> might as well mark it as owned by us.

Who is we?

Thanks,

        tglx

Re: [PATCH v6 05/11] MAINTAINERS: Mark printk-basics.rst as owned by printk subsystem

[Chris Down]

Just a slip from indicative mood ("This file will be updated in the next patch, 
so [...]". :-)

I'll change it if we get a v7.

Thanks,

Chris

Re: [PATCH v6 07/11] printk: Constrain hardware-addressed console checks to name position

[Tony Lindgren]

On Mon, Oct 28, 2024 at 04:45:48PM +0000, Chris Down wrote:
> Commit 7640f1a44eba ("printk: Add
> match_devname_and_update_preferred_console()") adds support for
> DEVNAME:n:n style console= registration. It determines whether or not
> this is a hardware-addressed console by looking at whether the console=
> string has a colon in it. However, this interferes with loglevel:n and
> potentially other future named options, which also make use of the
> character.
> 
> In order to avoid this, only search positions before options.

Looks good to me:

Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>

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

[Petr Mladek]

On Mon 2024-10-28 16:45:34, Chris Down 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. As part of the upcoming
> per-console loglevel support we need the console object here anyway, so
> it makes sense to amortise this now.
> 
> devkmsg_read() has special behaviour here, but all other consoles follow
> the same patterns and can have their extension/suppression states
> determined from their struct console.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2925,20 +2925,19 @@ void console_prepend_replay(struct printk_message *pmsg)
>   * @pmsg will contain the formatted result. @pmsg->pbufs must point to a
>   * struct printk_buffers.
>   *
> + * @con is the console in question. Only @con->flags and @con->level are
> + * guaranteed to be valid at this point. Note especially well that con->seq is
> + * not yet guaranteed to be consistent with @seq.

@con->level does not exist at this point.

But more importantly, the read of @con->flags and @con->level is
sychronized only by SRCU read lock. It means that the word "valid"
is a bit misleading. SRCU just guarantees that the struct console
can't disappear.

I would write something like:

<proposal>
 * @con might point to the console where the read message will be emitted.
 * It is used to determine the format of the message and whether it would get
 * suppressed. The sequence number stored in the struct console is updated
 * by the caller depending on whether the emission succeeds.
 *
 * @con might be NULL when the message is used for another purpose,
 * for example, devkmsg.
</proposal>

> + *
>   * @seq is the record to read and format. If it is not available, the next
>   * valid record is read.
>   *
> - * @is_extended specifies if the message should be formatted for extended
> - * console output.
> - *
> - * @may_supress specifies if records may be skipped based on loglevel.
> - *
>   * Returns false if no record is available. Otherwise true and all fields
>   * of @pmsg are valid. (See the documentation of struct printk_message
>   * for information about the @pmsg fields.)
>   */
> -bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
> -			     bool is_extended, bool may_suppress)
> +bool printk_get_next_message(struct printk_message *pmsg, struct console *con,
> +			     u64 seq)
>  {
>  	struct printk_buffers *pbufs = pmsg->pbufs;
>  	const size_t scratchbuf_sz = sizeof(pbufs->scratchbuf);
> @@ -2948,6 +2947,14 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>  	struct printk_info info;
>  	struct printk_record r;
>  	size_t len = 0;
> +	bool is_extended;
> +
> +	if (con) {
> +		is_extended = console_srcu_read_flags(con) & CON_EXTENDED;

I guess that we would need to implement similar API also for reading
the per-console loglevel. I mean that we should check the SRCU lock
is held. And describe the potential data_race() if the value might
be modified by a sysfs interface in parallel.

Let's discuss this in the patch adding the read. I mention it here
rather just as a mental note. The proposed comment says that it will
be synchronized using SRCU. We need to make sure that it will
be valid also for the loglevel stuff.

> +	} else {
> +		/* Used only by devkmsg_read(). */
> +		is_extended = true;
> +	}
>  
>  	/*
>  	 * Formatting extended messages requires a separate buffer, so use the

Anyway, the change looks fine. With the updated comment:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v6 03/11] printk: console: Implement core per-console loglevel infrastructure

[Petr Mladek]

On Mon 2024-10-28 16:45:37, Chris Down wrote:
> Consoles can have vastly different latencies and throughputs. For
> example, writing a message to the serial console can take on the order
> of tens of milliseconds to get the UART to successfully write a message.
> While this might be fine for a single, one-off message, this can cause
> significant application-level stalls in situations where the kernel
> writes large amounts of information to the console.
> 
> This means that while you might want to send at least INFO level
> messages to (for example) netconsole, which is relatively fast, you may
> only want to send at least WARN level messages to the serial console.
> Such an implementation would permit debugging using the serial console
> in cases that netconsole doesn't receive messages during particularly
> bad system issues, while still keeping the noise low enough to avoid
> inducing latency in userspace applications. To mitigate this, add such
> an interface, extending the existing console loglevel controls to allow
> each console to have its own loglevel.
> 
> One can't just disable the serial console, because one may actually need
> it in situations where the machine is in a bad enough state that nothing
> is received on netconsole. One also can't just bump the loglevel at
> runtime after the issue, because usually the machine is already so
> wedged by this point that it isn't responsive to such requests.

The motivation is described nicely.

> The sysfs and kernel command line interfaces to set the per-console
> loglevel will be added later. For now, simply add the necessary internal
> infrastructure to be used by later patches.

It would be nice to describe the basic logic in the commit message.
And also the handling when the global console_loglevel is modified
via sysrq. Something like:

<proposal>
This commit adds the internal infrastructure to support per-console
log levels, which will be configurable through sysfs and the kernel
command line in future commits.

The global `console_loglevel` is preserved and used as the default log
level for all consoles. Each console can override this global level
with its own specific log level stored in `struct console`. To
override the global level, the per-console log level must be greater
than 0; otherwise, the default value of -1 ensures the global level
is used.

The existing `ignore_loglevel` command line parameter will override
both the global and per-console log levels.
</proposal>

> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -101,11 +102,25 @@ __setup("sysrq_always_enabled", sysrq_always_enabled_setup);
>  static void sysrq_handle_loglevel(u8 key)
>  {
>  	u8 loglevel = key - '0';
> +	int cookie;
> +	int warned = 0;
> +	struct console *con;
>  
>  	console_loglevel = CONSOLE_LOGLEVEL_DEFAULT;
>  	pr_info("Loglevel set to %u\n", loglevel);
>  	console_loglevel = loglevel;
> +
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(con) {
> +		if (!warned && per_console_loglevel_is_set(con)) {
> +			warned = 1;
> +			pr_warn("Overriding per-console loglevel from sysrq\n");
> +		}
> +		WRITE_ONCE(con->level, -1);
> +	}
> +	console_srcu_read_unlock(cookie);
>  }
> +

I would prefer to move this into a separate patch. It is similar to
the syslog interface which is handled separately as well.

I would keep this patch only for the basic logic.


>  static const struct sysrq_key_op sysrq_loglevel_op = {
>  	.handler	= sysrq_handle_loglevel,
>  	.help_msg	= "loglevel(0-9)",
> diff --git a/include/linux/console.h b/include/linux/console.h
> index eba367bf605d..3ff22bfeef2a 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -349,6 +350,7 @@ struct console {
>  	unsigned long		dropped;
>  	void			*data;
>  	struct hlist_node	node;
> +	int			level;
>  
>  	/* nbcon console specific members */
>  
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index eca9bb2ee637..5fbd6b7f1ab4 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -303,6 +304,10 @@ static inline void nbcon_device_release(struct console *con)
>  static inline void nbcon_atomic_flush_unsafe(void)
>  {
>  }
> +static inline bool per_console_loglevel_is_set(const struct console *con)

There are similar functions in the printk API, for example,
is_printk_legacy_deferred(). It would be nice to use a similar
naming scheme. I would call it, something like:

	has_per_console_loglevel(con)

> +{
> +	return false;
> +}
>  
>  #endif
>  
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1287,9 +1287,62 @@ module_param(ignore_loglevel, bool, S_IRUGO | S_IWUSR);
>  MODULE_PARM_DESC(ignore_loglevel,
>  		 "ignore loglevel setting (prints all kernel messages to the console)");
>  
> -static bool suppress_message_printing(int level)
> +bool per_console_loglevel_is_set(const struct console *con)
>  {
> -	return (level >= console_loglevel && !ignore_loglevel);
> +	return con && (READ_ONCE(con->level) > 0);

We should document why READ_ONCE() is used.

Also it would be great to allow checking of locking dependencies to
make sure that the struct console could not disappear. Similiar
to console_srcu_read_flags(con).

In the followup patches, the funtion is mostly used
the console_srcu_read_lock(). The exception seems to be the sysfs
interface, e.g. effective_loglevel_source_show().

Hmm, we need to distinguish these two callers either by serapate API
or a parameter.

Both is a bit ugly, so I would prefer to reduce the number of these
ugly APIs by passing @con_loglevel instead of @con.

Anyway, we should create something similar to console_srcu_read_flags().


/**
 * console_srcu_read_loglevel - Locklessly read the console specific loglevel
 *				of a possibly registered console
 * @con:	struct console pointer of console to read flags from
 *
 * Locklessly reading @con->loglevel provides a consistent read value because
 * there is at most one CPU modifying @con->loglevel and that CPU is using only
 * read-modify-write operations to do so.
 *
 * Requires console_srcu_read_lock to be held, which implies that @con might
 * be a registered console. The purpose of holding console_srcu_read_lock is
 * to guarantee that the console state is valid (CON_SUSPENDED/CON_ENABLED)
 * and that no exit/cleanup routines will run if the console is currently
 * undergoing unregistration.
 *
 * If the caller is holding the console_list_lock or it is _certain_ that
 * @con is not and will not become registered, the caller may read
 * @con->loglevel directly instead.
 *
 * Context: Any context.
 * Return: The current value of the @con->level field.
 */
static inline int console_srcu_read_loglevel(const struct console *con)
{
	WARN_ON_ONCE(!console_srcu_read_lock_is_held());

	/* The READ_ONCE() matches the WRITE_ONCE() when @flags are modified. */
	return data_race(READ_ONCE(con->level));
}

Then I would suggest to pass @con_loglevel directly in most the other
API. For example, this function would be:

bool is_valid_per_console_loglevel(int con_level)
{
	return (con_level > 0);
}

> +}
> +
> +/*
> + * Hierarchy of loglevel authority:
> + *
> + * 1. con->level. The locally set, console-specific loglevel. Optional, only
> + *    valid if >0.
> + * 2. console_loglevel. The default global console loglevel, always present.

I would remove these comments. It is obvious from the code.

> + */
> +enum loglevel_source
> +console_effective_loglevel_source(const struct console *con)
> +{
> +	if (WARN_ON_ONCE(!con))
> +		return LLS_GLOBAL;
> +
> +	if (ignore_loglevel)
> +		return LLS_IGNORE_LOGLEVEL;
> +
> +	if (per_console_loglevel_is_set(con))
> +		return LLS_LOCAL;
> +
> +	return LLS_GLOBAL;
> +}

The @con parameter is nice. But it makes it hard to add lockdep
checks. So, I suggest to pass the console specific loglevel
as the parameter, like:

enum loglevel_source
console_effective_loglevel_source(int con_level)
{
	if (ignore_loglevel)
		return LLS_IGNORE_LOGLEVEL;

	if (is_valid_per_console_loglevel(con_level))
		return LLS_LOCAL;

	return LLS_GLOBAL;
}

IMHO, it is not that bad after all.


> +int console_effective_loglevel(const struct console *con)
> +{
> +	enum loglevel_source source;
> +	int level;
> +
> +	source = console_effective_loglevel_source(con);
> +
> +	switch (source) {
> +	case LLS_IGNORE_LOGLEVEL:
> +		level = CONSOLE_LOGLEVEL_MOTORMOUTH;
> +		break;
> +	case LLS_LOCAL:
> +		level = READ_ONCE(con->level);
> +		break;
> +	case LLS_GLOBAL:
> +		level = console_loglevel;
> +		break;
> +	default:
> +		pr_warn("Unhandled console loglevel source: %d", source);
> +		level = console_loglevel;
> +		break;
> +	}
> +
> +	return level;
> +}

and similar here:

int console_effective_loglevel(int con_level)
{
	enum loglevel_source source;
	int level;

	source = console_effective_loglevel_source(con_level);

	switch (source) {
	case LLS_IGNORE_LOGLEVEL:
		level = CONSOLE_LOGLEVEL_MOTORMOUTH;
		break;
	case LLS_LOCAL:
		level = con_level;
		break;
	case LLS_GLOBAL:
		level = console_level;
		break;
	default:
		pr_warn("Unhandled console loglevel source: %d", source);
		level = console_level;
		break;
	}

	return level;
}


> +
> +static bool suppress_message_printing(int level, struct console *con)
> +{
> +	return level >= console_effective_loglevel(con);
>  }

and here:

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

>  
>  #ifdef CONFIG_BOOT_PRINTK_DELAY
> @@ -2122,7 +2175,21 @@ int printk_delay_msec __read_mostly;
>  
>  static inline void printk_delay(int level)
>  {
> -	if (suppress_message_printing(level))
> +	bool will_emit = false;
> +	int cookie;
> +	struct console *con;
> +
> +	cookie = console_srcu_read_lock();
> +
> +	for_each_console_srcu(con) {
> +		if (!suppress_message_printing(level, con)) {

This is called under srcu read lock so that we could use the srcu_read
API here:

	for_each_console_srcu(con) {
		int con_level = console_srcu_read_loglevel(con);

		if (!suppress_message_printing(level, con_level)) {
		...

> +			will_emit = true;
> +			break;
> +		}
> +	}
> +	console_srcu_read_unlock(cookie);
> +
> +	if (!will_emit)
>  		return;
>  
>  	boot_delay_msec();
> @@ -2975,7 +3042,7 @@ bool printk_get_next_message(struct printk_message *pmsg, struct console *con,
>  	pmsg->dropped = r.info->seq - seq;
>  
>  	/* Never suppress when used in devkmsg_read() */
> -	if (con && suppress_message_printing(r.info->level))
> +	if (con && suppress_message_printing(r.info->level, con))

We could read the con_level at the beginning of the funcion. We
already read there the CON_EXTENDED attribute:

	int con_level:

	if (con) {
		is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
		con_level = console_srcu_read_loglevel(con);
	} else {
		/* Used only by devkmsg_read(). */
		is_extended = true;
		con_level = LOGLEVEL_DEFAULT;
	}

Note that I have used LOGLEVEL_DEFAULT instead of the hardcoded -1.
IMHO, it is better to use a NAME and LOGLEVEL_DEFAULT is defined as -1
and the name more or less fits here.

and then do:

	if (con && suppress_message_printing(r.info->level, con_level))



>  		goto out;
>  
>  	if (is_extended) {
> @@ -3789,6 +3856,9 @@ static int try_enable_preferred_console(struct console *newcon,
>  			if (newcon->index < 0)
>  				newcon->index = c->index;
>  
> +			// TODO: Will be configurable in a later patch

I would remove the comment. It is enough to mention this in the commit message.

> +			newcon->level = -1;

			newcon->level = LOGLEVEL_DEFAULT;

> +
>  			if (_braille_register_console(newcon, c))
>  				return 0;
>  


Best Regards,
Petr

PS: I am sorry for suggesting so many changes. I think that you did it as
    we agreed in the discussion about v5.

    v5 was discussed a long time ago and I have learned some new
    tricks with lockdep when working on the printk kthreads.

Re: [PATCH v6 03/11] printk: console: Implement core per-console loglevel infrastructure

[Petr Mladek]

On Fri 2024-11-08 17:10:31, Petr Mladek wrote:
> On Mon 2024-10-28 16:45:37, Chris Down wrote:
> It would be nice to describe the basic logic in the commit message.
> And also the handling when the global console_loglevel is modified
> via sysrq. Something like:
> 
> <proposal>
> This commit adds the internal infrastructure to support per-console
> log levels, which will be configurable through sysfs and the kernel
> command line in future commits.
> 
> The global `console_loglevel` is preserved and used as the default log
> level for all consoles. Each console can override this global level
> with its own specific log level stored in `struct console`. To
> override the global level, the per-console log level must be greater
> than 0; otherwise, the default value of -1 ensures the global level
> is used.
> 
> The existing `ignore_loglevel` command line parameter will override
> both the global and per-console log levels.
> </proposal>
> 
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -1287,9 +1287,62 @@ module_param(ignore_loglevel, bool, S_IRUGO | S_IWUSR);
> > +enum loglevel_source
> > +console_effective_loglevel_source(const struct console *con)
> > +{
> > +	if (WARN_ON_ONCE(!con))
> > +		return LLS_GLOBAL;
> > +
> > +	if (ignore_loglevel)
> > +		return LLS_IGNORE_LOGLEVEL;
> > +
> > +	if (per_console_loglevel_is_set(con))
> > +		return LLS_LOCAL;
> > +
> > +	return LLS_GLOBAL;
> > +}
> 
> The @con parameter is nice. But it makes it hard to add lockdep
> checks. So, I suggest to pass the console specific loglevel
> as the parameter, like:
> 
> enum loglevel_source
> console_effective_loglevel_source(int con_level)
> {
> 	if (ignore_loglevel)
> 		return LLS_IGNORE_LOGLEVEL;
> 
> 	if (is_valid_per_console_loglevel(con_level))
> 		return LLS_LOCAL;
> 
> 	return LLS_GLOBAL;
> }
> 
> int console_effective_loglevel(int con_level)
> {
> 	enum loglevel_source source;
> 	int level;
> 
> 	source = console_effective_loglevel_source(con_level);
> 
> 	switch (source) {
> 	case LLS_IGNORE_LOGLEVEL:
> 		level = CONSOLE_LOGLEVEL_MOTORMOUTH;
> 		break;
> 	case LLS_LOCAL:
> 		level = con_level;
> 		break;
> 	case LLS_GLOBAL:
> 		level = console_level;
> 		break;
> 	default:
> 		pr_warn("Unhandled console loglevel source: %d", source);
> 		level = console_level;
> 		break;
> 	}
> 
> 	return level;
> }
> 
> static bool suppress_message_printing(int level, int con_level)
> {
> 	return level >= console_effective_loglevel(con_level);
> }
> 
> >  
> >  #ifdef CONFIG_BOOT_PRINTK_DELAY
> > @@ -2975,7 +3042,7 @@ bool printk_get_next_message(struct printk_message *pmsg, struct console *con,
> >  	pmsg->dropped = r.info->seq - seq;
> >  
> >  	/* Never suppress when used in devkmsg_read() */
> > -	if (con && suppress_message_printing(r.info->level))
> > +	if (con && suppress_message_printing(r.info->level, con))
> 
> We could read the con_level at the beginning of the funcion. We
> already read there the CON_EXTENDED attribute:
> 
> 	int con_level:
> 
> 	if (con) {
> 		is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
> 		con_level = console_srcu_read_loglevel(con);
> 	} else {
> 		/* Used only by devkmsg_read(). */
> 		is_extended = true;
> 		con_level = LOGLEVEL_DEFAULT;

Or we could do:

	} else {
		/* Used only by devkmsg_read(). Show all messages there. */
		is_extended = true;
		con_level = CONSOLE_LOGLEVEL_MOTORMOUTH;

The LOGLEVE_DEFAULT would cause using the global loglevel.
But we want to force showing the message.


> 	}
> 
> Note that I have used LOGLEVEL_DEFAULT instead of the hardcoded -1.
> IMHO, it is better to use a NAME and LOGLEVEL_DEFAULT is defined as -1
> and the name more or less fits here.
> 
> and then do:
> 
> 	if (con && suppress_message_printing(r.info->level, con_level))

and then we do not need to check the @con here.

This idea came when I was looking at resolving conflicts against the patchset
which removed the hack in sysrq code, see
https://lore.kernel.org/r/20241105-printk-loud-con-v2-0-bd3ecdf7b0e4@suse.com

Best Regards,
Petr

Conflict with FORCE_CON: Re: [PATCH v6 04/11] printk: Support toggling per-console loglevel via syslog() and cmdline

[Petr Mladek]

On Mon 2024-10-28 16:45:40, Chris Down wrote:
> A new module parameter (ignore_per_console_loglevel) is added, which can
> be set via the kernel command line or at runtime through
> /sys/module/printk/parameters/ignore_per_console_loglevel. When set, the
> per-console loglevels are ignored, and the global console loglevel
> (console_loglevel) is used for all consoles.
> 
> During sysrq, we temporarily disable per-console loglevels to ensure all
> requisite messages are printed to the console. This is necessary because
> sysrq is often used in dire circumstances where access to
> /sys/class/console may not be trivially possible.

I have just pushed a patchset which removed the console_loglevel
manipulation from sysrq, see
https://lore.kernel.org/r/20241105-printk-loud-con-v2-0-bd3ecdf7b0e4@suse.com

As a result, the change in drivers/tty/sysrq.c is not needed anymore.

Note that the other patchset causes some conflict with this patchset.
But they does not seem to be hard to resolved:


1st conflict is in boot_delay_msec(). But the affected logic
has actually been moved to printk_delay(). As a result,
boot_delay_msec() might stay as it is:

static void boot_delay_msec(void)
{
	unsigned long long k;
	unsigned long timeout;

	if (boot_delay == 0 || system_state >= SYSTEM_RUNNING)
		return;

	k = (unsigned long long)loops_per_msec * boot_delay;

	timeout = jiffies + msecs_to_jiffies(boot_delay);
	while (k) {
		k--;
		cpu_relax();
		/*
		 * use (volatile) jiffies to prevent
		 * compiler reduction; loop termination via jiffies
		 * is secondary and may or may not happen.
		 */
		if (time_after(jiffies, timeout))
			break;
		touch_nmi_watchdog();
	}
}


Instead, we should add check of is_printk_force_console() into
printk_delay(). I suggest to do it the following way:

static bool suppress_message_printing_everywhere(int level)
{
	bool suppress_everywhere = true;
	struct console *con;
	int cookie;

	cookie = console_srcu_read_lock();

	for_each_console_srcu(con) {
		if (!suppress_message_printing(level, con)) {
			suppress_everywhere = false;
			break;
		}
	}
	console_srcu_read_unlock(cookie);

	return suppress_everywhere;
}

static inline void printk_delay(int level)
{
	if (!is_printk_force_console() &&
	    suppress_message_printing_everywhere(level))
		return;

	boot_delay_msec();

	if (unlikely(printk_delay_msec)) {
		int m = printk_delay_msec;

		while (m--) {
			mdelay(1);
			touch_nmi_watchdog();
		}
	}
}


2nd conflict is in printk_get_next_message(). I suggest to do
something like:

	force_con = r.info->flags & LOG_FORCE_CON;

	/*
	 * Skip records that are not forced to be printed on consoles and that
	 * has level above the console loglevel. Never suppress when used in
	 * devkmsg_read().
	 */
	if (!force_con && con && suppress_message_printing(r.info->level, con))
		goto out;


Actually, I suggested to pass @con_level instead of @con here.
In which case, we might need something like:

	if (con) {
		is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
		con_level = console_srcu_read_loglevel(con);
	} else {
		/* Used only by devkmsg_read(). Show all messages there. */
		is_extended = true;
		con_level = CONSOLE_LOGLEVEL_MOTORMOUTH;
	}
[...]
	force_con = r.info->flags & LOG_FORCE_CON;

	/*
	 * Skip records that are not forced to be printed on consoles and that
	 * has level above the console loglevel.
	 */
	if (!force_con && suppress_message_printing(r.info->level, con_level))
		goto out;



I hope that you find there code snippets useful. I provide them
because I feel a bit guilty that I have already merged the other
patchset... ;-)

Best Regards,
Petr

Re: [PATCH v6 04/11] printk: Support toggling per-console loglevel via syslog() and cmdline

[Petr Mladek]

On Mon 2024-10-28 16:45:40, Chris Down wrote:
> A new module parameter (ignore_per_console_loglevel) is added, which can
> be set via the kernel command line or at runtime through
> /sys/module/printk/parameters/ignore_per_console_loglevel. When set, the
> per-console loglevels are ignored, and the global console loglevel
> (console_loglevel) is used for all consoles.
> 
> During sysrq, we temporarily disable per-console loglevels to ensure all
> requisite messages are printed to the console. This is necessary because
> sysrq is often used in dire circumstances where access to
> /sys/class/console may not be trivially possible.
> 
> Additionally, the syslog actions SYSLOG_ACTION_CONSOLE_ON and
> SYSLOG_ACTION_CONSOLE_OFF are augmented to save and restore the state of
> ignore_per_console_loglevel. This allows administrators to enable or
> disable per-console loglevels dynamically using the syslog() system
> call, as supported in userspace by things like dmesg.
> 
> This is useful when debugging issues with message emission, or when
> needing to quickly break glass and revert to global loglevel only.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1298,6 +1313,16 @@ bool per_console_loglevel_is_set(const struct console *con)
>   * 1. con->level. The locally set, console-specific loglevel. Optional, only
>   *    valid if >0.
>   * 2. console_loglevel. The default global console loglevel, always present.

I think that I have suggested to remove the above comment because
it was obvious from the code. I am in doubts now because use
extendended it below ;-)

> + * The behaviour can be further changed by the following printk module
> + * parameters:
> + *
> + * 1. ignore_loglevel. Can be set at boot or at runtime with
> + *    /sys/module/printk/parameters/ignore_loglevel. Overrides absolutely
> + *    everything since it's used to debug.
> + * 2. ignore_per_console_loglevel. Existing per-console loglevel values are left
> + *    intact, but are ignored in favour of console_loglevel as long as this is
> + *    true. Also manipulated through syslog(SYSLOG_ACTION_CONSOLE_{ON,OFF}).

I like that it is summarized in one place. I like the comment after all ;-)

That said, it is also nicely summarized in
Documentation/admin-guide/per-console-loglevel.rst
So, it might be enough to mention it here.

>   */
>  enum loglevel_source
>  console_effective_loglevel_source(const struct console *con)

Best Regards,
Petr

Re: [PATCH v6 05/11] MAINTAINERS: Mark printk-basics.rst as owned by printk subsystem

[Petr Mladek]

On Mon 2024-10-28 16:45:43, Chris Down wrote:
> We are going to update this in the next patch, so while we're here, we
> might as well mark it as owned by us.
> 
> Signed-off-by: Chris Down <chris@chrisdown.name>

Great catch!

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Petr Mladek]

On Mon 2024-10-28 16:45:46, Chris Down wrote:
> A sysfs interface under /sys/class/console/ is created that permits
> viewing and configuring per-console attributes. This is the main
> interface with which we expect users to interact with and configure
> per-console loglevels.
> 
> Each console device now has its own directory (for example,
> /sys/class/console/ttyS0/) containing the following attributes:
> 
> - effective_loglevel (ro): The effective loglevel for the console after
>   considering all loglevel authorities (e.g., global loglevel,
>   per-console loglevel).
> - effective_loglevel_source (ro): The source of the effective loglevel
>   (e.g., local, global, ignore_loglevel).
> - enabled (ro): Indicates whether the console is enabled (1) or disabled
>   (0).
> - loglevel (rw): The per-console loglevel. Writing a value between 0
>   (KERN_EMERG) and 8 (KERN_DEBUG + 1) sets the per-console loglevel.
>   Writing -1 disables the per-console loglevel.

The function clamp_loglevel() uses "minimum_console_loglevel"
as the minimal boundary. This variable is initialized to
CONSOLE_LOGLEVEL_MIN which is defined as 1.

And indeed, the value 0 is not accepted:

  # echo 0 >/sys/class/console/ttyS0/loglevel 
  -bash: echo: write error: Numerical result out of range

CONSOLE_LOGLEVEL_MIN has been used since ages. I am not completely
sure about the motivation. I guess that KERN_EMERG messages
should never get disabled.

I would keep "1" as the minimal allowed value and update
the documentation.


> In terms of technical implementation, we embed a device pointer in the
> console struct, and register each console using it so we can expose
> attributes in sysfs. We currently expose the following attributes:
> 
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-console
[...]
> +What:		/sys/class/console/<C>/loglevel
> +Date:		October 2024
> +Contact:	Chris Down <chris@chrisdown.name>
> +Description:	Read write. The current per-console loglevel, which will take
> +		effect if not overridden by other non-sysfs controls (see
> +		Documentation/admin-guide/per-console-loglevel.rst). Bounds are
> +		0 (LOGLEVEL_EMERG) to 8 (LOGLEVEL_DEBUG + 1) inclusive. Also

The real minimal boundary is 1.

> +		takes the special value "-1" to indicate that no per-console
> +		loglevel is set, and we should defer to the global controls.
> diff --git a/Documentation/admin-guide/per-console-loglevel.rst b/Documentation/admin-guide/per-console-loglevel.rst
> index 1ec7608f94b0..41bf3befb2f3 100644
> --- a/Documentation/admin-guide/per-console-loglevel.rst
> +++ b/Documentation/admin-guide/per-console-loglevel.rst
> @@ -68,3 +68,44 @@ further:
>  The default value for ``kernel.console_loglevel`` comes from
>  ``CONFIG_CONSOLE_LOGLEVEL_DEFAULT``, or ``CONFIG_CONSOLE_LOGLEVEL_QUIET`` if
>  ``quiet`` is passed on the kernel command line.
> +
> +Console attributes
> +~~~~~~~~~~~~~~~~~~
> +
> +Registered consoles are exposed at ``/sys/class/console``. For example, if you
> +are using ``ttyS0``, the console backing it can be viewed at
> +``/sys/class/console/ttyS0/``. The following files are available:
> +
> +* ``effective_loglevel`` (r): The effective loglevel after considering all
> +  loglevel authorities. For example, it shows the value of the console-specific
> +  loglevel when a console-specific loglevel is defined, and shows the global
> +  console loglevel value when the console-specific one is not defined.
> +
> +* ``effective_loglevel_source`` (r): The loglevel authority which resulted in
> +  the effective loglevel being set. The following values can be present:
> +
> +    * ``local``: The console-specific loglevel is in effect.
> +
> +    * ``global``: The global loglevel (``kernel.console_loglevel``) is in
> +      effect. Set a console-specific loglevel to override it.
> +
> +    * ``ignore_loglevel``: ``ignore_loglevel`` was specified on the kernel
> +      command line or at ``/sys/module/printk/parameters/ignore_loglevel``.
> +      Disable it to use level controls.
> +
> +* ``enabled`` (r): Whether the console is enabled.

Please, remove the 'enabled' flag for now. All registered consoles are
enabled. It seems that only some serial consoles temporary disable
consoles during the suspend but the sysfs interface is not accessible
at this time anyway.

It has been discussed recently, see
https://lore.kernel.org/r/ZyoNZfLT6tlVAWjO@pathway.suse.cz

> +* ``loglevel`` (rw): The local, console-specific loglevel for this console.
> +  This will be in effect if no other global control overrides it. Look at
> +  ``effective_loglevel`` and ``effective_loglevel_source`` to verify that.
> +
> +Deprecated
> +~~~~~~~~~~
> +
> +* ``kernel.printk`` sysctl: this takes four values, setting
> +  ``kernel.console_loglevel``, ``kernel.default_message_loglevel``, the minimum
> +  console loglevel, and a fourth unused value. The interface is generally
> +  considered to quite confusing, doesn't perform checks on the values given,
> +  and is unaware of per-console loglevel semantics.
> +
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -77,6 +77,8 @@ extern bool ignore_per_console_loglevel;
>  
>  extern void console_verbose(void);
>  
> +int clamp_loglevel(int level);
> +

It is declared also in kernel/printk/internal.h. And it seems
that the internal header is enough.

>  /* strlen("ratelimit") + 1 */
>  #define DEVKMSG_STR_MAX_SIZE 10
>  extern char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE];
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -199,6 +199,12 @@ static int __init control_devkmsg(char *str)
>  }
>  __setup("printk.devkmsg=", control_devkmsg);
>  
> +int clamp_loglevel(int level)
> +{
> +	return clamp(level, minimum_console_loglevel,
> +		     CONSOLE_LOGLEVEL_MOTORMOUTH);

Documentation/ABI/testing/sysfs-class-console says:

   "Bounds are 1 (LOGLEVEL_ALERT) to 8 (LOGLEVEL_DEBUG + 1) inclusive."

I do not have strong opinion. But I would follow the documentation
because the limit is well defined there. On the contrary,
CONSOLE_LOGLEVEL_MOTORMOUTH is a randomly chosen value
and we should not leak it to the userspace ;-)

> +}
> +
>  char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE] = "ratelimit";
>  #if defined(CONFIG_PRINTK) && defined(CONFIG_SYSCTL)
>  int devkmsg_sysctl_set_loglvl(const struct ctl_table *table, int write,
> @@ -3894,6 +3900,8 @@ static int try_enable_preferred_console(struct console *newcon,
>  			// TODO: Will be configurable in a later patch
>  			newcon->level = -1;
>  
> +			newcon->classdev = NULL;
> +

The console can be enabled also by try_enable_default_console().
We need to initialize newcon->classdev there as well.

The same is true for newcon->level. I have missed this when
reviewing the 3rd patch.

>  			if (_braille_register_console(newcon, c))
>  				return 0;
>  
> --- /dev/null
> +++ b/kernel/printk/sysfs.c
> +void console_register_device(struct console *con)
> +{
> +	/*
> +	 * We might be called from register_console() before the class is
> +	 * registered. If that happens, we'll take care of it in
> +	 * printk_late_init.
> +	 */
> +	if (IS_ERR_OR_NULL(console_class))
> +		return;

This check is not enough to avoid race with printk_late_init():

CPU0					CPU1

register_console()			printk_late_init()
  [...]					[...]
  console_register_device()		console_register_device()
    if (IS_ERR_OR_NULL(console_class))    if (IS_ERR_OR_NULL(console_class))

BANG: Both CPUs see "console == NULL" and both CPUs try to
      register the device.

I suggest to avoid this race by taking console_list_lock() in
printk_late_init(), see below.

> +
> +	if (WARN_ON(con->classdev))
> +		return;
> +
> +	con->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
> +	if (!con->classdev)
> +		return;
> +
> +	device_initialize(con->classdev);
> +	dev_set_name(con->classdev, "%s%d", con->name, con->index);
> +	dev_set_drvdata(con->classdev, con);
> +	con->classdev->release = console_classdev_release;
> +	con->classdev->class = console_class;
> +	if (device_add(con->classdev))
> +		put_device(con->classdev);
> +}
> +
> +void console_setup_class(void)
> +{
> +	struct console *con;
> +	int cookie;
> +
> +	/*
> +	 * printk exists for the lifetime of the kernel, it cannot be unloaded,
> +	 * so we should never end up back in here.
> +	 */
> +	if (WARN_ON(console_class))
> +		return;
> +
> +	console_class = class_create("console");
> +	if (!IS_ERR(console_class))
> +		console_class->dev_groups = console_sysfs_groups;
> +
> +	cookie = console_srcu_read_lock();

We should take console_list_lock() here to avoid races with
register_console() and unregister_console(). Otherwise.
console_register_device(con) might be called twice.

> +	for_each_console_srcu(con)
> +		console_register_device(con);
> +	console_srcu_read_unlock(cookie);
> +}
> +#else /* CONFIG_PRINTK */
> +void console_register_device(struct console *new)
> +{
> +}
> +void console_setup_class(void)
> +{
> +}
> +#endif

Best Regards,
Petr

register_device: was: Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Petr Mladek]

> A sysfs interface under /sys/class/console/ is created that permits
> viewing and configuring per-console attributes. This is the main
> interface with which we expect users to interact with and configure
> per-console loglevels.

> diff --git a/kernel/printk/sysfs.c b/kernel/printk/sysfs.c
> new file mode 100644
> index 000000000000..e24590074861
> --- /dev/null
> +++ b/kernel/printk/sysfs.c
> +ATTRIBUTE_GROUPS(console_sysfs);
> +
> +static void console_classdev_release(struct device *dev)
> +{
> +	kfree(dev);
> +}
> +
> +void console_register_device(struct console *con)
> +{
> +	/*
> +	 * We might be called from register_console() before the class is
> +	 * registered. If that happens, we'll take care of it in
> +	 * printk_late_init.
> +	 */
> +	if (IS_ERR_OR_NULL(console_class))
> +		return;
> +
> +	if (WARN_ON(con->classdev))
> +		return;
> +
> +	con->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
> +	if (!con->classdev)
> +		return;
> +
> +	device_initialize(con->classdev);
> +	dev_set_name(con->classdev, "%s%d", con->name, con->index);
> +	dev_set_drvdata(con->classdev, con);
> +	con->classdev->release = console_classdev_release;
> +	con->classdev->class = console_class;
> +	if (device_add(con->classdev))
> +		put_device(con->classdev);

Honestly, I am not sure how to review this. I am not familiar with
these APIs. I have spent few hours trying to investigate various
drivers but I did not find any similar use case. I tried to look
for documentation but I did not find any good HOWTO.

It seems to work but it is the only thing that I could
say about it ;-)

Just by chance, do you have any pointers into a code or
documentation which could help me to feel more comfortable?

> +}
> +

Best Regards,
Petr

Re: [PATCH v6 07/11] printk: Constrain hardware-addressed console checks to name position

[Petr Mladek]

On Mon 2024-10-28 16:45:48, Chris Down wrote:
> Commit 7640f1a44eba ("printk: Add
> match_devname_and_update_preferred_console()") adds support for
> DEVNAME:n:n style console= registration. It determines whether or not
> this is a hardware-addressed console by looking at whether the console=
> string has a colon in it. However, this interferes with loglevel:n and
> potentially other future named options, which also make use of the
> character.
> 
> In order to avoid this, only search positions before options.
> 
> Signed-off-by: Chris Down <chris@chrisdown.name>

Nice trick!

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v6 08/11] printk: Support setting initial console loglevel via console= on cmdline

[Petr Mladek]

On Mon 2024-10-28 16:45:52, Chris Down wrote:
> Extend the console= kernel command line parameter to support specifying
> per-console loglevels at boot time. This is achieved by introducing a
> new loglevel option that can be passed as a loglevel option within a
> console= stanza.
> 
> For example, this is an example of how one might configure netconsole
> devices to print messages with a higher priority than loglevel 3
> (KERN_ERR) at startup:
> 
>     console=netcon0,loglevel:3
> 
> --- a/Documentation/admin-guide/serial-console.rst
> +++ b/Documentation/admin-guide/serial-console.rst
> @@ -32,6 +32,33 @@ The format of this option is::
>  			and F is flow control ('r' for RTS). Default is
>  			9600n8. The maximum baudrate is 115200.
>  
> +			One can also specify the per-console loglevel for this
> +			console by providing a loglevel parameter, for example
> +			"loglevel:4" to set this console's loglevel to 4. The
> +			value provided can be from 0 (LOGLEVEL_EMERG) to 8

The real lower limit, enforced by clamp_loglevel(), is 1.

> +			(LOGLEVEL_DEBUG + 1), and messages below that will be
> +			emitted onto the console as they become available.
> +
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3903,8 +3982,10 @@ static int try_enable_preferred_console(struct console *newcon,
>  			if (newcon->index < 0)
>  				newcon->index = c->index;
>  
> -			// TODO: Will be configurable in a later patch
> -			newcon->level = -1;
> +			if (c->level > 0)
> +				newcon->level = c->level;
> +			else
> +				newcon->level = -1;

It seems that c->level is already set to -1 when it is not defined on
the command line. I think that that we could simply do:

			newcon->level = c->level;

Just for record. We need to explicitely set newcon->level to -1 in
try_enable_default_console().


>  			newcon->classdev = NULL;
>  

Otherwise, it looks good.

Best Regards,
Petr

Re: [PATCH v6 09/11] printk: Add sysctl interface to set global loglevels

[Petr Mladek]

On Mon 2024-10-28 16:45:55, Chris Down wrote:
> Introduce two new sysctl interfaces for configuring global loglevels:
> 
> - kernel.console_loglevel: Sets the global console loglevel, determining
>   the minimum priority of messages printed to consoles. Messages with a
>   loglevel lower than this value will be printed.
> - kernel.default_message_loglevel: Sets the default loglevel for
>   messages that do not specify an explicit loglevel.
> 
> The kernel.printk sysctl was previously used to set multiple loglevel
> parameters simultaneously, but it was confusing and lacked proper
> validation. By introducing these dedicated sysctl interfaces, we provide
> a clearer and more granular way to configure the loglevels.
> 
> Signed-off-by: Chris Down <chris@chrisdown.name>
> ---
>  Documentation/admin-guide/sysctl/kernel.rst | 17 ++++++++-
>  kernel/printk/sysctl.c                      | 42 +++++++++++++++++++++
>  2 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index f8bc1630eba0..8019779b27f6 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -1044,6 +1044,20 @@ otherwise the 'doze' mode will be used.
>  
>  ==============================================================
>  
> +Some of these settings may be overridden per-console, see
> +Documentation/admin-guide/per-console-loglevel.rst. See ``man 2 syslog`` for
> +more information on the different loglevels.
> +
> +console_loglevel
> +================
> +
> +Messages with a higher priority than this will be printed to consoles.
> +
> +default_message_loglevel
> +========================
> +
> +Messages without an explicit priority will be printed with this priority.
> +
>  printk
>  ======
>  
> @@ -1052,8 +1066,7 @@ The four values in printk denote: ``console_loglevel``,
>  ``default_console_loglevel`` respectively.
>  
>  These values influence printk() behavior when printing or
> -logging error messages. See '``man 2 syslog``' for more info on
> -the different loglevels.
> +logging error messages.
>  
>  ======================== =====================================
>  console_loglevel         messages with a higher priority than
> diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
> index f5072dc85f7a..3bce8b89dacc 100644
> --- a/kernel/printk/sysctl.c
> +++ b/kernel/printk/sysctl.c
> @@ -11,6 +11,9 @@
>  
>  static const int ten_thousand = 10000;
>  
> +static int min_msg_loglevel = LOGLEVEL_EMERG;
> +static int max_msg_loglevel = LOGLEVEL_DEBUG;
> +
>  static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int write,
>  				void *buffer, size_t *lenp, loff_t *ppos)
>  {
> @@ -20,6 +23,29 @@ static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int writ
>  	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>  }
>  
> +static int printk_console_loglevel(const struct ctl_table *table, int write,
> +				   void *buffer, size_t *lenp, loff_t *ppos)

I would make it more clear that this function is using the procfs
based API and call it "proc_dointvec_console_loglevel()".

> +{
> +	struct ctl_table ltable = *table;
> +	int ret, level;
> +
> +	if (!write)
> +		return proc_dointvec(table, write, buffer, lenp, ppos);
> +
> +	ltable.data = &level;

Ah, I have missed that this is a copy and spent quite some time
wondering why it worked ;-) I remember that the same happened
last time I saw this trick.

It would deserve a comment for people like me. Or maybe, rename
the variable from ltable to table_copy.

Or I think about another solution, see below.

> +
> +	ret = proc_dointvec(&ltable, write, buffer, lenp, ppos);
> +	if (ret)
> +		return ret;
> +
> +	if (level != -1 && level != clamp_loglevel(level))
> +		return -ERANGE;
> +
> +	console_loglevel = level;

There is no locking. It seems that the original proc_dointvec code
handle this by using WRITE_ATOMIC(). It prevents compiler
optimizations. In particular, it makes sure that the entire value
will be updated in a single operation (atomically).

> +	return 0;
> +}
> +

I have mixed feelings. On one hand, the copy of the table entry looks
like a nice trick. On the other hand, I guess that this is the only
code using such a trick. It might make it more error prone when
some of the API internals change.

It seems that other users handle similar situations by
passing a custom @conv callback to do_proc_dointvec(),
for example, proc_dointvec_minmax(), proc_dointvec_jiffies().

This approach would require exporing do_proc_dointvec()
and do_proc_dointvec_conv(). But there already is a precedent
when do_proc_douintvec() is used in proc_dopipe_max_size().

I have tried to implement it to see how it looks. And I would
prefer to use it. Here is the updated patch:


From 05a75d464276da24b7f4b7b97b982041607bbae2 Mon Sep 17 00:00:00 2001
From: Chris Down <chris@chrisdown.name>
Date: Mon, 28 Oct 2024 16:45:55 +0000
Subject: [POC 09/11] printk: Add sysctl interface to set global loglevels

Introduce two new sysctl interfaces for configuring global loglevels:

- kernel.console_loglevel: Sets the global console loglevel, determining
  the minimum priority of messages printed to consoles. Messages with a
  loglevel lower than this value will be printed.
- kernel.default_message_loglevel: Sets the default loglevel for
  messages that do not specify an explicit loglevel.

The kernel.printk sysctl was previously used to set multiple loglevel
parameters simultaneously, but it was confusing and lacked proper
validation. By introducing these dedicated sysctl interfaces, we provide
a clearer and more granular way to configure the loglevels.

Signed-off-by: Chris Down <chris@chrisdown.name>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 Documentation/admin-guide/sysctl/kernel.rst | 17 ++++++-
 include/linux/sysctl.h                      |  7 +++
 kernel/printk/sysctl.c                      | 51 +++++++++++++++++++++
 kernel/sysctl.c                             |  4 +-
 4 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index f8bc1630eba0..8019779b27f6 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -1044,6 +1044,20 @@ otherwise the 'doze' mode will be used.
 
 ==============================================================
 
+Some of these settings may be overridden per-console, see
+Documentation/admin-guide/per-console-loglevel.rst. See ``man 2 syslog`` for
+more information on the different loglevels.
+
+console_loglevel
+================
+
+Messages with a higher priority than this will be printed to consoles.
+
+default_message_loglevel
+========================
+
+Messages without an explicit priority will be printed with this priority.
+
 printk
 ======
 
@@ -1052,8 +1066,7 @@ The four values in printk denote: ``console_loglevel``,
 ``default_console_loglevel`` respectively.
 
 These values influence printk() behavior when printing or
-logging error messages. See '``man 2 syslog``' for more info on
-the different loglevels.
+logging error messages.
 
 ======================== =====================================
 console_loglevel         messages with a higher priority than
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index aa4c6d44aaa0..a297ca0d4096 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -237,6 +237,13 @@ extern struct ctl_table_header *register_sysctl_mount_point(const char *path);
 
 void do_sysctl_args(void);
 bool sysctl_is_alias(char *param);
+int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, int *valp,
+			  int write, void *data);
+int do_proc_dointvec(const struct ctl_table *table, int write,
+		     void *buffer, size_t *lenp, loff_t *ppos,
+		     int (*conv)(bool *negp, unsigned long *lvalp, int *valp,
+				 int write, void *data),
+		     void *data);
 int do_proc_douintvec(const struct ctl_table *table, int write,
 		      void *buffer, size_t *lenp, loff_t *ppos,
 		      int (*conv)(unsigned long *lvalp,
diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
index f5072dc85f7a..749e3575f2d1 100644
--- a/kernel/printk/sysctl.c
+++ b/kernel/printk/sysctl.c
@@ -11,6 +11,9 @@
 
 static const int ten_thousand = 10000;
 
+static int min_msg_loglevel = LOGLEVEL_EMERG;
+static int max_msg_loglevel = LOGLEVEL_DEBUG;
+
 static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int write,
 				void *buffer, size_t *lenp, loff_t *ppos)
 {
@@ -20,6 +23,38 @@ static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int writ
 	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 }
 
+static int do_proc_dointvec_console_loglevel(bool *negp, unsigned long *lvalp,
+					int *valp,
+					int write, void *data)
+{
+	int level, ret;
+
+	/*
+	 * If writing, first do so via a temporary local int so we can
+	 * bounds-check it before touching *valp.
+	 */
+	int *intp = write ? &level : valp;
+
+	ret = do_proc_dointvec_conv(negp, lvalp, intp, write, data);
+	if (ret)
+		return ret;
+
+	if (write) {
+		if (level != -1 && level != clamp_loglevel(level))
+			return -ERANGE;
+		WRITE_ONCE(*valp, level);
+	}
+
+	return 0;
+}
+
+static int proc_dointvec_console_loglevel(const struct ctl_table *table,
+		int write, void *buffer, size_t *lenp, loff_t *ppos)
+{
+	return do_proc_dointvec(table, write, buffer, lenp, ppos,
+				do_proc_dointvec_console_loglevel, NULL);
+}
+
 static struct ctl_table printk_sysctls[] = {
 	{
 		.procname	= "printk",
@@ -76,6 +111,22 @@ static struct ctl_table printk_sysctls[] = {
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_TWO,
 	},
+	{
+		.procname	= "console_loglevel",
+		.data		= &console_loglevel,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_console_loglevel,
+	},
+	{
+		.procname	= "default_message_loglevel",
+		.data		= &default_message_loglevel,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &min_msg_loglevel,
+		.extra2		= &max_msg_loglevel,
+	},
 };
 
 void __init printk_sysctl_init(void)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 79e6cb1d5c48..225ef261d2fb 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -424,7 +424,7 @@ static void proc_put_char(void **buf, size_t *size, char c)
 	}
 }
 
-static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
+int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
 				 int *valp,
 				 int write, void *data)
 {
@@ -541,7 +541,7 @@ static int __do_proc_dointvec(void *tbl_data, const struct ctl_table *table,
 	return err;
 }
 
-static int do_proc_dointvec(const struct ctl_table *table, int write,
+int do_proc_dointvec(const struct ctl_table *table, int write,
 		  void *buffer, size_t *lenp, loff_t *ppos,
 		  int (*conv)(bool *negp, unsigned long *lvalp, int *valp,
 			      int write, void *data),
-- 
2.47.0

Re: [PATCH v6 10/11] printk: Deprecate the kernel.printk sysctl interface

[Petr Mladek]

On Mon 2024-10-28 16:45:59, Chris Down wrote:
> The kernel.printk sysctl interface is deprecated in favour of more
> granular and clearer sysctl interfaces for controlling loglevels, namely
> kernel.console_loglevel and kernel.default_message_loglevel.
> 
> kernel.printk has a number of fairly significant usability problems:
> 
> - It takes four values in a specific order, which is not intuitive.
>   Speaking from personal experience, even people working on printk
>   sometimes need to look up the order of these values, which doesn't
>   suggest much in the way of perspicuity.
> - There is no validation on the input values, so users can set them to
>   invalid or nonsensical values without receiving any errors or
>   warnings. This can lead to unpredictable behaviour.
> - One of the four values, default_console_loglevel, is not used in the
>   kernel (see below), making it redundant and potentially confusing.
> - Overall, the interface is complex, hard to understand, and combines
>   multiple controls into a single sysctl, which is not ideal. It should
>   be separated into distinct controls for clarity.
> 
> Setting kernel.printk now calls printk_sysctl_deprecated, which emits a
> pr_warn. The warning informs users about the deprecation and suggests
> using the new sysctl interfaces instead.
> 
> By deprecating kernel.printk, we aim to:
> 
> - Encourage users to adopt the new, dedicated sysctl interfaces
>   (kernel.console_loglevel and kernel.default_message_loglevel), which
>   are more straightforward and easier to understand.
> - Improve input validation and error handling, ensuring that users
>   receive appropriate feedback when setting invalid values.
> - Simplify the configuration of loglevels by exposing only the controls
>   that are necessary and relevant.
> 
> --- a/kernel/printk/sysctl.c
> +++ b/kernel/printk/sysctl.c
> @@ -7,6 +7,7 @@
>  #include <linux/printk.h>
>  #include <linux/capability.h>
>  #include <linux/ratelimit.h>
> +#include <linux/console.h>
>  #include "internal.h"
>  
>  static const int ten_thousand = 10000;
> @@ -46,13 +47,24 @@ static int printk_console_loglevel(const struct ctl_table *table, int write,
>  	return 0;
>  }
>  
> +static int printk_sysctl_deprecated(const struct ctl_table *table, int write,
> +				    void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	int res = proc_dointvec(table, write, buffer, lenp, ppos);
> +
> +	if (write)
> +		pr_warn_once("printk: The kernel.printk sysctl is deprecated. Consider using kernel.console_loglevel or kernel.default_message_loglevel instead.\n");
> +
> +	return res;
> +}
> +
>  static struct ctl_table printk_sysctls[] = {
>  	{
>  		.procname	= "printk",
>  		.data		= &console_loglevel,
>  		.maxlen		= 4*sizeof(int),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec,
> +		.proc_handler	= printk_sysctl_deprecated,

I would prefer to follow the existing naming scheme for
custom modifications of proc_dointvec() and call it
"proc_dointvec_printk_deprecated".

>  	},
>  	{
>  		.procname	= "printk_ratelimit",

With this cosmetic change:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v6 11/11] printk: Purge default_console_loglevel

[Petr Mladek]

On Mon 2024-10-28 16:46:01, Chris Down wrote:
> default_console_loglevel (then DEFAULT_LOGLEVEL) was created in Linux
> 0.99.13 (September 1993) to power what we now call
> SYSLOG_ACTION_CONSOLE_{ON,OFF}. It was originally hardcoded to 7.
> 
> In Linux 2.3.12 (March 1997), Chris Horn made it configurable by sysctl
> through kernel.printk.
> 
> Its demise came about in July 2009 in commit 1aaad49e856c ("printk:
> Restore previous console_loglevel when re-enabling logging"), which
> replaces default_console_loglevel with the previously used
> console_loglevel. However, the documentation was never updated, and we
> still kept on telling users that this sets the default value for
> console_loglevel, even though this hasn't been true for over 15 years.
> 
> Purge both the documentation and all remaining references to
> default_console_loglevel. It will still be initialised in the sysctl
> array.
> 
> Signed-off-by: Chris Down <chris@chrisdown.name>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v6 03/11] printk: console: Implement core per-console loglevel infrastructure

[Petr Mladek]

On Fri 2024-11-08 17:10:31, Petr Mladek wrote:
> On Mon 2024-10-28 16:45:37, Chris Down wrote:
> > Consoles can have vastly different latencies and throughputs. For
> > example, writing a message to the serial console can take on the order
> > of tens of milliseconds to get the UART to successfully write a message.
> > While this might be fine for a single, one-off message, this can cause
> > significant application-level stalls in situations where the kernel
> > writes large amounts of information to the console.
> > 
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -101,11 +102,25 @@ __setup("sysrq_always_enabled", sysrq_always_enabled_setup);
> >  static void sysrq_handle_loglevel(u8 key)
> >  {
> >  	u8 loglevel = key - '0';
> > +	int cookie;
> > +	int warned = 0;
> > +	struct console *con;
> >  
> >  	console_loglevel = CONSOLE_LOGLEVEL_DEFAULT;
> >  	pr_info("Loglevel set to %u\n", loglevel);
> >  	console_loglevel = loglevel;
> > +
> > +	cookie = console_srcu_read_lock();
> > +	for_each_console_srcu(con) {
> > +		if (!warned && per_console_loglevel_is_set(con)) {
> > +			warned = 1;
> > +			pr_warn("Overriding per-console loglevel from sysrq\n");

It just came to my mind. We could use pr_warn_once() and get rid
of the @warned variable. It is slightly less optimal. But this
is a slow path and the code would be easier.

> > +		}
> > +		WRITE_ONCE(con->level, -1);
> > +	}
> > +	console_srcu_read_unlock(cookie);
> >  }
> > +
> 

Best Regards,
Petr

syslog warning: was: Re: [PATCH v6 04/11] printk: Support toggling per-console loglevel via syslog() and cmdline

[Petr Mladek]

On Mon 2024-10-28 16:45:40, Chris Down wrote:
> A new module parameter (ignore_per_console_loglevel) is added, which can
> be set via the kernel command line or at runtime through
> /sys/module/printk/parameters/ignore_per_console_loglevel. When set, the
> per-console loglevels are ignored, and the global console loglevel
> (console_loglevel) is used for all consoles.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1836,19 +1862,28 @@ int do_syslog(int type, char __user *buf, int len, int source)
>  		break;
>  	/* Disable logging to console */
>  	case SYSLOG_ACTION_CONSOLE_OFF:
> -		if (saved_console_loglevel == LOGLEVEL_DEFAULT)
> +		if (saved_console_loglevel == LOGLEVEL_DEFAULT) {
>  			saved_console_loglevel = console_loglevel;
> +			saved_ignore_per_console_loglevel =
> +				ignore_per_console_loglevel;
> +		}
>  		console_loglevel = minimum_console_loglevel;
> +		ignore_per_console_loglevel = true;
>  		break;
>  	/* Enable logging to console */
>  	case SYSLOG_ACTION_CONSOLE_ON:
>  		if (saved_console_loglevel != LOGLEVEL_DEFAULT) {
>  			console_loglevel = saved_console_loglevel;
> +			ignore_per_console_loglevel =
> +				saved_ignore_per_console_loglevel;
>  			saved_console_loglevel = LOGLEVEL_DEFAULT;
>  		}
>  		break;
>  	/* Set level of messages printed to console */
>  	case SYSLOG_ACTION_CONSOLE_LEVEL:
> +		if (!ignore_per_console_loglevel)
> +			pr_warn_once(
> +				"SYSLOG_ACTION_CONSOLE_LEVEL is ignored by consoles with an explicitly set per-console loglevel, see Documentation/admin-guide/per-console-loglevel.rst\n");

I see this warning during every boot because rsyslogd() modifies the
global loglevel.

I am afraid that admins might not like it. I might live in dreams but
I guess that everyone would like to reach a clean boot without any
warning.

One the other hand, we should warn when it does not work as expected.

A compromise would be to warn only when there is a console with
the console specific loglevel set.

I am not sure if we have already discussed this in the past.
But I would prefer the compromise after all.

What do you think, please?

Best Regards,
Petr

Re: register_device: was: Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Chris Down]

Petr Mladek writes:
>Honestly, I am not sure how to review this. I am not familiar with
>these APIs. I have spent few hours trying to investigate various
>drivers but I did not find any similar use case. I tried to look
>for documentation but I did not find any good HOWTO.
>
>It seems to work but it is the only thing that I could
>say about it ;-)
>
>Just by chance, do you have any pointers into a code or
>documentation which could help me to feel more comfortable?

I think you and I are in a similar boat :-) Some similar code can be seen in 
places like block/genhd.c in device_add_disk and __alloc_disk_node, which also 
do this kind of dynamic setup. I mostly just spent my time looking at device.h 
and its users.

Greg, maybe you could also take a look?

Re: syslog warning: was: Re: [PATCH v6 04/11] printk: Support toggling per-console loglevel via syslog() and cmdline

[Chris Down]

Petr Mladek writes:
>I see this warning during every boot because rsyslogd() modifies the
>global loglevel.
>
>[...]
>
>I am not sure if we have already discussed this in the past.
>But I would prefer the compromise after all.

I initially implemented that way until v4, but during the v3 review, as I 
understood it you recommended changing it to use !ignore_per_console_loglevel 
based on the assumption that SYSLOG_ACTION_CONSOLE_{ON,OFF} wasn't widely used. 
Maybe I misunderstood what was intended?

Happy to revert to the previous approach with warn_on_local_loglevel(), just 
let me know :-)

Re: Conflict with FORCE_CON: Re: [PATCH v6 04/11] printk: Support toggling per-console loglevel via syslog() and cmdline

[Chris Down]

Petr Mladek writes:
>On Mon 2024-10-28 16:45:40, Chris Down wrote:
>> A new module parameter (ignore_per_console_loglevel) is added, which can
>> be set via the kernel command line or at runtime through
>> /sys/module/printk/parameters/ignore_per_console_loglevel. When set, the
>> per-console loglevels are ignored, and the global console loglevel
>> (console_loglevel) is used for all consoles.
>>
>> During sysrq, we temporarily disable per-console loglevels to ensure all
>> requisite messages are printed to the console. This is necessary because
>> sysrq is often used in dire circumstances where access to
>> /sys/class/console may not be trivially possible.
>
>I have just pushed a patchset which removed the console_loglevel
>manipulation from sysrq, see
>https://lore.kernel.org/r/20241105-printk-loud-con-v2-0-bd3ecdf7b0e4@suse.com
  
Noted, so would you like me to change this to be based on current 
printk/for-next, or is another branch preferred?

Re: register_device: was: Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Greg Kroah-Hartman]

On Thu, Nov 14, 2024 at 01:41:31PM -0500, Chris Down wrote:
> Petr Mladek writes:
> > Honestly, I am not sure how to review this. I am not familiar with
> > these APIs. I have spent few hours trying to investigate various
> > drivers but I did not find any similar use case. I tried to look
> > for documentation but I did not find any good HOWTO.
> > 
> > It seems to work but it is the only thing that I could
> > say about it ;-)
> > 
> > Just by chance, do you have any pointers into a code or
> > documentation which could help me to feel more comfortable?
> 
> I think you and I are in a similar boat :-) Some similar code can be seen in
> places like block/genhd.c in device_add_disk and __alloc_disk_node, which
> also do this kind of dynamic setup. I mostly just spent my time looking at
> device.h and its users.
> 
> Greg, maybe you could also take a look?

Sorry, I missed that you were adding a new sysfs api here.  Yes, I'll
add this to my queue to review, give me a few days...

thanks,

greg k-h

Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Greg Kroah-Hartman]

On Mon, Oct 28, 2024 at 04:45:46PM +0000, Chris Down wrote:
> A sysfs interface under /sys/class/console/ is created that permits
> viewing and configuring per-console attributes. This is the main
> interface with which we expect users to interact with and configure
> per-console loglevels.
> 
> Each console device now has its own directory (for example,
> /sys/class/console/ttyS0/) containing the following attributes:
> 
> - effective_loglevel (ro): The effective loglevel for the console after
>   considering all loglevel authorities (e.g., global loglevel,
>   per-console loglevel).
> - effective_loglevel_source (ro): The source of the effective loglevel
>   (e.g., local, global, ignore_loglevel).
> - enabled (ro): Indicates whether the console is enabled (1) or disabled
>   (0).
> - loglevel (rw): The per-console loglevel. Writing a value between 0
>   (KERN_EMERG) and 8 (KERN_DEBUG + 1) sets the per-console loglevel.
>   Writing -1 disables the per-console loglevel.
> 
> In terms of technical implementation, we embed a device pointer in the
> console struct, and register each console using it so we can expose
> attributes in sysfs. We currently expose the following attributes:
> 
>     % ls -l /sys/class/console/ttyS0/
>     total 0
>     lrwxrwxrwx 1 root root    0 Oct 23 13:17 subsystem -> ../../../../class/console/
>     -r--r--r-- 1 root root 4096 Oct 23 13:18 effective_loglevel
>     -r--r--r-- 1 root root 4096 Oct 23 13:18 effective_loglevel_source
>     -r--r--r-- 1 root root 4096 Oct 23 13:18 enabled
>     -rw-r--r-- 1 root root 4096 Oct 23 13:18 loglevel
>     -rw-r--r-- 1 root root 4096 Oct 23 13:17 uevent
> 
> The lifecycle of this classdev looks like this on registration:
> 
>     register_console(con)/printk_late_init()
>       console_register_device(con)
>         device_initialize(con->classdev) # refcount++
>         device_add(con->classdev)        # refcount++
> 
> At stable state, the refcount is two.
> 
> Console unregistration looks like this:
> 
>     [con->classdev refcount drops to 0]
>       console_classdev_release(con->classdev)
>         kfree(con->classdev)
> 
>     unregister_console(con)
>       device_unregister(con->classdev)
>         device_del(con->classdev) # refcount--
>           device_remove_class_symlinks()
>             kernfs_remove_by_name_ns()
>               kernfs_drain()
>                 kernfs_drain_open_files() # wait for close()
>         put_device(con->classdev) # refcount--
> 
> Signed-off-by: Chris Down <chris@chrisdown.name>
> ---
>  Documentation/ABI/testing/sysfs-class-console |  47 +++++
>  .../admin-guide/per-console-loglevel.rst      |  41 ++++
>  Documentation/core-api/printk-basics.rst      |  35 ++--
>  Documentation/networking/netconsole.rst       |  13 ++
>  MAINTAINERS                                   |   1 +
>  include/linux/console.h                       |   3 +
>  include/linux/printk.h                        |   2 +
>  kernel/printk/Makefile                        |   2 +-
>  kernel/printk/internal.h                      |   5 +
>  kernel/printk/printk.c                        |  15 ++
>  kernel/printk/sysfs.c                         | 178 ++++++++++++++++++
>  11 files changed, 324 insertions(+), 18 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-console
>  create mode 100644 kernel/printk/sysfs.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-console b/Documentation/ABI/testing/sysfs-class-console
> new file mode 100644
> index 000000000000..40b90b190af3
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-console
> @@ -0,0 +1,47 @@
> +What:		/sys/class/console/
> +Date:		October 2024

It's no longer October 2024 :(


> +Contact:	Chris Down <chris@chrisdown.name>
> +Description:	Interface for viewing and setting per-console attributes, like
> +		the per-console loglevel. For a high-level document describing
> +		the motivations for this interface and related non-sysfs
> +		controls, see
> +		Documentation/admin-guide/per-console-loglevel.rst.
> +
> +What:		/sys/class/console/<C>/effective_loglevel
> +Date:		October 2024
> +Contact:	Chris Down <chris@chrisdown.name>
> +Description:	Read only. The currently effective loglevel for this console.
> +		All messages emitted with a loglevel below the effective value
> +		will be emitted to the console.
> +
> +What:		/sys/class/console/<C>/effective_loglevel_source
> +Date:		October 2024
> +Contact:	Chris Down <chris@chrisdown.name>
> +Description:	Read only. The currently effective loglevel source for this
> +		console -- for example, whether it was set globally, or whether
> +		it was set locally for this console.
> +
> +		Possible values are:
> +			=============== ============================================
> +			local           The loglevel comes from the per-console
> +			                loglevel.
> +			global          The loglevel comes from the global loglevel.
> +			ignore_loglevel Both the per-console loglevel and global
> +			                loglevels are ignored as ignore_loglevel is
> +			                present on the kernel command line.
> +			=============== ============================================
> +
> +What:		/sys/class/console/<C>/enabled
> +Date:		October 2024
> +Contact:	Chris Down <chris@chrisdown.name>
> +Description:	Read only. "1" if the console is enabled, "0" otherwise.
> +
> +What:		/sys/class/console/<C>/loglevel
> +Date:		October 2024
> +Contact:	Chris Down <chris@chrisdown.name>
> +Description:	Read write. The current per-console loglevel, which will take
> +		effect if not overridden by other non-sysfs controls (see
> +		Documentation/admin-guide/per-console-loglevel.rst). Bounds are
> +		0 (LOGLEVEL_EMERG) to 8 (LOGLEVEL_DEBUG + 1) inclusive. Also
> +		takes the special value "-1" to indicate that no per-console
> +		loglevel is set, and we should defer to the global controls.

-1 is odd, why?  That's going to confuse everyone :(


> diff --git a/Documentation/admin-guide/per-console-loglevel.rst b/Documentation/admin-guide/per-console-loglevel.rst
> index 1ec7608f94b0..41bf3befb2f3 100644
> --- a/Documentation/admin-guide/per-console-loglevel.rst
> +++ b/Documentation/admin-guide/per-console-loglevel.rst
> @@ -68,3 +68,44 @@ further:
>  The default value for ``kernel.console_loglevel`` comes from
>  ``CONFIG_CONSOLE_LOGLEVEL_DEFAULT``, or ``CONFIG_CONSOLE_LOGLEVEL_QUIET`` if
>  ``quiet`` is passed on the kernel command line.
> +
> +Console attributes
> +~~~~~~~~~~~~~~~~~~
> +
> +Registered consoles are exposed at ``/sys/class/console``. For example, if you
> +are using ``ttyS0``, the console backing it can be viewed at
> +``/sys/class/console/ttyS0/``. The following files are available:
> +
> +* ``effective_loglevel`` (r): The effective loglevel after considering all
> +  loglevel authorities. For example, it shows the value of the console-specific
> +  loglevel when a console-specific loglevel is defined, and shows the global
> +  console loglevel value when the console-specific one is not defined.
> +
> +* ``effective_loglevel_source`` (r): The loglevel authority which resulted in
> +  the effective loglevel being set. The following values can be present:
> +
> +    * ``local``: The console-specific loglevel is in effect.
> +
> +    * ``global``: The global loglevel (``kernel.console_loglevel``) is in
> +      effect. Set a console-specific loglevel to override it.
> +
> +    * ``ignore_loglevel``: ``ignore_loglevel`` was specified on the kernel
> +      command line or at ``/sys/module/printk/parameters/ignore_loglevel``.
> +      Disable it to use level controls.
> +
> +* ``enabled`` (r): Whether the console is enabled.
> +
> +* ``loglevel`` (rw): The local, console-specific loglevel for this console.
> +  This will be in effect if no other global control overrides it. Look at
> +  ``effective_loglevel`` and ``effective_loglevel_source`` to verify that.
> +
> +Deprecated
> +~~~~~~~~~~
> +
> +* ``kernel.printk`` sysctl: this takes four values, setting
> +  ``kernel.console_loglevel``, ``kernel.default_message_loglevel``, the minimum
> +  console loglevel, and a fourth unused value. The interface is generally
> +  considered to quite confusing, doesn't perform checks on the values given,
> +  and is unaware of per-console loglevel semantics.
> +
> +Chris Down <chris@chrisdown.name>, 13-October-2024
> diff --git a/Documentation/core-api/printk-basics.rst b/Documentation/core-api/printk-basics.rst
> index 2dde24ca7d9f..bfad359505bb 100644
> --- a/Documentation/core-api/printk-basics.rst
> +++ b/Documentation/core-api/printk-basics.rst
> @@ -54,32 +54,33 @@ string, the log level is not a separate argument). The available log levels are:
>  
>  The log level specifies the importance of a message. The kernel decides whether
>  to show the message immediately (printing it to the current console) depending
> -on its log level and the current *console_loglevel* (a kernel variable). If the
> -message priority is higher (lower log level value) than the *console_loglevel*
> -the message will be printed to the console.
> +on its log level and the current global *console_loglevel* or local per-console
> +loglevel (kernel variables). If the message priority is higher (lower log level
> +value) than the effective loglevel the message will be printed to the console.
>  
>  If the log level is omitted, the message is printed with ``KERN_DEFAULT``
>  level.
>  
> -You can check the current *console_loglevel* with::
> +You can check the current console's loglevel -- for example if you want to
> +check the loglevel for serial consoles:
>  
> -  $ cat /proc/sys/kernel/printk
> -  4        4        1        7
> +  $ cat /sys/class/console/ttyS0/effective_loglevel
> +  6
> +  $ cat /sys/class/console/ttyS0/effective_loglevel_source
> +  local
>  
> -The result shows the *current*, *default*, *minimum* and *boot-time-default* log
> -levels.
> +To change the default loglevel for all consoles, simply write the desired level
> +to ``/proc/sys/kernel/console_loglevel``. For example::
>  
> -To change the current console_loglevel simply write the desired level to
> -``/proc/sys/kernel/printk``. For example, to print all messages to the console::
> +  # echo 5 > /proc/sys/kernel/console_loglevel
>  
> -  # echo 8 > /proc/sys/kernel/printk
> +This sets the console_loglevel to print KERN_WARNING (4) or more severe
> +messages to console. Consoles with a per-console loglevel set will ignore it
> +unless ``ignore_per_console_loglevel`` is set on the kernel command line or at
> +``/sys/module/printk/parameters/ignore_per_console_loglevel``.
>  
> -Another way, using ``dmesg``::
> -
> -  # dmesg -n 5
> -
> -sets the console_loglevel to print KERN_WARNING (4) or more severe messages to
> -console. See ``dmesg(1)`` for more information.
> +For more information on per-console loglevels, see
> +Documentation/admin-guide/per-console-loglevel.rst.
>  
>  As an alternative to printk() you can use the ``pr_*()`` aliases for
>  logging. This family of macros embed the log level in the macro names. For
> diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
> index d55c2a22ec7a..34419e6fe106 100644
> --- a/Documentation/networking/netconsole.rst
> +++ b/Documentation/networking/netconsole.rst
> @@ -72,6 +72,19 @@ Built-in netconsole starts immediately after the TCP stack is
>  initialized and attempts to bring up the supplied dev at the supplied
>  address.
>  
> +You can also set a loglevel at runtime::
> +
> +  $ ls -l /sys/class/console/netcon0/
> +  total 0
> +  lrwxrwxrwx 1 root root    0 May 18 13:28 subsystem -> ../../../../class/console/
> +  -r--r--r-- 1 root root 4096 May 18 13:28 effective_loglevel
> +  -r--r--r-- 1 root root 4096 May 18 13:28 effective_loglevel_source
> +  -r--r--r-- 1 root root 4096 May 18 13:28 enabled
> +  -rw-r--r-- 1 root root 4096 May 18 13:28 loglevel
> +  -rw-r--r-- 1 root root 4096 May 18 13:28 uevent
> +
> +See Documentation/admin-guide/per-console-loglevel.rst for more information.
> +
>  The remote host has several options to receive the kernel messages,
>  for example:
>  
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ea134675669e..003f999e531b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18449,6 +18449,7 @@ R:	John Ogness <john.ogness@linutronix.de>
>  R:	Sergey Senozhatsky <senozhatsky@chromium.org>
>  S:	Maintained
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git
> +F:	Documentation/ABI/testing/sysfs-class-console
>  F:	Documentation/admin-guide/per-console-loglevel.rst
>  F:	Documentation/core-api/printk-basics.rst
>  F:	include/linux/printk.h
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 3ff22bfeef2a..332eef0f95f7 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -16,6 +16,7 @@
>  
>  #include <linux/atomic.h>
>  #include <linux/bits.h>
> +#include <linux/device.h>
>  #include <linux/irq_work.h>
>  #include <linux/rculist.h>
>  #include <linux/rcuwait.h>
> @@ -322,6 +323,7 @@ struct nbcon_write_context {
>   * @data:		Driver private data
>   * @node:		hlist node for the console list
>   * @level:		Console-specific loglevel
> + * @classdev:		Console class device for /sys/class/console
>   *
>   * @nbcon_state:	State for nbcon consoles
>   * @nbcon_seq:		Sequence number of the next record for nbcon to print
> @@ -351,6 +353,7 @@ struct console {
>  	void			*data;
>  	struct hlist_node	node;
>  	int			level;
> +	struct device		*classdev;
>  
>  	/* nbcon console specific members */
>  
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 0053533dcfec..b7e8411e033d 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -77,6 +77,8 @@ extern bool ignore_per_console_loglevel;
>  
>  extern void console_verbose(void);
>  
> +int clamp_loglevel(int level);
> +
>  /* strlen("ratelimit") + 1 */
>  #define DEVKMSG_STR_MAX_SIZE 10
>  extern char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE];
> diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile
> index 39a2b61c7232..3c0b6e2a8083 100644
> --- a/kernel/printk/Makefile
> +++ b/kernel/printk/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -obj-y	= printk.o
> +obj-y	= printk.o sysfs.o

Don't build this in if you don't have to, see below for how to fix that
up (put the #ifdef in the .h file, not the c file...)

>  obj-$(CONFIG_PRINTK)	+= printk_safe.o nbcon.o
>  obj-$(CONFIG_A11Y_BRAILLE_CONSOLE)	+= braille.o
>  obj-$(CONFIG_PRINTK_INDEX)	+= index.o
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index 9303839d0551..ac607d6907a0 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -21,6 +21,11 @@ enum loglevel_source {
>  	LLS_IGNORE_LOGLEVEL,
>  };
>  
> +void console_register_device(struct console *new);
> +void console_setup_class(void);
> +
> +int clamp_loglevel(int level);

Here, put the #ifdef for the non-printk option.

> +
>  enum loglevel_source
>  console_effective_loglevel_source(const struct console *con);
>  int console_effective_loglevel(const struct console *con);
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 055619c5c7e8..bc456f7624d4 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -199,6 +199,12 @@ static int __init control_devkmsg(char *str)
>  }
>  __setup("printk.devkmsg=", control_devkmsg);
>  
> +int clamp_loglevel(int level)
> +{
> +	return clamp(level, minimum_console_loglevel,
> +		     CONSOLE_LOGLEVEL_MOTORMOUTH);
> +}
> +
>  char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE] = "ratelimit";
>  #if defined(CONFIG_PRINTK) && defined(CONFIG_SYSCTL)
>  int devkmsg_sysctl_set_loglvl(const struct ctl_table *table, int write,
> @@ -3894,6 +3900,8 @@ static int try_enable_preferred_console(struct console *newcon,
>  			// TODO: Will be configurable in a later patch
>  			newcon->level = -1;
>  
> +			newcon->classdev = NULL;

Why are you explicitly setting this to NULL here?

> +
>  			if (_braille_register_console(newcon, c))
>  				return 0;
>  
> @@ -4170,6 +4178,7 @@ void register_console(struct console *newcon)
>  	if (use_device_lock)
>  		newcon->device_unlock(newcon, flags);
>  
> +	console_register_device(newcon);
>  	console_sysfs_notify();
>  
>  	/*
> @@ -4264,6 +4273,9 @@ static int unregister_console_locked(struct console *console)
>  	if (console->flags & CON_NBCON)
>  		nbcon_free(console);
>  
> +	if (console->classdev)
> +		device_unregister(console->classdev);

How could this be NULL?

> +
>  	console_sysfs_notify();
>  
>  	if (console->exit)
> @@ -4428,6 +4440,9 @@ static int __init printk_late_init(void)
>  					console_cpu_notify, NULL);
>  	WARN_ON(ret < 0);
>  	printk_sysctl_init();
> +
> +	console_setup_class();
> +
>  	return 0;
>  }
>  late_initcall(printk_late_init);
> diff --git a/kernel/printk/sysfs.c b/kernel/printk/sysfs.c
> new file mode 100644
> index 000000000000..e24590074861
> --- /dev/null
> +++ b/kernel/printk/sysfs.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/kernel.h>
> +#include <linux/console.h>
> +#include <linux/device.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include "internal.h"
> +
> +#ifdef CONFIG_PRINTK

Don't put #ifdef in a .c file, put it in the .h file like we normally do
please.

> +static struct class *console_class;

I swept the kernel tree a while ago trying to move all users of dynamic
classes to be constant, but didn't catch them all.  So we shouldn't add
new ones.

This one should be a const structure that you register with a call to
class_register() instead of dynamically creating it.  As a random
example of how to do this, see commit 9565794b1b01 ("staging: fieldbus:
make controller_class constant").

> +static const char *
> +console_effective_loglevel_source_str(const struct console *con)
> +{
> +	enum loglevel_source source;
> +	const char *str;
> +
> +	source = console_effective_loglevel_source(con);
> +
> +	switch (source) {
> +	case LLS_IGNORE_LOGLEVEL:
> +		str = "ignore_loglevel";
> +		break;
> +	case LLS_LOCAL:
> +		str = "local";
> +		break;
> +	case LLS_GLOBAL:
> +		str = "global";
> +		break;
> +	default:
> +		pr_warn("Unhandled console loglevel source: %d", source);

Why send this to the console if something went wrong with the loglevel
of the console?  :)

> +		str = "unknown";

That should be all that is needed, as this is about to be sent to
userspace.

> +		break;
> +	}
> +
> +	return str;
> +}
> +
> +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", READ_ONCE(con->level));

While I admire the use of READ_ONCE() properly, it really doesn't matter
for sysfs as it could change right afterwards and no one cares.  So no
need for that here, right?


> +}
> +
> +static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t size)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +	ssize_t ret;
> +	int level;
> +
> +	ret = kstrtoint(buf, 10, &level);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (level == -1)
> +		goto out;

As I said above, -1 is an odd thing here, why use it?

> +
> +	if (clamp_loglevel(level) != level)
> +		return -ERANGE;
> +
> +out:
> +	WRITE_ONCE(con->level, level);

Same here, does this matter?

> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR_RW(loglevel);
> +
> +static ssize_t effective_loglevel_source_show(struct device *dev,
> +					      struct device_attribute *attr,
> +					      char *buf)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%s\n",
> +			  console_effective_loglevel_source_str(con));
> +}
> +
> +static DEVICE_ATTR_RO(effective_loglevel_source);
> +
> +static ssize_t effective_loglevel_show(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", console_effective_loglevel(con));
> +}
> +
> +static DEVICE_ATTR_RO(effective_loglevel);
> +
> +static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +	int cookie;
> +	bool enabled;
> +
> +	cookie = console_srcu_read_lock();
> +	enabled = console_srcu_read_flags(con) & CON_ENABLED;
> +	console_srcu_read_unlock(cookie);

As the values can change right after reading, do you really need to
worry about any read locks here?

> +	return sysfs_emit(buf, "%d\n", enabled);
> +}
> +
> +static DEVICE_ATTR_RO(enabled);
> +
> +static struct attribute *console_sysfs_attrs[] = {
> +	&dev_attr_loglevel.attr,
> +	&dev_attr_effective_loglevel_source.attr,
> +	&dev_attr_effective_loglevel.attr,
> +	&dev_attr_enabled.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(console_sysfs);
> +
> +static void console_classdev_release(struct device *dev)
> +{
> +	kfree(dev);
> +}
> +
> +void console_register_device(struct console *con)
> +{
> +	/*
> +	 * We might be called from register_console() before the class is
> +	 * registered. If that happens, we'll take care of it in
> +	 * printk_late_init.
> +	 */
> +	if (IS_ERR_OR_NULL(console_class))

When you change this to be a constant above, this isn't going to be
needed.

> +		return;
> +
> +	if (WARN_ON(con->classdev))
> +		return;

How can this ever happen?

> +
> +	con->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
> +	if (!con->classdev)
> +		return;

No error value returned?

thanks,

greg k-h

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

[John Ogness]

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)

John Ogness

Re: syslog warning: was: Re: [PATCH v6 04/11] printk: Support toggling per-console loglevel via syslog() and cmdline

[Petr Mladek]

On Thu 2024-11-14 13:53:40, Chris Down wrote:
> Petr Mladek writes:
> > I see this warning during every boot because rsyslogd() modifies the
> > global loglevel.
> > 
> > [...]
> > 
> > I am not sure if we have already discussed this in the past.
> > But I would prefer the compromise after all.
> 
> I initially implemented that way until v4, but during the v3 review, as I
> understood it you recommended changing it to use
> !ignore_per_console_loglevel based on the assumption that
> SYSLOG_ACTION_CONSOLE_{ON,OFF} wasn't widely used. Maybe I misunderstood
> what was intended?

The current solution of SYSLOG_ACTION_CONSOLE_{ON,OFF} is fine. I
still hope that it is not used much.

My concern is with SYSLOG_ACTION_CONSOLE_LEVEL. It seems to be used
by rsyslogd.

But wait! I see the warning only on SLE15-SP3 which I use for
testing. It is a pretty old system. I see that rsyslogd is not
longer used on never systems. I guess that it has been obsoleted
by systemd journal.

> Happy to revert to the previous approach with warn_on_local_loglevel(), just
> let me know :-)

No, I take it back ;-) Let's keep it simple as it is done in this patch [v6].
We could always add more conditions around the warning when people
complains.

Best Regards,
Petr

Re: Conflict with FORCE_CON: Re: [PATCH v6 04/11] printk: Support toggling per-console loglevel via syslog() and cmdline

[Petr Mladek]

On Thu 2024-11-14 14:28:22, Chris Down wrote:
> Petr Mladek writes:
> > On Mon 2024-10-28 16:45:40, Chris Down wrote:
> > > A new module parameter (ignore_per_console_loglevel) is added, which can
> > > be set via the kernel command line or at runtime through
> > > /sys/module/printk/parameters/ignore_per_console_loglevel. When set, the
> > > per-console loglevels are ignored, and the global console loglevel
> > > (console_loglevel) is used for all consoles.
> > > 
> > > During sysrq, we temporarily disable per-console loglevels to ensure all
> > > requisite messages are printed to the console. This is necessary because
> > > sysrq is often used in dire circumstances where access to
> > > /sys/class/console may not be trivially possible.
> > 
> > I have just pushed a patchset which removed the console_loglevel
> > manipulation from sysrq, see
> > https://lore.kernel.org/r/20241105-printk-loud-con-v2-0-bd3ecdf7b0e4@suse.com
>   Noted, so would you like me to change this to be based on current
> printk/for-next, or is another branch preferred?

Please, make v7 based on linux-next. It seems that the merge window
for 6.13 opens the following week. So that the other patchset should
reach the mainline in 6.13. The per-console loglevels would need to
wait for 6.14.

Best Regards,
Petr

Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Petr Mladek]

On Fri 2024-11-15 05:20:42, Greg Kroah-Hartman wrote:
> On Mon, Oct 28, 2024 at 04:45:46PM +0000, Chris Down wrote:
> > A sysfs interface under /sys/class/console/ is created that permits
> > viewing and configuring per-console attributes. This is the main
> > interface with which we expect users to interact with and configure
> > per-console loglevels.
> > 
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-console
> > @@ -0,0 +1,47 @@
> > +What:		/sys/class/console/<C>/loglevel
> > +Date:		October 2024
> > +Contact:	Chris Down <chris@chrisdown.name>
> > +Description:	Read write. The current per-console loglevel, which will take
> > +		effect if not overridden by other non-sysfs controls (see
> > +		Documentation/admin-guide/per-console-loglevel.rst). Bounds are
> > +		0 (LOGLEVEL_EMERG) to 8 (LOGLEVEL_DEBUG + 1) inclusive. Also
> > +		takes the special value "-1" to indicate that no per-console
> > +		loglevel is set, and we should defer to the global controls.
> 
> -1 is odd, why?  That's going to confuse everyone :(

IMHO, it is better than (0) because people might think that "0"
disables all messages or allows just "LOGLEVEL_EMERG".

On the other hand, (-1) is being used for default, undefined, or
unknown values in various situations. For example, see

     git grep "define.*(-1[^0-9]" include/linux/

> > --- /dev/null
> > +++ b/kernel/printk/sysfs.c
> > +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
> > +			     char *buf)
> > +{
> > +	struct console *con = dev_get_drvdata(dev);
> > +
> > +	return sysfs_emit(buf, "%d\n", READ_ONCE(con->level));
> 
> While I admire the use of READ_ONCE() properly, it really doesn't matter
> for sysfs as it could change right afterwards and no one cares.  So no
> need for that here, right?

READ_ONCE() prevents compiler optimizations. It makes sure that
the value will be read using a single read operation. It might
be outdated but it will be consistent. I believe that READ_ONCE()
should stay.


> > +}
> > +
> > +static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
> > +			      const char *buf, size_t size)
> > +{
> > +	struct console *con = dev_get_drvdata(dev);
> > +	ssize_t ret;
> > +	int level;
> > +
> > +	ret = kstrtoint(buf, 10, &level);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (level == -1)
> > +		goto out;
> 
> As I said above, -1 is an odd thing here, why use it?
> 
> > +
> > +	if (clamp_loglevel(level) != level)
> > +		return -ERANGE;
> > +
> > +out:
> > +	WRITE_ONCE(con->level, level);
> 
> Same here, does this matter?

Same here. I believe that WRITE_ONCE() should stay to guarantee an atomic write.

> > +	return size;
> > +}
> > +
> > +static DEVICE_ATTR_RW(loglevel);
> > +
> > +static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
> > +			    char *buf)
> > +{
> > +	struct console *con = dev_get_drvdata(dev);
> > +	int cookie;
> > +	bool enabled;
> > +
> > +	cookie = console_srcu_read_lock();
> > +	enabled = console_srcu_read_flags(con) & CON_ENABLED;
> > +	console_srcu_read_unlock(cookie);
> 
> As the values can change right after reading, do you really need to
> worry about any read locks here?

It is true that the related struct console could not disappear
when this sysfs interface exists. Also it is true that
the read_lock does not prevent any race here.

A plain read is OK.

That said, I suggest to remove this sysfs interface anyway because
the CON_ENABLED flag semantic is bogus. See
https://lore.kernel.org/r/ZzTMrFEcYZf58aqj@pathway.suse.cz and
https://lore.kernel.org/r/ZyoNZfLT6tlVAWjO@pathway.suse.cz


> > +	return sysfs_emit(buf, "%d\n", enabled);
> > +}
> > +
> > +static DEVICE_ATTR_RO(enabled);
> > +
> > +static struct attribute *console_sysfs_attrs[] = {
> > +	&dev_attr_loglevel.attr,
> > +	&dev_attr_effective_loglevel_source.attr,
> > +	&dev_attr_effective_loglevel.attr,
> > +	&dev_attr_enabled.attr,
> > +	NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(console_sysfs);
> > +
> > +static void console_classdev_release(struct device *dev)
> > +{
> > +	kfree(dev);
> > +}
> > +
> > +void console_register_device(struct console *con)
> > +{
> > +	/*
> > +	 * We might be called from register_console() before the class is
> > +	 * registered. If that happens, we'll take care of it in
> > +	 * printk_late_init.
> > +	 */
> > +	if (IS_ERR_OR_NULL(console_class))
> 
> When you change this to be a constant above, this isn't going to be
> needed.
> 
> > +		return;
> > +
> > +	if (WARN_ON(con->classdev))
> > +		return;
> 
> How can this ever happen?
> 
> > +
> > +	con->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
> > +	if (!con->classdev)
> > +		return;
>
> No error value returned?

Good question.

IMHO, a missing sysfs interface should not be a fatal error
because it might prevent debugging bugs in the sysfs/kobject APIs.
I mean that an error here should not block register_console().

But it is true that we should not ignore this quietly.
We should print an error message at least.

Another question is why is the struct device allocated dynamically?

I guess that it is a memory optimization because struct console
is static. I am not sure if it is worth it. We could always make
it dynamic when people complain.

Best Regards,
Petr

Re: register_device: was: Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Petr Mladek]

On Wed 2024-11-13 16:59:17, Petr Mladek wrote:
> > A sysfs interface under /sys/class/console/ is created that permits
> > viewing and configuring per-console attributes. This is the main
> > interface with which we expect users to interact with and configure
> > per-console loglevels.
> 
> > diff --git a/kernel/printk/sysfs.c b/kernel/printk/sysfs.c
> > new file mode 100644
> > index 000000000000..e24590074861
> > --- /dev/null
> > +++ b/kernel/printk/sysfs.c
> > +ATTRIBUTE_GROUPS(console_sysfs);
> > +
> > +static void console_classdev_release(struct device *dev)
> > +{
> > +	kfree(dev);
> > +}
> > +
> > +void console_register_device(struct console *con)
> > +{
> > +	/*
> > +	 * We might be called from register_console() before the class is
> > +	 * registered. If that happens, we'll take care of it in
> > +	 * printk_late_init.
> > +	 */
> > +	if (IS_ERR_OR_NULL(console_class))
> > +		return;
> > +
> > +	if (WARN_ON(con->classdev))
> > +		return;
> > +
> > +	con->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
> > +	if (!con->classdev)
> > +		return;
> > +
> > +	device_initialize(con->classdev);
> > +	dev_set_name(con->classdev, "%s%d", con->name, con->index);
> > +	dev_set_drvdata(con->classdev, con);
> > +	con->classdev->release = console_classdev_release;
> > +	con->classdev->class = console_class;
> > +	if (device_add(con->classdev))
> > +		put_device(con->classdev);
> 
> Honestly, I am not sure how to review this. I am not familiar with
> these APIs. I have spent few hours trying to investigate various
> drivers but I did not find any similar use case. I tried to look
> for documentation but I did not find any good HOWTO.

BTW: When investigating other users of these APIs, I saw
a use of pm_runtime_no_callbacks() in

static int i2c_register_adapter(struct i2c_adapter *adap)
{
	dev_set_name(&adap->dev, "i2c-%d", adap->nr);
[...]
	device_initialize(&adap->dev);
[...]
	/*
	 * This adapter can be used as a parent immediately after device_add(),
	 * setup runtime-pm (especially ignore-children) before hand.
	 */
	device_enable_async_suspend(&adap->dev);
	pm_runtime_no_callbacks(&adap->dev);

It removed part of the sysfs interface in the power subdirectory.

It might make sense to use this for the console devices as well.
If I get it correctly then the newly added struct device in
struct console can't affect the power management of the underlying
HW device.

Best Regards,
Petr

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

[Chris Down]

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.

Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Chris Down]

Thanks for looking this over :-) All not mentioned points in this reply are 
acked.

Greg Kroah-Hartman writes:
>> diff --git a/Documentation/ABI/testing/sysfs-class-console b/Documentation/ABI/testing/sysfs-class-console
>> new file mode 100644
>> index 000000000000..40b90b190af3
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-console
>> @@ -0,0 +1,47 @@
>> +What:		/sys/class/console/
>> +Date:		October 2024
>
>It's no longer October 2024 :(

What would you recommend? When I sent them it was, and it doesn't seem 
realistic to think that it's going to be less than one month from me sending 
the patches to when it gets merged, no?

>> +What:		/sys/class/console/<C>/loglevel
>> +Date:		October 2024
>> +Contact:	Chris Down <chris@chrisdown.name>
>> +Description:	Read write. The current per-console loglevel, which will take
>> +		effect if not overridden by other non-sysfs controls (see
>> +		Documentation/admin-guide/per-console-loglevel.rst). Bounds are
>> +		0 (LOGLEVEL_EMERG) to 8 (LOGLEVEL_DEBUG + 1) inclusive. Also
>> +		takes the special value "-1" to indicate that no per-console
>> +		loglevel is set, and we should defer to the global controls.
>
>-1 is odd, why?  That's going to confuse everyone :(

I originally had it that you had to send "unset" instead of -1, but in 
discussion with Petr it was suggested to change it to -1.

Petr, what do you think?

>> +	if (console->classdev)
>> +		device_unregister(console->classdev);
>
>How could this be NULL?

I think it's from an earlier version of the patch where we would still continue 
setup even if we couldn't allocate it. I'm okay removing it.

>> +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
>> +			     char *buf)
>> +{
>> +	struct console *con = dev_get_drvdata(dev);
>> +
>> +	return sysfs_emit(buf, "%d\n", READ_ONCE(con->level));
>
>While I admire the use of READ_ONCE() properly, it really doesn't matter
>for sysfs as it could change right afterwards and no one cares.  So no
>need for that here, right?

I'm not sure I understand, could you clarify? From my reading of the code it 
looks like we need this to avoid tearing.

Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[John Ogness]

On 2024-11-20, Chris Down <chris@chrisdown.name> wrote:
>>> +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
>>> +			     char *buf)
>>> +{
>>> +	struct console *con = dev_get_drvdata(dev);
>>> +
>>> +	return sysfs_emit(buf, "%d\n", READ_ONCE(con->level));
>>
>>While I admire the use of READ_ONCE() properly, it really doesn't matter
>>for sysfs as it could change right afterwards and no one cares.  So no
>>need for that here, right?
>
> From my reading of the code it looks like we need this to avoid
> tearing.

I cannot imagine that any compiler would perform multiple reads to read
an aligned field of 4-bytes. Particularly since this function only reads
this one field.

At most it is kind of annotating lockless access to con->level. But
since it is not using data_race(), it would still trigger KCSAN with a
warning. I recommend removing it.

John Ogness

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

[Petr Mladek]

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

Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Petr Mladek]

On Wed 2024-11-20 05:01:47, Chris Down wrote:
> Thanks for looking this over :-) All not mentioned points in this reply are
> acked.
> 
> Greg Kroah-Hartman writes:
> > > diff --git a/Documentation/ABI/testing/sysfs-class-console b/Documentation/ABI/testing/sysfs-class-console
> > > new file mode 100644
> > > index 000000000000..40b90b190af3
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-class-console
> > > @@ -0,0 +1,47 @@
> > > +What:		/sys/class/console/
> > > +Date:		October 2024
> > 
> > It's no longer October 2024 :(

I am not sure what people do. But I suggest to use whatever is the
actual month. I could update it when pushing the patch.


> What would you recommend? When I sent them it was, and it doesn't seem
> realistic to think that it's going to be less than one month from me sending
> the patches to when it gets merged, no?
> 
> > > +What:		/sys/class/console/<C>/loglevel
> > > +Date:		October 2024
> > > +Contact:	Chris Down <chris@chrisdown.name>
> > > +Description:	Read write. The current per-console loglevel, which will take
> > > +		effect if not overridden by other non-sysfs controls (see
> > > +		Documentation/admin-guide/per-console-loglevel.rst). Bounds are
> > > +		0 (LOGLEVEL_EMERG) to 8 (LOGLEVEL_DEBUG + 1) inclusive. Also
> > > +		takes the special value "-1" to indicate that no per-console
> > > +		loglevel is set, and we should defer to the global controls.
> > 
> > -1 is odd, why?  That's going to confuse everyone :(
> 
> I originally had it that you had to send "unset" instead of -1, but in
> discussion with Petr it was suggested to change it to -1.
>
> Petr, what do you think?

I personally prefer -1. It is a number attribute. And I think that -1
is self explanatory enough.

Best Regards,
Petr

Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Petr Mladek]

On Wed 2024-11-20 09:49:08, John Ogness wrote:
> On 2024-11-20, Chris Down <chris@chrisdown.name> wrote:
> >>> +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
> >>> +			     char *buf)
> >>> +{
> >>> +	struct console *con = dev_get_drvdata(dev);
> >>> +
> >>> +	return sysfs_emit(buf, "%d\n", READ_ONCE(con->level));
> >>
> >>While I admire the use of READ_ONCE() properly, it really doesn't matter
> >>for sysfs as it could change right afterwards and no one cares.  So no
> >>need for that here, right?
> >
> > From my reading of the code it looks like we need this to avoid
> > tearing.
> 
> I cannot imagine that any compiler would perform multiple reads to read
> an aligned field of 4-bytes. Particularly since this function only reads
> this one field.

I believe that the chance is very very small. But are you 100% sure, please?

Honestly, it seems that everyone agrees that the READ_ONCE() makes
some sense. I do not understand why some many people wants to remove
it. I personally prefer to be on the safe side.

> At most it is kind of annotating lockless access to con->level. But
> since it is not using data_race(), it would still trigger KCSAN with a
> warning. I recommend removing it.

I actually suggested to make a wrapper similar to
console_srcu_read_flags() and use the data_race() there, see
https://lore.kernel.org/r/Zy4368zf-sJyyzja@pathway.suse.cz

Best Regards,
Petr

Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[John Ogness]

On 2024-11-20, Petr Mladek <pmladek@suse.com> wrote:
>> +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
>> +			     char *buf)
>> +{
>> +	struct console *con = dev_get_drvdata(dev);
>> +
>> +	return sysfs_emit(buf, "%d\n", READ_ONCE(con->level));
>
> Honestly, it seems that everyone agrees that the READ_ONCE() makes
> some sense. I do not understand why some many people wants to remove
> it. I personally prefer to be on the safe side.

OK, then please also annotate the data race to keep KCSAN happy:

	data_race(READ_ONCE(con->level));

From the data_race kerneldoc:

   If the access must be atomic *and* KCSAN should ignore the access,
   use both data_race() and READ_ONCE(), for example,
   data_race(READ_ONCE(x)).

John

Re: Re: [PATCH v6 09/11] printk: Add sysctl interface to set global loglevels

[Joel Granados]

On Thu, Nov 14, 2024 at 05:21:11PM +0100, Petr Mladek wrote:
> On Mon 2024-10-28 16:45:55, Chris Down wrote:
> > Introduce two new sysctl interfaces for configuring global loglevels:
> > 
> > - kernel.console_loglevel: Sets the global console loglevel, determining
> >   the minimum priority of messages printed to consoles. Messages with a
> >   loglevel lower than this value will be printed.
> > - kernel.default_message_loglevel: Sets the default loglevel for
> >   messages that do not specify an explicit loglevel.
> > 
> > The kernel.printk sysctl was previously used to set multiple loglevel
> > parameters simultaneously, but it was confusing and lacked proper
> > validation. By introducing these dedicated sysctl interfaces, we provide
> > a clearer and more granular way to configure the loglevels.
> > 
> > Signed-off-by: Chris Down <chris@chrisdown.name>
> > ---
> >  Documentation/admin-guide/sysctl/kernel.rst | 17 ++++++++-
> >  kernel/printk/sysctl.c                      | 42 +++++++++++++++++++++
> >  2 files changed, 57 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> > index f8bc1630eba0..8019779b27f6 100644
> > --- a/Documentation/admin-guide/sysctl/kernel.rst
> > +++ b/Documentation/admin-guide/sysctl/kernel.rst
> > @@ -1044,6 +1044,20 @@ otherwise the 'doze' mode will be used.
> >  
> >  ==============================================================
> >  
> > +Some of these settings may be overridden per-console, see
> > +Documentation/admin-guide/per-console-loglevel.rst. See ``man 2 syslog`` for
> > +more information on the different loglevels.
> > +
> > +console_loglevel
> > +================
> > +
> > +Messages with a higher priority than this will be printed to consoles.
> > +
> > +default_message_loglevel
> > +========================
> > +
> > +Messages without an explicit priority will be printed with this priority.
> > +
> >  printk
> >  ======
> >  
> > @@ -1052,8 +1066,7 @@ The four values in printk denote: ``console_loglevel``,
> >  ``default_console_loglevel`` respectively.
> >  
> >  These values influence printk() behavior when printing or
> > -logging error messages. See '``man 2 syslog``' for more info on
> > -the different loglevels.
> > +logging error messages.
> >  
> >  ======================== =====================================
> >  console_loglevel         messages with a higher priority than
> > diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
> > index f5072dc85f7a..3bce8b89dacc 100644
> > --- a/kernel/printk/sysctl.c
> > +++ b/kernel/printk/sysctl.c
> > @@ -11,6 +11,9 @@
> >  
> >  static const int ten_thousand = 10000;
> >  
> > +static int min_msg_loglevel = LOGLEVEL_EMERG;
> > +static int max_msg_loglevel = LOGLEVEL_DEBUG;
> > +
> >  static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int write,
> >  				void *buffer, size_t *lenp, loff_t *ppos)
> >  {
> > @@ -20,6 +23,29 @@ static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int writ
> >  	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> >  }
> >  
> > +static int printk_console_loglevel(const struct ctl_table *table, int write,
> > +				   void *buffer, size_t *lenp, loff_t *ppos)
> 
> I would make it more clear that this function is using the procfs
> based API and call it "proc_dointvec_console_loglevel()".
> 
> > +{
> > +	struct ctl_table ltable = *table;
> > +	int ret, level;
> > +
> > +	if (!write)
> > +		return proc_dointvec(table, write, buffer, lenp, ppos);
> > +
> > +	ltable.data = &level;
> 
> Ah, I have missed that this is a copy and spent quite some time
> wondering why it worked ;-) I remember that the same happened
> last time I saw this trick.
> 
> It would deserve a comment for people like me. Or maybe, rename
> the variable from ltable to table_copy.
> 
> Or I think about another solution, see below.
> 
> > +
> > +	ret = proc_dointvec(&ltable, write, buffer, lenp, ppos);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (level != -1 && level != clamp_loglevel(level))
> > +		return -ERANGE;
> > +
> > +	console_loglevel = level;
> 
> There is no locking. It seems that the original proc_dointvec code
> handle this by using WRITE_ATOMIC(). It prevents compiler
> optimizations. In particular, it makes sure that the entire value
> will be updated in a single operation (atomically).
> 
> > +	return 0;
> > +}
> > +
> 
> I have mixed feelings. On one hand, the copy of the table entry looks
> like a nice trick. On the other hand, I guess that this is the only
> code using such a trick. It might make it more error prone when
The "copy the table" approach is used in sysctl_max_threads. And I'm
working on another for sysrq. In these two cases, it is used because
data is NULL; which is not applicable to your case. Just thought I
mention it as another case for its use.

> some of the API internals change.
> 
> It seems that other users handle similar situations by
> passing a custom @conv callback to do_proc_dointvec(),
> for example, proc_dointvec_minmax(), proc_dointvec_jiffies().
This is correct, But they all exist within kernel/sysctl.c; which we are
trying to make smaller by moving the non-sysctl logic out. I have not
gotten to these functions yet in my "move things away from
kerne/sysctl.c" proces, but I believe that your proposal of exporting
the do_proc_* functions is a good one for the users that are within the
kernel dir.

> 
> This approach would require exporing do_proc_dointvec()
> and do_proc_dointvec_conv(). But there already is a precedent
> when do_proc_douintvec() is used in proc_dopipe_max_size().
> 
> I have tried to implement it to see how it looks. And I would
> prefer to use it. Here is the updated patch:
This seems like a good approach. Is there a new Version of this patch
with this in it?

I arrived a bit late to the review, hope it is still relevant.

Best
> 
> From 05a75d464276da24b7f4b7b97b982041607bbae2 Mon Sep 17 00:00:00 2001
> From: Chris Down <chris@chrisdown.name>
> Date: Mon, 28 Oct 2024 16:45:55 +0000
> Subject: [POC 09/11] printk: Add sysctl interface to set global loglevels
> 
> Introduce two new sysctl interfaces for configuring global loglevels:
> 
> - kernel.console_loglevel: Sets the global console loglevel, determining
>   the minimum priority of messages printed to consoles. Messages with a
>   loglevel lower than this value will be printed.
> - kernel.default_message_loglevel: Sets the default loglevel for
>   messages that do not specify an explicit loglevel.
> 
> The kernel.printk sysctl was previously used to set multiple loglevel
> parameters simultaneously, but it was confusing and lacked proper
> validation. By introducing these dedicated sysctl interfaces, we provide
> a clearer and more granular way to configure the loglevels.
> 
> Signed-off-by: Chris Down <chris@chrisdown.name>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  Documentation/admin-guide/sysctl/kernel.rst | 17 ++++++-
>  include/linux/sysctl.h                      |  7 +++
>  kernel/printk/sysctl.c                      | 51 +++++++++++++++++++++
>  kernel/sysctl.c                             |  4 +-
>  4 files changed, 75 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index f8bc1630eba0..8019779b27f6 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -1044,6 +1044,20 @@ otherwise the 'doze' mode will be used.
>  
>  ==============================================================
>  
> +Some of these settings may be overridden per-console, see
> +Documentation/admin-guide/per-console-loglevel.rst. See ``man 2 syslog`` for
> +more information on the different loglevels.
> +
> +console_loglevel
> +================
> +
> +Messages with a higher priority than this will be printed to consoles.
> +
> +default_message_loglevel
> +========================
> +
> +Messages without an explicit priority will be printed with this priority.
> +
>  printk
>  ======
>  
> @@ -1052,8 +1066,7 @@ The four values in printk denote: ``console_loglevel``,
>  ``default_console_loglevel`` respectively.
>  
>  These values influence printk() behavior when printing or
> -logging error messages. See '``man 2 syslog``' for more info on
> -the different loglevels.
> +logging error messages.
>  
>  ======================== =====================================
>  console_loglevel         messages with a higher priority than
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index aa4c6d44aaa0..a297ca0d4096 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -237,6 +237,13 @@ extern struct ctl_table_header *register_sysctl_mount_point(const char *path);
>  
>  void do_sysctl_args(void);
>  bool sysctl_is_alias(char *param);
> +int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, int *valp,
> +			  int write, void *data);
> +int do_proc_dointvec(const struct ctl_table *table, int write,
> +		     void *buffer, size_t *lenp, loff_t *ppos,
> +		     int (*conv)(bool *negp, unsigned long *lvalp, int *valp,
> +				 int write, void *data),
> +		     void *data);
>  int do_proc_douintvec(const struct ctl_table *table, int write,
>  		      void *buffer, size_t *lenp, loff_t *ppos,
>  		      int (*conv)(unsigned long *lvalp,
> diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
> index f5072dc85f7a..749e3575f2d1 100644
> --- a/kernel/printk/sysctl.c
> +++ b/kernel/printk/sysctl.c
> @@ -11,6 +11,9 @@
>  
>  static const int ten_thousand = 10000;
>  
> +static int min_msg_loglevel = LOGLEVEL_EMERG;
> +static int max_msg_loglevel = LOGLEVEL_DEBUG;
> +
>  static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int write,
>  				void *buffer, size_t *lenp, loff_t *ppos)
>  {
> @@ -20,6 +23,38 @@ static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int writ
>  	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>  }
>  
> +static int do_proc_dointvec_console_loglevel(bool *negp, unsigned long *lvalp,
> +					int *valp,
> +					int write, void *data)
> +{
> +	int level, ret;
> +
> +	/*
> +	 * If writing, first do so via a temporary local int so we can
> +	 * bounds-check it before touching *valp.
> +	 */
> +	int *intp = write ? &level : valp;
> +
> +	ret = do_proc_dointvec_conv(negp, lvalp, intp, write, data);
> +	if (ret)
> +		return ret;
> +
> +	if (write) {
> +		if (level != -1 && level != clamp_loglevel(level))
> +			return -ERANGE;
> +		WRITE_ONCE(*valp, level);
> +	}
> +
> +	return 0;
> +}
> +
> +static int proc_dointvec_console_loglevel(const struct ctl_table *table,
> +		int write, void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	return do_proc_dointvec(table, write, buffer, lenp, ppos,
> +				do_proc_dointvec_console_loglevel, NULL);
> +}
> +
>  static struct ctl_table printk_sysctls[] = {
>  	{
>  		.procname	= "printk",
> @@ -76,6 +111,22 @@ static struct ctl_table printk_sysctls[] = {
>  		.extra1		= SYSCTL_ZERO,
>  		.extra2		= SYSCTL_TWO,
>  	},
> +	{
> +		.procname	= "console_loglevel",
> +		.data		= &console_loglevel,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_console_loglevel,
> +	},
> +	{
> +		.procname	= "default_message_loglevel",
> +		.data		= &default_message_loglevel,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &min_msg_loglevel,
> +		.extra2		= &max_msg_loglevel,
> +	},
>  };
>  
>  void __init printk_sysctl_init(void)
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 79e6cb1d5c48..225ef261d2fb 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -424,7 +424,7 @@ static void proc_put_char(void **buf, size_t *size, char c)
>  	}
>  }
>  
> -static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
> +int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
>  				 int *valp,
>  				 int write, void *data)
>  {
> @@ -541,7 +541,7 @@ static int __do_proc_dointvec(void *tbl_data, const struct ctl_table *table,
>  	return err;
>  }
>  
> -static int do_proc_dointvec(const struct ctl_table *table, int write,
> +int do_proc_dointvec(const struct ctl_table *table, int write,
>  		  void *buffer, size_t *lenp, loff_t *ppos,
>  		  int (*conv)(bool *negp, unsigned long *lvalp, int *valp,
>  			      int write, void *data),
> -- 
> 2.47.0
> 
> 

-- 

Joel Granados

Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Joel Granados]

On Mon, Oct 28, 2024 at 04:45:46PM +0000, Chris Down wrote:
> A sysfs interface under /sys/class/console/ is created that permits
> viewing and configuring per-console attributes. This is the main
> interface with which we expect users to interact with and configure
> per-console loglevels.
These are different from the knobs that you added to sysctl. right? The
ones in sysctl are general values as opposed to console specific.

Would it make sense to put everything in here (sys) instead of having
them separate?

Best
> 
> Each console device now has its own directory (for example,
> /sys/class/console/ttyS0/) containing the following attributes:
> 
> - effective_loglevel (ro): The effective loglevel for the console after
>   considering all loglevel authorities (e.g., global loglevel,
>   per-console loglevel).
> - effective_loglevel_source (ro): The source of the effective loglevel
>   (e.g., local, global, ignore_loglevel).
> - enabled (ro): Indicates whether the console is enabled (1) or disabled
>   (0).
> - loglevel (rw): The per-console loglevel. Writing a value between 0
>   (KERN_EMERG) and 8 (KERN_DEBUG + 1) sets the per-console loglevel.
>   Writing -1 disables the per-console loglevel.
> 
> In terms of technical implementation, we embed a device pointer in the
> console struct, and register each console using it so we can expose
> attributes in sysfs. We currently expose the following attributes:
> 
>     % ls -l /sys/class/console/ttyS0/
>     total 0
>     lrwxrwxrwx 1 root root    0 Oct 23 13:17 subsystem -> ../../../../class/console/
>     -r--r--r-- 1 root root 4096 Oct 23 13:18 effective_loglevel
>     -r--r--r-- 1 root root 4096 Oct 23 13:18 effective_loglevel_source
>     -r--r--r-- 1 root root 4096 Oct 23 13:18 enabled
>     -rw-r--r-- 1 root root 4096 Oct 23 13:18 loglevel
>     -rw-r--r-- 1 root root 4096 Oct 23 13:17 uevent
> 
> The lifecycle of this classdev looks like this on registration:
> 
>     register_console(con)/printk_late_init()
>       console_register_device(con)
>         device_initialize(con->classdev) # refcount++
>         device_add(con->classdev)        # refcount++
> 
> At stable state, the refcount is two.
> 
> Console unregistration looks like this:
> 
>     [con->classdev refcount drops to 0]
>       console_classdev_release(con->classdev)
>         kfree(con->classdev)
> 
>     unregister_console(con)
>       device_unregister(con->classdev)
>         device_del(con->classdev) # refcount--
>           device_remove_class_symlinks()
>             kernfs_remove_by_name_ns()
>               kernfs_drain()
>                 kernfs_drain_open_files() # wait for close()
>         put_device(con->classdev) # refcount--
> 
> Signed-off-by: Chris Down <chris@chrisdown.name>
> ---
>  Documentation/ABI/testing/sysfs-class-console |  47 +++++
>  .../admin-guide/per-console-loglevel.rst      |  41 ++++
>  Documentation/core-api/printk-basics.rst      |  35 ++--
>  Documentation/networking/netconsole.rst       |  13 ++
>  MAINTAINERS                                   |   1 +
>  include/linux/console.h                       |   3 +
>  include/linux/printk.h                        |   2 +
>  kernel/printk/Makefile                        |   2 +-
>  kernel/printk/internal.h                      |   5 +
>  kernel/printk/printk.c                        |  15 ++
>  kernel/printk/sysfs.c                         | 178 ++++++++++++++++++
>  11 files changed, 324 insertions(+), 18 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-console
>  create mode 100644 kernel/printk/sysfs.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-console b/Documentation/ABI/testing/sysfs-class-console
> new file mode 100644
> index 000000000000..40b90b190af3
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-console
> @@ -0,0 +1,47 @@
> +What:		/sys/class/console/
> +Date:		October 2024
> +Contact:	Chris Down <chris@chrisdown.name>
> +Description:	Interface for viewing and setting per-console attributes, like
> +		the per-console loglevel. For a high-level document describing
> +		the motivations for this interface and related non-sysfs
> +		controls, see
> +		Documentation/admin-guide/per-console-loglevel.rst.
> +
> +What:		/sys/class/console/<C>/effective_loglevel
> +Date:		October 2024
> +Contact:	Chris Down <chris@chrisdown.name>
> +Description:	Read only. The currently effective loglevel for this console.
> +		All messages emitted with a loglevel below the effective value
> +		will be emitted to the console.
> +
> +What:		/sys/class/console/<C>/effective_loglevel_source
> +Date:		October 2024
> +Contact:	Chris Down <chris@chrisdown.name>
> +Description:	Read only. The currently effective loglevel source for this
> +		console -- for example, whether it was set globally, or whether
> +		it was set locally for this console.
> +
> +		Possible values are:
> +			=============== ============================================
> +			local           The loglevel comes from the per-console
> +			                loglevel.
> +			global          The loglevel comes from the global loglevel.
> +			ignore_loglevel Both the per-console loglevel and global
> +			                loglevels are ignored as ignore_loglevel is
> +			                present on the kernel command line.
> +			=============== ============================================
> +
> +What:		/sys/class/console/<C>/enabled
> +Date:		October 2024
> +Contact:	Chris Down <chris@chrisdown.name>
> +Description:	Read only. "1" if the console is enabled, "0" otherwise.
> +
> +What:		/sys/class/console/<C>/loglevel
> +Date:		October 2024
> +Contact:	Chris Down <chris@chrisdown.name>
> +Description:	Read write. The current per-console loglevel, which will take
> +		effect if not overridden by other non-sysfs controls (see
> +		Documentation/admin-guide/per-console-loglevel.rst). Bounds are
> +		0 (LOGLEVEL_EMERG) to 8 (LOGLEVEL_DEBUG + 1) inclusive. Also
> +		takes the special value "-1" to indicate that no per-console
> +		loglevel is set, and we should defer to the global controls.
> diff --git a/Documentation/admin-guide/per-console-loglevel.rst b/Documentation/admin-guide/per-console-loglevel.rst
> index 1ec7608f94b0..41bf3befb2f3 100644
> --- a/Documentation/admin-guide/per-console-loglevel.rst
> +++ b/Documentation/admin-guide/per-console-loglevel.rst
> @@ -68,3 +68,44 @@ further:
>  The default value for ``kernel.console_loglevel`` comes from
>  ``CONFIG_CONSOLE_LOGLEVEL_DEFAULT``, or ``CONFIG_CONSOLE_LOGLEVEL_QUIET`` if
>  ``quiet`` is passed on the kernel command line.
> +
> +Console attributes
> +~~~~~~~~~~~~~~~~~~
> +
> +Registered consoles are exposed at ``/sys/class/console``. For example, if you
> +are using ``ttyS0``, the console backing it can be viewed at
> +``/sys/class/console/ttyS0/``. The following files are available:
> +
> +* ``effective_loglevel`` (r): The effective loglevel after considering all
> +  loglevel authorities. For example, it shows the value of the console-specific
> +  loglevel when a console-specific loglevel is defined, and shows the global
> +  console loglevel value when the console-specific one is not defined.
> +
> +* ``effective_loglevel_source`` (r): The loglevel authority which resulted in
> +  the effective loglevel being set. The following values can be present:
> +
> +    * ``local``: The console-specific loglevel is in effect.
> +
> +    * ``global``: The global loglevel (``kernel.console_loglevel``) is in
> +      effect. Set a console-specific loglevel to override it.
> +
> +    * ``ignore_loglevel``: ``ignore_loglevel`` was specified on the kernel
> +      command line or at ``/sys/module/printk/parameters/ignore_loglevel``.
> +      Disable it to use level controls.
> +
> +* ``enabled`` (r): Whether the console is enabled.
> +
> +* ``loglevel`` (rw): The local, console-specific loglevel for this console.
> +  This will be in effect if no other global control overrides it. Look at
> +  ``effective_loglevel`` and ``effective_loglevel_source`` to verify that.
> +
> +Deprecated
> +~~~~~~~~~~
> +
> +* ``kernel.printk`` sysctl: this takes four values, setting
> +  ``kernel.console_loglevel``, ``kernel.default_message_loglevel``, the minimum
> +  console loglevel, and a fourth unused value. The interface is generally
> +  considered to quite confusing, doesn't perform checks on the values given,
> +  and is unaware of per-console loglevel semantics.
> +
> +Chris Down <chris@chrisdown.name>, 13-October-2024
> diff --git a/Documentation/core-api/printk-basics.rst b/Documentation/core-api/printk-basics.rst
> index 2dde24ca7d9f..bfad359505bb 100644
> --- a/Documentation/core-api/printk-basics.rst
> +++ b/Documentation/core-api/printk-basics.rst
> @@ -54,32 +54,33 @@ string, the log level is not a separate argument). The available log levels are:
>  
>  The log level specifies the importance of a message. The kernel decides whether
>  to show the message immediately (printing it to the current console) depending
> -on its log level and the current *console_loglevel* (a kernel variable). If the
> -message priority is higher (lower log level value) than the *console_loglevel*
> -the message will be printed to the console.
> +on its log level and the current global *console_loglevel* or local per-console
> +loglevel (kernel variables). If the message priority is higher (lower log level
> +value) than the effective loglevel the message will be printed to the console.
>  
>  If the log level is omitted, the message is printed with ``KERN_DEFAULT``
>  level.
>  
> -You can check the current *console_loglevel* with::
> +You can check the current console's loglevel -- for example if you want to
> +check the loglevel for serial consoles:
>  
> -  $ cat /proc/sys/kernel/printk
> -  4        4        1        7
> +  $ cat /sys/class/console/ttyS0/effective_loglevel
> +  6
> +  $ cat /sys/class/console/ttyS0/effective_loglevel_source
> +  local
>  
> -The result shows the *current*, *default*, *minimum* and *boot-time-default* log
> -levels.
> +To change the default loglevel for all consoles, simply write the desired level
> +to ``/proc/sys/kernel/console_loglevel``. For example::
>  
> -To change the current console_loglevel simply write the desired level to
> -``/proc/sys/kernel/printk``. For example, to print all messages to the console::
> +  # echo 5 > /proc/sys/kernel/console_loglevel
>  
> -  # echo 8 > /proc/sys/kernel/printk
> +This sets the console_loglevel to print KERN_WARNING (4) or more severe
> +messages to console. Consoles with a per-console loglevel set will ignore it
> +unless ``ignore_per_console_loglevel`` is set on the kernel command line or at
> +``/sys/module/printk/parameters/ignore_per_console_loglevel``.
>  
> -Another way, using ``dmesg``::
> -
> -  # dmesg -n 5
> -
> -sets the console_loglevel to print KERN_WARNING (4) or more severe messages to
> -console. See ``dmesg(1)`` for more information.
> +For more information on per-console loglevels, see
> +Documentation/admin-guide/per-console-loglevel.rst.
>  
>  As an alternative to printk() you can use the ``pr_*()`` aliases for
>  logging. This family of macros embed the log level in the macro names. For
> diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
> index d55c2a22ec7a..34419e6fe106 100644
> --- a/Documentation/networking/netconsole.rst
> +++ b/Documentation/networking/netconsole.rst
> @@ -72,6 +72,19 @@ Built-in netconsole starts immediately after the TCP stack is
>  initialized and attempts to bring up the supplied dev at the supplied
>  address.
>  
> +You can also set a loglevel at runtime::
> +
> +  $ ls -l /sys/class/console/netcon0/
> +  total 0
> +  lrwxrwxrwx 1 root root    0 May 18 13:28 subsystem -> ../../../../class/console/
> +  -r--r--r-- 1 root root 4096 May 18 13:28 effective_loglevel
> +  -r--r--r-- 1 root root 4096 May 18 13:28 effective_loglevel_source
> +  -r--r--r-- 1 root root 4096 May 18 13:28 enabled
> +  -rw-r--r-- 1 root root 4096 May 18 13:28 loglevel
> +  -rw-r--r-- 1 root root 4096 May 18 13:28 uevent
> +
> +See Documentation/admin-guide/per-console-loglevel.rst for more information.
> +
>  The remote host has several options to receive the kernel messages,
>  for example:
>  
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ea134675669e..003f999e531b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18449,6 +18449,7 @@ R:	John Ogness <john.ogness@linutronix.de>
>  R:	Sergey Senozhatsky <senozhatsky@chromium.org>
>  S:	Maintained
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git
> +F:	Documentation/ABI/testing/sysfs-class-console
>  F:	Documentation/admin-guide/per-console-loglevel.rst
>  F:	Documentation/core-api/printk-basics.rst
>  F:	include/linux/printk.h
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 3ff22bfeef2a..332eef0f95f7 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -16,6 +16,7 @@
>  
>  #include <linux/atomic.h>
>  #include <linux/bits.h>
> +#include <linux/device.h>
>  #include <linux/irq_work.h>
>  #include <linux/rculist.h>
>  #include <linux/rcuwait.h>
> @@ -322,6 +323,7 @@ struct nbcon_write_context {
>   * @data:		Driver private data
>   * @node:		hlist node for the console list
>   * @level:		Console-specific loglevel
> + * @classdev:		Console class device for /sys/class/console
>   *
>   * @nbcon_state:	State for nbcon consoles
>   * @nbcon_seq:		Sequence number of the next record for nbcon to print
> @@ -351,6 +353,7 @@ struct console {
>  	void			*data;
>  	struct hlist_node	node;
>  	int			level;
> +	struct device		*classdev;
>  
>  	/* nbcon console specific members */
>  
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 0053533dcfec..b7e8411e033d 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -77,6 +77,8 @@ extern bool ignore_per_console_loglevel;
>  
>  extern void console_verbose(void);
>  
> +int clamp_loglevel(int level);
> +
>  /* strlen("ratelimit") + 1 */
>  #define DEVKMSG_STR_MAX_SIZE 10
>  extern char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE];
> diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile
> index 39a2b61c7232..3c0b6e2a8083 100644
> --- a/kernel/printk/Makefile
> +++ b/kernel/printk/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -obj-y	= printk.o
> +obj-y	= printk.o sysfs.o
>  obj-$(CONFIG_PRINTK)	+= printk_safe.o nbcon.o
>  obj-$(CONFIG_A11Y_BRAILLE_CONSOLE)	+= braille.o
>  obj-$(CONFIG_PRINTK_INDEX)	+= index.o
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index 9303839d0551..ac607d6907a0 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -21,6 +21,11 @@ enum loglevel_source {
>  	LLS_IGNORE_LOGLEVEL,
>  };
>  
> +void console_register_device(struct console *new);
> +void console_setup_class(void);
> +
> +int clamp_loglevel(int level);
> +
>  enum loglevel_source
>  console_effective_loglevel_source(const struct console *con);
>  int console_effective_loglevel(const struct console *con);
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 055619c5c7e8..bc456f7624d4 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -199,6 +199,12 @@ static int __init control_devkmsg(char *str)
>  }
>  __setup("printk.devkmsg=", control_devkmsg);
>  
> +int clamp_loglevel(int level)
> +{
> +	return clamp(level, minimum_console_loglevel,
> +		     CONSOLE_LOGLEVEL_MOTORMOUTH);
> +}
> +
>  char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE] = "ratelimit";
>  #if defined(CONFIG_PRINTK) && defined(CONFIG_SYSCTL)
>  int devkmsg_sysctl_set_loglvl(const struct ctl_table *table, int write,
> @@ -3894,6 +3900,8 @@ static int try_enable_preferred_console(struct console *newcon,
>  			// TODO: Will be configurable in a later patch
>  			newcon->level = -1;
>  
> +			newcon->classdev = NULL;
> +
>  			if (_braille_register_console(newcon, c))
>  				return 0;
>  
> @@ -4170,6 +4178,7 @@ void register_console(struct console *newcon)
>  	if (use_device_lock)
>  		newcon->device_unlock(newcon, flags);
>  
> +	console_register_device(newcon);
>  	console_sysfs_notify();
>  
>  	/*
> @@ -4264,6 +4273,9 @@ static int unregister_console_locked(struct console *console)
>  	if (console->flags & CON_NBCON)
>  		nbcon_free(console);
>  
> +	if (console->classdev)
> +		device_unregister(console->classdev);
> +
>  	console_sysfs_notify();
>  
>  	if (console->exit)
> @@ -4428,6 +4440,9 @@ static int __init printk_late_init(void)
>  					console_cpu_notify, NULL);
>  	WARN_ON(ret < 0);
>  	printk_sysctl_init();
> +
> +	console_setup_class();
> +
>  	return 0;
>  }
>  late_initcall(printk_late_init);
> diff --git a/kernel/printk/sysfs.c b/kernel/printk/sysfs.c
> new file mode 100644
> index 000000000000..e24590074861
> --- /dev/null
> +++ b/kernel/printk/sysfs.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/kernel.h>
> +#include <linux/console.h>
> +#include <linux/device.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include "internal.h"
> +
> +#ifdef CONFIG_PRINTK
> +static struct class *console_class;
> +
> +static const char *
> +console_effective_loglevel_source_str(const struct console *con)
> +{
> +	enum loglevel_source source;
> +	const char *str;
> +
> +	source = console_effective_loglevel_source(con);
> +
> +	switch (source) {
> +	case LLS_IGNORE_LOGLEVEL:
> +		str = "ignore_loglevel";
> +		break;
> +	case LLS_LOCAL:
> +		str = "local";
> +		break;
> +	case LLS_GLOBAL:
> +		str = "global";
> +		break;
> +	default:
> +		pr_warn("Unhandled console loglevel source: %d", source);
> +		str = "unknown";
> +		break;
> +	}
> +
> +	return str;
> +}
> +
> +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", READ_ONCE(con->level));
> +}
> +
> +static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t size)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +	ssize_t ret;
> +	int level;
> +
> +	ret = kstrtoint(buf, 10, &level);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (level == -1)
> +		goto out;
> +
> +	if (clamp_loglevel(level) != level)
> +		return -ERANGE;
> +
> +out:
> +	WRITE_ONCE(con->level, level);
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR_RW(loglevel);
> +
> +static ssize_t effective_loglevel_source_show(struct device *dev,
> +					      struct device_attribute *attr,
> +					      char *buf)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%s\n",
> +			  console_effective_loglevel_source_str(con));
> +}
> +
> +static DEVICE_ATTR_RO(effective_loglevel_source);
> +
> +static ssize_t effective_loglevel_show(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", console_effective_loglevel(con));
> +}
> +
> +static DEVICE_ATTR_RO(effective_loglevel);
> +
> +static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +	int cookie;
> +	bool enabled;
> +
> +	cookie = console_srcu_read_lock();
> +	enabled = console_srcu_read_flags(con) & CON_ENABLED;
> +	console_srcu_read_unlock(cookie);
> +	return sysfs_emit(buf, "%d\n", enabled);
> +}
> +
> +static DEVICE_ATTR_RO(enabled);
> +
> +static struct attribute *console_sysfs_attrs[] = {
> +	&dev_attr_loglevel.attr,
> +	&dev_attr_effective_loglevel_source.attr,
> +	&dev_attr_effective_loglevel.attr,
> +	&dev_attr_enabled.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(console_sysfs);
> +
> +static void console_classdev_release(struct device *dev)
> +{
> +	kfree(dev);
> +}
> +
> +void console_register_device(struct console *con)
> +{
> +	/*
> +	 * We might be called from register_console() before the class is
> +	 * registered. If that happens, we'll take care of it in
> +	 * printk_late_init.
> +	 */
> +	if (IS_ERR_OR_NULL(console_class))
> +		return;
> +
> +	if (WARN_ON(con->classdev))
> +		return;
> +
> +	con->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
> +	if (!con->classdev)
> +		return;
> +
> +	device_initialize(con->classdev);
> +	dev_set_name(con->classdev, "%s%d", con->name, con->index);
> +	dev_set_drvdata(con->classdev, con);
> +	con->classdev->release = console_classdev_release;
> +	con->classdev->class = console_class;
> +	if (device_add(con->classdev))
> +		put_device(con->classdev);
> +}
> +
> +void console_setup_class(void)
> +{
> +	struct console *con;
> +	int cookie;
> +
> +	/*
> +	 * printk exists for the lifetime of the kernel, it cannot be unloaded,
> +	 * so we should never end up back in here.
> +	 */
> +	if (WARN_ON(console_class))
> +		return;
> +
> +	console_class = class_create("console");
> +	if (!IS_ERR(console_class))
> +		console_class->dev_groups = console_sysfs_groups;
> +
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(con)
> +		console_register_device(con);
> +	console_srcu_read_unlock(cookie);
> +}
> +#else /* CONFIG_PRINTK */
> +void console_register_device(struct console *new)
> +{
> +}
> +void console_setup_class(void)
> +{
> +}
> +#endif
> -- 
> 2.46.0
> 

-- 

Joel Granados

Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

[Petr Mladek]

On Fri 2025-01-10 11:27:53, Joel Granados wrote:
> On Mon, Oct 28, 2024 at 04:45:46PM +0000, Chris Down wrote:
> > A sysfs interface under /sys/class/console/ is created that permits
> > viewing and configuring per-console attributes. This is the main
> > interface with which we expect users to interact with and configure
> > per-console loglevels.
> These are different from the knobs that you added to sysctl. right? The
> ones in sysctl are general values as opposed to console specific.
> 
> Would it make sense to put everything in here (sys) instead of having
> them separate?

I thought about this as well. It would be nice to have the setting
in one place.

But I am afraid that it can't be done easily. I think that I looked
this variant and did not find a way how to do it with the existing
API around struct device, /sys/class, ... That said, I am not familiar
with the API. It is quite possible that I just missed it.

One advantage of the /proc/sys/ is that the values might be modified
using the sysctl tool. It might be easier than searching the maze
of directories under /sys. And the global values might be enough
for most users.

Best Regards,
Petr

PS: AFAIK, Chris is going to send an updated version of this patchset.
    He postponed it until the per-console kthreads were introduced...