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

[Finn Thain]

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
>