On Tue 2019-03-12 21:52:13, Calvin Owens wrote: > On Monday 03/11 at 14:33 +0100, Petr Mladek wrote: > > On Fri 2019-03-01 16:48:19, Calvin Owens wrote: > > > This patch embeds a device struct in the console struct, and registers > > > them on a "console" bus so we can expose attributes in sysfs. > > > > > > Currently, most drivers declare static console structs, and that is > > > incompatible with the dev refcount model. So we end up needing to patch > > > all of the console drivers to: > > > > > > 1. Dynamically allocate the console struct using a new helper > > > 2. Handle the allocation in (1) possibly failing > > > 3. Dispose of (1) with put_device() > > > > > > Early console structures must still be static, since they're required > > > before we're able to allocate memory. The least ugly way I can come up > > > with to handle this is an "is_static" flag in the structure which makes > > > the gets and puts NOPs, and is checked in ->release() to catch mistakes. > > > > > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > > > index 2e0eb89f046c..67e1e993ab80 100644 > > > --- a/kernel/printk/printk.c > > > +++ b/kernel/printk/printk.c > > > @@ -2706,6 +2770,8 @@ void register_console(struct console *newcon) > > > console_drivers->next = newcon; > > > } > > > > > > + get_console(newcon); > > > + > > > if (newcon->flags & CON_EXTENDED) > > > nr_ext_console_drivers++; > > > > > > @@ -2730,6 +2796,7 @@ void register_console(struct console *newcon) > > > exclusive_console_stop_seq = console_seq; > > > logbuf_unlock_irqrestore(flags); > > > } > > > + console_register_device(newcon); > > > > This calls console_init_device() for the statically defined > > early consoles. I guess that it would invalidate the above > > get_console(). > > The get_console() macro checks for ->is_static and is a NOP if it's > set, which is definitely confusing. > > > Also it is not symmetric with unregister_console(). We add it > > to the console_subsys here and did not remove it when > > the console is unregistered. > > We do a put() in the unregister path, somebody could hold a reference > through sysfs so we can't just remove it. Or am I misunderstanding? To be honest, I am not sure what the effect of get_device() and put_device() is. But they are called also in device_add(). This suggests that the sysfs interface and struct device stay even when we call device_put() in unregister_console(). This is an asymmetry. The sysfs interface is created only for successfully registered console but it stays even after unregistration (if it works as I expect). Another problem is that register_console()/unregister_console() might get called repeatedly for the same console. But device_add() should get called only once. I think that we could do better, see below. > > IMHO, this might be done already in allocate_console(). > > And eventually in printk_late_init() for consoles that > > are allocated earlier. > > > > I am not familiar with the device-related sysfs API. > > But I see that device_initialize() and device_add() > > can be done by device_register(). The separate > > calls are needed only when a special reference > > handling is needed. Are we special? > > We're special: the console initcalls run before the device core is > initialized, initialize() lets us use the refcount. But console_init_device() is called later from printk_late_init() for the statically defined structures (early consoles). This would reset the refcount for these consoles. I think that we should delay calling console_init_device() for all consoles until printk_late_done is set. Then it should be safe to call device_register() there. Best Regards, Petr
