Re: [PATCH 08/17] tty: New RISC-V SBI console driver

[Palmer Dabbelt]

On Thu, 13 Jul 2017 05:32:26 PDT (-0700), james.hogan@xxxxxxxxxx wrote:
> On Thu, Jul 13, 2017 at 09:59:53PM +1000, Michael Ellerman wrote:
>> Palmer Dabbelt <palmer@xxxxxxxxxxx> writes:
>>
>> > On Wed, 12 Jul 2017 04:04:00 PDT (-0700), mpe@xxxxxxxxxxxxxx wrote:
>> >> Palmer Dabbelt <palmer@xxxxxxxxxxx> writes:
>> >>
>> >>> On Mon, 10 Jul 2017 23:21:07 PDT (-0700), mpe@xxxxxxxxxxxxxx wrote:
>> >>>> Palmer Dabbelt <palmer@xxxxxxxxxxx> writes:
>> >>>>>
>> >>>> ...
>> >>>>> +#ifdef CONFIG_EARLY_PRINTK
>> >>>>> +static void sbi_console_write(struct console *co, const char *buf,
>> >>>>> +			      unsigned int n)
>> >>>>> +{
>> >>>>> +	int i;
>> >>>>> +
>> >>>>> +	for (i = 0; i < n; ++i) {
>> >>>>> +		if (buf[i] == '\n')
>> >>>>> +			sbi_console_putchar('\r');
>> >>>>> +		sbi_console_putchar(buf[i]);
>> >>>>> +	}
>> >>>>> +}
>> >>>>> +
>> >>>>> +static struct console early_console_dev __initdata = {
>> >>>>> +	.name	= "early",
>> >>>>> +	.write	= sbi_console_write,
>> >>>>> +	.flags	= CON_PRINTBUFFER | CON_BOOT,
>> >>>>
>> >>>> AFAICS you could add CON_ANYTIME here, which would mean this console
>> >>>> would print output before the CPU is online.
>> >>>>
>> >>>> I think it doesn't currently matter because you call parse_early_param()
>> >>>> from setup_arch(), at which point the boot CPU has been marked online.
>> >>>>
>> >>>> But if this console can actually work earlier then you might be better
>> >>>> off just registering it unconditionally very early.
>> >>>
>> >>> That seems like a good idea.  I'm not familiar with how all this works, but
>> >>> from my understanding of this early_initcall() should be sufficient to make
>> >>> this work?  The only other driver that sets CON_ANYTIME and supports
>> >>> EARLY_PRINTK is hvc_xen, but that installs a header to let init code register
>> >>> the console directly.  The early_initcall mechanism seems cleaner if it does
>> >>> the right thing.
>> >>
>> >> Unfortunately early_initcall is not very "early" :)  It's earlier than
>> >> all the other initcalls, but it's late compared to most of the arch boot
>> >> code.
>> >>
>> >> The early_param() will work better, ie. register the console earlier
>> >> and increase the chance of you getting output from an early crash, than
>> >> early_initcall. But it requires you to put earlyprintk on the command line.
>> >>
>> >> The best option is to just register the console as early as you can, ie.
>> >> as soon as it can give you output. So somewhere in your setup_arch(), or
>> >> even earlier (I haven't read your boot code).
>> >
>> > Doing it that way would require either moving the TTY driver into arch code (it
>> > was specifically suggested we move it out) or adding a header file to allow
>> > setup_arch() to call into the driver (XEN does this, and we're doing it for our
>> > timer, but it seems hacky).
>>
>> I think it's fairly uncontroversial to have the early console in arch
>> code, especially in a case like this where there's no code shared
>> between the console and the TTY driver. But maybe someone will prove me
>> wrong.
>>
>> Doing it the other way is not really hacky IMO, you can just have an
>> extern for the early console in one of your asm headers.
>
> For reference both metag and mips do something like this for JTAG based
> consoles (with drivers both residing in drivers/tty/):
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/metag/kernel/setup.c#n107
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/metag/kernel/setup.c#n234
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/include/asm/cdmm.h#n98
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/setup.c#n958
>
> Its not all that pretty but it gets you console output that much
> earlier and is a fairly special case, so I think its worth it.

If someone else is doing it, then it's good enough for me :).  How does this
look?

diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 319fad96f537..148fd0dc414b 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -59,6 +59,14 @@ unsigned long pfn_base;
 /* The lucky hart to first increment this variable will boot the other cores */
 atomic_t hart_lottery;

+#if defined(CONFIG_HVC_RISCV_SBI) && defined(CONFIG_EARLY_PRINTK)
+/*
+ * The SBI's early console lives in hvc_riscv_sbi.c, but we want very early
+ * access
+ */
+extern struct console riscv_sbi_early_console_dev;
+#endif
+
 #ifdef CONFIG_BLK_DEV_INITRD
 static void __init setup_initrd(void)
 {
@@ -203,6 +211,13 @@ static void __init setup_bootmem(void)

 void __init setup_arch(char **cmdline_p)
 {
+#if defined(CONFIG_TTY_RISCV_SBI) && defined(CONFIG_EARLY_PRINTK)
+       if (likely(early_console == NULL)) {
+               early_console = &riscv_sbi_early_console;
+               register_console(early_console);
+       }
+#endif
+
 #ifdef CONFIG_CMDLINE_BOOL
 #ifdef CONFIG_CMDLINE_OVERRIDE
        strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
diff --git a/drivers/tty/hvc/hvc_riscv_sbi.c b/drivers/tty/hvc/hvc_riscv_sbi.c
index 534d6b75a2c6..20a6bfda4e32 100644
--- a/drivers/tty/hvc/hvc_riscv_sbi.c
+++ b/drivers/tty/hvc/hvc_riscv_sbi.c
@@ -84,20 +84,11 @@ static void sbi_console_write(struct console *co, const char *buf,
        }
 }

-static struct console early_console_dev __initdata = {
+/* This is used by arch/riscv/kernel/setup.c */
+struct console riscv_sbi_early_console_dev __initdata = {
        .name   = "early",
        .write  = sbi_console_write,
        .flags  = CON_PRINTBUFFER | CON_BOOT | CON_ANYTIME,
        .index  = -1
 };
-
-static int __init setup_early_printk(void)
-{
-       if (early_console == NULL) {
-               early_console = &early_console_dev;
-               register_console(early_console);
-       }
-       return 0;
-}
-early_initcall(setup_early_printk);
 #endif