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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Mon, 2014-03-31 at 15:08 +1100, Michael Neuling wrote:

> > +/* OPAL in-memory console. Defined in OPAL source at core/console.c */
> > +struct memcons {
> > +	__be64 magic;
> > +#define MEMCONS_MAGIC	0x6630696567726173L
> 
> 0x6630696567726173 == f0iegras ... Ben!!! :-P

Yummy ! :-)

> > +	__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?

Good point. out_pos should probably be read only once into a local
variable using the ACCESS_ONCE macro, and then only be broken up.

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

Unlikely but yes, it can happen.

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

On the other hand it doesn't cost much and if the FW goes bonkers it
will give us a better handle to debug from.

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

Cheers,
Ben.

> 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
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev