[Skiboot] [PATCH v3 3/4] libflash: blocklevel backend for MBOX flash access
Cyril Bur
cyril.bur at au1.ibm.com
Tue Feb 7 10:53:33 AEDT 2017
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.
> > + * 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.
> > +}
> > +
> > +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.
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.
> 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
> > +
> > + 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
?
> > +
> > + /*
> > + * 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