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

Chris Down
2022-05-19T14:12:29Z

Greg Kroah-Hartman writes:
>The two stanzas in my reply do NOT refer to the same thing.
>
>The first one is for the device that is assigned to the class. That
>must be freed and properly reference counted and handled as that is a
>dynamic object that can come and go as people add and remove consoles.

Ah, I see. So to be clear, this change would fix what you're concerned about,
right? :-)

If so I'll do this in v2. Thanks!

---

diff --git include/linux/console.h include/linux/console.h
index ce5ba405285a..408dd86be8eb 100644
--- include/linux/console.h
+++ include/linux/console.h
@@ -156,12 +156,6 @@ static inline int con_debug_leave(void)
*/
#define CON_LOGLEVEL (128) /* Level set locally for this console */

-/*
- * Console has active class device, so may have active readers/writers from
- * /sys/class hierarchy.
- */
-#define CON_CLASSDEV_ACTIVE (256)
-
struct console {
char name[16];
void (*write)(struct console *, const char *, unsigned);
@@ -179,9 +173,11 @@ struct console {
void *data;
struct console *next;
int level;
- struct device classdev;
+ struct device *classdev;
};

+#define classdev_to_console(dev) dev_get_drvdata(dev)
+
/*
* for_each_console() allows you to iterate on each console
*/
diff --git kernel/printk/printk.c kernel/printk/printk.c
index ee3328f7b6fb..a60bcf296c25 100644
--- kernel/printk/printk.c
+++ kernel/printk/printk.c
@@ -3076,7 +3076,7 @@ early_param("keep_bootcon", keep_bootcon_setup);
static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- struct console *con = container_of(dev, struct console, classdev);
+ struct console *con = classdev_to_console(dev);

if (con->flags & CON_LOGLEVEL)
return sprintf(buf, "%d\n", con->level);
@@ -3087,7 +3087,7 @@ static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t size)
{
- struct console *con = container_of(dev, struct console, classdev);
+ struct console *con = classdev_to_console(dev);
ssize_t ret;
int tmp;

@@ -3115,7 +3115,7 @@ static ssize_t effective_loglevel_source_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct console *con = container_of(dev, struct console, classdev);
+ struct console *con = classdev_to_console(dev);
enum loglevel_source source;

console_effective_loglevel(con, &source);
@@ -3128,7 +3128,7 @@ static ssize_t effective_loglevel_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct console *con = container_of(dev, struct console, classdev);
+ struct console *con = classdev_to_console(dev);
enum loglevel_source source;

return sprintf(buf, "%d\n", console_effective_loglevel(con, &source));
@@ -3139,7 +3139,7 @@ static DEVICE_ATTR_RO(effective_loglevel);
static ssize_t enabled_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- struct console *con = container_of(dev, struct console, classdev);
+ struct console *con = classdev_to_console(dev);

return sprintf(buf, "%d\n", !!(con->flags & CON_ENABLED));
}
@@ -3158,15 +3158,7 @@ ATTRIBUTE_GROUPS(console_sysfs);

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;
+ kfree(dev);
}

static void console_register_device(struct console *new)
@@ -3179,13 +3171,17 @@ static void console_register_device(struct console *new)
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)))
- put_device(&new->classdev);
+ new->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
+ if (!new->classdev)
+ return;
+
+ device_initialize(new->classdev);
+ dev_set_name(new->classdev, "%s", new->name);
+ dev_set_drvdata(new->classdev, new);
+ new->classdev->release = console_classdev_release;
+ new->classdev->class = console_class;
+ if (WARN_ON(device_add(new->classdev)))
+ put_device(new->classdev);
}

/*
@@ -3462,7 +3458,7 @@ int unregister_console(struct console *console)
console_drivers->flags |= CON_CONSDEV;

console->flags &= ~CON_ENABLED;
- device_unregister(&console->classdev);
+ device_unregister(console->classdev);
console_unlock();
console_sysfs_notify();

@@ -3475,14 +3471,6 @@ int unregister_console(struct console *console)
console->flags &= ~CON_ENABLED;
console_unlock();

- /*
- * Wait for anyone holding the classdev open to close it so that we
- * don't remove the module prematurely. Once classdev refcnt is 0,
- * CON_CLASSDEV_ACTIVE will be unset in console_classdev_release.
- */
- while (console->flags & CON_CLASSDEV_ACTIVE)
- schedule_timeout_uninterruptible(msecs_to_jiffies(1));
-
return res;
}
EXPORT_SYMBOL(unregister_console);