On Fri, Nov 03, 2017 at 03:21:14PM +0100, Petr Mladek wrote:

On Thu 2017-09-28 17:43:56, Calvin Owens wrote:

quoted

This adds a new sysfs interface that contains a directory for each
console registered on the system. Each directory contains a single
"loglevel" file for reading and setting the per-console loglevel.

We can let kobject destruction race with console removal: if it does,
loglevel_{show,store}() will safely fail with -ENODEV. This is a little
weird, but avoids embedding the kobject and therefore needing to totally
refactor the way we handle console struct lifetime.

It looks like a sane approach. It might be worth a comment in the code.

quoted

 Documentation/ABI/testing/sysfs-consoles | 13 +++++
 include/linux/console.h                  |  1 +
 kernel/printk/printk.c                   | 88 ++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-consoles

diff --git a/Documentation/ABI/testing/sysfs-consoles b/Documentation/ABI/testing/sysfs-consoles
new file mode 100644
index 0000000..6a1593e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-consoles

@@ -0,0 +1,13 @@
+What:		/sys/consoles/

Eeek, what!

I rather add Greg in CC. I am not 100% sure that the top level
directory is the right thing to do.

Neither do I.

Alternative might be to hide this under /sys/kernel/consoles/.

No no no.

quoted

+Date:		September 2017
+KernelVersion:	4.15
+Contact:	Calvin Owens [off-list ref]
+Description:	The /sys/consoles tree contains a directory for each console
+		configured on the system. These directories contain the
+		following attributes:
+
+		* "loglevel"	Set the per-console loglevel: the kernel uses
+				max(system_loglevel, perconsole_loglevel) when
+				deciding whether to emit a given message. The
+				default is 0, which means max() always yields
+				the system setting in the kernel.printk sysctl.

I would call the attribute "min_loglevel". The name "loglevel" should
be reserved for the really used loglevel that depends also on the
global loglevel value.

quoted

diff --git a/include/linux/console.h b/include/linux/console.h
index a5b5d79..76840be 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h

@@ -148,6 +148,7 @@ struct console {
 	void	*data;
 	struct	 console *next;
 	int	level;
+	struct kobject *kobj;

Why are you using "raw" kobjects and not a "real" struct device?  This
is a device, use that interface instead please.

If you need a console 'bus' to place them on, fine, but the virtual bus
is probably best and simpler to use.

That is if you _really_ feel you need sysfs interaction with the console
layer (hint, I am not yet convinced...)

quoted

 };
 
 /*

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 3f1675e..488bda3 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c

@@ -105,6 +105,8 @@ enum devkmsg_log_masks {
 
 static unsigned int __read_mostly devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
 
+static struct kobject *consoles_dir_kobj;

 static int __control_devkmsg(char *str)
 {
 	if (!str)

@@ -2371,6 +2373,82 @@ static int __init keep_bootcon_setup(char *str)
 
 early_param("keep_bootcon", keep_bootcon_setup);
 
+static ssize_t loglevel_show(struct kobject *kobj, struct kobj_attribute *attr,
+			     char *buf)
+{
+	struct console *con;
+	ssize_t ret = -ENODEV;
+

This might deserve a comment. Something like:

	/*
	 * Find the related struct console a safe way. The kobject
	 * desctruction is asynchronous.
	 */

quoted

+	console_lock();
+	for_each_console(con) {
+		if (con->kobj == kobj) {

You are doing something wrong, go from kobj to your console directly,
the fact that you can not do that here is a _huge_ hint that your
structure is not correct.

Hint, it's not correct at all :)

Please cc: me on stuff like this if you want a review, and as you are
adding a random new sysfs root directory, please always cc: me on that
so I can talk you out of it...

thanks,

greg k-h