[1/2] powerpc/powernv: new function to access OPAL msglog

Currently, the OPAL msglog/console buffer is exposed as a sysfs file, with
the sysfs read handler responsible for retrieving the log from the OPAL
buffer. We'd like to be able to use it in xmon as well.

Refactor the OPAL msglog code to create a new function, opal_msglog_copy(),
that copies to an arbitrary buffer.

Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
---
 arch/powerpc/include/asm/opal.h              |  2 ++
 arch/powerpc/platforms/powernv/opal-msglog.c | 15 +++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

On Wed, 2016-20-01 at 06:50:17 UTC, Andrew Donnellan wrote:
> Currently, the OPAL msglog/console buffer is exposed as a sysfs file, with
> the sysfs read handler responsible for retrieving the log from the OPAL
> buffer. We'd like to be able to use it in xmon as well.
> 
> Refactor the OPAL msglog code to create a new function, opal_msglog_copy(),
> that copies to an arbitrary buffer.

This is a good idea, but the implementation is not quite right, comments below.

> diff --git a/arch/powerpc/platforms/powernv/opal-msglog.c b/arch/powerpc/platforms/powernv/opal-msglog.c
> index 44ed78a..a06191c 100644
> --- a/arch/powerpc/platforms/powernv/opal-msglog.c
> +++ b/arch/powerpc/platforms/powernv/opal-msglog.c
> @@ -31,11 +31,11 @@ struct memcons {
>  	__be32 in_cons;
>  };
>  
> -static ssize_t opal_msglog_read(struct file *file, struct kobject *kobj,
> -				struct bin_attribute *bin_attr, char *to,
> -				loff_t pos, size_t count)
> +static struct bin_attribute opal_msglog_attr;
> +
> +ssize_t opal_msglog_copy(char *to, loff_t pos, size_t count)
>  {
> -	struct memcons *mc = bin_attr->private;
> +	struct memcons *mc = opal_msglog_attr.private;

Pulling the memcons out of the bin_attr here is not all that nice. This routine
should really stand on its own without reference to the bin_attr. In theory I
might want to disable building sysfs but still have this routine available.

It's also a bit fishy if it's called before the bin_attr is initialised or when
the memcons initialisation fails. In both cases it should be OK, because the
structs in question are static and so the private pointer will be NULL, but
that's a bit fragile.

I think the solution is simply to create a:

  static struct memcons *opal_memcons;

And use that in opal_msglog_copy() and so on.

cheers

On 08/02/16 22:31, Michael Ellerman wrote:
> Pulling the memcons out of the bin_attr here is not all that nice. This routine
> should really stand on its own without reference to the bin_attr. In theory I
> might want to disable building sysfs but still have this routine available.

Yeah it's a bit ugly, though does disabling sysfs actually break it? I 
can separate it out anyway - there's no reason for the memcons to be 
tied to the sysfs entry.

> It's also a bit fishy if it's called before the bin_attr is initialised or when
> the memcons initialisation fails. In both cases it should be OK, because the
> structs in question are static and so the private pointer will be NULL, but
> that's a bit fragile.
>
> I think the solution is simply to create a:
>
>    static struct memcons *opal_memcons;
>
> And use that in opal_msglog_copy() and so on.

Will respin.

On Tue, 2016-02-09 at 16:29 +1100, Andrew Donnellan wrote:
> On 08/02/16 22:31, Michael Ellerman wrote:
> > Pulling the memcons out of the bin_attr here is not all that nice. This routine
> > should really stand on its own without reference to the bin_attr. In theory I
> > might want to disable building sysfs but still have this routine available.
>
> Yeah it's a bit ugly, though does disabling sysfs actually break it?

Probably not, it looks like bin_attribute is still defined even when sysfs is
disabled. And the build would break in other places too.

> I can separate it out anyway - there's no reason for the memcons to be
> tied to the sysfs entry.

Yeah that was more my point.

> > It's also a bit fishy if it's called before the bin_attr is initialised or when
> > the memcons initialisation fails. In both cases it should be OK, because the
> > structs in question are static and so the private pointer will be NULL, but
> > that's a bit fragile.
> >
> > I think the solution is simply to create a:
> >
> >    static struct memcons *opal_memcons;
> >
> > And use that in opal_msglog_copy() and so on.
>
> Will respin.

Thanks.

cheers