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

Adopt nvram module to reduce code duplication. This means CONFIG_NVRAM
becomes available to CONFIG_PPC64 builds (until now it was only PPC32).

The IOC_NVRAM_GET_OFFSET ioctl as implemented on PPC64 validates the offset
returned by pmac_get_partition(). Add this test to the nvram module.

Note that the old PPC32 generic_nvram module lacked this test.
So when CONFIG_PPC32 && CONFIG_PPC_PMAC, the IOC_NVRAM_GET_OFFSET ioctl
would have returned 0 (always). But when CONFIG_PPC64 && CONFIG_PPC_PMAC,
the IOC_NVRAM_GET_OFFSET ioctl would have returned -1 (which is -EPERM)
when the requested partition was not found.

With this patch, the result is now -EINVAL on both PPC32 and PPC64 when
the requested PowerMac NVRAM partition is not found. This is a userspace-
visible change, in the non-existent partition case, which would be in
an error path for an IOC_NVRAM_GET_OFFSET ioctl syscall.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Laurent Vivier <lvivier@redhat.com>
Tested-by: Stan Johnson <userm57@yahoo.com>
---
BTW, the IOC_NVRAM_SYNC ioctl call returns -EINVAL on PPC64. This patch
retains this behaviour though it might be better to actually perform a sync.
Both PPC64 and PPC32 kernels implement ppc_md.nvram_sync() for Core99,
but on PPC64 the ioctl is unimplemented (unlike PPC32).

Changed since v7:
 - Drop pointless comment edit.
---
 arch/powerpc/Kconfig                     |   3 +-
 arch/powerpc/kernel/nvram_64.c           | 185 +++++------------------
 arch/powerpc/platforms/powermac/Makefile |   2 -
 arch/powerpc/platforms/powermac/setup.c  |   2 +-
 arch/powerpc/platforms/powermac/time.c   |   2 +-
 arch/powerpc/platforms/pseries/nvram.c   |   2 -
 drivers/char/nvram.c                     |   2 +
 7 files changed, 40 insertions(+), 158 deletions(-)

On Wed, Dec 26, 2018 at 1:43 AM Finn Thain <fthain@telegraphics.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.

       Arnd

On Sat, 29 Dec 2018, Arnd Bergmann wrote:

> On Wed, Dec 26, 2018 at 1:43 AM Finn Thain <fthain@telegraphics.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?
> 

I considered a few alternatives. I figured that it was refactoring that 
could be deferred, as it would be confined to arch/powerpc. I was more 
interested in the cross-platform API.

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

You're right, there is overhead here.

With a bit of auditing, wrappers like the one you quoted (which merely 
checks whether or not a ppc_md method is implemented) could surely be 
avoided.

The arch_nvram_ops methods are supposed to optional (that is, they are 
allowed to be NULL).

We could call exactly the same function pointers though either ppc_md or 
arch_nvram_ops. That would avoid the double indirection.

On Sun, Dec 30, 2018 at 4:29 AM Finn Thain <fthain@telegraphics.com.au> wrote:
>
> On Sat, 29 Dec 2018, Arnd Bergmann wrote:
>
> > On Wed, Dec 26, 2018 at 1:43 AM Finn Thain <fthain@telegraphics.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?
> >
>
> I considered a few alternatives. I figured that it was refactoring that
> could be deferred, as it would be confined to arch/powerpc. I was more
> interested in the cross-platform API.

Fair enough.

> > 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.
> >
>
> You're right, there is overhead here.
>
> With a bit of auditing, wrappers like the one you quoted (which merely
> checks whether or not a ppc_md method is implemented) could surely be
> avoided.
>
> The arch_nvram_ops methods are supposed to optional (that is, they are
> allowed to be NULL).
>
> We could call exactly the same function pointers though either ppc_md or
> arch_nvram_ops. That would avoid the double indirection.

I think you can have a 'const' structure in the __ro_after_init section,
so without changing anything else, powerpc could just copy the
function pointers from ppc_md into the arch_nvram_ops at early
init time, which should ideally simplify your implementation as well.

        Arnd

On Mon, 31 Dec 2018, Arnd Bergmann wrote:

> On Sun, Dec 30, 2018 at 4:29 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> >
> > On Sat, 29 Dec 2018, Arnd Bergmann wrote:
> >
> > > On Wed, Dec 26, 2018 at 1:43 AM Finn Thain <fthain@telegraphics.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?
> > >
> >
> > I considered a few alternatives. I figured that it was refactoring 
> > that could be deferred, as it would be confined to arch/powerpc. I was 
> > more interested in the cross-platform API.
> 
> Fair enough.
> 
> > > 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.
> > >
> >
> > You're right, there is overhead here.
> >
> > With a bit of auditing, wrappers like the one you quoted (which merely 
> > checks whether or not a ppc_md method is implemented) could surely be 
> > avoided.
> >
> > The arch_nvram_ops methods are supposed to optional (that is, they are 
> > allowed to be NULL).
> >
> > We could call exactly the same function pointers though either ppc_md 
> > or arch_nvram_ops. That would avoid the double indirection.
> 
> I think you can have a 'const' structure in the __ro_after_init section, 
> so without changing anything else, powerpc could just copy the function 
> pointers from ppc_md into the arch_nvram_ops at early init time, which 
> should ideally simplify your implementation as well.
> 

This "early init time" could be hard to pin down... It has to be after 
ppc_md methods are initialized but before the nvram_ops methods get used 
(e.g. by the framebuffer console). Seems a bit fragile (?)

Your suggestion to completely remove the ppc_md.nvram* methods might be a 
better way. It just means functions get assigned to nvram_ops pointers 
instead of ppc_md pointers.

The patch is simple enough, but it assumes that arch_nvram_ops is not 
const. The struct machdep_calls ppc_md is not const, so should we worry 
about dropping the const for the struct nvram_ops arch_nvram_ops?

On Mon, 31 Dec 2018, Arnd Bergmann wrote:

> On Sun, Dec 30, 2018 at 4:29 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> >
> > On Sat, 29 Dec 2018, Arnd Bergmann wrote:
> >
> > > 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.
> > >
> >
> > You're right, there is overhead here.
> >
> > With a bit of auditing, wrappers like the one you quoted (which merely 
> > checks whether or not a ppc_md method is implemented) could surely be 
> > avoided.
> >
> > The arch_nvram_ops methods are supposed to optional (that is, they are 
> > allowed to be NULL).
> >
> > We could call exactly the same function pointers though either ppc_md 
> > or arch_nvram_ops. That would avoid the double indirection.
> 
> I think you can have a 'const' structure in the __ro_after_init section, 
> so without changing anything else, powerpc could just copy the function 
> pointers from ppc_md into the arch_nvram_ops at early init time, which 
> should ideally simplify your implementation as well.
> 

Does this require removing the 'const' from the powerpc arch_nvram_ops 
definition? That would mean removing the 'const' from the declaration in 
nvram.h, which means removing 'const' for every other instance of that 
struct too.

That's what happened when I tried removing the ppc_md.nvram_* methods 
entirely and assigning the same function pointers to arch_nvram_ops 
methods instead. Apparently all instances of arch_nvram_ops have to be 
const or none of them. Otherwise gcc says, "error: conflicting type 
qualifiers for 'arch_nvram_ops'".

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

On Mon, 7 Jan 2019, Michael Ellerman wrote:

> Arnd Bergmann <arnd@arndb.de> writes:
> > On Wed, Dec 26, 2018 at 1:43 AM Finn Thain <fthain@telegraphics.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?
> 

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.

Finn Thain <fthain@telegraphics.com.au> writes:
> On Mon, 7 Jan 2019, Michael Ellerman wrote:
>
>> Arnd Bergmann <arnd@arndb.de> writes:
>> > On Wed, Dec 26, 2018 at 1:43 AM Finn Thain <fthain@telegraphics.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?
>> 
>
> 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.)

Right, but we only need to make a runtime decision on powerpc (right?).
So it seems to me we should isolate that in the powerpc code.

> 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.)

I just don't see the advantage of having arch_nvram_ops which is a
structure of function pointers that are always the same on a given arch,
vs some static inlines that implement the same ops for that arch.

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

OK. I know you've been working on this series for a long time and I
don't want to roadblock it, so at the end of the day I don't feel that
strongly about it as long as the code works.

I'll wait for v9 and have another look then.

cheers

On Tue, 8 Jan 2019, Michael Ellerman wrote:

> > 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.)
> 
> Right, but we only need to make a runtime decision on powerpc (right?).

It's needed in many places outside of powerpc. Otherwise the caller can't 
determine at run-time which ops are implemented.

Hence you have to duplicate the caller for each supported configuration 
that you build.

Already, this precludes a shared misc device implementation and belies the 
"generic" in drivers/char/generic_nvram.c.

--