[Skiboot] [PATCH v3 3/4] libflash: blocklevel backend for MBOX flash access
Cyril Bur
cyril.bur at au1.ibm.com
Tue Feb 7 11:47:06 AEDT 2017
On Tue, 2017-02-07 at 11:43 +1100, Suraj Jitindar Singh wrote:
> On Tue, 2017-02-07 at 10:53 +1100, Cyril Bur wrote:
> > On Mon, 2017-02-06 at 18:44 +1100, Suraj Jitindar Singh wrote:
> > >
> > > On Thu, 2017-02-02 at 17:54 +1100, Cyril Bur wrote:
> > > >
> > > > The use MBOX protocol to request flash access from the BMC. Then
> > > > read/write to the 'flash' through the LPC bus.
> > > >
> > > > Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> > > > ---
> > > > libflash/Makefile.inc | 2 +-
> > > > libflash/mbox-flash.c | 562
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > libflash/mbox-flash.h | 24 +++
> > > > 3 files changed, 587 insertions(+), 1 deletion(-)
> > > > create mode 100644 libflash/mbox-flash.c
> > > > create mode 100644 libflash/mbox-flash.h
> > > >
> > > > diff --git a/libflash/Makefile.inc b/libflash/Makefile.inc
> > > > index 4db02a1f..ea64eb46 100644
> > > > --- a/libflash/Makefile.inc
> > > > +++ b/libflash/Makefile.inc
> > > > @@ -1,4 +1,4 @@
> > > > -LIBFLASH_SRCS = libflash.c libffs.c ecc.c blocklevel.c
> > > > +LIBFLASH_SRCS = libflash.c libffs.c ecc.c blocklevel.c mbox-
> > > > flash.c
> > > > LIBFLASH_OBJS = $(LIBFLASH_SRCS:%.c=%.o)
> > > >
> > > > SUBDIRS += libflash
> > > > diff --git a/libflash/mbox-flash.c b/libflash/mbox-flash.c
> > > > new file mode 100644
> > > > index 00000000..69456bcb
> > > > --- /dev/null
> > > > +++ b/libflash/mbox-flash.c
> > > > @@ -0,0 +1,562 @@
> > > > +/* Copyright 2017 IBM Corp.
> > > > + *
> > > > + * Licensed under the Apache License, Version 2.0 (the
> > > > "License");
> > > > + * you may not use this file except in compliance with the
> > > > License.
> > > > + * You may obtain a copy of the License at
> > > > + *
> > > > + * http://www.apache.org/licenses/LICENSE-2.0
> > > > + *
> > > > + * Unless required by applicable law or agreed to in writing,
> > > > software
> > > > + * distributed under the License is distributed on an "AS IS"
> > > > BASIS,
> > > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
> > > > or
> > > > + * implied.
> > > > + * See the License for the specific language governing
> > > > permissions
> > > > and
> > > > + * limitations under the License.
> > > > + */
> > > > +
> > > > +#define pr_fmt(fmt) "MBOX-FLASH: " fmt
> > > > +
> > > > +#define _GNU_SOURCE
> > > > +#include <errno.h>
> > > > +#include <stdio.h>
> > > > +#include <stdlib.h>
> > > > +#include <string.h>
> > > > +
> > > > +#include <skiboot.h>
> > > > +#include <timebase.h>
> > > > +#include <timer.h>
> > > > +#include <libflash/libflash.h>
> > > > +#include <libflash/mbox-flash.h>
> > > > +#include <lpc.h>
> > > > +#include <lpc-mbox.h>
> > > > +
> > > > +#include <ccan/container_of/container_of.h>
> > > > +
> > > > +#ifndef __SKIBOOT__
> > > > +#error "This libflash backend must be compiled with skiboot"
> > > > +#endif
> > > > +
> > > > +#define MBOX_DEFAULT_TIMEOUT 30
> > > > +
> > > > +struct lpc_window {
> > > > + uint32_t lpc;
> > >
> > > Can this be addr or lpc_addr?
> > > >
> > > > + uint32_t pos;
> > >
> > > Can this be flash_pos or flash_addr or cur_pos?
> >
> > Sure.
> >
> > >
> > > >
> > > > + uint32_t size;
> > > > + bool open;
> > > > +};
> > > > +
> > > > +struct mbox_flash_data {
> > > > + uint32_t shift;
> > > > + struct lpc_window read;
> > > > + struct lpc_window write;
> > > > + struct blocklevel_device bl;
> > > > + uint32_t total_size;
> > > > + uint32_t erase_granule;
> > > > + int rc;
> > > > + bool busy;
> > > > + struct bmc_mbox_msg msg_mem;
> > > > +};
> > > > +
> > > > +static uint16_t get_u16(uint8_t *ptr)
> > > > +{
> > > > + return le16_to_cpu(*(uint16_t *)ptr);
> > > > +}
> > > > +
> > > > +static void put_u16(uint8_t *ptr, uint16_t val)
> > > > +{
> > > > + uint16_t valt = cpu_to_le16(val);
> > >
> > > "valt"???
> > > Only thing I can thing of is you meant vault? How about tmp or
> > > tmp_val
> > > or i or something else?
> >
> > valt[emp]? I can make it whatever your heart desires.
> >
> > >
> > > >
> > > > + memcpy(ptr, &valt, sizeof(val));
> > > > +}
> > > > +
> > > > +static uint32_t get_u32(uint8_t *ptr)
> > > > +{
> > > > + return le32_to_cpu(*(uint32_t *)ptr);
> > > > +}
> > > > +
> > > > +static void put_u32(uint8_t *ptr, uint32_t val)
> > > > +{
> > > > + uint32_t valt = cpu_to_le32(val);
> > >
> > > see above
> > > >
> > > > + memcpy(ptr, &valt, sizeof(val));
> > > > +}
> > > > +
> > >
> > > I would prefer get_msg_zeroed or something
> > > >
> > > > +static struct bmc_mbox_msg *get_msg_memory(struct
> > > > mbox_flash_data
> > > > *lpc)
> > > > +{
> > > > + /*
> > > > + * Yes this causes *slow*.
> > >
> > > Please reword for the better use english of
> >
> > Slow the memset causes.
>
> Better the very much
> >
> > >
> > > >
> > > > + * This file and lpc-mbox have far greater slow points,
> > > > zeroed
> > > > + * data regs are VERY useful for debugging. Think twice
> > > > if
> > > > this is
> > > > + * really the performance optimisation you want to make.
> > > > + */
> > > > + memset(&lpc->msg_mem, 0, sizeof(lpc->msg_mem));
> > > > + return &lpc->msg_mem;
> > > > +}
> > > > +
> > > > +static void free_msg_memory(struct bmc_mbox_msg *mem __unused)
> > > > +{
> > > > + /* Allocation is so simple this isn't required */
> > >
> > > Can we remove this then?
> >
> > In the interest of getting this going I made the alloc/free functions
> > totally trivial using memory in the struct. I expect in the future we
> > will rework this, especially when we have the ability to have
> > multiple
> > in flight messages. Since this will live for the life of skiboot and
> > may get called a fair bit I wanted to avoid malloc to not completely
> > fragment the heap.
> >
> > I've gone to quite a bit of care to match the allocations with a free
> > so that changing the underlying allocation hopefully won't require
> > surgery in this file, just adding whatever logic is required to these
> > two functions.
>
> Sounds like these should hang around to make future updates easier
> then.
> >
> > >
> > > >
> > > > +}
> > > > +
> > > > +static bool wait_for_bmc(struct mbox_flash_data *lpc, unsigned
> > > > int
> > > > timeout_sec)
> > > > +{
> > > > + unsigned long last = 1, start = tb_to_secs(mftb());
> > > > + prlog(PR_DEBUG, "Waiting for BMC\n");
> > > > + while (lpc->busy && timeout_sec) {
> > > > + long now = tb_to_secs(mftb());
> > > > + if (now - start > last) {
> > > > + timeout_sec--;
> > > > + last = now - start;
> > > > + if (last < timeout_sec / 2)
> > > > + prlog(PR_TRACE, "Been waiting
> > > > for
> > > > the BMC for %lu secs\n", last);
> > > > + else
> > > > + prlog(PR_ERR, "BMC NOT
> > > > RESPONDING
> > > > %lu second wait\n", last);
> > > > + }
> > > > + /*
> > > > + * Both functions are important.
> > > > + * Well time_wait_ms() relaxes the spin... so...
> > > > its
> > > > nice
> > > > + */
> > > > + time_wait_ms(MBOX_DEFAULT_POLL_MS);
> > > > + check_timers(false);
> > > > + asm volatile ("" ::: "memory");
> > > > + }
> > > > +
> > > > + return lpc->busy;
> > > > +}
> > > > +
> > > > +static int lpc_window_read(struct mbox_flash_data *lpc, uint32_t
> > > > pos, void *buf, uint32_t len)
> > > > +{
> > > > + int rc;
> > > > + uint32_t off = lpc->read.lpc + (pos - lpc->read.pos);
> > > > +
> > > > + prlog(PR_TRACE, "Reading at 0x%08x for 0x%08x offset:
> > > > 0x%08x\n",
> > > > + pos, len, off);
> > > > +
> > > > + while(len) {
> > > > + uint32_t chunk;
> > > > + uint32_t dat;
> > > > +
> > > > + /* Choose access size */
> > > > + if (len > 3 && !(off & 3)) {
> > > > + rc = lpc_read(OPAL_LPC_FW, off, &dat,
> > > > 4);
> > > > + if (!rc)
> > > > + *(uint32_t *)buf = dat;
> > > > + chunk = 4;
> > >
> > > You can only read/write 4 bytes at a time?! *ouch* Might want to
> > > note
> > > this in a comment.
> >
> > Yes, but that is only true on P8, with P9 being memory mapped I
> > should
> > double check that an abstraction hasn't appeared that I can make use
> > of.
> >
> > >
> > > >
> > > > + } else {
> > > > + rc = lpc_read(OPAL_LPC_FW, off, &dat,
> > > > 1);
> > > > + if (!rc)
> > > > + *(uint8_t *)buf = dat;
> > > > + chunk = 1;
> > > > + }
> > > > + if (rc) {
> > > > + prlog(PR_ERR, "lpc_read failure %d to FW
> > > > 0x%08x\n", rc, off);
> > > > + return rc;
> > > > + }
> > > > + len -= chunk;
> > > > + off += chunk;
> > > > + buf += chunk;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int lpc_window_write(struct mbox_flash_data *lpc,
> > > > uint32_t
> > > > pos, const void *buf, uint32_t len)
> > > > +{
> > > > + uint32_t off = lpc->write.lpc + (pos - lpc->write.pos);
> > > > + int rc;
> > > > +
> > > > +
> > > > + prlog(PR_TRACE, "Writing at 0x%08x for 0x%08x offset:
> > > > 0x%08x\n",
> > > > + pos, len, off);
> > > > +
> > > > + while(len) {
> > > > + uint32_t chunk;
> > > > +
> > > > + if (len > 3 && !(off & 3)) {
> > > > + rc = lpc_write(OPAL_LPC_FW, off,
> > > > + *(uint32_t *)buf, 4);
> > > > + chunk = 4;
> > > > + } else {
> > > > + rc = lpc_write(OPAL_LPC_FW, off,
> > > > + *(uint8_t *)buf, 1);
> > > > + chunk = 1;
> > > > + }
> > > > + if (rc) {
> > > > + prlog(PR_ERR, "lpc_write failure %d to
> > > > FW
> > > > 0x%08x\n", rc, off);
> > > > + return rc;
> > > > + }
> > > > + len -= chunk;
> > > > + off += chunk;
> > > > + buf += chunk;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void write_flush_cb(struct bmc_mbox_msg *msg)
> > > > +{
> > > > + struct mbox_flash_data *lpc;
> > > > +
> > > > + lpc = msg->priv;
> > > > +
> > > > + prlog(PR_DEBUG, "WRITE_FLUSH CB, BMC OK\n");
> > > > +
> > > > + if (msg->response != MBOX_R_SUCCESS) {
> > > > + prlog(PR_ERR, "Bad response code from BMC %d\n",
> > > > msg->response);
> > > > + lpc->rc = msg->response;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + lpc->rc = 0;
> > > > +
> > > > +out:
> > > > + lpc->busy = false;
> > > > +}
> > > > +
> > > > +static void write_cb(struct bmc_mbox_msg *msg)
> > > > +{
> > > > + struct mbox_flash_data *lpc;
> > > > +
> > > > + lpc = msg->priv;
> > > > +
> > > > + prlog(PR_DEBUG, "CREATE_WRITE_WINDOW CB, BMC OK\n");
> > > > +
> > > > + if (msg->response != MBOX_R_SUCCESS) {
> > > > + prlog(PR_ERR, "Bad response code from BMC %d\n",
> > > > msg->response);
> > > > + lpc->rc = msg->response;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + lpc->write.lpc = get_u16(&msg->data[0]) << lpc->shift;
> > > > + lpc->write.open = true;
> > > > + lpc->read.open = false;
> > > > +
> > > > + lpc->rc = 0;
> > > > +
> > > > +out:
> > > > + lpc->busy = false;
> > > > +}
> > > > +
> > > > +static int mbox_flash_write(struct blocklevel_device *bl,
> > > > uint64_t
> > > > pos,
> > > > + const void *buf, uint64_t len)
> > > > +{
> > > > + struct mbox_flash_data *lpc;
> > > > + struct bmc_mbox_msg *msg;
> > > > + int rc;
> > > > +
> > > > + /* LPC is only 32bit */
> > > > + if (pos > UINT_MAX || len > UINT_MAX)
> > > > + return FLASH_ERR_PARM_ERROR;
> > > > +
> > > > + lpc = container_of(bl, struct mbox_flash_data, bl);
> > > > + msg = get_msg_memory(lpc);
> > > > + if (!msg)
> > > > + return FLASH_ERR_MALLOC_FAILED;
> > > > +
> > > > + prlog(PR_TRACE, "Flash write at 0x%08llx for
> > > > 0x%08llx\n",
> > > > pos, len);
> > > > + if (!lpc->write.open || pos < lpc->write.pos || pos +
> > > > len >
> > > > lpc->write.pos + lpc->write.size) {
> > >
> > > See comments in read function below, I believe this will suffer
> > > from
> > > the same problems.
> > > >
> > > > + prlog(PR_INFO, "Adjusting the write window\n");
> > > > +
> > > > + lpc->write.pos = pos & ~(lpc->shift - 1);
> > > > + msg->priv = lpc;
> > > > + msg->command = MBOX_C_CREATE_WRITE_WINDOW;
> > > > + msg->callback = &write_cb;
> > > > + put_u16(&msg->data[0], pos >> lpc->shift);
> > > > +
> > > > + lpc->busy = true;
> > > > + rc = bmc_mbox_enqueue(msg);
> > > > + if (rc) {
> > > > + prlog(PR_ERR, "Failed to enqueue/send
> > > > BMC
> > > > MBOX message\n");
> > > > + rc = FLASH_ERR_MALLOC_FAILED; /* Not
> > > > necessarily the reason... */
> > > > + goto out;
> > > > + }
> > > > +
> > > > + if (wait_for_bmc(lpc, MBOX_DEFAULT_TIMEOUT)) {
> > > > + prlog(PR_ERR, "Timeout waiting for
> > > > BMC\n");
> > > > + rc = FLASH_ERR_CTRL_TIMEOUT;
> > > > + return rc;
> > > > + }
> > > > +
> > > > + if (lpc->rc) {
> > > > + rc = lpc->rc;
> > > > + goto out;
> > > > + }
> > > > + }
> > > > +
> > > > + rc = lpc_window_write(lpc, pos, buf, len);
> > > > + if (rc)
> > > > + return rc;
> > > > +
> > >
> > > Can the whole *flush* business be a separate function please?
> >
> > Sure
> >
> > >
> > > >
> > > > + msg->priv = lpc;
> > > > + msg->command = MBOX_C_WRITE_FLUSH;
> > > > + msg->callback = &write_flush_cb;
> > > > + put_u16(&msg->data[0], pos >> lpc->shift);
> > > > + put_u32(&msg->data[2], len);
> > > > +
> > > > + lpc->busy = true;
> > > > + rc = bmc_mbox_enqueue(msg);
> > > > + if (rc) {
> > > > + prlog(PR_ERR, "Failed to enqueue/send BMC MBOX
> > > > message\n");
> > > > + rc = FLASH_ERR_MALLOC_FAILED; /* Not necessarily
> > > > the
> > > > reason... */
> > > > + goto out;
> > > > + }
> > > > +
> > > > + if (wait_for_bmc(lpc, MBOX_DEFAULT_TIMEOUT)) {
> > > > + prlog(PR_ERR, "Timeout waiting for BMC\n");
> > > > + rc = FLASH_ERR_CTRL_TIMEOUT;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + rc = lpc->rc;
> > > > +
> > > > +out:
> > > > + free_msg_memory(msg);
> > > > + return rc;
> > > > +}
> > > > +
> > > > +static void read_cb(struct bmc_mbox_msg *msg)
> > > > +{
> > > > + struct mbox_flash_data *lpc;
> > > > +
> > > > + lpc = msg->priv;
> > > > +
> > > > + prlog(PR_DEBUG, "CREATE_READ_WINDOW CB, BMC OK\n");
> > > > +
> > > > + if (msg->response != MBOX_R_SUCCESS) {
> > > > + prlog(PR_ERR, "Bad response code from BMC %d\n",
> > > > msg->response);
> > > > + lpc->rc = msg->response;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + lpc->read.lpc = get_u16(&msg->data[0]) << lpc->shift;
> > > > + lpc->read.open = true;
> > > > + lpc->write.open = false;
> > > > +
> > > > + lpc->rc = 0;
> > > > +out:
> > > > + lpc->busy = false;
> > > > +}
> > > > +
> > > > +static int mbox_flash_read(struct blocklevel_device *bl,
> > > > uint64_t
> > > > pos, void *buf, uint64_t len)
> > > > +{
> > > > + struct mbox_flash_data *lpc;
> > > > + int rc = 0;
> > > > +
> > > > + /* LPC is only 32bit */
> > > > + if (pos > UINT_MAX || len > UINT_MAX)
> > > > + return FLASH_ERR_PARM_ERROR;
> > > > +
> > > > + lpc = container_of(bl, struct mbox_flash_data, bl);
> > > > +
> > > > + prlog(PR_TRACE, "Flash read at 0x%08llx for 0x%08llx\n",
> > > > pos, len);
> > > > + if (!lpc->read.open || pos < lpc->read.pos || pos + len
> > > > >
> > > >
> > > > lpc->read.pos + lpc->read.size) {
> > >
> > > What if len > lpc->read.size as far as I can tell this will
> > > currently
> > > just keep reading past the end of the window into unknown lpc
> > > address
> > > space. Shouldn't you check, if len < lpc->read.size then current
> > > code
> > > is fine. Otherwise you need to open a window, read lpc->read.size
> > > bytes
> > > into buffer, len -= lpc->read.size && pos+=lpc->read.size, close
> > > window
> > > and repeat while len > 0. You also probably want to check total
> > > size to
> > > be sure your not trying to open a window past the end of flash as
> > > well
> > > I don't think anything currently stops you from doing this?! Maybe
> > > return end of file if you try read past the end of flash?
> > >
> >
> > So I see two problems.
> > 1. If I follow, I don't check that the read fits within a window. I
> > whether or not I have a window that can handle that read but in the
> > case of window sizes being smaller than the read I'll read past the
> > end
> > of the window. Good catch.
>
> Yep exactly.
> > 2. Reading beyond the end of flash. I think that might actually get
> > caught by fixing 1. in that we'll realize we can't open a window that
> > far up and bail. As much as the typical linux read() style returning
> > what I did read would be nice, I think we'll just bail if you read
> > too
> > far, I don't see skiboot making use of that feature.
>
> And as discussed the daemon should stop you doing this anyway.
> >
> > >
> > > Alternatively can you just return the amount you did read (so up to
> > > a
> > > max of lpc->read.size e.g. len = len > lpc->read.size ? lpc-
> > > > read.size
> > >
> > > : len;) and the caller will auto retry with an incremented pos and
> > > a
> > > decremented len link the linux read/write calls?
> > > >
> > > > + struct bmc_mbox_msg *msg = get_msg_memory(lpc);
> > > > + if (!msg)
> > > > + return FLASH_ERR_MALLOC_FAILED;
> > > > +
> > > > + prlog(PR_INFO, "Adjusting the read window\n");
> > > > +
> > > > + lpc->read.pos = pos & ~(lpc->shift - 1);
> > > > + msg->priv = lpc;
> > > > + msg->command = MBOX_C_CREATE_READ_WINDOW;
> > > > + msg->callback = &read_cb;
> > > > + put_u16(&msg->data[0], pos >> lpc->shift);
> > > > +
> > > > + lpc->busy = true;
> > > > + rc = bmc_mbox_enqueue(msg);
> > > > + if (rc) {
> > > > + prlog(PR_ERR, "Failed to enqueue/send
> > > > BMC
> > > > MBOX message\n");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + if (wait_for_bmc(lpc, MBOX_DEFAULT_TIMEOUT)) {
> > > > + prlog(PR_ERR, "Timeout waiting for
> > > > BMC\n");
> > > > + rc = FLASH_ERR_CTRL_TIMEOUT;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + rc = lpc->rc;
> > > > +out:
> > > > + free_msg_memory(msg);
> > > > + }
> > > > +
> > > > + return rc ? rc : lpc_window_read(lpc, pos, buf, len);
> > > > +}
> > > > +
> > > > +static void get_info_cb(struct bmc_mbox_msg *msg)
> > > > +{
> > > > + struct mbox_flash_data *lpc;
> > > > +
> > > > + prlog(PR_DEBUG, "GET_FLASH_INFO CB, BMC OK\n");
> > > > +
> > > > + lpc = msg->priv;
> > > > +
> > > > + if (msg->response != MBOX_R_SUCCESS) {
> > > > + prlog(PR_ERR, "Bad response code from BMC %d\n",
> > > > msg->response);
> > > > + lpc->rc = msg->response;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + lpc->total_size = get_u32(&msg->data[0]);
> > > > + lpc->erase_granule = get_u32(&msg->data[4]);
> > > > +
> > > > + lpc->rc = 0;
> > > > +out:
> > > > + lpc->busy = false;
> > > > +}
> > > > +
> > > > +static int mbox_flash_get_info(struct blocklevel_device *bl,
> > > > const
> > > > char **name,
> > > > + uint64_t *total_size, uint32_t *erase_granule)
> > > > +{
> > > > + struct mbox_flash_data *lpc;
> > > > + struct bmc_mbox_msg *msg;
> > > > + int rc;
> > > > +
> > > > + lpc = container_of(bl, struct mbox_flash_data, bl);
> > > > + msg = get_msg_memory(lpc);
> > > > + if (!msg)
> > > > + return FLASH_ERR_MALLOC_FAILED;
> > >
> > > Can this really happen?
> >
> > Not as it stands but perhaps of allocation method changes
>
> Sounds like this should stay then.
> >
> > >
> > > >
> > > > +
> > > > + msg->command = MBOX_C_GET_FLASH_INFO;
> > > > + msg->callback = &get_info_cb;
> > > > + msg->priv = lpc;
> > >
> > > I guess you want to be sure, but do you have to set this everytime?
> > > >
> > > > + msg->data[0] = 1; /* V1, do better */
> > >
> > > Should the commands be defined somewhere? I don't like this *magic*
> > > 1.
> >
> > Well, it is version, one.
> > #define ONE 1
> > ?
>
> Yeah and while you're at it
> #define TWO 2
> #define THREE 3
> #define FOUR 4
> #define BUNNY /) /) \
> ( ^.^ ) \
> C(") (")
> ...
> My bad, I thought this was the command byte.
> AFAIK the get flash info command doesn't take any data (args)? Are you
> confusing this with the get info command which takes a version number?
Ah yes, the real issue is that theres a copy paste issue here :), I'll
remove it from that command. You're correct GET_FLASH_INFO doesn't take
args.
> >
> > >
> > > >
> > > > +
> > > > + /*
> > > > + * We want to avoid runtime mallocs in skiboot. The
> > > > expected
> > > > + * behavour to uses of libflash is that one can free()
> > > > the
> > > > memory
> > > > + * returned.
> > > > + * NULL will do for now.
> > > > + */
> > > > + if (name)
> > > > + *name = NULL;
> > > > +
> > > > + lpc->busy = true;
> > > > + rc = bmc_mbox_enqueue(msg);
> > > > + if (rc) {
> > > > + prlog(PR_ERR, "Failed to enqueue/send BMC MBOX
> > > > message\n");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + if (wait_for_bmc(lpc, MBOX_DEFAULT_TIMEOUT)) {
> > > > + prlog(PR_ERR, "Timeout waiting for BMC\n");
> > > > + rc = FLASH_ERR_CTRL_TIMEOUT;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + rc = lpc->rc;
> > > > + if (rc)
> > > > + goto out;
> > > > +
> > > > + lpc->bl.erase_mask = lpc->erase_granule - 1;
> > > > +
> > > > + if (total_size)
> > > > + *total_size = lpc->total_size;
> > > > + if (erase_granule)
> > > > + *erase_granule = lpc->erase_granule;
> > > > +
> > > > +out:
> > > > + free_msg_memory(msg);
> > > > + return rc;
> > > > +}
> > > > +
> > > > +static int mbox_flash_erase(struct blocklevel_device *bl
> > > > __unused,
> > > > uint64_t pos __unused, uint64_t len __unused)
> > > > +{
> > > > + /*
> > > > + * We can probably get away with doing nothing.
> > > > + * TODO: Rethink this, causes interesting behaviour in
> > > > pflash.
> > > > + * Users do expect pflash -{e,E} to do something
> > > > + */
> > >
> > > What is the expected behaviour of erase, should it be erasing the
> > > flash? If so shouldn't this write zeros, dirty the blocks and
> > > request a
> > > flush.
> >
> > The reason for this is that the blocklevel abstraction was designed
> > to
> > abstract out flash. As you know flash requires an erase before a
> > write,
> > so really calls to blocklevel erase means "prepare the flash for a
> > write here" hence why we don't need to do anything
> >
> > >
> > > >
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void init_cb(struct bmc_mbox_msg *msg)
> > > > +{
> > > > + struct mbox_flash_data *lpc;
> > > > +
> > > > + prlog(PR_DEBUG, "INIT CB, BMC OK\n");
> > > > +
> > > > + lpc = msg->priv;
> > > > +
> > > > + if (msg->response != MBOX_R_SUCCESS) {
> > > > + prlog(PR_ERR, "Bad response code from BMC %d\n",
> > > > msg->response);
> > > > + lpc->rc = msg->response;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + lpc->read.size = get_u16(&msg->data[1]) << lpc->shift;
> > > > + lpc->write.size = get_u16(&msg->data[3]) << lpc->shift;
> > > > +
> > > > + lpc->rc = 0;
> > > > +out:
> > > > + lpc->busy = false;
> > > > +}
> > > > +
> > > > +int mbox_flash_init(struct blocklevel_device **bl)
> > > > +{
> > > > + struct mbox_flash_data *lpc;
> > >
> > > As much as it's vague I'd prefer state to lpc, this isn't really
> > > **only** the lpc as it holds other state information. How about
> > > mbox_state?
> > > (Update this everywhere)
> >
> > Yeah that might be a better name...
> >
> > >
> > > >
> > > > + struct bmc_mbox_msg *msg;
> > > > + int rc;
> > > > +
> > > > + if (!bl)
> > > > + return FLASH_ERR_PARM_ERROR;
> > > > +
> > > > + *bl = NULL;
> > > > +
> > > > + lpc = zalloc(sizeof(struct mbox_flash_data));
> > > > + if (!lpc)
> > > > + return FLASH_ERR_MALLOC_FAILED;
> > > > +
> > > > + /* For V1 of the protocol this is fixed. This could
> > > > change
> > > > */
> > > > + lpc->shift = 12;
> > > > +
> > > > + msg = get_msg_memory(lpc);
> > > > + if (!msg) {
> > >
> > > As far as I can tell, this check is unnecessary, this can only fail
> > > if
> > > the whole lpc allocation failed which would be caught above?!
> >
> > True but if allocation method changes it could happen.
> >
> > >
> > > >
> > > > + rc = FLASH_ERR_MALLOC_FAILED;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + msg->command = MBOX_C_GET_MBOX_INFO;
> > > > + msg->callback = &init_cb;
> > >
> > > Can the callback member be a pointer to an array of function
> > > pointers
> > > indexed by the command? Then the call back wouldn't need to be set
> > > everytime.
> > > e.g.
> > > typedef void (* msg_callback_t)(struct bmc_mbox_msg *msg)
> > >
> > > struct bmc_mbox_msg {
> > > <snip>
> > > static msg_callback_t callbacks[NUM_CMDS]
> > > <snip>
> > > }
> > >
> > > static msg_callback_t msg_callbacks[] = {
> > > <snip>
> > > &init_cb,
> > > <snip>
> > > }
> > >
> > > msg->callbacks = msg_callbacks;
> > >
> > > then in the driver you call:
> > >
> > > msg->callbacks[CMD](msg);
> > >
> > > Just an idea.
> >
> > As discussed offline I'll do a method that you can register your
> > callbacks with the driver, doing it in the bmc_mbox_msg struct has
> > the
> > same problem but just telling the driver what to do is great.
> >
> >
> > Thanks for high quality review,
> >
> > Cyril
> >
> > >
> > > >
> > > > + msg->priv = lpc;
> > > > + msg->data[0] = 1; /* V1, do better */
> > > > +
> > > > + lpc->busy = true;
> > > > + rc = bmc_mbox_enqueue(msg);
> > > > + if (rc) {
> > > > + prlog(PR_ERR, "Failed to enqueue/send BMC MBOX
> > > > message\n");
> > > > + goto out_msg;
> > > > + }
> > > > +
> > > > + if (wait_for_bmc(lpc, MBOX_DEFAULT_TIMEOUT)) {
> > > > + prlog(PR_ERR, "Timeout waiting for BMC\n");
> > > > + rc = FLASH_ERR_CTRL_TIMEOUT;
> > > > + goto out_msg;
> > > > + }
> > > > +
> > > > + rc = lpc->rc;
> > > > + if (rc)
> > > > + goto out_msg;
> > > > +
> > > > + lpc->bl.keep_alive = 0;
> > > > + lpc->bl.read = &mbox_flash_read;
> > > > + lpc->bl.write = &mbox_flash_write;
> > > > + lpc->bl.erase = &mbox_flash_erase;
> > > > + lpc->bl.get_info = &mbox_flash_get_info;
> > > > +
> > > > + *bl = &(lpc->bl);
> > > > + return 0;
> > > > +
> > > > +out_msg:
> > > > + free_msg_memory(msg);
> > > > +out:
> > > > + free(lpc);
> > > > + return rc;
> > > > +}
> > > > +
> > > > +void mbox_flash_exit(struct blocklevel_device *bl)
> > > > +{
> > > > + struct mbox_flash_data *lpc;
> > > > + if (bl) {
> > > > + lpc = container_of(bl, struct mbox_flash_data,
> > > > bl);
> > > > + free(lpc);
> > > > + }
> > > > +}
> > > > diff --git a/libflash/mbox-flash.h b/libflash/mbox-flash.h
> > > > new file mode 100644
> > > > index 00000000..cd587d4c
> > > > --- /dev/null
> > > > +++ b/libflash/mbox-flash.h
> > > > @@ -0,0 +1,24 @@
> > > > +/* Copyright 2017 IBM Corp.
> > > > + *
> > > > + * Licensed under the Apache License, Version 2.0 (the
> > > > "License");
> > > > + * you may not use this file except in compliance with the
> > > > License.
> > > > + * You may obtain a copy of the License at
> > > > + *
> > > > + * http://www.apache.org/licenses/LICENSE-2.0
> > > > + *
> > > > + * Unless required by applicable law or agreed to in writing,
> > > > software
> > > > + * distributed under the License is distributed on an "AS IS"
> > > > BASIS,
> > > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
> > > > or
> > > > + * implied.
> > > > + * See the License for the specific language governing
> > > > permissions
> > > > and
> > > > + * limitations under the License.
> > > > + */
> > > > +
> > > > +#ifndef __LIBFLASH_MBOX_FLASH_H
> > > > +#define __LIBFLASH_MBOX_FLASH_H
> > > > +
> > > > +int mbox_flash_init(struct blocklevel_device **bl);
> > > > +void mbox_flash_exit(struct blocklevel_device *bl);
> > > > +#endif /* __LIBFLASH_MBOX_FLASH_H */
> > > > +
> > > > +
>
>
More information about the Skiboot
mailing list