[PATCH] console: Don't prefer first registered if DT specifies stdout-path

[PATCH] console: Don't prefer first registered if DT specifies stdout-path

[Paul Burton]

If a device tree specifies a preferred device for kernel console output
via the stdout-path or linux,stdout-path chosen node properties or the
stdout alias then the kernel ought to honor it & output the kernel
console to that device. As it stands, this isn't the case. Whilst we
parse the stdout-path properties & set an of_stdout variable from
of_alias_scan(), and use that from of_console_check() to determine
whether to add a console device as a preferred console whilst
registering it, we also prefer the first registered console if no other
has been selected at the time of its registration.

This means that if a console other than the one the device tree selects
via stdout-path is registered first, we will switch to using it & when
the stdout-path console is later registered the call to
add_preferred_console() via of_console_check() is too late to do
anything useful. In practice this seems to mean that we switch to the
dummy console device fairly early & see no further console output:

    Console: colour dummy device 80x25
    console [tty0] enabled
    bootconsole [ns16550a0] disabled

Fix this by not automatically preferring the first registered console if
one is specified by the device tree. This allows consoles to be
registered but not enabled, and once the driver for the console selected
by stdout-path calls of_console_check() the driver will be added to the
list of preferred consoles before any other console has been enabled.
When that console is then registered via register_console() it will be
enabled as expected.

Signed-off-by: Paul Burton <paul.burton-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

---

 drivers/of/base.c       |  2 ++
 include/linux/console.h |  6 ++++++
 kernel/printk/printk.c  | 13 ++++++++++++-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ebf84e3..2ea6877 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1974,6 +1974,8 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
 			name = of_get_property(of_aliases, "stdout", NULL);
 		if (name)
 			of_stdout = of_find_node_opts_by_path(name, &of_stdout_options);
+		if (of_stdout)
+			console_set_by_of();
 	}
 
 	if (!of_aliases)
diff --git a/include/linux/console.h b/include/linux/console.h
index 98c8615..71cc04f 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -166,6 +166,12 @@ static inline void console_sysfs_notify(void)
 #endif
 extern bool console_suspend_enabled;
 
+#ifdef CONFIG_OF
+extern void console_set_by_of(void);
+#else
+static inline void console_set_by_of(void);
+#endif
+
 /* Suspend and resume console messages over PM events */
 extern void suspend_console(void);
 extern void resume_console(void);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 60cdf63..f1f03ca 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -149,6 +149,17 @@ static int preferred_console = -1;
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
 
+#ifdef CONFIG_OF
+static bool of_specified_console;
+
+void console_set_by_of(void)
+{
+	of_specified_console = true;
+}
+#else
+# define of_specified_console false
+#endif
+
 /* Flag: console code may call schedule() */
 static int console_may_schedule;
 
@@ -2509,7 +2520,7 @@ void register_console(struct console *newcon)
 	 *	didn't select a console we take the first one
 	 *	that registers here.
 	 */
-	if (preferred_console < 0) {
+	if (preferred_console < 0 && !of_specified_console) {
 		if (newcon->index < 0)
 			newcon->index = 0;
 		if (newcon->setup == NULL ||
-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] console: Don't prefer first registered if DT specifies stdout-path

[kbuild test robot]

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

Hi Paul,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.8-rc1 next-20160809]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Paul-Burton/console-Don-t-prefer-first-registered-if-DT-specifies-stdout-path/20160809-205816
config: sh-ecovec24_defconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sh 

All errors (new ones prefixed by >>):

   In file included from arch/sh/kernel/setup.c:14:0:
>> include/linux/console.h:179:20: error: 'console_set_by_of' declared 'static' but never defined [-Werror=unused-function]
    static inline void console_set_by_of(void);
                       ^
   cc1: all warnings being treated as errors

vim +179 include/linux/console.h

   173	#endif
   174	extern bool console_suspend_enabled;
   175	
   176	#ifdef CONFIG_OF
   177	extern void console_set_by_of(void);
   178	#else
 > 179	static inline void console_set_by_of(void);
   180	#endif
   181	
   182	/* Suspend and resume console messages over PM events */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 15889 bytes --]

Re: [PATCH] console: Don't prefer first registered if DT specifies stdout-path

[kbuild test robot]

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

Hi Paul,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.8-rc1 next-20160809]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Paul-Burton/console-Don-t-prefer-first-registered-if-DT-specifies-stdout-path/20160809-205816
config: x86_64-randconfig-s2-08092115 (attached as .config)
compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

>> include/linux/console.h:179: warning: 'console_set_by_of' declared 'static' but never defined
--
>> include/linux/console.h:179: warning: 'console_set_by_of' declared 'static' but never defined
   kernel/printk/printk.c: In function 'devkmsg_sysctl_set_loglvl':
   kernel/printk/printk.c:158: warning: 'old' may be used uninitialized in this function
--
>> include/linux/console.h:179: warning: 'console_set_by_of' declared 'static' but never defined
   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c: In function 'amdgpu_debugfs_regs_read':
   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:2182: warning: 'instance_bank' may be used uninitialized in this function
   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:2182: warning: 'sh_bank' may be used uninitialized in this function
   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:2182: warning: 'se_bank' may be used uninitialized in this function
--
>> include/linux/console.h:179: warning: 'console_set_by_of' declared 'static' but never defined
   drivers/gpu/drm/bochs/bochs_mm.c: In function 'bochs_gem_create':
   drivers/gpu/drm/bochs/bochs_mm.c:384: warning: 'bochsbo' may be used uninitialized in this function
--
>> include/linux/console.h:179: warning: 'console_set_by_of' declared 'static' but never defined
   drivers/tty/serial/serial_core.c: In function 'uart_start':
   drivers/tty/serial/serial_core.c:142: warning: 'flags' may be used uninitialized in this function
   drivers/tty/serial/serial_core.c: In function 'uart_stop':
   drivers/tty/serial/serial_core.c:121: warning: 'flags' may be used uninitialized in this function
   drivers/tty/serial/serial_core.c: In function 'uart_chars_in_buffer':
   drivers/tty/serial/serial_core.c:611: warning: 'flags' may be used uninitialized in this function
   drivers/tty/serial/serial_core.c: In function 'uart_write_room':
   drivers/tty/serial/serial_core.c:598: warning: 'flags' may be used uninitialized in this function
   drivers/tty/serial/serial_core.c: In function 'uart_put_char':
   drivers/tty/serial/serial_core.c:531: warning: 'flags' may be used uninitialized in this function
   drivers/tty/serial/serial_core.c: In function 'uart_flush_buffer':
   drivers/tty/serial/serial_core.c:624: warning: 'flags' may be used uninitialized in this function
   drivers/tty/serial/serial_core.c: In function 'uart_write':
   drivers/tty/serial/serial_core.c:559: warning: 'flags' may be used uninitialized in this function
--
>> include/linux/console.h:179: warning: 'console_set_by_of' declared 'static' but never defined
   drivers/tty/vt/vt_ioctl.c: In function 'vt_ioctl':
   drivers/tty/vt/vt_ioctl.c:825: warning: 'cc' may be used uninitialized in this function
--
>> include/linux/console.h:179: warning: 'console_set_by_of' declared 'static' but never defined
   drivers/tty/vt/consolemap.c: In function 'conv_uni_to_pc':
   drivers/tty/vt/consolemap.c:801: warning: 'h' may be used uninitialized in this function
--
>> include/linux/console.h:179: warning: 'console_set_by_of' declared 'static' but never defined
   drivers/tty/serial/8250/8250_port.c: In function 'serial8250_console_write':
   drivers/tty/serial/8250/8250_port.c:3110: warning: 'flags' may be used uninitialized in this function
--
>> include/linux/console.h:179: warning: 'console_set_by_of' declared 'static' but never defined
   drivers/video/fbdev/aty/aty128fb.c: In function 'aty128_decode_var':
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.bpp' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.depth' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.yoffset' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.xoffset' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.vyres' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.vxres' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.offset_cntl' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.pitch' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.v_sync_strt_wid' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.h_sync_strt_wid' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.v_total' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.h_total' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1567: warning: 'crtc.gen_cntl' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1569: warning: 'fifo_reg.dda_on_off' may be used uninitialized in this function
   drivers/video/fbdev/aty/aty128fb.c:1569: warning: 'fifo_reg.dda_config' may be used uninitialized in this function

vim +179 include/linux/console.h

   163	extern void console_start(struct console *);
   164	extern int is_console_locked(void);
   165	extern int braille_register_console(struct console *, int index,
   166			char *console_options, char *braille_options);
   167	extern int braille_unregister_console(struct console *);
   168	#ifdef CONFIG_TTY
   169	extern void console_sysfs_notify(void);
   170	#else
   171	static inline void console_sysfs_notify(void)
   172	{ }
   173	#endif
   174	extern bool console_suspend_enabled;
   175	
   176	#ifdef CONFIG_OF
   177	extern void console_set_by_of(void);
   178	#else
 > 179	static inline void console_set_by_of(void);
   180	#endif
   181	
   182	/* Suspend and resume console messages over PM events */
   183	extern void suspend_console(void);
   184	extern void resume_console(void);
   185	
   186	int mda_console_init(void);
   187	void prom_con_init(void);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23471 bytes --]

[PATCH v2] console: Don't prefer first registered if DT specifies stdout-path

[Paul Burton]

If a device tree specifies a preferred device for kernel console output
via the stdout-path or linux,stdout-path chosen node properties or the
stdout alias then the kernel ought to honor it & output the kernel
console to that device. As it stands, this isn't the case. Whilst we
parse the stdout-path properties & set an of_stdout variable from
of_alias_scan(), and use that from of_console_check() to determine
whether to add a console device as a preferred console whilst
registering it, we also prefer the first registered console if no other
has been selected at the time of its registration.

This means that if a console other than the one the device tree selects
via stdout-path is registered first, we will switch to using it & when
the stdout-path console is later registered the call to
add_preferred_console() via of_console_check() is too late to do
anything useful. In practice this seems to mean that we switch to the
dummy console device fairly early & see no further console output:

    Console: colour dummy device 80x25
    console [tty0] enabled
    bootconsole [ns16550a0] disabled

Fix this by not automatically preferring the first registered console if
one is specified by the device tree. This allows consoles to be
registered but not enabled, and once the driver for the console selected
by stdout-path calls of_console_check() the driver will be added to the
list of preferred consoles before any other console has been enabled.
When that console is then registered via register_console() it will be
enabled as expected.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>

---

Changes in v2:
- Declare, not just define, static dummy function.

 drivers/of/base.c       |  2 ++
 include/linux/console.h |  6 ++++++
 kernel/printk/printk.c  | 13 ++++++++++++-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 7792266..a930ec9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2011,6 +2011,8 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
 			name = of_get_property(of_aliases, "stdout", NULL);
 		if (name)
 			of_stdout = of_find_node_opts_by_path(name, &of_stdout_options);
+		if (of_stdout)
+			console_set_by_of();
 	}
 
 	if (!of_aliases)
diff --git a/include/linux/console.h b/include/linux/console.h
index d530c46..3672809 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -173,6 +173,12 @@ static inline void console_sysfs_notify(void)
 #endif
 extern bool console_suspend_enabled;
 
+#ifdef CONFIG_OF
+extern void console_set_by_of(void);
+#else
+static inline void console_set_by_of(void) {}
+#endif
+
 /* Suspend and resume console messages over PM events */
 extern void suspend_console(void);
 extern void resume_console(void);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index a5ef95c..2b4947c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -253,6 +253,17 @@ static int preferred_console = -1;
 int console_set_on_cmdline;
 EXPORT_SYMBOL(console_set_on_cmdline);
 
+#ifdef CONFIG_OF
+static bool of_specified_console;
+
+void console_set_by_of(void)
+{
+	of_specified_console = true;
+}
+#else
+# define of_specified_console false
+#endif
+
 /* Flag: console code may call schedule() */
 static int console_may_schedule;
 
@@ -2668,7 +2679,7 @@ void register_console(struct console *newcon)
 	 *	didn't select a console we take the first one
 	 *	that registers here.
 	 */
-	if (preferred_console < 0) {
+	if (preferred_console < 0 && !of_specified_console) {
 		if (newcon->index < 0)
 			newcon->index = 0;
 		if (newcon->setup == NULL ||
-- 
2.9.2

Re: [PATCH v2] console: Don't prefer first registered if DT specifies stdout-path

[Andrew Morton]

On Tue, 9 Aug 2016 16:19:37 +0100 Paul Burton <paul.burton-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:

> If a device tree specifies a preferred device for kernel console output
> via the stdout-path or linux,stdout-path chosen node properties or the
> stdout alias then the kernel ought to honor it & output the kernel
> console to that device. As it stands, this isn't the case. Whilst we
> parse the stdout-path properties & set an of_stdout variable from
> of_alias_scan(), and use that from of_console_check() to determine
> whether to add a console device as a preferred console whilst
> registering it, we also prefer the first registered console if no other
> has been selected at the time of its registration.
> 
> This means that if a console other than the one the device tree selects
> via stdout-path is registered first, we will switch to using it & when
> the stdout-path console is later registered the call to
> add_preferred_console() via of_console_check() is too late to do
> anything useful. In practice this seems to mean that we switch to the
> dummy console device fairly early & see no further console output:
> 
>     Console: colour dummy device 80x25
>     console [tty0] enabled
>     bootconsole [ns16550a0] disabled
> 
> Fix this by not automatically preferring the first registered console if
> one is specified by the device tree. This allows consoles to be
> registered but not enabled, and once the driver for the console selected
> by stdout-path calls of_console_check() the driver will be added to the
> list of preferred consoles before any other console has been enabled.
> When that console is then registered via register_console() it will be
> enabled as expected.

Looks reasonable.  Could you please do `grep -r stdout-path
Documentation' and check that everything therein is complete, accurate
and necessary?

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] console: Don't prefer first registered if DT specifies stdout-path

[Andreas Schwab]

On Aug 09 2016, Paul Burton <paul.burton-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:

> Fix this by not automatically preferring the first registered console if
> one is specified by the device tree. This allows consoles to be
> registered but not enabled, and once the driver for the console selected
> by stdout-path calls of_console_check() the driver will be added to the
> list of preferred consoles before any other console has been enabled.
> When that console is then registered via register_console() it will be
> enabled as expected.

This breaks the console on PowerMac.  There is no output and it panics
eventually.

Andreas.

-- 
Andreas Schwab, schwab-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] console: Don't prefer first registered if DT specifies stdout-path

[Paul Burton]

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

On Sunday, 16 October 2016 20:07:18 BST Andreas Schwab wrote:
> On Aug 09 2016, Paul Burton <paul.burton@imgtec.com> wrote:
> > Fix this by not automatically preferring the first registered console if
> > one is specified by the device tree. This allows consoles to be
> > registered but not enabled, and once the driver for the console selected
> > by stdout-path calls of_console_check() the driver will be added to the
> > list of preferred consoles before any other console has been enabled.
> > When that console is then registered via register_console() it will be
> > enabled as expected.
> 
> This breaks the console on PowerMac.  There is no output and it panics
> eventually.
> 
> Andreas.

Hi Andreas,

Could you share the device tree from your system?

Thanks,
    Paul

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

Re: [PATCH v2] console: Don't prefer first registered if DT specifies stdout-path

[Andreas Schwab]

On Okt 17 2016, Paul Burton <paul.burton-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:

> Could you share the device tree from your system?

This is the contents of chosen/linux,stdout-path on the systems I have:

chosen/linux,stdout-path
                 "/pci@f0000000/ATY,SnowyParent@10/ATY,Snowy_A@0"

chosen/linux,stdout-path                                                        
                 "/pci@0,f0000000/NVDA,Parent@10/NVDA,Display-B@1"

Is that what you need?  There is also chosen/stdout, but no
aliases/stdout.

Andreas.

-- 
Andreas Schwab, schwab-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] console: Don't prefer first registered if DT specifies stdout-path

[Paul Burton]

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

On Monday, 17 October 2016 19:39:57 BST Andreas Schwab wrote:
> On Okt 17 2016, Paul Burton <paul.burton@imgtec.com> wrote:
> > Could you share the device tree from your system?
> 
> This is the contents of chosen/linux,stdout-path on the systems I have:
> 
> chosen/linux,stdout-path
>                  "/pci@f0000000/ATY,SnowyParent@10/ATY,Snowy_A@0"
> 
> chosen/linux,stdout-path
>                  "/pci@0,f0000000/NVDA,Parent@10/NVDA,Display-B@1"
> 
> Is that what you need?  There is also chosen/stdout, but no
> aliases/stdout.
> 
> Andreas.

Hi Andreas,

I think I see the problem & I'm hoping this patch will fix it:

https://lkml.org/lkml/2016/10/18/142

Could you give it a try & let me know?

Thanks,
    Paul

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