On Fri, Nov 03, 2017 at 03:21:14PM +0100, Petr Mladek wrote:
> On Thu 2017-09-28 17:43:56, Calvin Owens wrote:
> > 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.
>
>
> > 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.
> > +Date: September 2017
> > +KernelVersion: 4.15
> > +Contact: Calvin Owens <calvinowens@xxxxxx>
> > +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.
>
>
> > 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...)
> > };
> >
> > /*
> > 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.
> */
> > + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
