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

Chris Down
2022-05-18T19:46:14Z

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