Re: [RFC PATCH] printk: console: Allow each console to have its own loglevel

[Chris Down]

Greg Kroah-Hartman writes:
> >  .../admin-guide/kernel-parameters.txt         |  18 +-
> >  .../admin-guide/per-console-loglevel.rst      | 116 ++++++
>
> All sysfs attributes need to be documented in Documentation/ABI/ so that
> the automated tools can properly pick them up and check them.  Please
> don't bury them in some other random Documentation file.

Ack.

> > +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
> > +			     char *buf)
> > +{
> > +	struct console *con = container_of(dev, struct console, classdev);
> > +
> > +	if (con->flags & CON_LOGLEVEL)
> > +		return sprintf(buf, "%d\n", con->level);
> > +	else
> > +		return sprintf(buf, "unset\n");
>
> sysfs_emit() is your friend, always use it please.  For all of the sysfs
> attributes.

Ack.

> > +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);
>
> Thanks for using an attribute group properly, that's nice to see.

Hah, I have no idea what I'm doing with the device model in general, but at 
least I vaguely know how to keep the code tidy ;-)

> > +static void console_classdev_release(struct device *dev)
> > +{
> > +	struct console *con = container_of(dev, struct console, classdev);
> > +
> > +	/*
> > +	 * `struct console' objects are statically allocated (or at the very
> > +	 * least managed outside of our lifecycle), nothing to do. Just set a
> > +	 * flag so that we know we are no longer waiting for anyone and can
> > +	 * return control in unregister_console().
> > +	 */
> > +	con->flags &= ~CON_CLASSDEV_ACTIVE;
> > +}
>
> The old documentation rules said I could complain about this a lot, so
> I'll be nice here just say "this is not ok at all."  You have a dynamic
> object, properly free the memory here please.  class objects can NOT be
> static, sorry.  If you are doing that here, it is really wrong and
> broken and will cause problems when you try to remove the device from
> the system.

Let me check I understand you correctly. The class is:

    static struct class *console_class;
    [...]
    console_class = class_create(THIS_MODULE, "console");

Which exists within printk.c. This class exists to contain all consoles which 
present themselves over the lifetime of the kernel.

console_classdev_release is the release for a single console's "classdev" 
object, rather than the release of the class itself.

If you're talking about properly freeing the memory, I suppose it should happen 
by doing something like the following in unregister_console():

    if (!console_drivers)
        /* free the class object under console lock */

...right? Let me know if I'm misunderstanding you.

Any suggestions you have here are more than welcome, I'm definitely not that 
familiar with the device/class API.

> > +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 so, is the device properly registered there as well?  I missed that
> logic...

We call console_register_device() on all previously known consoles at 
late_initcall() time. Or were you thinking of something else?

> > +	 */
> > +	if (IS_ERR_OR_NULL(console_class))
> > +		return;
> > +
> > +	new->flags |= CON_CLASSDEV_ACTIVE;
> > +	device_initialize(&new->classdev);
> > +	dev_set_name(&new->classdev, "%s", new->name);
> > +	new->classdev.release = console_classdev_release;
> > +	new->classdev.class = console_class;
> > +	if (WARN_ON(device_add(&new->classdev)))
>
> What is with these random WARN_ON() stuff?  Don't do that, just handle
> the error and move on properly.  Systems with panic_on_warn() should not
> fail from stuff like this, that's rude to cause them to reboot.

Oh, that's fair enough, I hadn't thought of that. Ack.

> > +	console_class = class_create(THIS_MODULE, "console");
> > +	if (!WARN_ON(IS_ERR(console_class)))
> > +		console_class->dev_groups = console_sysfs_groups;
>
> Do you ever free the class?

Currently no. What do you think about the above proposal to do it once the 
console driver list is exhausted?

> > +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_ratelimited(
> > +			"printk: The kernel.printk sysctl is deprecated and will be removed soon. Use kernel.force_console_loglevel, kernel.default_message_loglevel, kernel.minimum_console_loglevel, or kernel.default_console_loglevel instead.\n"
>
> Please define "soon".

Petr, what do you think about the timebounds here? :-)

Thanks for the feedback!

Chris