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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 14 Jul 2017 15:07:23 +1000
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Palmer Dabbelt <palmer@...belt.com>, james.hogan@...tec.com
Cc:     yamada.masahiro@...ionext.com, mmarek@...e.com,
        will.deacon@....com, peterz@...radead.org, boqun.feng@...il.com,
        mingo@...hat.com, daniel.lezcano@...aro.org, tglx@...utronix.de,
        jason@...edaemon.net, marc.zyngier@....com,
        gregkh@...uxfoundation.org, jslaby@...e.com, davem@...emloft.net,
        mchehab@...nel.org, sfr@...b.auug.org.au, fweisbec@...il.com,
        viro@...iv.linux.org.uk, mcgrof@...nel.org, dledford@...hat.com,
        bart.vanassche@...disk.com, sstabellini@...nel.org,
        daniel.vetter@...ll.ch, msalter@...hat.com,
        nicolas.dichtel@...nd.com, paul.gortmaker@...driver.com,
        linux@...ck-us.net, heiko.carstens@...ibm.com,
        schwidefsky@...ibm.com, linux-kernel@...r.kernel.org,
        patches@...ups.riscv.org, akpm@...ux-foundation.org,
        albert@...ive.com
Subject: Re: [PATCH 08/17] tty: New RISC-V SBI console driver

Palmer Dabbelt <palmer@...belt.com> writes:
> On Thu, 13 Jul 2017 05:32:26 PDT (-0700), james.hogan@...tec.com wrote:
>> On Thu, Jul 13, 2017 at 09:59:53PM +1000, Michael Ellerman wrote:
>>>
>>> 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/):
...
>>
>> 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)
                                        ^
                                        This is always true because you
                                        said so in your Kconfig.

> +/*
> + * 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

I would have put it in one of your arch headers, so that the hvc driver
can include it too.

Personally I tend not to bother #ifdef'ing every extern declaration, but
there are arguments both ways.

>  #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)
                      ^
                      HVC

> +       if (likely(early_console == NULL)) {

I don't think you need to check. You're the boss of early_console at
this point in boot.

> +               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);

cheers