From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 3CDC32C009C for ; Wed, 9 Oct 2013 15:23:33 +1100 (EST) Message-ID: <1381292601.645.258.camel@pasglop> Subject: Re: [PATCH] powerpc/powernv: Add a debugfs file to read the firmware console From: Benjamin Herrenschmidt To: Michael Ellerman Date: Wed, 09 Oct 2013 15:23:21 +1100 In-Reply-To: <20131009032314.GD23780@concordia> References: <1381218400.645.204.camel@pasglop> <20131009032314.GD23780@concordia> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Cc: linuxppc-dev List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Wed, 2013-10-09 at 14:23 +1100, Michael Ellerman wrote: > On Tue, Oct 08, 2013 at 06:46:40PM +1100, Benjamin Herrenschmidt wrote: > > With OPALv3, the firmware can provide the address of it's internal console > > to Linux, which we can then display using debugfs. This is handy for > > diagnostics and debugging. > > > > Signed-off-by: Benjamin Herrenschmidt > > --- > > > > diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c > > index 2911abe..10d7894 100644 > > --- a/arch/powerpc/platforms/powernv/opal.c > > +++ b/arch/powerpc/platforms/powernv/opal.c > > @@ -17,6 +17,8 @@ > > #include > > #include > > #include > > +#include > > +#include > > #include > > #include > > > > @@ -27,6 +29,21 @@ struct opal { > > u64 entry; > > } opal; > > > > +/* OPAL in-memory console */ > > It might be nice to point out that the format of the struct is defined > by OPAL and must be in sync with what OPAL is using. Yes, we could move the structure definition to opal.h... > > +struct memcons { > > + uint64_t magic; > > u64 ? Who cares ? Especially if it goes into opal.h it should stick to the types used in that file. > > +#define MEMCONS_MAGIC 0x6630696567726173 > > + uint64_t obuf_phys; > > + uint64_t ibuf_phys; > > + uint32_t obuf_size; > > + uint32_t ibuf_size; > > + uint32_t out_pos; > > +#define MEMCONS_OUT_POS_WRAP 0x80000000u > > +#define MEMCONS_OUT_POS_MASK 0x00ffffffu > > Where does this come from? My a** :-) I made it up as I wrote the OPAL side one, why ? > > + uint32_t in_prod; > > + uint32_t in_cons; > > +}; > > Should it be packed? Nope, no need. It's all nice and naturally aligned. > > @@ -369,6 +386,90 @@ static irqreturn_t opal_interrupt(int irq, void *data) > > return IRQ_HANDLED; > > } > > > > +#ifdef CONFIG_DEBUG_FS > > +static ssize_t opal_memcons_read(struct file *file, char __user *to, > > + size_t count, loff_t *ppos) > > +{ > > + struct memcons *mc = file->private_data; > > + size_t available, ret, chunk0, chunk1, lcount; > > + const char *start, *conbuf = __va(mc->obuf_phys); > > + loff_t opos, pos; > > + > > + /* > > + * Find out how much is in the buffer. If it has wrapped > > + * the whole buffer, else just the beginning. It has wrapped > > + * if the next character is not \0 > > + */ > > + if (mc->out_pos & MEMCONS_OUT_POS_WRAP) { > > + available = mc->obuf_size; > > + chunk1 = mc->out_pos & MEMCONS_OUT_POS_MASK; > > + start = conbuf + chunk1; > > + chunk0 = mc->obuf_size - chunk1; > > + } else { > > + available = mc->out_pos; > > + start = conbuf; > > + chunk0 = available; > > + chunk1 = 0; > > + } > > Surely simple_read_from_buffer() could make some of this simpler? If you can find a way to make it deal with a ring buffer... Cheers, Ben.