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

From: Finn Thain
Date: Mon Jan 07 2019 - 00:01:10 EST


On Mon, 7 Jan 2019, Michael Ellerman wrote:

> 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?
>

The reason why that doesn't work boils down to introspection. (This was
mentioned elsewhere in this email thread.) For example, we presently have
code like this,

ssize_t nvram_get_size(void)
{
if (ppc_md.nvram_size)
return ppc_md.nvram_size();
return -1;
}
EXPORT_SYMBOL(nvram_get_size);

This construction means we get to decide at run-time which of the NVRAM
functions should be used. (Whereas your example makes a build-time decision.)

The purpose of arch_nvram_ops is much the same. That is, it does for m68k
and x86 what ppc_md already does for ppc32 and ppc64. (And once these
platforms share an API like this, they can share more driver code, and
reduce duplicated code.)

The approach taken in this series was to push the arch_nvram_ops approach
as far as possible, because by making everything fit into that regime it
immediately became apparent where architectures and platforms have
diverged, creating weird inconsistencies like the differences in sync
ioctl behaviour between ppc32 and ppc64 for core99. (Early revisions of
this series exposed more issues like bugs and dead code that got addressed
elsewhere.)

Problem is, as Arnd pointed out, powerpc doesn't need both kinds of ops
struct. So I'm rewriting this series in such a way that powerpc doesn't
have to implement both. This rewrite is going to look totally different
for powerpc (though not for x86 or m68k) so you might want to wait for me
to post v9 before spending more time on code review.

Thanks.

--

> cheers
>