Re: [REGRESSION] "console: don't prefer first registered if DT specifies stdout-path" breaks console on video outputs of various ARM boards

[Paul Burton <paul.burton@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 4562 bytes --]

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?

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.

Thanks,
    Paul

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]