From: Paul Burton <paul.burton@imgtec•com>
To: Sergey Senozhatsky <sergey.senozhatsky@gmail•com>
Cc: Larry Finger <Larry.Finger@lwfinger•net>,
Michael Ellerman <mpe@ellerman•id.au>,
Andreas Schwab <schwab@linux-m68k•org>,
Andrew Morton <akpm@linux-foundation•org>,
Borislav Petkov <bp@suse•de>, Petr Mladek <pmladek@suse•com>,
Tejun Heo <tj@kernel•org>, <linux-kernel@vger•kernel.org>,
<linuxppc-dev@lists•ozlabs.org>
Subject: Re: [PATCH v3] console: use first console if stdout-path device doesn't appear
Date: Thu, 3 Nov 2016 21:17:01 +0000 [thread overview]
Message-ID: <2317993.vxfTP7Yo3N@np-p-burton> (raw)
In-Reply-To: <20161103174040.GB423@swordfish>
[-- Attachment #1: Type: text/plain, Size: 5141 bytes --]
Hi Sergey,
On Friday, 4 November 2016 02:40:40 GMT Sergey Senozhatsky wrote:
> On (11/03/16 12:57), Paul Burton wrote:
> > If a device tree specified a preferred device for kernel console output
> > via the stdout-path or linux,stdout-path chosen node properties there's
> > no guarantee that it will have specified a device for which we have a
> > driver. It may also be the case that we do have a driver but it doesn't
> > call of_console_check() to register as a preferred console (eg. offb
> > driver as used on powermac systems).
>
> so why that driver doesn't call of_console_check() then? if there is a
> misconfiguration then why do we want to fix it/fallback in printk code?
Ideally I think the driver (or perhaps fbdev/fbcon) ought to call
of_console_check() which would allow the console to be enabled earlier again.
However there isn't any single place I can see that currently has all the
information required to do so - the device tree node, the name & index of the
console.
Even if we change that in the future & do call of_console_check(), I can't
guarantee that offb is the only driver that would encounter this case, and it
still wouldn't cover the case of us not having any driver for the specified
stdout-path device. The fallback thus seems like a sensible thing to do.
>
> [..]
>
> > @@ -260,10 +260,18 @@ void console_set_by_of(void)
> >
> > {
> >
> > of_specified_console = true;
> >
> > }
> >
> > +
> > +static void clear_of_specified_console(void)
> > +{
> > + of_specified_console = false;
> > +}
> >
> > #else
> > # define of_specified_console false
> >
> > +static void clear_of_specified_console(void) { }
> >
> > #endif
> >
> > +struct console *of_fallback_console;
> > +
> >
> > /* Flag: console code may call schedule() */
> > static int console_may_schedule;
> >
> > @@ -2657,10 +2665,26 @@ void register_console(struct console *newcon)
> >
> > * didn't select a console we take the first one
> > * that registers here.
> > */
> >
> > - if (preferred_console < 0 && !of_specified_console) {
> > + if (preferred_console < 0) {
> >
> > if (newcon->index < 0)
> >
> > newcon->index = 0;
> >
> > - if (newcon->setup == NULL ||
> > + if (of_specified_console) {
> > + /*
> > + * The device tree stdout-path chosen node property was
> > + * specified so we don't want to enable the first
> > + * registered console just now in order to give the
> > + * device indicated by stdout-path a chance to be
> > + * registered first. Do however keep track of the
> > + * first console we see so that we can fall back to
> > + * using it if we don't see the desired device, either
> > + * because stdout-path isn't valid, or because we have
> > + * no driver for the device or our driver doesn't call
> > + * of_console_check(). See printk_late_init() for this
> > + * fallback.
>
> if the path is not valid then correct the path. no?
...but what if the path is valid and we simply don't have a driver for the
device it references? As I said in that comment we may not have a driver at
all.
>
> > + */
> > + if (!of_fallback_console)
> > + of_fallback_console = newcon;
> > + } else if (newcon->setup == NULL ||
> >
> > newcon->setup(newcon, NULL) == 0) {
> >
> > newcon->flags |= CON_ENABLED;
> > if (newcon->device) {
> >
> > @@ -2844,6 +2868,22 @@ static int __init printk_late_init(void)
> >
> > {
> >
> > struct console *con;
> >
> > + if (of_specified_console && of_fallback_console &&
> > + (!console_drivers || !(console_drivers->flags & CON_CONSDEV))) {
> > + /*
> > + * The system has a device tree which specified stdout-path,
> > + * but we haven't seen a console associated with the device
> > + * specified by the stdout-path chosen node property.
> > + *
> > + * We do however know which console would have been used
> > + * if stdout-path weren't specified at all, so in an attempt
> > + * to provide some output we'll re-register that console
> > + * pretending that we never saw stdout-path.
> > + */
>
> DT screwed up, so why would printk() care? does any other
> sub-system/driver fixes up a DT misconfiguration?
>
> -ss
...and again, it may not be a misconfiguration - that's one possibility of a
few that I mentioned. Even if the DT is misconfigured & stdout-path is
completely bonkers it's still arguable that falling back to the first console
registered (ie. our previous behaviour) is the sensible thing to do. Perhaps
we ought to warn in such cases, but even then we can't distinguish between the
3 cases I mentioned (invalid stdout-path, driver which doesn't call
of_console_check() or no driver at all) so we'd certainly end up warning on
systems like these affected PowerPC systems, which makes me think that may be
a better warning to introduce once we expect systems to not hit it.
Thanks,
Paul
>
> > + clear_of_specified_console();
> > + register_console(of_fallback_console);
> > + }
> > +
> >
> > for_each_console(con) {
> >
> > if (!keep_bootcon && con->flags & CON_BOOT) {
> >
> > /*
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
next prev parent reply other threads:[~2016-11-03 21:17 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20160809125010.14150-1-paul.burton@imgtec.com>
[not found] ` <20160809151937.26118-1-paul.burton@imgtec.com>
2016-10-16 18:07 ` [PATCH v2] console: Don't prefer first registered if DT specifies stdout-path Andreas Schwab
2016-10-17 10:33 ` Paul Burton
2016-10-17 17:39 ` Andreas Schwab
2016-10-18 9:18 ` [PATCH] console: use first console if stdout-path device doesn't appear Paul Burton
2016-10-18 18:58 ` Andreas Schwab
2016-10-30 9:46 ` Andreas Schwab
2016-10-31 5:28 ` Michael Ellerman
2016-10-31 12:14 ` [PATCH v2] " Paul Burton
2016-10-31 15:50 ` Paul Burton
2016-10-31 19:21 ` Larry Finger
2016-10-31 23:09 ` Sergey Senozhatsky
2016-10-31 23:31 ` Larry Finger
2016-11-03 12:57 ` [PATCH v3] " Paul Burton
2016-11-03 17:40 ` Sergey Senozhatsky
2016-11-03 21:17 ` Paul Burton [this message]
2016-11-04 15:44 ` Sergey Senozhatsky
2016-11-04 8:05 ` Andreas Schwab
2016-11-07 8:27 ` Michael Ellerman
2016-11-07 9:18 ` Paul Burton
2016-11-07 15:26 ` Larry Finger
2016-11-07 17:21 ` Paul Burton
2016-11-07 18:27 ` Larry Finger
2016-11-08 13:21 ` revert 05fd007e46296afb (was: [PATCH v3] console: use first console if stdout-path device doesn't appear) Sergey Senozhatsky
2016-11-03 13:04 ` [PATCH v2] console: use first console if stdout-path device doesn't appear Paul Burton
2016-11-01 4:39 ` Michael Ellerman
2016-10-31 15:58 ` Larry Finger
2016-10-31 12:23 ` [PATCH] " Paul Burton
2016-10-18 9:21 ` [PATCH v2] console: Don't prefer first registered if DT specifies stdout-path Paul Burton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2317993.vxfTP7Yo3N@np-p-burton \
--to=paul.burton@imgtec$(echo .)com \
--cc=Larry.Finger@lwfinger$(echo .)net \
--cc=akpm@linux-foundation$(echo .)org \
--cc=bp@suse$(echo .)de \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=mpe@ellerman$(echo .)id.au \
--cc=pmladek@suse$(echo .)com \
--cc=schwab@linux-m68k$(echo .)org \
--cc=sergey.senozhatsky@gmail$(echo .)com \
--cc=tj@kernel$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox