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

[Michael Ellerman]

Arnd Bergmann <arnd@xxxxxxxx> writes:
> On Wed, Dec 26, 2018 at 1:43 AM Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx> 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