| Message ID | 20161103125758.3415-1-paul.burton@imgtec.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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? [..] > @@ -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? > + */ > + 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 > + clear_of_specified_console(); > + register_console(of_fallback_console); > + } > + > for_each_console(con) { > if (!keep_bootcon && con->flags & CON_BOOT) { > /* > -- > 2.10.2 >
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) { > > > > /*
On Nov 03 2016, Paul Burton <paul.burton@imgtec.com> 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). > > In these cases try to ensure that we provide some console output by > enabling the first usable registered console, which we keep track of > with the of_fallback_console variable. Affected systems will enable > their console later than they did prior to commit 05fd007e4629 > ("console: don't prefer first registered if DT specifies stdout-path") > but should otherwise produce the same output. > > Tested in QEMU with a PowerPC pseries_defconfig kernel. Unfortunately, that still doesn't work on PowerMac. Andreas.
Hi Paul, On (11/03/16 21:17), Paul Burton wrote: > > [..] > > > + * 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. well, I suppose, in this case normally one would go and enable the missing .config option. no? -ss
Paul Burton <paul.burton@imgtec.com> writes: > 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). > > In these cases try to ensure that we provide some console output by > enabling the first usable registered console, which we keep track of > with the of_fallback_console variable. Affected systems will enable > their console later than they did prior to commit 05fd007e4629 > ("console: don't prefer first registered if DT specifies stdout-path") > but should otherwise produce the same output. > > Tested in QEMU with a PowerPC pseries_defconfig kernel. Hi Paul, This does "work", as in it boots and I get a console. But the delay in getting output on the VGA is not workable. I get pretty much no output until the machine is booted entirely to userspace, meaning any crash prior to that will be undebuggable. I also note Andreas reports it doesn't work at all on PowerMac. Please send a revert and we can try again next cycle. cheers
On Monday, 7 November 2016 19:27:32 GMT Michael Ellerman wrote: > Paul Burton <paul.burton@imgtec.com> writes: > > 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). > > > > In these cases try to ensure that we provide some console output by > > enabling the first usable registered console, which we keep track of > > with the of_fallback_console variable. Affected systems will enable > > their console later than they did prior to commit 05fd007e4629 > > ("console: don't prefer first registered if DT specifies stdout-path") > > but should otherwise produce the same output. > > > > Tested in QEMU with a PowerPC pseries_defconfig kernel. > > Hi Paul, > > This does "work", as in it boots and I get a console. But the delay in > getting output on the VGA is not workable. I get pretty much no output > until the machine is booted entirely to userspace, meaning any crash > prior to that will be undebuggable. > > I also note Andreas reports it doesn't work at all on PowerMac. > > Please send a revert and we can try again next cycle. > > cheers Hi Michael, A revert was already submitted by Hans de Goede & is being discussed over here: https://marc.info/?l=linux-kernel&m=147826151427455&w=2 Thanks, Paul
On 11/07/2016 03:18 AM, Paul Burton wrote: > On Monday, 7 November 2016 19:27:32 GMT Michael Ellerman wrote: >> Paul Burton <paul.burton@imgtec.com> writes: >>> 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). >>> >>> In these cases try to ensure that we provide some console output by >>> enabling the first usable registered console, which we keep track of >>> with the of_fallback_console variable. Affected systems will enable >>> their console later than they did prior to commit 05fd007e4629 >>> ("console: don't prefer first registered if DT specifies stdout-path") >>> but should otherwise produce the same output. >>> >>> Tested in QEMU with a PowerPC pseries_defconfig kernel. >> >> Hi Paul, >> >> This does "work", as in it boots and I get a console. But the delay in >> getting output on the VGA is not workable. I get pretty much no output >> until the machine is booted entirely to userspace, meaning any crash >> prior to that will be undebuggable. >> >> I also note Andreas reports it doesn't work at all on PowerMac. >> >> Please send a revert and we can try again next cycle. >> >> cheers > > Hi Michael, > > A revert was already submitted by Hans de Goede & is being discussed over > here: > > https://marc.info/?l=linux-kernel&m=147826151427455&w=2 I am a little surprised that I was not CCd on that thread. To reiterate, my PowerBook G4 with a PPC32 processor CRASHES on boot. That is a lot more serious than the console output disappearing. As it seems unlikely that this regression will be fixed in the current cycle, I recommend that the reversion of commit 05fd007e4629 until a proper fix is available. Larry
Hi Larry, On Monday, 7 November 2016 09:26:37 GMT Larry Finger wrote: > > A revert was already submitted by Hans de Goede & is being discussed over > > here: > > > > https://marc.info/?l=linux-kernel&m=147826151427455&w=2 > > I am a little surprised that I was not CCd on that thread. Hans started that thread without copying you just as you started your thread without copying Andreas, who reported issues first. > To reiterate, my > PowerBook G4 with a PPC32 processor CRASHES on boot. That is a lot more > serious than the console output disappearing. Crashes as in init dies due to lack of a console, right? > As it seems unlikely that this regression will be fixed in the current > cycle, I recommend that the reversion of commit 05fd007e4629 until a proper > fix is available. I agree that reverting is probably the best option for now, and have replied with that in the other thread too. Thanks, Paul
On 11/07/2016 11:21 AM, Paul Burton wrote: > Hi Larry, > > On Monday, 7 November 2016 09:26:37 GMT Larry Finger wrote: >>> A revert was already submitted by Hans de Goede & is being discussed over >>> here: >>> >>> https://marc.info/?l=linux-kernel&m=147826151427455&w=2 >> >> I am a little surprised that I was not CCd on that thread. > > Hans started that thread without copying you just as you started your thread > without copying Andreas, who reported issues first. My searching had failed to find his report. >> To reiterate, my >> PowerBook G4 with a PPC32 processor CRASHES on boot. That is a lot more >> serious than the console output disappearing. > > Crashes as in init dies due to lack of a console, right? It gets a kernel panic because of an attempt to kill process 1 (init). It then waits 120 seconds and tries to reboot. >> As it seems unlikely that this regression will be fixed in the current >> cycle, I recommend that the reversion of commit 05fd007e4629 until a proper >> fix is available. > > I agree that reverting is probably the best option for now, and have replied > with that in the other thread too. Good. Larry
Hello Andrew,
(just in case)
please revert 'commit 05fd007e46296afb24 ("console: don't prefer first
registered if DT specifies stdout-path")'
-ss
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index de08fc9..c86e6d0 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -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 (!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. + */ + clear_of_specified_console(); + register_console(of_fallback_console); + } + for_each_console(con) { if (!keep_bootcon && con->flags & CON_BOOT) { /*
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). In these cases try to ensure that we provide some console output by enabling the first usable registered console, which we keep track of with the of_fallback_console variable. Affected systems will enable their console later than they did prior to commit 05fd007e4629 ("console: don't prefer first registered if DT specifies stdout-path") but should otherwise produce the same output. Tested in QEMU with a PowerPC pseries_defconfig kernel. Signed-off-by: Paul Burton <paul.burton@imgtec.com> Fixes: 05fd007e4629 ("console: don't prefer first registered if DT specifies stdout-path") Reported-by: Andreas Schwab <schwab@linux-m68k.org> Reported-by: Larry Finger <Larry.Finger@lwfinger.net> Reported-by: Michael Ellerman <mpe@ellerman.id.au> Cc: Andreas Schwab <schwab@linux-m68k.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Borislav Petkov <bp@suse.de> Cc: Larry Finger <Larry.Finger@lwfinger.net> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Petr Mladek <pmladek@suse.com> Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> Cc: Tejun Heo <tj@kernel.org> Cc: linux-kernel@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org --- Changes in v3: - Be less invasive by simply calling register_console() again rather than splitting it. - Check for CON_CONSDEV on the first console driver rather than for CON_ENABLED on any. - Clean up the tracking of the fallback console. Changes in v2: - Split enable_console() out of register_console() & call in the fallback case. - Track the console we would have enabled as of_fallback_console. kernel/printk/printk.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-)