[PATCH v3 0/2] printk: console: Per-console loglevels

[PATCH v3 0/2] printk: console: Per-console loglevels

[Chris Down]

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 (2):
  printk: console: Create console= parser that supports named options
  printk: console: Support console-specific loglevels

 Documentation/ABI/testing/sysfs-class-console |  43 +++
 .../admin-guide/kernel-parameters.txt         |  28 +-
 .../admin-guide/per-console-loglevel.rst      |  92 ++++++
 Documentation/admin-guide/serial-console.rst  |  17 +-
 Documentation/core-api/printk-basics.rst      |  35 +-
 Documentation/networking/netconsole.rst       |  17 +
 MAINTAINERS                                   |   3 +
 include/linux/console.h                       |  24 ++
 kernel/printk/console_cmdline.h               |   2 +
 kernel/printk/printk.c                        | 311 +++++++++++++++++-
 kernel/printk/sysctl.c                        |  64 +++-
 11 files changed, 599 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-console
 create mode 100644 Documentation/admin-guide/per-console-loglevel.rst


base-commit: 9d882352bac8f2ff3753d691e2dc65fcaf738729
-- 
2.37.1

[PATCH v3 1/2] printk: console: Create console= parser that supports named options

[Chris Down]

This will be used in the next patch, and for future changes like the
proposed sync/nothread named options. This avoids having to use sigils
like "/" to get different semantic meaning for different parts of the
option string, and helps to make things more human readable and
easily extensible.

There are no functional changes for existing console= users.

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 kernel/printk/printk.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b49c6ff6dca0..6094f773ad4a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2368,6 +2368,30 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified)
 	console_set_on_cmdline = 1;
 }
 
+static void parse_console_cmdline_options(struct console_cmdline *c,
+					  char *options)
+{
+	bool seen_serial_opts = false;
+	char *key;
+
+	while ((key = strsep(&options, ",")) != NULL) {
+		char *value;
+
+		value = strchr(key, ':');
+		if (value)
+			*(value++) = '\0';
+
+		if (!seen_serial_opts && isdigit(key[0]) && !value) {
+			seen_serial_opts = true;
+			c->options = key;
+			continue;
+		}
+
+		pr_err("ignoring invalid console option: '%s%s%s'\n", key,
+		       value ? ":" : "", value ?: "");
+	}
+}
+
 static int __add_preferred_console(char *name, int idx, char *options,
 				   char *brl_options, bool user_specified)
 {
@@ -2393,7 +2417,7 @@ static int __add_preferred_console(char *name, int idx, char *options,
 	if (!brl_options)
 		preferred_console = i;
 	strlcpy(c->name, name, sizeof(c->name));
-	c->options = options;
+	parse_console_cmdline_options(c, options);
 	set_user_specified(c, user_specified);
 	braille_set_options(c, brl_options);
 
-- 
2.37.1

[PATCH v3 2/2] printk: console: Support console-specific loglevels

[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. This patch adds 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.

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

    % ls -l /sys/class/console/ttyS/
    total 0
    lrwxrwxrwx 1 root root    0 Jul 20 17:37 subsystem -> ../../../../class/console/
    -r--r--r-- 1 root root 4096 Jul 20 17:38 effective_loglevel
    -r--r--r-- 1 root root 4096 Jul 20 17:38 effective_loglevel_source
    -r--r--r-- 1 root root 4096 Jul 20 17:38 enabled
    -rw-r--r-- 1 root root 4096 Jul 20 17:38 loglevel
    -rw-r--r-- 1 root root 4096 Jul 20 17:37 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--

We also deprecate the kernel.printk sysctl as it doesn't know about
per-console loglevels, and is generally pretty confusing.

For information on the precedence and application of the new controls,
see Documentation/ABI/testing/sysfs-class-console and
Documentation/admin-guide/per-console-loglevel.rst.

Signed-off-by: Chris Down <chris@chrisdown.name>
---
 Documentation/ABI/testing/sysfs-class-console |  43 +++
 .../admin-guide/kernel-parameters.txt         |  28 +-
 .../admin-guide/per-console-loglevel.rst      |  92 ++++++
 Documentation/admin-guide/serial-console.rst  |  17 +-
 Documentation/core-api/printk-basics.rst      |  35 +--
 Documentation/networking/netconsole.rst       |  17 ++
 MAINTAINERS                                   |   3 +
 include/linux/console.h                       |  24 ++
 kernel/printk/console_cmdline.h               |   2 +
 kernel/printk/printk.c                        | 285 +++++++++++++++++-
 kernel/printk/sysctl.c                        |  64 +++-
 11 files changed, 574 insertions(+), 36 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-console
 create mode 100644 Documentation/admin-guide/per-console-loglevel.rst

diff --git a/Documentation/ABI/testing/sysfs-class-console b/Documentation/ABI/testing/sysfs-class-console
new file mode 100644
index 000000000000..4abbde9b57e0
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-console
@@ -0,0 +1,43 @@
+What:		/sys/class/console/
+Date:		May 2022
+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:		May 2022
+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:		May 2022
+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:		May 2022
+Contact:	Chris Down <chris@chrisdown.name>
+Description:	Read only. "1" if the console is enabled, "0" otherwise.
+
+What:		/sys/class/console/<C>/loglevel
+Date:		May 2022
+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 "unset" to indicate that no per-console
+		loglevel is set, and we should defer to the global controls.
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2522b11e593f..a8ef6d0b7c1d 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -702,13 +702,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.
 
@@ -2806,10 +2811,13 @@
 	logibm.irq=	[HW,MOUSE] Logitech Bus Mouse Driver
 			Format: <irq>
 
-	loglevel=	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:
+	loglevel=	Sets the global loglevel. All messages with a loglevel
+			smaller than the console loglevel will be printed to
+			the console. 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/per-console-loglevel.rst b/Documentation/admin-guide/per-console-loglevel.rst
new file mode 100644
index 000000000000..11ffdc53c8e8
--- /dev/null
+++ b/Documentation/admin-guide/per-console-loglevel.rst
@@ -0,0 +1,92 @@
+.. 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.
+
+Tunables
+--------
+
+In order to allow tuning this, 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.
+
+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/ttyS/``. The following files are available:
+
+* ``effective_loglevel`` (r): The effective loglevel after considering all
+  loglevel authorities. For example, if the console-specific loglevel is 3, but
+  the global minimum console loglevel[*]_ is 5, then the value will be 5.
+* ``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.
+    * ``ignore_per_console_loglevel``: ``ignore_per_console_loglevel`` was
+      specified on the kernel command line or at
+      ``/sys/module/printk/parameters/ignore_per_console_loglevel``. Disable it
+      to use per-console 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.
+
+.. [*] The existence of a minimum console loglevel is generally considered to
+   be a confusing and rarely used interface, and as such is not exposed through
+   the modern printk sysctl APIs that obsoleted ``kernel.printk``. Use the
+   legacy ``kernel.printk`` sysctl to control it if you have a rare use case
+   that requires changing it. The default value is ``CONSOLE_LOGLEVEL_MIN``.
+
+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>, 17-May-2022
diff --git a/Documentation/admin-guide/serial-console.rst b/Documentation/admin-guide/serial-console.rst
index 58b32832e50a..794c1a51497b 100644
--- a/Documentation/admin-guide/serial-console.rst
+++ b/Documentation/admin-guide/serial-console.rst
@@ -32,15 +32,25 @@ 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.
+
 You can specify multiple console= options on the kernel command line.
 Output will appear on all of them. 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).
 
 Note that you can only define one console per device type (serial, video).
 
@@ -113,3 +123,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>, 17-May-2022
diff --git a/Documentation/core-api/printk-basics.rst b/Documentation/core-api/printk-basics.rst
index 2dde24ca7d9f..44a4e62558b8 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/ttyS/effective_loglevel
+  6
+  $ cat /sys/class/console/ttyS/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 1f5c4a04027c..fd566840b429 100644
--- a/Documentation/networking/netconsole.rst
+++ b/Documentation/networking/netconsole.rst
@@ -67,6 +67,23 @@ 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 boot time on the kernel command line::
+
+  console=netcon0,loglevel:2
+
+This can also be changed at runtime::
+
+  $ ls -l /sys/class/console/netcon/
+  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 6cc825857722..e51e25ec3cdf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16044,6 +16044,9 @@ R:	Steven Rostedt <rostedt@goodmis.org>
 R:	John Ogness <john.ogness@linutronix.de>
 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
 F:	kernel/printk/
 
diff --git a/include/linux/console.h b/include/linux/console.h
index 8c1686e2c233..4ed9bde63139 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -15,6 +15,7 @@
 #define _LINUX_CONSOLE_H_ 1
 
 #include <linux/atomic.h>
+#include <linux/device.h>
 #include <linux/types.h>
 
 struct vc_data;
@@ -137,6 +138,22 @@ static inline int con_debug_leave(void)
 #define CON_BRL		(32) /* Used for a braille device */
 #define CON_EXTENDED	(64) /* Use the extended output format a la /dev/kmsg */
 
+/*
+ * The loglevel for a console can be set in many places:
+ *
+ * 1. It can be forced to emit everything (ignore_loglevel);
+ * 2. It can be set globally (sysctls kernel.printk (deprecated),
+ *    kernel.console_loglevel, magic sysrq, loglevel= on kernel command line);
+ * 3. It can be locally set for this specific console (console=...,loglevel:N on
+ *    kernel command line, /sys/class/console/.../loglevel);
+ * 4. It can be set by a compile-time default
+ *    (CONFIG_CONSOLE_LOGLEVEL_{DEFAULT,QUIET})
+ *
+ * If case 3 happens, even if another global value in effect, CON_LOGLEVEL will
+ * be set.
+ */
+#define CON_LOGLEVEL	(128) /* Level set locally for this console */
+
 struct console {
 	char	name[16];
 	void	(*write)(struct console *, const char *, unsigned);
@@ -155,8 +172,15 @@ struct console {
 	unsigned long dropped;
 	void	*data;
 	struct	 console *next;
+	int	level;
+	struct	device *classdev;
 };
 
+static inline struct console *classdev_to_console(struct device *dev)
+{
+	return dev_get_drvdata(dev);
+}
+
 /*
  * for_each_console() allows you to iterate on each console
  */
diff --git a/kernel/printk/console_cmdline.h b/kernel/printk/console_cmdline.h
index 3ca74ad391d6..40f1a1ff0965 100644
--- a/kernel/printk/console_cmdline.h
+++ b/kernel/printk/console_cmdline.h
@@ -6,6 +6,8 @@ struct console_cmdline
 {
 	char	name[16];			/* Name of the driver	    */
 	int	index;				/* Minor dev. to use	    */
+	int	level;				/* Log level to use */
+	short	flags;				/* Initial flags */
 	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 6094f773ad4a..6f5e29b60875 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -44,6 +44,7 @@
 #include <linux/irq_work.h>
 #include <linux/ctype.h>
 #include <linux/uio.h>
+#include <linux/device.h>
 #include <linux/sched/clock.h>
 #include <linux/sched/debug.h>
 #include <linux/sched/task_stack.h>
@@ -385,6 +386,20 @@ static struct latched_seq clear_seq = {
 	.val[1]		= 0,
 };
 
+static struct class *console_class;
+
+enum loglevel_source {
+	LLS_GLOBAL,
+	LLS_LOCAL,
+	LLS_IGNORE_LOGLEVEL,
+};
+
+static const char *const loglevel_source_names[] = {
+	[LLS_GLOBAL] = "global",
+	[LLS_LOCAL] = "local",
+	[LLS_IGNORE_LOGLEVEL] = "ignore_loglevel",
+};
+
 #ifdef CONFIG_PRINTK_CALLER
 #define PREFIX_MAX		48
 #else
@@ -1202,9 +1217,72 @@ 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)
+static bool __read_mostly ignore_per_console_loglevel;
+
+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)");
+
+/*
+ * Hierarchy of loglevel authority:
+ *
+ * 1. con->level. The locally set, console-specific loglevel. Optional, only
+ *    valid if the CON_LOGLEVEL flag is set.
+ * 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.
+ *
+ * Callers typically only need the level _or_ the source, but they're both
+ * emitted from this function so that the effective loglevel logic can be
+ * kept in one place.
+ */
+static int console_effective_loglevel(const struct console *con,
+				      enum loglevel_source *source)
+{
+	enum loglevel_source lsource;
+	int level;
+
+	if (ignore_loglevel) {
+		lsource = LLS_IGNORE_LOGLEVEL;
+		level = CONSOLE_LOGLEVEL_MOTORMOUTH;
+		goto out;
+	}
+
+	if (!ignore_per_console_loglevel &&
+	    (con && (con->flags & CON_LOGLEVEL))) {
+		lsource = LLS_LOCAL;
+		level = con->level;
+		goto out;
+	}
+
+	lsource = LLS_GLOBAL;
+	level = console_loglevel;
+
+out:
+	*source = lsource;
+	return level;
+}
+
+static bool suppress_message_printing(int level, struct console *con)
 {
-	return (level >= console_loglevel && !ignore_loglevel);
+	enum loglevel_source source;
+
+	return level >= console_effective_loglevel(con, &source);
 }
 
 #ifdef CONFIG_BOOT_PRINTK_DELAY
@@ -1236,7 +1314,7 @@ static void boot_delay_msec(int level)
 	unsigned long timeout;
 
 	if ((boot_delay == 0 || system_state >= SYSTEM_RUNNING)
-		|| suppress_message_printing(level)) {
+		|| suppress_message_printing(level, NULL)) {
 		return;
 	}
 
@@ -1656,6 +1734,33 @@ static void syslog_clear(void)
 	mutex_unlock(&syslog_lock);
 }
 
+/*
+ * Using the global klogctl/syslog API is unlikely to do what you want if you
+ * also have console specific loglevels. Warn about it.
+ */
+static void warn_on_local_loglevel(void)
+{
+	static bool warned;
+	struct console *con;
+
+	if (warned)
+		return;
+
+	if (ignore_per_console_loglevel)
+		return;
+
+	console_lock();
+	for_each_console(con) {
+		if (con->flags & CON_LOGLEVEL) {
+			warned = true;
+			pr_warn("%s (%d) used syslog(SYSLOG_ACTION_CONSOLE_*) with per-console loglevels set. Consoles with per-console loglevels will ignore the updated value.\n",
+				current->comm, current->pid);
+			break;
+		}
+	}
+	console_unlock();
+}
+
 int do_syslog(int type, char __user *buf, int len, int source)
 {
 	struct printk_info info;
@@ -1701,12 +1806,14 @@ int do_syslog(int type, char __user *buf, int len, int source)
 		break;
 	/* Disable logging to console */
 	case SYSLOG_ACTION_CONSOLE_OFF:
+		warn_on_local_loglevel();
 		if (saved_console_loglevel == LOGLEVEL_DEFAULT)
 			saved_console_loglevel = console_loglevel;
 		console_loglevel = minimum_console_loglevel;
 		break;
 	/* Enable logging to console */
 	case SYSLOG_ACTION_CONSOLE_ON:
+		warn_on_local_loglevel();
 		if (saved_console_loglevel != LOGLEVEL_DEFAULT) {
 			console_loglevel = saved_console_loglevel;
 			saved_console_loglevel = LOGLEVEL_DEFAULT;
@@ -1714,6 +1821,7 @@ int do_syslog(int type, char __user *buf, int len, int source)
 		break;
 	/* Set level of messages printed to console */
 	case SYSLOG_ACTION_CONSOLE_LEVEL:
+		warn_on_local_loglevel();
 		if (len < 1 || len > 8)
 			return -EINVAL;
 		if (len < minimum_console_loglevel)
@@ -2329,7 +2437,10 @@ static void call_console_driver(struct console *con, const char *text, size_t le
 				char *dropped_text)
 {
 }
-static bool suppress_message_printing(int level) { return false; }
+static bool suppress_message_printing(int level, struct console *con)
+{
+	return false;
+}
 static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress) { return true; }
 
 #endif /* CONFIG_PRINTK */
@@ -2381,6 +2492,14 @@ static void parse_console_cmdline_options(struct console_cmdline *c,
 		if (value)
 			*(value++) = '\0';
 
+		if (strcmp(key, "loglevel") == 0 && value &&
+		    isdigit(value[0]) && strlen(value) == 1) {
+			c->level = clamp(value[0] - '0', LOGLEVEL_EMERG,
+					 LOGLEVEL_DEBUG + 1);
+			c->flags |= CON_LOGLEVEL;
+			continue;
+		}
+
 		if (!seen_serial_opts && isdigit(key[0]) && !value) {
 			seen_serial_opts = true;
 			c->options = key;
@@ -2724,7 +2843,7 @@ static bool console_emit_next_record(struct console *con, char *text, char *ext_
 	}
 
 	/* Skip record that has level above the console loglevel. */
-	if (suppress_message_printing(r.info->level)) {
+	if (suppress_message_printing(r.info->level, con)) {
 		con->seq++;
 		goto skip;
 	}
@@ -3030,6 +3149,145 @@ static int __init keep_bootcon_setup(char *str)
 
 early_param("keep_bootcon", keep_bootcon_setup);
 
+#ifdef CONFIG_PRINTK
+static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct console *con = classdev_to_console(dev);
+
+	if (con->flags & CON_LOGLEVEL)
+		return sysfs_emit(buf, "%d\n", con->level);
+	else
+		return sysfs_emit(buf, "unset\n");
+}
+
+static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct console *con = classdev_to_console(dev);
+	ssize_t ret;
+	int tmp;
+
+	if (!strcmp(buf, "unset") || !strcmp(buf, "unset\n")) {
+		con->flags &= ~CON_LOGLEVEL;
+		return size;
+	}
+
+	ret = kstrtoint(buf, 10, &tmp);
+	if (ret < 0)
+		return ret;
+
+	if (tmp < LOGLEVEL_EMERG || tmp > LOGLEVEL_DEBUG + 1)
+		return -ERANGE;
+
+	if (tmp < minimum_console_loglevel)
+		return -EINVAL;
+
+	con->level = tmp;
+	con->flags |= CON_LOGLEVEL;
+
+	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 = classdev_to_console(dev);
+	enum loglevel_source source;
+
+	console_effective_loglevel(con, &source);
+	return sysfs_emit(buf, "%s\n", loglevel_source_names[source]);
+}
+
+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 = classdev_to_console(dev);
+	enum loglevel_source source;
+
+	return sysfs_emit(buf, "%d\n",
+			  console_effective_loglevel(con, &source));
+}
+
+static DEVICE_ATTR_RO(effective_loglevel);
+
+static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	struct console *con = classdev_to_console(dev);
+
+	return sysfs_emit(buf, "%d\n", !!(con->flags & CON_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);
+}
+
+static void console_register_device(struct console *new)
+{
+	/*
+	 * 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;
+
+	new->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
+	if (!new->classdev)
+		return;
+
+	device_initialize(new->classdev);
+	dev_set_name(new->classdev, "%s", new->name);
+	dev_set_drvdata(new->classdev, new);
+	new->classdev->release = console_classdev_release;
+	new->classdev->class = console_class;
+	if (device_add(new->classdev))
+		put_device(new->classdev);
+}
+
+static void console_setup_class(void)
+{
+	struct console *con;
+
+	/*
+	 * 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(THIS_MODULE, "console");
+	if (!IS_ERR(console_class))
+		console_class->dev_groups = console_sysfs_groups;
+
+	for_each_console(con)
+		console_register_device(con);
+}
+#else /* CONFIG_PRINTK */
+static void console_register_device(struct console *new) {}
+static void console_setup_class(void) {}
+#endif
+
 /*
  * This is called by register_console() to try to match
  * the newly registered console with any of the ones selected
@@ -3062,6 +3320,11 @@ static int try_enable_preferred_console(struct console *newcon,
 			if (newcon->index < 0)
 				newcon->index = c->index;
 
+			if (c->flags & CON_LOGLEVEL)
+				newcon->level = c->level;
+			newcon->flags |= c->flags;
+			newcon->classdev = NULL;
+
 			if (_braille_register_console(newcon, c))
 				return 0;
 
@@ -3223,6 +3486,7 @@ void register_console(struct console *newcon)
 		/* Begin with next message. */
 		newcon->seq = prb_next_seq(prb);
 	}
+	console_register_device(newcon);
 	console_unlock();
 	console_sysfs_notify();
 
@@ -3289,6 +3553,10 @@ int unregister_console(struct console *console)
 		console_drivers->flags |= CON_CONSDEV;
 
 	console->flags &= ~CON_ENABLED;
+
+	if (console->classdev)
+		device_unregister(console->classdev);
+
 	console_unlock();
 	console_sysfs_notify();
 
@@ -3348,6 +3616,10 @@ void __init console_init(void)
  * To mitigate this problem somewhat, only unregister consoles whose memory
  * intersects with the init section. Note that all other boot consoles will
  * get unregistered when the real preferred console is registered.
+ *
+ * Early consoles will also have been registered before we had the
+ * infrastructure to put them into /sys/class/console, so make sure they get
+ * set up now that we're ready.
  */
 static int __init printk_late_init(void)
 {
@@ -3381,6 +3653,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/sysctl.c b/kernel/printk/sysctl.c
index c228343eeb97..97689d728d3e 100644
--- a/kernel/printk/sysctl.c
+++ b/kernel/printk/sysctl.c
@@ -7,10 +7,14 @@
 #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;
 
+static int min_loglevel = LOGLEVEL_EMERG;
+static int max_loglevel = LOGLEVEL_DEBUG + 1;
+
 static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
 				void *buffer, size_t *lenp, loff_t *ppos)
 {
@@ -20,13 +24,55 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
 	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 }
 
+static int printk_sysctl_deprecated(struct ctl_table *table, int write,
+				    void __user *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 int printk_console_loglevel(struct ctl_table *table, int write,
+				   void __user *buffer, size_t *lenp,
+				   loff_t *ppos)
+{
+
+	struct ctl_table ltable = *table;
+	int ret, value;
+
+	if (!write)
+		return proc_dointvec(table, write, buffer, lenp, ppos);
+
+	ltable.data = &value;
+
+	ret = proc_dointvec(&ltable, write, buffer, lenp, ppos);
+	if (ret)
+		return ret;
+
+	if (value < min_loglevel || value > max_loglevel)
+		return -ERANGE;
+
+	if (value < minimum_console_loglevel)
+		return -EINVAL;
+
+	console_loglevel = value;
+
+	return 0;
+}
+
 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",
@@ -76,6 +122,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_loglevel,
+		.extra2		= &max_loglevel,
+	},
 	{}
 };
 
-- 
2.37.1

Re: [PATCH v3 0/2] printk: console: Per-console loglevels

[John Ogness]

On 2022-07-20, Chris Down <chris@chrisdown.name> wrote:
> v3:
>
> - Update to work with John's kthread patches

This will get a bit tricky, since I am also preparing a new kthread
series. But for now it is helpful to base your work on the previous
kthread work.

Thanks for your efforts on this!

John

Re: [PATCH v3 0/2] printk: console: Per-console loglevels

[Chris Down]

John Ogness writes:
>On 2022-07-20, Chris Down <chris@chrisdown.name> wrote:
>> v3:
>>
>> - Update to work with John's kthread patches
>
>This will get a bit tricky, since I am also preparing a new kthread
>series. But for now it is helpful to base your work on the previous
>kthread work.

No worries -- I don't think it should get too bad. If I recall correctly, the 
only real changes were around handling suppress_message_printing in the new 
model, and they were fairly minor.

>Thanks for your efforts on this!

Likewise on the kthread work! Just let me know if/when you want me to rebase. 
:-)

Re: [PATCH v3 2/2] printk: console: Support console-specific loglevels

[kernel test robot]

Hi Chris,

I love your patch! Perhaps something to improve:

[auto build test WARNING on 9d882352bac8f2ff3753d691e2dc65fcaf738729]

url:    https://github.com/intel-lab-lkp/linux/commits/Chris-Down/printk-console-Per-console-loglevels/20220721-015315
base:   9d882352bac8f2ff3753d691e2dc65fcaf738729
reproduce: make htmldocs

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> Documentation/ABI/testing/sysfs-class-console:18: WARNING: Unexpected indentation.
>> Documentation/admin-guide/per-console-loglevel.rst: WARNING: document isn't included in any toctree

vim +18 Documentation/ABI/testing/sysfs-class-console

  > 18	Date:		May 2022
    19	Contact:	Chris Down <chris@chrisdown.name>
    20	Description:	Read only. The currently effective loglevel source for this
    21			console -- for example, whether it was set globally, or whether
    22			it was set locally for this console. Possible values are:
    23	
    24			local: The loglevel comes from the per-console loglevel.
    25			global: The loglevel comes from the global loglevel.
    26			ignore_loglevel: Both the per-console loglevel and global
    27					 loglevels are ignored as ignore_loglevel is
    28					 present on the kernel command line.
    29	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v3 2/2] printk: console: Support console-specific loglevels

[kernel test robot]

Hi Chris,

I love your patch! Perhaps something to improve:

[auto build test WARNING on 9d882352bac8f2ff3753d691e2dc65fcaf738729]

url:    https://github.com/intel-lab-lkp/linux/commits/Chris-Down/printk-console-Per-console-loglevels/20220721-015315
base:   9d882352bac8f2ff3753d691e2dc65fcaf738729
config: riscv-randconfig-s053-20220718 (https://download.01.org/0day-ci/archive/20220722/202207220029.VYxE2uAL-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/fac1dc8424bd6e1ae37f6e180f96ed7e4f44e2fc
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Chris-Down/printk-console-Per-console-loglevels/20220721-015315
        git checkout fac1dc8424bd6e1ae37f6e180f96ed7e4f44e2fc
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=riscv SHELL=/bin/bash kernel/printk/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

sparse warnings: (new ones prefixed by >>)
>> kernel/printk/sysctl.c:31:47: sparse: sparse: incorrect type in argument 3 (different address spaces) @@     expected void * @@     got void [noderef] __user *buffer @@
   kernel/printk/sysctl.c:31:47: sparse:     expected void *
   kernel/printk/sysctl.c:31:47: sparse:     got void [noderef] __user *buffer
   kernel/printk/sysctl.c:50:52: sparse: sparse: incorrect type in argument 3 (different address spaces) @@     expected void * @@     got void [noderef] __user *buffer @@
   kernel/printk/sysctl.c:50:52: sparse:     expected void *
   kernel/printk/sysctl.c:50:52: sparse:     got void [noderef] __user *buffer
   kernel/printk/sysctl.c:54:45: sparse: sparse: incorrect type in argument 3 (different address spaces) @@     expected void * @@     got void [noderef] __user *buffer @@
   kernel/printk/sysctl.c:54:45: sparse:     expected void *
   kernel/printk/sysctl.c:54:45: sparse:     got void [noderef] __user *buffer
>> kernel/printk/sysctl.c:75:35: sparse: sparse: incorrect type in initializer (incompatible argument 3 (different address spaces)) @@     expected int ( [usertype] *proc_handler )( ... ) @@     got int ( * )( ... ) @@
   kernel/printk/sysctl.c:75:35: sparse:     expected int ( [usertype] *proc_handler )( ... )
   kernel/printk/sysctl.c:75:35: sparse:     got int ( * )( ... )
   kernel/printk/sysctl.c:130:35: sparse: sparse: incorrect type in initializer (incompatible argument 3 (different address spaces)) @@     expected int ( [usertype] *proc_handler )( ... ) @@     got int ( * )( ... ) @@
   kernel/printk/sysctl.c:130:35: sparse:     expected int ( [usertype] *proc_handler )( ... )
   kernel/printk/sysctl.c:130:35: sparse:     got int ( * )( ... )

vim +31 kernel/printk/sysctl.c

    26	
    27	static int printk_sysctl_deprecated(struct ctl_table *table, int write,
    28					    void __user *buffer, size_t *lenp,
    29					    loff_t *ppos)
    30	{
  > 31		int res = proc_dointvec(table, write, buffer, lenp, ppos);
    32	
    33		if (write)
    34			pr_warn_once(
    35				"printk: The kernel.printk sysctl is deprecated. Consider using kernel.console_loglevel or kernel.default_message_loglevel instead.\n"
    36			);
    37	
    38		return res;
    39	}
    40	
    41	static int printk_console_loglevel(struct ctl_table *table, int write,
    42					   void __user *buffer, size_t *lenp,
    43					   loff_t *ppos)
    44	{
    45	
    46		struct ctl_table ltable = *table;
    47		int ret, value;
    48	
    49		if (!write)
    50			return proc_dointvec(table, write, buffer, lenp, ppos);
    51	
    52		ltable.data = &value;
    53	
    54		ret = proc_dointvec(&ltable, write, buffer, lenp, ppos);
    55		if (ret)
    56			return ret;
    57	
    58		if (value < min_loglevel || value > max_loglevel)
    59			return -ERANGE;
    60	
    61		if (value < minimum_console_loglevel)
    62			return -EINVAL;
    63	
    64		console_loglevel = value;
    65	
    66		return 0;
    67	}
    68	
    69	static struct ctl_table printk_sysctls[] = {
    70		{
    71			.procname	= "printk",
    72			.data		= &console_loglevel,
    73			.maxlen		= 4*sizeof(int),
    74			.mode		= 0644,
  > 75			.proc_handler	= printk_sysctl_deprecated,
    76		},
    77		{
    78			.procname	= "printk_ratelimit",
    79			.data		= &printk_ratelimit_state.interval,
    80			.maxlen		= sizeof(int),
    81			.mode		= 0644,
    82			.proc_handler	= proc_dointvec_jiffies,
    83		},
    84		{
    85			.procname	= "printk_ratelimit_burst",
    86			.data		= &printk_ratelimit_state.burst,
    87			.maxlen		= sizeof(int),
    88			.mode		= 0644,
    89			.proc_handler	= proc_dointvec,
    90		},
    91		{
    92			.procname	= "printk_delay",
    93			.data		= &printk_delay_msec,
    94			.maxlen		= sizeof(int),
    95			.mode		= 0644,
    96			.proc_handler	= proc_dointvec_minmax,
    97			.extra1		= SYSCTL_ZERO,
    98			.extra2		= (void *)&ten_thousand,
    99		},
   100		{
   101			.procname	= "printk_devkmsg",
   102			.data		= devkmsg_log_str,
   103			.maxlen		= DEVKMSG_STR_MAX_SIZE,
   104			.mode		= 0644,
   105			.proc_handler	= devkmsg_sysctl_set_loglvl,
   106		},
   107		{
   108			.procname	= "dmesg_restrict",
   109			.data		= &dmesg_restrict,
   110			.maxlen		= sizeof(int),
   111			.mode		= 0644,
   112			.proc_handler	= proc_dointvec_minmax_sysadmin,
   113			.extra1		= SYSCTL_ZERO,
   114			.extra2		= SYSCTL_ONE,
   115		},
   116		{
   117			.procname	= "kptr_restrict",
   118			.data		= &kptr_restrict,
   119			.maxlen		= sizeof(int),
   120			.mode		= 0644,
   121			.proc_handler	= proc_dointvec_minmax_sysadmin,
   122			.extra1		= SYSCTL_ZERO,
   123			.extra2		= SYSCTL_TWO,
   124		},
   125		{
   126			.procname	= "console_loglevel",
   127			.data		= &console_loglevel,
   128			.maxlen		= sizeof(int),
   129			.mode		= 0644,
   130			.proc_handler	= printk_console_loglevel,
   131		},
   132		{
   133			.procname	= "default_message_loglevel",
   134			.data		= &default_message_loglevel,
   135			.maxlen		= sizeof(int),
   136			.mode		= 0644,
   137			.proc_handler	= proc_dointvec_minmax,
   138			.extra1		= &min_loglevel,
   139			.extra2		= &max_loglevel,
   140		},
   141		{}
   142	};
   143	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v3 2/2] printk: console: Support console-specific loglevels

[Chris Down]

Will fix for v4.

Re: [PATCH v3 2/2] printk: console: Support console-specific loglevels

[Chris Down]

kernel test robot writes:
>sparse warnings: (new ones prefixed by >>)
>>> kernel/printk/sysctl.c:31:47: sparse: sparse: incorrect type in argument 3 (different address spaces) @@     expected void * @@     got void [noderef] __user *buffer @@
>   kernel/printk/sysctl.c:31:47: sparse:     expected void *
>   kernel/printk/sysctl.c:31:47: sparse:     got void [noderef] __user *buffer

Ah, since commit 32927393dc1c ("sysctl: pass kernel pointers to 
->proc_handler") we copy to kernelspace before passing to the callback, so it's 
not a userspace pointer. That's certainly an improvement.

Will fix for v4.

Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options

[Petr Mladek]

On Wed 2022-07-20 18:48:11, Chris Down wrote:
> This will be used in the next patch, and for future changes like the
> proposed sync/nothread named options. This avoids having to use sigils
> like "/" to get different semantic meaning for different parts of the
> option string, and helps to make things more human readable and
> easily extensible.
> 
> There are no functional changes for existing console= users.
> 
> Signed-off-by: Chris Down <chris@chrisdown.name>
> ---
>  kernel/printk/printk.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index b49c6ff6dca0..6094f773ad4a 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2368,6 +2368,30 @@ static void set_user_specified(struct console_cmdline *c, bool user_specified)
>  	console_set_on_cmdline = 1;
>  }
>  
> +static void parse_console_cmdline_options(struct console_cmdline *c,
> +					  char *options)
> +{

I think that the function does not work as expected.

> +	bool seen_serial_opts = false;
> +	char *key;
> +
> +	while ((key = strsep(&options, ",")) != NULL) {
> +		char *value;

strsep() replaces ',' with '\0'.

> +
> +		value = strchr(key, ':');
> +		if (value)
> +			*(value++) = '\0';

This replaces ':' with '\0'.

> +
> +		if (!seen_serial_opts && isdigit(key[0]) && !value) {

This catches the classic options in the format "bbbbpnf".

> +			seen_serial_opts = true;
> +			c->options = key;
> +			continue;
> +		}
> +
> +		pr_err("ignoring invalid console option: '%s%s%s'\n", key,
> +		       value ? ":" : "", value ?: "");

IMHO, this would warn even about "io", "mmio", ...  that are used by:

		console=uart[8250],io,<addr>[,options]
		console=uart[8250],mmio,<addr>[,options]

Warning: I am not completely sure about this. It seems that
	 this format is handled by univ8250_console_match().

	 IMHO, the "8250" is taken as "idx" in console_setup().
	 And "idx" parameter is ignored by univ8250_console_match().
	 This probably explains why "8250" is optional in console=
	 parameter.

> +	}
> +}

Sigh, the parsing of console= parameter is really hacky. Very old code.
The name and idx is handled in console_setup(). The rest
is handled by particular drivers via "options".

This patch makes it even more tricky. It tries to handle some
*options in add_preferred_console(). But it replaces all ','
and ':' by '\0' so that drivers do not see the original *options
any longer.

I thought a lot how to do it a clean way. IMHO, it would be great to
parse everything at a single place but it might require updating
all drivers. I am not sure if it is worth it.

So, I suggest to do it another way. We could implement a generic
function to find in the new key[:value] format. It would check
if the given option (key) exists and read the optional value.

The optional value would allow to define another new options
that would not need any value, e.g. "kthread" or "atomic" that
might be used in the upcoming code that allows to offload
console handling to kthreads.

The function would look like:

/**
  * find_and_remove_console_option() - find and remove particular option from
  *	console= parameter
  *
  * @options: pointer to the buffer with original "options"
  * @key: option name to be found
  * @buf: buffer for the found value, might be NULL
  *
  * Try to find "key[:value]" in the given @options string. Copy the value
  * into the given @buf when any.
  *
  * Warning: The function is designed only to handle new generic console
  *	options. The found "key[:value]" is removed from the given
  *	@options string. The aim is to pass only the driver specific
  *	options to the legacy driver code.
  *
  *	The new options might stay when all console drivers are able
  *	to handle it.
  *
  *     The given @options buffer is modified to avoid allocations. It
  *     makes the life easier during early boot.
  */
bool find_and_remove_console_option(char *options, const char *key,
				    char *buf, int size)
{
	bool found = false, copy_failed = false. trailing_comma = false;
	char *opt, *val;

	/* Find the option: key[:value] */
	while (options && *options) {
		opt = options;

		options = strchr(options, ",");
		if (options)
			*options++ = '\0';

		val = strchr(opt, ":");
		if (val)
			*val++ = '\0';

		if (strcmp(opt, key) == 0) {
			found = true;
			break;
		}

		/* Not our key. Keep it in options. */
		if (options) {
			*(options - 1) = ',';
			trailing_comma = 1;
		}
		if (val)
			*(val - 1) = ':';
	}

	/* Copy the value if found. */
	if (found && val) {
		if (buf && size > strlen(val)) {
			strscpy(buf, val, size);
		} else {
			pr_warn("Can't copy console= option value. Ignoring %s:%s\n",
				opt, val);
			copy_failed = true;
		}
	}

	/* Remove the found option. */
	if (found) {
		if (options) {
			strcpy(opt, options);
			options = opt;
		} else if (trailing_comma) {
			/* Removing last option */
			*(opt - 1) = '\0';
		} else
			*(opt) = '\0';
		}
	}

	return found && !copy_failed;
}

then we could do something like:

void handle_per_console_loglevel_option(struct console_cmdline *c, char *options)
{
	char buf[10];
	int loglevel, ret;

	if (!find_and_remove_console_option("options, "loglevel", buf, sizeof(buf)))
		return;

	if (kstrtoint(buf, 10, &loglevel)) {
		pr_warn("Invalid console loglevel:%s\n", buf);
		return;
	}

	if (loglevel < CONSOLE_LOGLEVEL_MIN || loglevel > CONSOLE_MOTORMOUTH) {
		pr_warn("Console loglevel out of range: %d\n", loglevel);
		return;
	}

	c->loglevel = loglevel;
	c->flags |= CON_LOGLEVEL;
}

Note that the code is not even compile tested.

Any better ideas are highly appreciated.

Best Regards,
Petr

Re: [PATCH v3 2/2] printk: console: Support console-specific loglevels

[Petr Mladek]

On Wed 2022-07-20 18:48:16, Chris Down wrote:
> In terms of technical implementation, this patch embeds a device pointer
> in the console struct, and registers each console using it so we can
> expose attributes in sysfs.
>
> For information on the precedence and application of the new controls,
> see Documentation/ABI/testing/sysfs-class-console and
> Documentation/admin-guide/per-console-loglevel.rst.

The overall logic looks good to me. I finally have good feeling that
the behavior is "easy" to understand.

The detailed documentation is very nice!

See below for comments about various implementation details.

> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -137,6 +138,22 @@ static inline int con_debug_leave(void)
>  #define CON_BRL		(32) /* Used for a braille device */
>  #define CON_EXTENDED	(64) /* Use the extended output format a la /dev/kmsg */
>  
> +/*
> + * The loglevel for a console can be set in many places:
> + *
> + * 1. It can be forced to emit everything (ignore_loglevel);
> + * 2. It can be set globally (sysctls kernel.printk (deprecated),
> + *    kernel.console_loglevel, magic sysrq, loglevel= on kernel command line);
> + * 3. It can be locally set for this specific console (console=...,loglevel:N on
> + *    kernel command line, /sys/class/console/.../loglevel);
> + * 4. It can be set by a compile-time default
> + *    (CONFIG_CONSOLE_LOGLEVEL_{DEFAULT,QUIET})
> + *
> + * If case 3 happens, even if another global value in effect, CON_LOGLEVEL will
> + * be set.

The last sentence is not clear to me.

Well, I suggest to keep it simple and remove this comment
completely. The meaning of the flag is simple. It is set when
the local (per-console) loglevel is set.

The precedence of the various loglevel setting is explained above
console_effective_loglevel() where it belongs.

> + */
> +#define CON_LOGLEVEL	(128) /* Level set locally for this console */

I would write:

#define CON_LOGLEVEL	(128) /* Local (per-console) loglevel is set. */

Alternatively we could avoid the flag completely. The per-console
loglevel is set when con->level > 0. A valid value must never
be below CONSOLE_MIN_LOGLEVEL which is 1. And it is perfectly fine
to say that 0 or -1 is not a valid loglevel. The same effect could
be achieved by disabling the console completely.

I do not have strong opinion. The flag has obvious meaning and might
make the code better readable. On the other hand, it adds an extra
code and complexity.

I slightly prefer to do it without the flag.

Anyway, if we add the new flag, we should also show it in
/proc/consoles, see fs/proc/consoles.c.

> +
>  struct console {
>  	char	name[16];
>  	void	(*write)(struct console *, const char *, unsigned);
> @@ -155,8 +172,15 @@ struct console {
>  	unsigned long dropped;
>  	void	*data;
>  	struct	 console *next;
> +	int	level;
> +	struct	device *classdev;
>  };
>  
> +static inline struct console *classdev_to_console(struct device *dev)
> +{
> +	return dev_get_drvdata(dev);
> +}

Please, open code this in the _show()/_store() callbacks. dev_get_drvdata()
seems to be the standard way how it is done. Hiding it into a custom
function just adds an extra step when reading the code.

> +
>  /*
>   * for_each_console() allows you to iterate on each console
>   */
> diff --git a/kernel/printk/console_cmdline.h b/kernel/printk/console_cmdline.h
> index 3ca74ad391d6..40f1a1ff0965 100644
> --- a/kernel/printk/console_cmdline.h
> +++ b/kernel/printk/console_cmdline.h
> @@ -6,6 +6,8 @@ struct console_cmdline
>  {
>  	char	name[16];			/* Name of the driver	    */
>  	int	index;				/* Minor dev. to use	    */
> +	int	level;				/* Log level to use */
> +	short	flags;				/* Initial flags */
>  	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 6094f773ad4a..6f5e29b60875 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1202,9 +1217,72 @@ 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)
> +static bool __read_mostly ignore_per_console_loglevel;
> +
> +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)");
> +
> +/*
> + * Hierarchy of loglevel authority:
> + *
> + * 1. con->level. The locally set, console-specific loglevel. Optional, only
> + *    valid if the CON_LOGLEVEL flag is set.
> + * 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.
> + *
> + * Callers typically only need the level _or_ the source, but they're both
> + * emitted from this function so that the effective loglevel logic can be
> + * kept in one place.
> + */
> +static int console_effective_loglevel(const struct console *con,
> +				      enum loglevel_source *source)
> +{
> +	enum loglevel_source lsource;
> +	int level;
> +
> +	if (ignore_loglevel) {
> +		lsource = LLS_IGNORE_LOGLEVEL;
> +		level = CONSOLE_LOGLEVEL_MOTORMOUTH;
> +		goto out;
> +	}
> +
> +	if (!ignore_per_console_loglevel &&
> +	    (con && (con->flags & CON_LOGLEVEL))) {
> +		lsource = LLS_LOCAL;
> +		level = con->level;
> +		goto out;
> +	}
> +
> +	lsource = LLS_GLOBAL;
> +	level = console_loglevel;
> +
> +out:
> +	*source = lsource;
> +	return level;
> +}

It might be a matter of taste. But I would probably do it the
following way (note that these would not be used in
boot_delay_msec()):

static int console_effective_loglevel(const struct console *con)
{
	enum loglevel_source source;
	int level;

	if (WARN_ON_ONCE(!con))
		return;

	source = console_effective_loglevel_source(con);

	switch (source) {
	case LLS_IGNORE_LOGLEVEL:
		level = CONSOLE_LOGLEVEL_MOTORMOUTH;
		break;
	case LSS_LOCAL:
		level = con->level;
		break;
	case LSS_GLOBAL:
		level = console_loglevel;
		break;
	default:
		pr_warn("Unhandled console loglevel source: %d,	source);
		level = default_console_loglevel;
		break;
	}

	return level;
}

static const char *console_effective_loglevel_source_str(const struct *con)
{
	enum loglevel_source source;
	const char *str;

	if (WARN_ON_ONCE(!con))
		return;

	source = console_effective_loglevel_source(con);

	switch (source) {
	case LLS_IGNORE_LOGLEVEL:
		str = "ignore_loglevel";
		break;
	case LSS_LOCAL:
		str = "local"
		break;
	case LSS_GLOBAL:
		str = "global";
		break;
	default:
		pr_warn("Unhandled console loglevel source: %d,	source);
		str = "unknown";
		break;
	}

	return str;
}

static enum loglevel_source
console_effective_loglevel_source(const struct console *con)
{
	if (WARN_ON_ONCE(!con))
		return;

	if (ignore_loglevel)
		return LLS_IGNORE_LOGLEVEL;

	if (con->flags & CON_LOGLEVEL && !ignore_per_console_loglevel))
		return LLS_LOCAL;

	return LLS_GLOBAL;
}

It looks like a bit cleaner and better separated (layered) logic.

There is no need to define "enum loglevel_source" variable when
the caller is interested only into the loglevel value.

The advantage of console_effective_loglevel_source_str() is that it
always returns a valid string. It prevents a potential out-of-bound
access to loglevel_source_names[].

> +
> +static bool suppress_message_printing(int level, struct console *con)
>  {
> -	return (level >= console_loglevel && !ignore_loglevel);
> +	enum loglevel_source source;
> +
> +	return level >= console_effective_loglevel(con, &source);
>  }
>  
>  #ifdef CONFIG_BOOT_PRINTK_DELAY
> @@ -1236,7 +1314,7 @@ static void boot_delay_msec(int level)
>  	unsigned long timeout;
>  
>  	if ((boot_delay == 0 || system_state >= SYSTEM_RUNNING)
> -		|| suppress_message_printing(level)) {
> +		|| suppress_message_printing(level, NULL)) {

This does not take into account per-console loglevels.
And we could not check them in vprintk_emit() because
we could not take console_lock() there.

AFAIK, the purpose of this call is to allow reading the messages
on consoles when they do not support scrolling.

A solution would be to call boot_delay_msec() in
console_flush_all(). It would need adding parameter
into console_emit_next_record() that pass information
whether it emitted or suppressed the message.
Something like:

/*
[...]
 * @emitted will be set to "true" when the message was really emitted to the
 * console. It means that it was not suppressed because of console loglevel.
[...]
*/
static bool console_emit_next_record(struct console *con, char *text, char *ext_text,
				     char *dropped_text, bool *emitted, bool *handover)


>  		return;
>  	}
>  
> @@ -1701,12 +1806,14 @@ int do_syslog(int type, char __user *buf, int len, int source)
>  		break;
>  	/* Disable logging to console */
>  	case SYSLOG_ACTION_CONSOLE_OFF:
> +		warn_on_local_loglevel();
>  		if (saved_console_loglevel == LOGLEVEL_DEFAULT)
>  			saved_console_loglevel = console_loglevel;
>  		console_loglevel = minimum_console_loglevel;
>  		break;

We actually could disable logging on all consoles by setting
ignore_per_console_loglevel. Something like:

	case SYSLOG_ACTION_CONSOLE_OFF:
		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:
> +		warn_on_local_loglevel();
>  		if (saved_console_loglevel != LOGLEVEL_DEFAULT) {
>  			console_loglevel = saved_console_loglevel;
>  			saved_console_loglevel = LOGLEVEL_DEFAULT;

and here:

	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;
		}


> @@ -1714,6 +1821,7 @@ int do_syslog(int type, char __user *buf, int len, int source)
>  		break;
>  	/* Set level of messages printed to console */
>  	case SYSLOG_ACTION_CONSOLE_LEVEL:
> +		warn_on_local_loglevel();

I would keep it simple:

		if (!ignore_per_console_loglevel)
			pr_warn_once("SYSLOG_ACTION_CONSOLE_LEVEL is ignored by consoles with explicitely set per-console loglevel, see Documentation/admin-guide/per-console-loglevel\n");

People should know that this API has limits. The check of
ignore_per_console_loglevel allows to prevent the warning
when the users are not interested into the per-console
loglevels feature.

I think about opening discussion about obsoleting the syslog
syscall. It has many drawbacks. Everything is stored in global
variables. As a result, it supports only one reader and one
writer.


>  		if (len < 1 || len > 8)
>  			return -EINVAL;
>  		if (len < minimum_console_loglevel)
> @@ -3030,6 +3149,145 @@ static int __init keep_bootcon_setup(char *str)
>  
>  early_param("keep_bootcon", keep_bootcon_setup);
>  
> +#ifdef CONFIG_PRINTK
> +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct console *con = classdev_to_console(dev);
> +
> +	if (con->flags & CON_LOGLEVEL)
> +		return sysfs_emit(buf, "%d\n", con->level);
> +	else
> +		return sysfs_emit(buf, "unset\n");

I can't find any other sysfs interface using this style (number or
"unset"). Or any other interface combining number or a string values.

I feel a bit uneasy to introduce a completely new semantic. Instead I
prefer using either -1 or 0 when the per-console is not set. It might
be slightly less obvious but it is a standard way so it might be
easier for users in the end.

Both 0 and -1 values are usable as explained above. I personally
prefer -1 because the meaning is more obvious. I would even
use it instead of CON_LOGLEVEL flag.


> +}
> +
> +static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t size)
> +{
> +	struct console *con = classdev_to_console(dev);
> +	ssize_t ret;
> +	int tmp;
> +
> +	if (!strcmp(buf, "unset") || !strcmp(buf, "unset\n")) {
> +		con->flags &= ~CON_LOGLEVEL;
> +		return size;
> +	}
> +
> +	ret = kstrtoint(buf, 10, &tmp);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (tmp < LOGLEVEL_EMERG || tmp > LOGLEVEL_DEBUG + 1)
> +		return -ERANGE;
> +
> +	if (tmp < minimum_console_loglevel)
> +		return -EINVAL;

This looks superfluous. Please, use minimum_console_loglevel
instead of LOGLEVEL_EMERG in the above range check.

Also, we should make a helper function for this. The same range check
is useful also for the global console_loglevel.

> +
> +	con->level = tmp;
> +	con->flags |= CON_LOGLEVEL;
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR_RW(loglevel);
> +
[...]
> +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);
> +}
> +
> +static void console_register_device(struct console *new)

Please use "con" instead of "new" like it is done in the other
API manipulating struct console. It helps when reading the code.

> +{
> +	/*
> +	 * 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;
> +

We should check whether new->classdev is NULL to prevent double
initialization. It should not happen but better be on the safe side.


> +	new->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
> +	if (!new->classdev)
> +		return;
> +
> +	device_initialize(new->classdev);
> +	dev_set_name(new->classdev, "%s", new->name);

This should be:

	dev_set_name(new->classdev, "%s%d", con->name, con->index);

It should match console names defined on the command line
and shown by /proc/consoles. See how the name is printed in
fs/proc/consoles.c.

> +	dev_set_drvdata(new->classdev, new);
> +	new->classdev->release = console_classdev_release;
> +	new->classdev->class = console_class;
> +	if (device_add(new->classdev))
> +		put_device(new->classdev);
> +}
> +
> +static void console_setup_class(void)
> +{
> +	struct console *con;
> +
> +	/*
> +	 * 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(THIS_MODULE, "console");
> +	if (!IS_ERR(console_class))
> +		console_class->dev_groups = console_sysfs_groups;
> +
> +	for_each_console(con)
> +		console_register_device(con);

This should be done under console_lock so that the list of registered
console could not get manipulated.

> +}
> +#else /* CONFIG_PRINTK */
> +static void console_register_device(struct console *new) {}
> +static void console_setup_class(void) {}
> +#endif
> +
>  /*
>   * This is called by register_console() to try to match
>   * the newly registered console with any of the ones selected
> --- a/kernel/printk/sysctl.c
> +++ b/kernel/printk/sysctl.c
> @@ -7,10 +7,14 @@
>  #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;
>  
> +static int min_loglevel = LOGLEVEL_EMERG;

We should use minimum_console_loglevel instead.

> +static int max_loglevel = LOGLEVEL_DEBUG + 1;

Please, define maximum_console_loglevel in kernel/printk/printk.c
after the console_printk[4] array.

This will allow to create a function for the range check that might
be used for both sysfs and proc interface.

Also I would set the maximal value to CONSOLE_LOGLEVEL_MOTORMOUTH.

> +
>  static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
>  				void *buffer, size_t *lenp, loff_t *ppos)
>  {
> @@ -76,6 +122,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_loglevel,
> +		.extra2		= &max_loglevel,
> +	},

Is there any chance to add this into /sys/class/console instead?
I mean:

	/sys/class/console/loglevel
	/sys/class/console/default_message_loglevel

It would be nice to have the things are on the same place.
Especially it would be nice to have the global loglevel there.

Best Regards,
Petr

Re: [PATCH v3 2/2] printk: console: Support console-specific loglevels

[Chris Down]

Hi Petr,

Thanks a lot for getting back! :-)

Any comments not explicitly addressed are acked.

Petr Mladek writes:
>On Wed 2022-07-20 18:48:16, Chris Down wrote:
>> In terms of technical implementation, this patch embeds a device pointer
>> in the console struct, and registers each console using it so we can
>> expose attributes in sysfs.
>>
>> For information on the precedence and application of the new controls,
>> see Documentation/ABI/testing/sysfs-class-console and
>> Documentation/admin-guide/per-console-loglevel.rst.
>
>The overall logic looks good to me. I finally have good feeling that
>the behavior is "easy" to understand.

That's great! Thank you for all your help improving it.

>> + */
>> +#define CON_LOGLEVEL	(128) /* Level set locally for this console */
>
>I would write:
>
>#define CON_LOGLEVEL	(128) /* Local (per-console) loglevel is set. */
>
>Alternatively we could avoid the flag completely. The per-console
>loglevel is set when con->level > 0. A valid value must never
>be below CONSOLE_MIN_LOGLEVEL which is 1. And it is perfectly fine
>to say that 0 or -1 is not a valid loglevel. The same effect could
>be achieved by disabling the console completely.
>
>I do not have strong opinion. The flag has obvious meaning and might
>make the code better readable. On the other hand, it adds an extra
>code and complexity.
>
>I slightly prefer to do it without the flag.
>
>Anyway, if we add the new flag, we should also show it in
>/proc/consoles, see fs/proc/consoles.c.

Hmm, it's true it can be done without the flag, I mostly preferred to do it 
this way out of a concern for code readability.

That said, with the changes suggested below to just show -1 instead of "unset", 
maybe the best solution is just to set -1.

I will think about it some more before v4 and probably just have -1 mean unset. 
Thanks!

>> +static int console_effective_loglevel(const struct console *con,
>> +				      enum loglevel_source *source)
>> +{
>> +	enum loglevel_source lsource;
>> +	int level;
>> +
>> +	if (ignore_loglevel) {
>> +		lsource = LLS_IGNORE_LOGLEVEL;
>> +		level = CONSOLE_LOGLEVEL_MOTORMOUTH;
>> +		goto out;
>> +	}
>> +
>> +	if (!ignore_per_console_loglevel &&
>> +	    (con && (con->flags & CON_LOGLEVEL))) {
>> +		lsource = LLS_LOCAL;
>> +		level = con->level;
>> +		goto out;
>> +	}
>> +
>> +	lsource = LLS_GLOBAL;
>> +	level = console_loglevel;
>> +
>> +out:
>> +	*source = lsource;
>> +	return level;
>> +}
>
>It might be a matter of taste. But I would probably do it the
>following way (note that these would not be used in
>boot_delay_msec()):
>
>static int console_effective_loglevel(const struct console *con)
>{
>	enum loglevel_source source;
>	int level;
>
>	if (WARN_ON_ONCE(!con))
>		return;
>
>	source = console_effective_loglevel_source(con);
>
>	switch (source) {
>	case LLS_IGNORE_LOGLEVEL:
>		level = CONSOLE_LOGLEVEL_MOTORMOUTH;
>		break;
>	case LSS_LOCAL:
>		level = con->level;
>		break;
>	case LSS_GLOBAL:
>		level = console_loglevel;
>		break;
>	default:
>		pr_warn("Unhandled console loglevel source: %d,	source);
>		level = default_console_loglevel;
>		break;
>	}
>
>	return level;
>}
>
>static const char *console_effective_loglevel_source_str(const struct *con)
>{
>	enum loglevel_source source;
>	const char *str;
>
>	if (WARN_ON_ONCE(!con))
>		return;
>
>	source = console_effective_loglevel_source(con);
>
>	switch (source) {
>	case LLS_IGNORE_LOGLEVEL:
>		str = "ignore_loglevel";
>		break;
>	case LSS_LOCAL:
>		str = "local"
>		break;
>	case LSS_GLOBAL:
>		str = "global";
>		break;
>	default:
>		pr_warn("Unhandled console loglevel source: %d,	source);
>		str = "unknown";
>		break;
>	}
>
>	return str;
>}
>
>static enum loglevel_source
>console_effective_loglevel_source(const struct console *con)
>{
>	if (WARN_ON_ONCE(!con))
>		return;
>
>	if (ignore_loglevel)
>		return LLS_IGNORE_LOGLEVEL;
>
>	if (con->flags & CON_LOGLEVEL && !ignore_per_console_loglevel))
>		return LLS_LOCAL;
>
>	return LLS_GLOBAL;
>}
>
>It looks like a bit cleaner and better separated (layered) logic.
>
>There is no need to define "enum loglevel_source" variable when
>the caller is interested only into the loglevel value.
>
>The advantage of console_effective_loglevel_source_str() is that it
>always returns a valid string. It prevents a potential out-of-bound
>access to loglevel_source_names[].

No strong opinions, so I'll do this for v4. Thanks!

>>
>> @@ -1701,12 +1806,14 @@ int do_syslog(int type, char __user *buf, int len, int source)
>>  		break;
>>  	/* Disable logging to console */
>>  	case SYSLOG_ACTION_CONSOLE_OFF:
>> +		warn_on_local_loglevel();
>>  		if (saved_console_loglevel == LOGLEVEL_DEFAULT)
>>  			saved_console_loglevel = console_loglevel;
>>  		console_loglevel = minimum_console_loglevel;
>>  		break;
>
>We actually could disable logging on all consoles by setting
>ignore_per_console_loglevel. Something like:
>
>	case SYSLOG_ACTION_CONSOLE_OFF:
>		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;

Oh, that's very true. Thanks!

>> +		warn_on_local_loglevel();
>
>I would keep it simple:
>
>		if (!ignore_per_console_loglevel)
>			pr_warn_once("SYSLOG_ACTION_CONSOLE_LEVEL is ignored by consoles with explicitely set per-console loglevel, see Documentation/admin-guide/per-console-loglevel\n");

My concern with this is that this will then warn on basically any first 
syslog() use, even for people who don't care about the per-console loglevel 
semantics. They will now get the warning, since by default 
ignore_per_console_loglevel isn't true -- however no per-console loglevel is 
set either, so it's not really relevant.

That's why I implemented it as warn_on_local_loglevel() checking for 
CON_LOGLEVEL, because otherwise it seems noisy for those that are not using the 
feature.

Maybe you have thoughts on that? :-)

>> +static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
>> +			      const char *buf, size_t size)
>> +{
>> +	struct console *con = classdev_to_console(dev);
>> +	ssize_t ret;
>> +	int tmp;
>> +
>> +	if (!strcmp(buf, "unset") || !strcmp(buf, "unset\n")) {
>> +		con->flags &= ~CON_LOGLEVEL;
>> +		return size;
>> +	}
>> +
>> +	ret = kstrtoint(buf, 10, &tmp);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (tmp < LOGLEVEL_EMERG || tmp > LOGLEVEL_DEBUG + 1)
>> +		return -ERANGE;
>> +
>> +	if (tmp < minimum_console_loglevel)
>> +		return -EINVAL;
>
>This looks superfluous. Please, use minimum_console_loglevel
>instead of LOGLEVEL_EMERG in the above range check.

That's fair. In which case we probably end up with one error for all cases: do 
you prefer we should return -EINVAL or -ERANGE?

>> +
>>  static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
>>  				void *buffer, size_t *lenp, loff_t *ppos)
>>  {
>> @@ -76,6 +122,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_loglevel,
>> +		.extra2		= &max_loglevel,
>> +	},
>
>Is there any chance to add this into /sys/class/console instead?
>I mean:
>
>	/sys/class/console/loglevel
>	/sys/class/console/default_message_loglevel
>
>It would be nice to have the things are on the same place.
>Especially it would be nice to have the global loglevel there.

I think this one is a little complicated: on the one hand, yes, it does seem 
more ergonomic to keep everything together in /sys/class/console. On the other 
hand, this means that users can no longer use the sysctl infrastructure, which 
makes things more unwieldy than with kernel.printk.

Not really a problem with sysfs as much as a problem with userspace ergonomics: 
sysctls have a really easy way to set them at boot, but sysfs stuff, not so. 
You can hack it with systemd-tmpfiles, a boot unit, or similar, but the 
infrastructure is a lot less specialised and requires more work. I am worried 
that people may complain that that's unhelpful, especially since we're 
deprecating kernel.printk.

Maybe I shouldn't worry about that so much? I'm curious to hear your further 
thoughts.

Thanks a lot for the detailed feedback!

Chris

Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options

[Chris Down]

Hi Petr,

Thanks a lot for looking this over!

Petr Mladek writes:
>I think that the function does not work as expected.
>
>> +	bool seen_serial_opts = false;
>> +	char *key;
>> +
>> +	while ((key = strsep(&options, ",")) != NULL) {
>> +		char *value;
>
>strsep() replaces ',' with '\0'.
>
>> +
>> +		value = strchr(key, ':');
>> +		if (value)
>> +			*(value++) = '\0';
>
>This replaces ':' with '\0'.
>
>> +
>> +		if (!seen_serial_opts && isdigit(key[0]) && !value) {
>
>This catches the classic options in the format "bbbbpnf".
>
>> +			seen_serial_opts = true;
>> +			c->options = key;
>> +			continue;
>> +		}
>> +
>> +		pr_err("ignoring invalid console option: '%s%s%s'\n", key,
>> +		       value ? ":" : "", value ?: "");
>
>IMHO, this would warn even about "io", "mmio", ...  that are used by:

Oh dear, I should have known it won't be that simple :-D

>
>		console=uart[8250],io,<addr>[,options]
>		console=uart[8250],mmio,<addr>[,options]
>
>Warning: I am not completely sure about this. It seems that
>	 this format is handled by univ8250_console_match().
>
>	 IMHO, the "8250" is taken as "idx" in console_setup().
>	 And "idx" parameter is ignored by univ8250_console_match().
>	 This probably explains why "8250" is optional in console=
>	 parameter.
>
>> +	}
>> +}
>
>Sigh, the parsing of console= parameter is really hacky. Very old code.
>The name and idx is handled in console_setup(). The rest
>is handled by particular drivers via "options".
>
>This patch makes it even more tricky. It tries to handle some
>*options in add_preferred_console(). But it replaces all ','
>and ':' by '\0' so that drivers do not see the original *options
>any longer.

Other than the mmio/io stuff, I think it works properly, right? Maybe I'm 
missing something? :-)

Here's a userspace test for the parser that seems to work.  
parse_console_cmdline_options() should also ignore empty options instead of 
warning, but it still functions correctly in that case, it's just noisy right 
now.

---

/* Only change is pr_err to fprintf */

#define _DEFAULT_SOURCE

#include <stdio.h>
#include <string.h>
#include <stdbool.h>
#include <ctype.h>
#include <stdlib.h>

#define clamp(a, b, c) (a)
#define CON_LOGLEVEL 128

struct console_cmdline {
	char *options;
	int level;
	short flags;
};

static void parse_console_cmdline_options(struct console_cmdline *c,
					  char *options)
{
	bool seen_serial_opts = false;
	char *key;

	while ((key = strsep(&options, ",")) != NULL) {
		char *value;

		value = strchr(key, ':');
		if (value)
			*(value++) = '\0';

		if (strcmp(key, "loglevel") == 0 && value &&
		    isdigit(value[0]) && strlen(value) == 1) {
			c->level = clamp(value[0] - '0', LOGLEVEL_EMERG,
					 LOGLEVEL_DEBUG + 1);
			c->flags |= CON_LOGLEVEL;
			continue;
		}

		if (!seen_serial_opts && isdigit(key[0]) && !value) {
			seen_serial_opts = true;
			c->options = key;
			continue;
		}

		fprintf(stderr,
			"ignoring invalid console option: '%s%s%s'\n", key,
			value ? ":" : "", value ?: "");
	}
}

int main(int argc, char *argv[])
{
	struct console_cmdline con = { 0 };

	if (argc != 2)
		return EXIT_FAILURE;

	parse_console_cmdline_options(&con, argv[1]);
	printf("options: %s\n", con.options);
	printf("level: %d\n", con.level);
}

---

% make CFLAGS='-Wall -Wextra -Werror' loglevel
cc -Wall -Wextra -Werror    loglevel.c   -o loglevel
% ./loglevel 9600n8
options: 9600n8
level: 0
level set: 0
% ./loglevel 9600n8,loglevel:3
options: 9600n8
level: 3
level set: 1
% ./loglevel 9600n8,loglevel:123
ignoring invalid console option: 'loglevel:123'
options: 9600n8
level: 0
level set: 0
% ./loglevel 9600n8,loglevel:3,foo:bar
ignoring invalid console option: 'foo:bar'
options: 9600n8
level: 3
level set: 1
% ./loglevel 9600n8,loglevel
ignoring invalid console option: 'loglevel'
options: 9600n8
level: 0
level set: 0
% ./loglevel loglevel
ignoring invalid console option: 'loglevel'
options: (null)
level: 0
level set: 0
% ./loglevel loglevel:7
options: (null)
level: 7
level set: 1

---

Seems to work ok as far as I can tell, maybe I've misunderstood your concern?  
Or maybe your concern is just about the mmio/io case where the driver wants 
that as part of the options?

>I thought a lot how to do it a clean way. IMHO, it would be great to
>parse everything at a single place but it might require updating
>all drivers. I am not sure if it is worth it.
>
>So, I suggest to do it another way. We could implement a generic
>function to find in the new key[:value] format. It would check
>if the given option (key) exists and read the optional value.
>
>The optional value would allow to define another new options
>that would not need any value, e.g. "kthread" or "atomic" that
>might be used in the upcoming code that allows to offload
>console handling to kthreads.

This could also work, and avoids the current null pointer shoving. It also can 
make the ordering less strict, which seems like a good thing.

I will think about it some more. I'm curious about your comments on the above 
test still though.

Thanks a lot for the detailed review and ideas!

Chris

Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options

[Chris Down]

Chris Down writes:
>[code]

Er, forgot to update that for the one used in the test:

---

#define _DEFAULT_SOURCE

#include <stdio.h>
#include <string.h>
#include <stdbool.h>
#include <ctype.h>
#include <stdlib.h>

#define clamp(a, b, c) (a)
#define CON_LOGLEVEL 128

struct console_cmdline {
	char *options;
	int level;
	short flags;
};

static void parse_console_cmdline_options(struct console_cmdline *c,
					  char *options)
{
	bool seen_serial_opts = false;
	char *key;

	while ((key = strsep(&options, ",")) != NULL) {
		char *value;

		value = strchr(key, ':');
		if (value)
			*(value++) = '\0';

		if (strcmp(key, "loglevel") == 0 && value &&
		    isdigit(value[0]) && strlen(value) == 1) {
			c->level = clamp(value[0] - '0', LOGLEVEL_EMERG,
					 LOGLEVEL_DEBUG + 1);
			c->flags |= CON_LOGLEVEL;
			continue;
		}

		if (!seen_serial_opts && isdigit(key[0]) && !value) {
			seen_serial_opts = true;
			c->options = key;
			continue;
		}

		fprintf(stderr,
			"ignoring invalid console option: '%s%s%s'\n", key,
			value ? ":" : "", value ?: "");
	}
}

int main(int argc, char *argv[])
{
	struct console_cmdline con = { 0 };

	if (argc != 2)
		return EXIT_FAILURE;

	parse_console_cmdline_options(&con, argv[1]);
	printf("options: %s\n", con.options);
	printf("level: %d\n", con.level);
	printf("level set: %d\n", !!(con.flags & CON_LOGLEVEL));
}

Re: [PATCH v3 2/2] printk: console: Support console-specific loglevels

[Petr Mladek]

On Mon 2022-09-05 15:07:44, Chris Down wrote:
> Hi Petr,
> 
> Thanks a lot for getting back! :-)
> 
> Any comments not explicitly addressed are acked.
> 
> Petr Mladek writes:
> > On Wed 2022-07-20 18:48:16, Chris Down wrote:
> > > In terms of technical implementation, this patch embeds a device pointer
> > > in the console struct, and registers each console using it so we can
> > > expose attributes in sysfs.
> > > 
> > > For information on the precedence and application of the new controls,
> > > see Documentation/ABI/testing/sysfs-class-console and
> > > Documentation/admin-guide/per-console-loglevel.rst.

> > > @@ -1701,12 +1806,14 @@ int do_syslog(int type, char __user *buf, int len, int source)
> > >  		break;
> > >  	/* Disable logging to console */
> > >  	case SYSLOG_ACTION_CONSOLE_OFF:
> > > +		warn_on_local_loglevel();
> > >  		if (saved_console_loglevel == LOGLEVEL_DEFAULT)
> > >  			saved_console_loglevel = console_loglevel;
> > >  		console_loglevel = minimum_console_loglevel;
> > >  		break;
> > 
> > We actually could disable logging on all consoles by setting
> > ignore_per_console_loglevel. Something like:
> > 
> > 	case SYSLOG_ACTION_CONSOLE_OFF:
> > 		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;
> 
> Oh, that's very true. Thanks!
> 
> > > +		warn_on_local_loglevel();
> > 
> > I would keep it simple:
> > 
> > 		if (!ignore_per_console_loglevel)
> > 			pr_warn_once("SYSLOG_ACTION_CONSOLE_LEVEL is ignored by consoles with explicitely set per-console loglevel, see Documentation/admin-guide/per-console-loglevel\n");
> 
> My concern with this is that this will then warn on basically any first
> syslog() use, even for people who don't care about the per-console loglevel
> semantics. They will now get the warning, since by default
> ignore_per_console_loglevel isn't true -- however no per-console loglevel is
> set either, so it's not really relevant.
> 
> That's why I implemented it as warn_on_local_loglevel() checking for
> CON_LOGLEVEL, because otherwise it seems noisy for those that are not using
> the feature.

IMHO, the question is if any commonly used tool is using syslog
SYSLOG_ACTION_CONSOLE_ON/OFF these days.

It is supported by dmesg but I am not sure if anyone is really
using it. And I am not sure if anyone uses this during boot, suspend,
or so.

I think that I really should open the discussion whether to obsolete syslog
syscall in general. I am sure that it won't me possible to remove
it anytime soon, maybe it would need to stay forever. Anyway, it has
many problems because they modify global variables. And even reading
does not work well when there are more readers.

I am going to send it. Well, I would need to some time to think
about it.

In the meantime, you could either do it the conservative way or
always show it for these operations. It would be simple to fix
when anyone complains.

> > > +static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
> > > +			      const char *buf, size_t size)
> > > +{
> > > +	struct console *con = classdev_to_console(dev);
> > > +	ssize_t ret;
> > > +	int tmp;
> > > +
> > > +	if (!strcmp(buf, "unset") || !strcmp(buf, "unset\n")) {
> > > +		con->flags &= ~CON_LOGLEVEL;
> > > +		return size;
> > > +	}
> > > +
> > > +	ret = kstrtoint(buf, 10, &tmp);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	if (tmp < LOGLEVEL_EMERG || tmp > LOGLEVEL_DEBUG + 1)
> > > +		return -ERANGE;
> > > +
> > > +	if (tmp < minimum_console_loglevel)
> > > +		return -EINVAL;
> > 
> > This looks superfluous. Please, use minimum_console_loglevel
> > instead of LOGLEVEL_EMERG in the above range check.
> 
> That's fair. In which case we probably end up with one error for all cases:
> do you prefer we should return -EINVAL or -ERANGE?

I prefer -ERANGE.

> > > +
> > >  static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
> > >  				void *buffer, size_t *lenp, loff_t *ppos)
> > >  {
> > > @@ -76,6 +122,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_loglevel,
> > > +		.extra2		= &max_loglevel,
> > > +	},
> > 
> > Is there any chance to add this into /sys/class/console instead?
> > I mean:
> > 
> > 	/sys/class/console/loglevel
> > 	/sys/class/console/default_message_loglevel
> > 
> > It would be nice to have the things are on the same place.
> > Especially it would be nice to have the global loglevel there.
> 
> I think this one is a little complicated: on the one hand, yes, it does seem
> more ergonomic to keep everything together in /sys/class/console. On the
> other hand, this means that users can no longer use the sysctl
> infrastructure, which makes things more unwieldy than with kernel.printk.
> 
> Not really a problem with sysfs as much as a problem with userspace
> ergonomics: sysctls have a really easy way to set them at boot, but sysfs
> stuff, not so. You can hack it with systemd-tmpfiles, a boot unit, or
> similar, but the infrastructure is a lot less specialised and requires more
> work. I am worried that people may complain that that's unhelpful,
> especially since we're deprecating kernel.printk.

Good point. Let's keep the global values in /proc so that they might
be modified by sysctl.

Best Regards,
Petr

Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options

[Petr Mladek]

On Mon 2022-09-05 15:43:53, Chris Down wrote:
> Hi Petr,
> 
> Thanks a lot for looking this over!
> 
> Petr Mladek writes:
> > I think that the function does not work as expected.
> > 
> > > +	bool seen_serial_opts = false;
> > > +	char *key;
> > > +
> > > +	while ((key = strsep(&options, ",")) != NULL) {
> > > +		char *value;
> > 
> > strsep() replaces ',' with '\0'.
> > 
> > > +
> > > +		value = strchr(key, ':');
> > > +		if (value)
> > > +			*(value++) = '\0';
> > 
> > This replaces ':' with '\0'.
> > 
> > > +
> > > +		if (!seen_serial_opts && isdigit(key[0]) && !value) {
> > 
> > This catches the classic options in the format "bbbbpnf".
> > 
> > > +			seen_serial_opts = true;
> > > +			c->options = key;
> > > +			continue;
> > > +		}
> > > +
> > > +		pr_err("ignoring invalid console option: '%s%s%s'\n", key,
> > > +		       value ? ":" : "", value ?: "");
> > 
> > IMHO, this would warn even about "io", "mmio", ...  that are used by:
> 
> Oh dear, I should have known it won't be that simple :-D
> 
> > 
> > 		console=uart[8250],io,<addr>[,options]
> > 		console=uart[8250],mmio,<addr>[,options]
> > 
> > Warning: I am not completely sure about this. It seems that
> > 	 this format is handled by univ8250_console_match().
> > 
> > 	 IMHO, the "8250" is taken as "idx" in console_setup().
> > 	 And "idx" parameter is ignored by univ8250_console_match().
> > 	 This probably explains why "8250" is optional in console=
> > 	 parameter.
> > 
> > > +	}
> > > +}
> > 
> > Sigh, the parsing of console= parameter is really hacky. Very old code.
> > The name and idx is handled in console_setup(). The rest
> > is handled by particular drivers via "options".
> > 
> > This patch makes it even more tricky. It tries to handle some
> > *options in add_preferred_console(). But it replaces all ','
> > and ':' by '\0' so that drivers do not see the original *options
> > any longer.
> 
> Other than the mmio/io stuff, I think it works properly, right? Maybe I'm
> missing something? :-)

One important thing is to do not warn about mmio/io when they are
valid options.

Another important thing is to restore *options string. I mean to
revert all the added '\0' when they are not longer needed.

The *options buffer is passed as paramter to both con->match()
and con->setup() callbacks, see try_enable_preferred_console().
They must be able to see all the values.

Well, I would remove the newly added "logleve:X" when it is handled
in __add_preferred_console(). Otherwise, we would need to double
check all con->match() and con->setup() callbacks that they are
able to handle (ignore) this new option.


> Here's a userspace test for the parser that seems to work.
> parse_console_cmdline_options() should also ignore empty options instead of
> warning, but it still functions correctly in that case, it's just noisy
> right now.

> % make CFLAGS='-Wall -Wextra -Werror' loglevel
> cc -Wall -Wextra -Werror    loglevel.c   -o loglevel
> % ./loglevel 9600n8
> options: 9600n8
> level: 0
> level set: 0
> % ./loglevel 9600n8,loglevel:3
> options: 9600n8
> level: 3
> level set: 1
> % ./loglevel 9600n8,loglevel:123
> ignoring invalid console option: 'loglevel:123'
> options: 9600n8
> level: 0
> level set: 0
> % ./loglevel 9600n8,loglevel:3,foo:bar
> ignoring invalid console option: 'foo:bar'
> options: 9600n8
> level: 3
> level set: 1
> % ./loglevel 9600n8,loglevel
> ignoring invalid console option: 'loglevel'
> options: 9600n8
> level: 0
> level set: 0
> % ./loglevel loglevel
> ignoring invalid console option: 'loglevel'
> options: (null)
> level: 0
> level set: 0
> % ./loglevel loglevel:7
> options: (null)
> level: 7
> level set: 1

> Seems to work ok as far as I can tell, maybe I've misunderstood your
> concern?  Or maybe your concern is just about the mmio/io case where the
> driver wants that as part of the options?

Yes, the mmio/io stuff is the known problem.

My concern is that the function is destructive. It modifies the given
*options buffer that it later passed to another functions. It means
that it modifies the existing behavior.

I am afraid that you might just add an extra check to keep the
particular mmio/io parameters. But I am not sure if these are there only
special parameters used by console drivers.

We found about mmio/io parameters because they were mentioned in
Documentation/admin-guide/kernel-parameters.txt. But there might
be another parameters used by some exotic console that are documented
somewhere else.

I would prefer to do it the other (conservative) way around. I mean
that the new parser in __add_preferred_console() will only search
for the newly added ",loglevel:X" option and remove it from *options
buffer. Everything else should stay in the buffer so that it can
be parsed by the particular console drivers.

It actually already works this way. console_setup() searches for
"tty[S]X" prefix and it "eats" this prefix. The rest of *str buffer
is passed via *options parameter down to __add_preferred_console().

Best Regards,
Petr

Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options

[Chris Down]

(To others on this thread wondering about this patchset, Petr and I have had 
some discussions offlist about v4 and it should be up soon.)

Petr Mladek writes:
>I thought a lot how to do it a clean way. IMHO, it would be great to
>parse everything at a single place but it might require updating
>all drivers. I am not sure if it is worth it.
>
>So, I suggest to do it another way. We could implement a generic
>function to find in the new key[:value] format. It would check
>if the given option (key) exists and read the optional value.
>
>The optional value would allow to define another new options
>that would not need any value, e.g. "kthread" or "atomic" that
>might be used in the upcoming code that allows to offload
>console handling to kthreads.

Any thoughts on something simple like this that takes advantage of memmove()? 
This should overcome the mmio/io concerns, and it's fairly simple.

---

static bool find_and_remove_console_option(char *buf, size_t size,
					   const char *wanted, char *options)
{
	bool found = false, first = true;
	char *item, *opt = options;

	while ((item = strsep(&opt, ","))) {
		char *key = item, *value;

		value = strchr(item, ':');
		if (value)
			*(value++) = '\0';

		if (strcmp(key, wanted) == 0) {
			found = true;
			if (value) {
				if (strlen(value) > size - 1) {
					pr_warn("Can't copy console option value for %s:%s: not enough space (%zu)\n",
						key, value, size);
					found = false;
				} else {
					strscpy(buf, value, size);
				}
			} else
				*buf = '\0';
		}

		if (!found && opt)
			*(opt - 1) = ',';
		if (!found && value)
			*(value - 1) = ':';
		if (!first)
			*(item - 1) = ',';

		if (found)
			break;

		first = false;
	}

	if (found) {
		if (opt)
			memmove(item, opt, strlen(opt) + 1);
		else if (first)
			*item = '\0';
		else
			*--item = '\0';
	}

	return found;
}

Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options

[Chris Down]

Chris Down writes:
>Any thoughts on something simple like this that takes advantage of 
>memmove()? This should overcome the mmio/io concerns, and it's fairly 
>simple.

(although coming to think of it, this can just use memcpy(), but the same idea 
applies)

Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options

[Petr Mladek]

On Mon 2023-04-17 16:04:27, Chris Down wrote:
> (To others on this thread wondering about this patchset, Petr and I have had
> some discussions offlist about v4 and it should be up soon.)
> 
> Petr Mladek writes:
> > I thought a lot how to do it a clean way. IMHO, it would be great to
> > parse everything at a single place but it might require updating
> > all drivers. I am not sure if it is worth it.
> > 
> > So, I suggest to do it another way. We could implement a generic
> > function to find in the new key[:value] format. It would check
> > if the given option (key) exists and read the optional value.
> > 
> > The optional value would allow to define another new options
> > that would not need any value, e.g. "kthread" or "atomic" that
> > might be used in the upcoming code that allows to offload
> > console handling to kthreads.
> 
> Any thoughts on something simple like this that takes advantage of
> memmove()? This should overcome the mmio/io concerns, and it's fairly
> simple.
> 
> ---
> 
> static bool find_and_remove_console_option(char *buf, size_t size,
> 					   const char *wanted, char *options)

Nit: I would change the ordering of the parameters. The above uses
     the semantic of copy functions (desc, src). But this function
     is more about searching or reading. I would use semantic like
     strchr() or read() (where, what, buf).

     Also I would use the key:value names.

     Something like:

static bool
find_and_remove_console_option(char *options, const char
			       char *val_buf, size_t val_buf_size)

> {
> 	bool found = false, first = true;
> 	char *item, *opt = options;

Nit: I would rename these:

   + item -> option: the function is searching for an option that
	has the format value:key.

   + opt -> next: make it more clear that it points behind the
	currently proceed option (string token).

> 	while ((item = strsep(&opt, ","))) {
> 		char *key = item, *value;
> 
> 		value = strchr(item, ':');
> 		if (value)
> 			*(value++) = '\0';
> 
> 		if (strcmp(key, wanted) == 0) {
> 			found = true;
> 			if (value) {
> 				if (strlen(value) > size - 1) {
> 					pr_warn("Can't copy console option value for %s:%s: not enough space (%zu)\n",
> 						key, value, size);
> 					found = false;
> 				} else {
> 					strscpy(buf, value, size);
> 				}
> 			} else
> 				*buf = '\0';
> 		}
> 
> 		if (!found && opt)
> 			*(opt - 1) = ',';
> 		if (!found && value)
> 			*(value - 1) = ':';
> 		if (!first)
> 			*(item - 1) = ',';

This last assigned should not be needed. The above code replaced
at max one ',' and one ':'. It should be enough to restore
just the two.

> 		if (found)
> 			break;
> 
> 		first = false;
> 	}
> 
> 	if (found) {
> 		if (opt)
> 			memmove(item, opt, strlen(opt) + 1);
> 		else if (first)
> 			*item = '\0';
> 		else
> 			*--item = '\0';
> 	}
> 
> 	return found;
> }

Otherwise, it looks correct.

Note: I though about using strnchr() and strncmp() instead of
      replacing/restoring the two delimiters by '\0'.
      But the code looks more hairy in the end.

      Just for record, here is my attempt:

static bool
find_and_remove_console_option(char *options, const char *key,
			       char *val_buf, size_t val_buf_size)
{
	char *start, *next, *val;
	int option_len, key_len, found_key_len;
	bool found = false;

	if (val_buf && val_buf_size)
		*val_buf = '\0';

	key_len = strlen(key);
	next = options;
	do {
		start = next;
		next = strchr(start, ',');
		if (next) {
			option_len = next - start;
			next++;
		} else {
			option_len = strlen(start);
		}

		val = strnchr(start, option_len, ':');
		if (val) {
			found_key_len = val - start;
			val++;
		} else {
			found_key_len = option_len;
		}

		if (key_len != found_key_len)
			continue;

		if (!strncmp(start, key, key_len)) {
			found = true;
			break;
		}
	} while (next);

	if (found && val) {
		int val_len = option_len - key_len - 1;

		if (!val_buf || val_buf_size < val_len + 1) {
			pr_err("Can't copy value for the console option key: %s:%.*s\n",
			       key, val_len, val);
			return false;
		}

		strscpy(val_buf, val, val_len + 1);
	}

	/* Remove the found value[:key][,] */
	if (found) {
		if (next)
			memmove(start, next, strlen(next) + 1);
		else if (start == options)
			*options = '\0';
		else
			*(start - 1) = '\0';
	}

	return found;
}

Best Regards,
Petr

Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options

[Petr Mladek]

On Mon 2023-04-17 16:08:11, Chris Down wrote:
> Chris Down writes:
> > Any thoughts on something simple like this that takes advantage of
> > memmove()? This should overcome the mmio/io concerns, and it's fairly
> > simple.
> 
> (although coming to think of it, this can just use memcpy(), but the same
> idea applies)

I think that we have to use memmove() because the locations might be
overlaping. The to-be-moved options might be longer than the replaced
option.

Best Regards,
Petr

Re: [PATCH v3 1/2] printk: console: Create console= parser that supports named options

[Chris Down]

Thanks for the feedback! Ack to everything and will change for v4.