Messages in this thread | | | Subject | Re: [REGRESSION] "console: don't prefer first registered if DT specifies stdout-path" breaks console on video outputs of various ARM boards | From | Hans de Goede <> | Date | Sun, 6 Nov 2016 11:54:35 +0100 |
| |
Hi,
On 05-11-16 11:40, Paul Burton wrote: > On Friday, 4 November 2016 14:22:17 GMT Hans de Goede wrote: >> Hi, >> >> On 04-11-16 13:30, Paul Burton wrote: >>> Hi Hans, >>> >>> On Friday, 4 November 2016 13:11:34 GMT Hans de Goede wrote: >>>> Hi All, >>>> >>>> While booting 4.9-rc# for the first time on an Allwinner A33 tablet, >>>> I noticed that after u-boot the LCD display stayed black. It turns out >>>> that there was an issue which caused X to never get up, and all kernel >>>> (and other startup) messages prior to that only went to ttyS0 which >>>> consists of 2 tiny testpads on the PCB with this tablet. >>>> >>>> The same issue will also happen on any ARM boards which have a HDMI or >>>> composite video output and which use a stdout-path pointing to their >>>> serial console. I think this will e.g. also impact the Raspberry Pi, >>>> I know for certain that this will impact the 99 different Allwinnner >>>> boards currently supported by mainline u-boot + the mainline kernel. >>>> >>>> This is a behavior changes from previous kernels and I consider this >>>> a regression. Thus I propose to revert the commit in question, for more >>>> info here is a partial copy of the commit message of the proposed revert: >>>> >>>> The reverted commit changes existing behavior on which many ARM boards >>>> rely. Many ARM small-board-computers, like e.g. the Raspberry Pi have >>>> both a video output and a serial console. Depending on whether the user >>>> is using the device as a more regular computer; or as a headless device >>>> we need to have the console on either one or the other. >>>> >>>> Many users rely on the kernel behavior of the console being present on >>>> both outputs, before the reverted commit the console setup with no >>>> console= kernel arguments on an ARM board which sets stdout-path in dt >>>> would look like this: >>>> >>>> [root@localhost ~]# cat /proc/consoles >>>> ttyS0 -W- (EC p a) 4:64 >>>> tty0 -WU (E p ) 4:1 >>>> >>>> Where as after the reverted commit, it looks like this: >>>> >>>> [root@localhost ~]# cat /proc/consoles >>>> ttyS0 -W- (EC p a) 4:64 >>>> >>>> This commit reverts commit 05fd007e4629 ("console: don't prefer first >>>> registered if DT specifies stdout-path") restoring the original behavior. >>>> >>>> Regards, >>>> >>>> Hans >>> >>> Ugh... so the devices you're talking about rely upon set stdout-path in >>> their device tree but effectively rely upon us ignoring it? >> >> No they rely on the kernel using stdout-path as an extra console while >> keeping tty0 as console, not ignoring it. This how stdout-path has always >> worked (at least as long as the Allwinner boards have used it, which has >> been 2 - 3 years now). >> >> If you want only the console specified by stdout-path you can get this by >> specifying it with console=... on the kernel cmdline. >> >>> If that's the case then I guess reverting is probably the best option, but >>> it does restore us to a position where we honor stdout-path for earlycon >>> & then essentially ignore it for the proper kernel console. That seems >>> pretty bust to me... >> >> We do not ignore it, we use both the tty pointed to by stdout-path and tty0. >> >> Regards, >> >> Hans > > Hi Hans, > > Could you walk me though how you're getting that behaviour from the current > code? I don't see how that would happen besides perhaps if drivers are probed > in a fortunate order. Is that what you're relying upon?
I guess so, I never looked carefully at this, it has just always worked until your patch.
> What I see in my systems, and what 05fd007e4629 ("console: don't prefer first > registered if DT specifies stdout-path") addressed, is that if there are for > example 2 UARTs uart0 & uart1 that are probed in that order and stdout-path > indicates that we should use uart1 we wind up essentially ignoring it because > the ordering of the relevant calls goes: > > - of_console_check() for uart0 > - add_preferred_console() for uart0 > - register_console() for uart0 > - of_console_check() for uart1 > - add_preferred_console() for uart1 > - register_console() for uart1 > > Since of_check_console() doesn't get called for uart1 until after uart0 has > been probed, we don't add an entry for it to the console_cmdline array until > after register_console() has already decided to enable uart0 because > preferred_console == -1. > > I'm not the only one seeing this oddity either, for example see the discussion > on this patch: > > https://patchwork.kernel.org/patch/9263753/ > > By simply reverting my patch you restore us to a position where so far as I > can see we simply do not honor stdout-path for the real kernel console.
As said before, we do still honor it, but in your probe example we also get a (second) serial console on uart0, where as you only want one on uart1.
So I see a few possible solutions here:
1) Do a new version of your patch which changes the "&& !of_specified_console" check to "&& (newcon == tty0 || !of_specified_console)", then we would still always register tty0 (as long as it gets registered first, just like now) and we would not register uart0 in your above example, note the "newcon == tty0" check in my example is pseudo-code. I would be happy to try out such a patch
2) Add a new dt property to enable the new behavior you seek
I'm myself tending towards 1 as a better solution: treat tty0 special, because some existing setups rely on it always being registered as a console even if stdout-path is specified and otherwise always honor stdout-path.
Regards,
Hans
| |