Messages in this thread | | | Date | Thu, 30 Jan 2020 14:22:46 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH v3 5/5] console: Introduce ->exit() callback |
| |
On Wed 2020-01-29 16:25:58, Andy Shevchenko wrote: > On Wed, Jan 29, 2020 at 10:41:41PM +0900, Sergey Senozhatsky wrote: > > On (20/01/28 11:44), Andy Shevchenko wrote: > > > > If the console was not registered (hence not enabled) is it still required > > > > to call ->exit()? Is there a requirement that ->exit() should handle such > > > > cases? > > > > > > This is a good point. The ->exit() purpose is to keep balance for whatever > > > happened at ->setup(). > > > > > > But ->setup() is being called either when we have has_preferred == false or > > > when we got no matching we call it for all such consoles, till it returns an > > > error (can you elaborate the logic behind it?). > > > > ->match() does alias matching and ->setup(). If alias matching failed, > > exact name match takes place. We don't call ->setup() for all consoles, > > but only for those that have exact name match: > > > > if (strcmp(c->name, newcon->name) != 0) > > continue; > > > > As to why we don't stop sooner in that loop - I need to to do some > > archaeology. We need to have CON_CONSDEV at proper place, which is > > IIRC the last matching console. > > > > Pretty much every time we tried to change the logic we ended up > > reverting the changes. > > I understand. Seems the ->setup() has to be idempotent. We can tell the same > for ->exit() in some comment.
I believe that ->setup() can succeesfully be called only once. It is tricky like hell:
1st piece:
if (!has_preferred || bcon || !console_drivers) has_preferred = preferred_console >= 0;
note:
+ "has_preferred" is updated here only when it was not "true" before. + "has_preferred" is set to "true" here only when "preferred_console" is set in __add_preferred_console()
2nd piece:
+ __add_preferred_console() is called for console defined on the command line. "preferred_console" points to the console defined by the last "console=" parameter.
3rd piece:
+ "has_preferred" is set to "true" later in register_console() when a console with tty binding gets enabled.
4th piece:
+ The code:
/* * See if we want to use this console driver. If we * didn't select a console we take the first one * that registers here. */ if (!has_preferred) ... try to enable the given console
The comment is a bit unclear. The code is used as a fallback when no console was defined on the command line.
Note that "has_preferred" is always true when "preferred_console" was defined via command line, see 2nd piece above.
By other words:
+ The fallback code (4th piece) is called only when "preferred_console" was not defined on the command line.
+ The cycle below matches the given console only when it was defined on the command line.
As a result, I believe that ->setup() could never be called in both paths for the same console. Especially I think that fallback code should not be used when the console was defined on the command line.
I am not 100% sure but I am ready to risk this. Anyway, I think that many ->setup() callbacks are not ready to be successfully called twice.
(Sigh, I have started to clean up this code two years ago. But I have never found time to finish the patchset. It is such a huge mess.)
> Can you describe, btw, struct console in kernel doc format? > It will be very helpful! > > > > In both cases we will get the console to have CON_ENABLED flag set. > > > > And there are sneaky consoles that have CON_ENABLED before we even > > register them. > > So, taking into consideration my comment to the previous patch, what would be > suggested guard here? > > For a starter something like this? > > if ((console->flags & CON_ENABLED) && console->exit) > console->exit(console);
I would do:
if (!res && console->exit) console->exit(console);
I mean. I would call ->exit() only when console->setup() succeeded in register_console(). In this case, the console was later added to the console_drivers list.
Best Regards, Petr
| |