[Skiboot] [PATCH v3 3/4] libflash: blocklevel backend for MBOX flash access
Suraj Jitindar Singh
sjitindarsingh at gmail.com
Tue Feb 7 11:43:48 AEDT 2017
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?
>
> >
> > >
> > > +
> > > + /*
> > > + * 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