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

[Michael Ellerman <mpe@ellerman.id.au>]

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