Re: [PATCH v7 07/13] printk: console: Introduce sysfs interface for per-console loglevels

[Petr Mladek <pmladek@suse.com>]

On Wed 2025-11-19 03:07:27, Chris Down wrote:
> A sysfs interface under /sys/class/console/ is created that permits
> viewing and configuring per-console attributes. This is the main
> interface with which we expect users to interact with and configure
> per-console loglevels.
> 
> --- a/Documentation/admin-guide/per-console-loglevel.rst
> +++ b/Documentation/admin-guide/per-console-loglevel.rst
> @@ -69,3 +69,41 @@ The default value for ``kernel.console_loglevel`` comes from
>  ``CONFIG_CONSOLE_LOGLEVEL_DEFAULT``, or ``CONFIG_CONSOLE_LOGLEVEL_QUIET`` if
>  ``quiet`` is passed on the kernel command line.
>  
> +Console attributes
> +~~~~~~~~~~~~~~~~~~
> +
> +Registered consoles are exposed at ``/sys/class/console``. For example, if you
> +are using ``ttyS0``, the console backing it can be viewed at
> +``/sys/class/console/ttyS0/``. The following files are available:
> +
> +* ``effective_loglevel`` (r): The effective loglevel after considering all
> +  loglevel authorities. For example, it shows the value of the console-specific
> +  loglevel when a console-specific loglevel is defined, and shows the global
> +  console loglevel value when the console-specific one is not defined.
> +
> +* ``effective_loglevel_source`` (r): The loglevel authority which resulted in
> +  the effective loglevel being set. The following values can be present:
> +
> +    * ``local``: The console-specific loglevel is in effect.
> +
> +    * ``global``: The global loglevel (``kernel.console_loglevel``) is in
> +      effect. Set a console-specific loglevel to override it.
> +
> +    * ``ignore_loglevel``: ``ignore_loglevel`` was specified on the kernel
> +      command line or at ``/sys/module/printk/parameters/ignore_loglevel``.
> +      Disable it to use level controls.
> +
> +* ``loglevel`` (rw): The local, console-specific loglevel for this console.
> +  This will be in effect if no other global control overrides it. Look at
> +  ``effective_loglevel`` and ``effective_loglevel_source`` to verify that.
> +
> +Deprecated
> +~~~~~~~~~~
> +
> +* ``kernel.printk`` sysctl: this takes four values, setting
> +  ``kernel.console_loglevel``, ``kernel.default_message_loglevel``, the minimum
> +  console loglevel, and a fourth unused value. The interface is generally
> +  considered to quite confusing, doesn't perform checks on the values given,

The seems to be a typo. I would use either of

   + "considered as quite confusing"
   + "considered to be quite confusing"

> +  and is unaware of per-console loglevel semantics.
> +
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -4010,6 +4010,9 @@ static void try_enable_default_console(struct console *newcon)
>  	if (newcon->index < 0)
>  		newcon->index = 0;
>  
> +	newcon->level = LOGLEVEL_DEFAULT;

This does not fit in this patch. A better place would be the 3rd patch
which added .level into struct console.

But I am not sure if we need it at all. The LOGLEVEL_DEFAULT will
be set by register_console() as a fallback anyway. It is the same
reason why I suggested to remove a similar code from
try_enable_preferred_console().

> +	newcon->classdev = NULL;

This should not be needed. struct console is defined statically
and this field should always be NULL before the console
is registered.

> +
>  	if (console_call_setup(newcon, NULL) != 0)
>  		return;
>  
> diff --git a/kernel/printk/sysfs.c b/kernel/printk/sysfs.c
> new file mode 100644
> index 000000000000..5252e6e04908
> --- /dev/null
> +++ b/kernel/printk/sysfs.c
> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/kernel.h>
> +#include <linux/console.h>
> +#include <linux/device.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include "internal.h"
> +
> +static const char *
> +console_effective_loglevel_source_str(const struct console *con)
> +{
> +	enum loglevel_source source;
> +	const char *str;
> +	int con_level;
> +	int cookie;
> +
> +	cookie = console_srcu_read_lock();
> +	con_level = console_srcu_read_loglevel(con);
> +	console_srcu_read_unlock(cookie);

I think that this was discussed in v6. But I guess that we did not
reach a consensus.

The SRCU read lock is not need here. In fact, it is wrong because
unregister_console_locked() calls device_unregister() after
synchronize_srcu(&console_srcu).

The sysfs API makes sure that struct console could not get removed
when the interface is accessed. I suggest to create a custom
wrappers for the sysfs interface. Something like:

/**
 * console_srcu_read_loglevel - Locklessly read the console specific loglevel
 *				when accessing the related sysfs interface
 * @con:	struct console pointer of console to read loglevel from
 *
 * Locklessly reading @con->level provides a consistent read value because
 * there is at most one CPU modifying @con->level and that CPU is using only
 * read-modify-write operations to do so.
 *
 * Only use this function to read flags via the related sysfs interface.
 * The sysfs API makes sure that the structure could not disappear while
 * the interface is used.
 *
 * Context: Sysfs interface for the given console.
 * Return: The current value of the @con->level field.
 */
static inline
int console_sysfs_read_loglevel(const struct console *con)
{
	/*
	 * The READ_ONCE() matches the WRITE_ONCE() when @level is modified
	 * for registered consoles.
	 */
	return data_race(READ_ONCE(con->level));
}

/**
 * console_sysfs_write_flags - Write the console specific loglevel via
 *	sysfs interface.
 * @con:	struct console pointer of console to write flags to
 * @flags:	new flags value to write
 *
 * Only use this function to write flags via the related sysfs interface.
 * The sysfs API makes sure that the structure could not disappear while
 * the interface is used.
 *
 * Context: Any context.
 */
static inline
void console_sysfs_write_loglevel(struct console *con, int con_level)
{
	/* This matches the READ_ONCE() in console_srcu_read_flags(). */
	WRITE_ONCE(con->level, con_level);
}

> +	source = console_effective_loglevel_source(con_level);
> +
> +	switch (source) {
> +	case LLS_IGNORE_LOGLEVEL:
> +		str = "ignore_loglevel";
> +		break;
> +	case LLS_LOCAL:
> +		str = "local";
> +		break;
> +	case LLS_GLOBAL:
> +		str = "global";
> +		break;
> +	default:
> +		str = "unknown";
> +		break;
> +	}
> +
> +	return str;
> +}
> +
> +static ssize_t effective_loglevel_source_show(struct device *dev,
> +					      struct device_attribute *attr,
> +					      char *buf)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +	const char *str;
> +
> +	str = console_effective_loglevel_source_str(con);
> +	return sysfs_emit(buf, "%s\n", str);
> +}
> +
> +static DEVICE_ATTR_RO(effective_loglevel_source);
> +
> +static ssize_t effective_loglevel_show(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +	int con_level;
> +	int cookie;
> +
> +	cookie = console_srcu_read_lock();
> +	con_level = console_srcu_read_loglevel(con);
> +	console_srcu_read_unlock(cookie);

Same here.

> +	return sysfs_emit(buf, "%d\n", console_effective_loglevel(con_level));
> +}
> +
> +static DEVICE_ATTR_RO(effective_loglevel);
> +
> +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +	int con_level;
> +	int cookie;
> +
> +	cookie = console_srcu_read_lock();
> +	con_level = console_srcu_read_loglevel(con);
> +	console_srcu_read_unlock(cookie);

and here.

> +	return sysfs_emit(buf, "%d\n", con_level);
> +}
> +
> +static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t size)
> +{
> +	struct console *con = dev_get_drvdata(dev);
> +	ssize_t ret;
> +	int level;
> +	int cookie;
> +
> +	ret = kstrtoint(buf, 10, &level);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* -1 means "use global loglevel" */
> +	if (level == -1)
> +		goto out;
> +
> +	/*
> +	 * Reject level 0 (KERN_EMERG) - per-console loglevel must be > 0.
> +	 * Emergency messages should go to all consoles, so they cannot be
> +	 * filtered per-console.
> +	 */
> +	if (level == 0)
> +		return -ERANGE;
> +
> +	if (console_clamp_loglevel(level) != level)
> +		return -ERANGE;
> +
> +	/*
> +	 * If the system has a minimum console loglevel set (via sysctl or
> +	 * kernel parameter), enforce it. This prevents setting per-console
> +	 * loglevels below the system minimum.
> +	 */
> +	if (minimum_console_loglevel > CONSOLE_LOGLEVEL_MIN &&
> +	    level < minimum_console_loglevel)
> +		return -ERANGE;
> +
> +out:
> +	cookie = console_srcu_read_lock();
> +	WRITE_ONCE(con->level, level);
> +	console_srcu_read_unlock(cookie);

and here. Note that we need to add the data_race() as discussed
in v6.

> +
> +	return size;
> +}
> +


I propose to squash the following changes into this patch:

diff --git a/Documentation/admin-guide/per-console-loglevel.rst b/Documentation/admin-guide/per-console-loglevel.rst
index 69eede12e20f..f621a6785f81 100644
--- a/Documentation/admin-guide/per-console-loglevel.rst
+++ b/Documentation/admin-guide/per-console-loglevel.rst
@@ -255,7 +255,7 @@ 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,
+  considered to be quite confusing, doesn't perform checks on the values given,
   and is unaware of per-console loglevel semantics.
 
 Chris Down <chris@chrisdown.name>, 18-November-2025
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 170fa8e14ea9..e73276880d51 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -4118,9 +4118,6 @@ static void try_enable_default_console(struct console *newcon)
 	if (newcon->index < 0)
 		newcon->index = 0;
 
-	newcon->level = LOGLEVEL_DEFAULT;
-	newcon->classdev = NULL;
-
 	if (console_call_setup(newcon, NULL) != 0)
 		return;
 
diff --git a/kernel/printk/sysfs.c b/kernel/printk/sysfs.c
index 5252e6e04908..fc029b425809 100644
--- a/kernel/printk/sysfs.c
+++ b/kernel/printk/sysfs.c
@@ -6,17 +6,57 @@
 #include <linux/slab.h>
 #include "internal.h"
 
+/**
+ * console_srcu_read_loglevel - Locklessly read the console specific loglevel
+ *				when accessing the related sysfs interface
+ * @con:	struct console pointer of console to read loglevel from
+ *
+ * Locklessly reading @con->level provides a consistent read value because
+ * there is at most one CPU modifying @con->level and that CPU is using only
+ * read-modify-write operations to do so.
+ *
+ * Only use this function to read flags via the related sysfs interface.
+ * The sysfs API makes sure that the structure could not disappear while
+ * the interface is used.
+ *
+ * Context: Sysfs interface for the given console.
+ * Return: The current value of the @con->level field.
+ */
+static inline
+int console_sysfs_read_loglevel(const struct console *con)
+{
+	/*
+	 * The READ_ONCE() matches the WRITE_ONCE() when @level is modified
+	 * for registered consoles.
+	 */
+	return data_race(READ_ONCE(con->level));
+}
+
+/**
+ * console_sysfs_write_flags - Write the console specific loglevel via
+ *	sysfs interface.
+ * @con:	struct console pointer of console to write flags to
+ * @flags:	new flags value to write
+ *
+ * Only use this function to write flags via the related sysfs interface.
+ * The sysfs API makes sure that the structure could not disappear while
+ * the interface is used.
+ *
+ * Context: Any context.
+ */
+static inline
+void console_sysfs_write_loglevel(struct console *con, int con_level)
+{
+	/* This matches the READ_ONCE() in console_srcu_read_flags(). */
+	WRITE_ONCE(con->level, con_level);
+}
+
 static const char *
-console_effective_loglevel_source_str(const struct console *con)
+console_effective_loglevel_source_str(int con_level)
 {
 	enum loglevel_source source;
 	const char *str;
-	int con_level;
-	int cookie;
 
-	cookie = console_srcu_read_lock();
-	con_level = console_srcu_read_loglevel(con);
-	console_srcu_read_unlock(cookie);
 	source = console_effective_loglevel_source(con_level);
 
 	switch (source) {
@@ -43,8 +83,10 @@ static ssize_t effective_loglevel_source_show(struct device *dev,
 {
 	struct console *con = dev_get_drvdata(dev);
 	const char *str;
+	int con_level;
 
-	str = console_effective_loglevel_source_str(con);
+	con_level = console_sysfs_read_loglevel(con);
+	str = console_effective_loglevel_source_str(con_level);
 	return sysfs_emit(buf, "%s\n", str);
 }
 
@@ -55,11 +97,8 @@ static ssize_t effective_loglevel_show(struct device *dev,
 {
 	struct console *con = dev_get_drvdata(dev);
 	int con_level;
-	int cookie;
 
-	cookie = console_srcu_read_lock();
-	con_level = console_srcu_read_loglevel(con);
-	console_srcu_read_unlock(cookie);
+	con_level = console_sysfs_read_loglevel(con);
 	return sysfs_emit(buf, "%d\n", console_effective_loglevel(con_level));
 }
 
@@ -70,11 +109,8 @@ static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
 {
 	struct console *con = dev_get_drvdata(dev);
 	int con_level;
-	int cookie;
 
-	cookie = console_srcu_read_lock();
-	con_level = console_srcu_read_loglevel(con);
-	console_srcu_read_unlock(cookie);
+	con_level = console_sysfs_read_loglevel(con);
 	return sysfs_emit(buf, "%d\n", con_level);
 }
 
@@ -84,7 +120,6 @@ static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
 	struct console *con = dev_get_drvdata(dev);
 	ssize_t ret;
 	int level;
-	int cookie;
 
 	ret = kstrtoint(buf, 10, &level);
 	if (ret < 0)
@@ -115,9 +150,7 @@ static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
 		return -ERANGE;
 
 out:
-	cookie = console_srcu_read_lock();
-	WRITE_ONCE(con->level, level);
-	console_srcu_read_unlock(cookie);
+	console_sysfs_write_loglevel(con, level);
 
 	return size;
 }

Best Regards,
Petr