Re: [PATCH 3/4] printk: Add consoles to a virtual "console" bus

[Petr Mladek]

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