linux-kernel - Re: [PATCH v8 24/25] powerpc: Adopt nvram module for PPC64

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:   Mon, 07 Jan 2019 10:36:10 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Arnd Bergmann <arnd@...db.de>,
        Finn Thain <fthain@...egraphics.com.au>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-m68k <linux-m68k@...ts.linux-m68k.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH v8 24/25] powerpc: Adopt nvram module for PPC64

Arnd Bergmann <arnd@...db.de> writes:
> On Wed, Dec 26, 2018 at 1:43 AM Finn Thain <fthain@...egraphics.com.au> wrote:
>
>> +static ssize_t ppc_nvram_get_size(void)
>> +{
>> +       if (ppc_md.nvram_size)
>> +               return ppc_md.nvram_size();
>> +       return -ENODEV;
>> +}
>
>> +const struct nvram_ops arch_nvram_ops = {
>> +       .read           = ppc_nvram_read,
>> +       .write          = ppc_nvram_write,
>> +       .get_size       = ppc_nvram_get_size,
>> +       .sync           = ppc_nvram_sync,
>> +};
>
> Coming back to this after my comment on the m68k side, I notice that
> there is now a double indirection through function pointers. Have you
> considered completely removing the operations from ppc_md instead
> by having multiple copies of nvram_ops?
>
> With the current method, it does seem odd to have a single
> per-architecture instance of the exported structure containing
> function pointers. This doesn't give us the flexibility of having
> multiple copies in the kernel the way that ppc_md does, but it adds
> overhead compared to simply exporting the functions directly.

Yeah TBH I'm not convinced the arch ops is the best solution.

Why can't each arch just implement the required ops functions? On ppc
we'd still use ppc_md but that would be a ppc detail.

Optional ops are fairly easy to support by providing a default
implementation, eg. instead of:

+	if (arch_nvram_ops.get_size == NULL)
+		return -ENODEV;
+
+	nvram_size = arch_nvram_ops.get_size();
+	if (nvram_size < 0)
+		return nvram_size;


We do in some header:

#ifndef arch_nvram_get_size
static inline int arch_nvram_get_size(void)
{
	return -ENODEV;
}
#endif

And then:

	nvram_size = arch_nvram_get_size();
	if (nvram_size < 0)
		return nvram_size;


But I haven't digested the whole series so maybe I'm missing something?

cheers