Subject: Re: [PATCH 1/2] powerpc/powernv: Add OPAL message log interface

From: Michael Neuling <[email-masked]>
Date: Mon, 31 Mar 2014 15:21:38 +1100

Michael Neuling <[email-masked]> wrote:

> Joel Stanley <[email-masked]> wrote:
> 
> > OPAL provides an in-memory circular buffer containing a message log
> > populated with various runtime messages produced by the firmware.
> > 
> > Provide a sysfs interface /sys/firmware/opal/messages for userspace to
> > view the messages.
> > 
> > Signed-off-by: Joel Stanley <[email-masked]>
> > ---
> >  arch/powerpc/include/asm/opal.h                |  4 ++
> >  arch/powerpc/platforms/powernv/Makefile        |  1 +
> >  arch/powerpc/platforms/powernv/opal-messages.c | 97 ++++++++++++++++++++++++++
> >  arch/powerpc/platforms/powernv/opal.c          |  4 +-
> >  4 files changed, 105 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/powerpc/platforms/powernv/opal-messages.c
> > 
> > diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> > index ffafab0..6aa757e 100644
> > --- a/arch/powerpc/include/asm/opal.h
> > +++ b/arch/powerpc/include/asm/opal.h
> > @@ -729,6 +729,9 @@ typedef struct oppanel_line {
> >  /* /sys/firmware/opal */
> >  extern struct kobject *opal_kobj;
> >  
> > +/* /ibm,opal */
> > +extern struct device_node *opal_node;
> > +
> >  /* API functions */
> >  int64_t opal_console_write(int64_t term_number, __be64 *length,
> >  			   const uint8_t *buffer);
> > @@ -918,6 +921,7 @@ extern void opal_flash_init(void);
> >  extern int opal_elog_init(void);
> >  extern void opal_platform_dump_init(void);
> >  extern void opal_sys_param_init(void);
> > +extern void opal_messages_init(void);
> >  
> >  extern int opal_machine_check(struct pt_regs *regs);
> >  extern bool opal_mce_check_early_recovery(struct pt_regs *regs);
> > diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
> > index f324ea0..e2ba418 100644
> > --- a/arch/powerpc/platforms/powernv/Makefile
> > +++ b/arch/powerpc/platforms/powernv/Makefile
> > @@ -1,6 +1,7 @@
> >  obj-y			+= setup.o opal-takeover.o opal-wrappers.o opal.o opal-async.o
> >  obj-y			+= opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
> >  obj-y			+= rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
> > +obj-y			+= opal-messages.o
> >  
> >  obj-$(CONFIG_SMP)	+= smp.o
> >  obj-$(CONFIG_PCI)	+= pci.o pci-p5ioc2.o pci-ioda.o
> > diff --git a/arch/powerpc/platforms/powernv/opal-messages.c b/arch/powerpc/platforms/powernv/opal-messages.c
> > new file mode 100644
> > index 0000000..3a863e8
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/opal-messages.c
> > @@ -0,0 +1,97 @@
> > +/*
> > + * PowerNV OPAL in-memory console interface
> > + *
> > + * Copyright 2014 IBM Corp.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +#include <asm/io.h>
> > +#include <asm/opal.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/of.h>
> > +#include <linux/types.h>
> > +
> > +/* OPAL in-memory console. Defined in OPAL source at core/console.c */
> > +struct memcons {
> > +	__be64 magic;
> > +#define MEMCONS_MAGIC	0x6630696567726173L
> 
> 0x6630696567726173 == f0iegras ... Ben!!! :-P
> 
> > +	__be64 obuf_phys;
> > +	__be64 ibuf_phys;
> > +	__be32 obuf_size;
> > +	__be32 ibuf_size;
> > +	__be32 out_pos;
> > +#define MEMCONS_OUT_POS_WRAP	0x80000000u
> > +#define MEMCONS_OUT_POS_MASK	0x00ffffffu
> > +	__be32 in_prod;
> > +	__be32 in_cons;
> > +};
> > +
> > +static ssize_t opal_messages_read(struct file *file, struct kobject *kobj,
> > +	struct bin_attribute *bin_attr, char *to, loff_t pos, size_t count)
> > +{
> > +	struct memcons *mc = bin_attr->private;
> > +	const char *conbuf;
> > +	bool wrapped;
> > +	size_t num_read;
> > +	int out_pos;
> > +
> > +	if (!mc)
> > +		return -ENODEV;
> > +
> > +	conbuf = phys_to_virt(be64_to_cpu(mc->obuf_phys));
> > +	wrapped = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_WRAP;
> > +	out_pos = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_MASK;
> > +
> 
> Are there ordering issues we need to think about here with reading
> these?  Can the messages be written on another CPU at the same time as
> these are being read?
> 
> What happens if in between reading wrapped and out_pos the buffer wraps?
> You'd end up getting only a few bytes of console?  Maybe you need to
> read wrapped before and after out_pos to make should it's not wrapped in
> between.

wrapped = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_WRAP;
out_pos = be32_to_cpu(mc->out_pos) & MEMCONS_OUT_POS_MASK;

OK, I just realised this is reading from the same location.  So yeah,
don't do that.  Read it once and calculate wrapped and out_pos from that
single read.

> 
> > +	if (!wrapped) {
> 
> Why the negative case first?  Just make it:
> 
>    if (wrapped) {
>       wrapped case
>    } else {
>       not wrapped case
>    }
> 
> Also, no curlies needed for single statement.
> 
> 
> > +		num_read = memory_read_from_buffer(to, count, &pos, conbuf,
> > +				out_pos);
> 
> This is probably not necessary, but do we need to sanity check out_pos <
> obuf_size?  I guess we don't generally sanity check numbers from OPAL as
> it can screw us in many other ways anyway.
> 
> > +	} else {
> > +		num_read = memory_read_from_buffer(to, count, &pos,
> > +				conbuf + out_pos,
> > +				be32_to_cpu(mc->obuf_size) - out_pos);
> > +
> > +		if (num_read < 0)
> > +			goto out;
> > +
> > +		num_read += memory_read_from_buffer(to + num_read,
> > +				count - num_read, &pos, conbuf,
> >                out_pos);
> 
> What if this second read returns an error?  num_read += -ERRNO?  I think
> you need to check this return independently.
> 
> Mikey
> 
> > +	}
> > +out:
> > +	return num_read;
> > +}
> > +
> > +static struct bin_attribute messages_attr = {
> > +	.attr = {.name = "messages", .mode = 0444},
> > +	.read = opal_messages_read
> > +};
> > +
> > +void __init opal_messages_init(void)
> > +{
> > +	u64 mcaddr;
> > +	struct memcons *mc;
> > +
> > +	if (of_property_read_u64(opal_node, "ibm,opal-memcons", &mcaddr)) {
> > +		pr_warn("OPAL: Property ibm,opal-memcons not found, no message log\n");
> > +		return;
> > +	}
> > +
> > +	mc = phys_to_virt(mcaddr);
> > +	if (!mc) {
> > +		pr_warn("OPAL: memory console address is invalid\n");
> > +		return;
> > +	}
> > +
> > +	if (be64_to_cpu(mc->magic) != MEMCONS_MAGIC) {
> > +		pr_warn("OPAL: memory console version is invalid\n");
> > +		return;
> > +	}
> > +
> > +	messages_attr.private = mc;
> > +
> > +	if (sysfs_create_bin_file(opal_kobj, &messages_attr) != 0)
> > +		pr_warn("OPAL: sysfs file creation failed\n");
> > +}
> > diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> > index e92f2f6..2bc032a 100644
> > --- a/arch/powerpc/platforms/powernv/opal.c
> > +++ b/arch/powerpc/platforms/powernv/opal.c
> > @@ -46,7 +46,7 @@ struct mcheck_recoverable_range {
> >  static struct mcheck_recoverable_range *mc_recoverable_range;
> >  static int mc_recoverable_range_len;
> >  
> > -static struct device_node *opal_node;
> > +struct device_node *opal_node;
> >  static DEFINE_SPINLOCK(opal_write_lock);
> >  extern u64 opal_mc_secondary_handler[];
> >  static unsigned int *opal_irqs;
> > @@ -574,6 +574,8 @@ static int __init opal_init(void)
> >  		opal_platform_dump_init();
> >  		/* Setup system parameters interface */
> >  		opal_sys_param_init();
> > +		/* Setup message log interface. */
> > +		opal_messages_init();
> >  	}
> >  
> >  	return 0;
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Linuxppc-dev mailing list
> > [email-masked]
> > https://lists.ozlabs.org/listinfo/linuxppc-dev