Messages in this thread | | | Date | Wed, 18 May 2022 20:46:00 +0100 | From | Chris Down <> | Subject | Re: [RFC PATCH] printk: console: Allow each console to have its own loglevel |
| |
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
| |