[Skiboot] [PATCH 06/11] libflash/mbox-flash: Simplify message sending
Samuel Mendoza-Jonas
sam at mendozajonas.com
Fri Jul 14 14:20:15 AEST 2017
On Thu, 2017-06-29 at 22:39 +1000, Cyril Bur wrote:
> hw/lpc-mbox no longer requires that the memory associated with commands
> exist for the lifetime of the message, simply long enough for the
> sending function and for the receving callback.
This confused me for a minute but Cyril says the commit message is a bit
confusing is all :)
Apparently /a/ message must exist for the sending function and for the
callback, but these need not be literally the same message struct?
If so, LGTM.
>
> Remove all code to deal with allocating messages.
>
> Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> ---
> libflash/mbox-flash.c | 123 +++++++++++++-------------------------------------
> 1 file changed, 31 insertions(+), 92 deletions(-)
>
> diff --git a/libflash/mbox-flash.c b/libflash/mbox-flash.c
> index 2914901e..999f663c 100644
> --- a/libflash/mbox-flash.c
> +++ b/libflash/mbox-flash.c
> @@ -39,6 +39,8 @@
>
> #define MBOX_DEFAULT_TIMEOUT 30
>
> +#define MSG_CREATE(init_command) { .command = init_command }
> +
> struct lpc_window {
> uint32_t lpc_addr; /* Offset into LPC space */
> uint32_t cur_pos; /* Current position of the window in the flash */
> @@ -61,7 +63,6 @@ struct mbox_flash_data {
> bool ack;
> /* Plus one, commands start at 1 */
> void (*handlers[MBOX_COMMAND_COUNT + 1])(struct mbox_flash_data *, struct bmc_mbox_msg*);
> - struct bmc_mbox_msg msg_mem;
> };
>
> static void mbox_flash_callback(struct bmc_mbox_msg *msg, void *priv);
> @@ -188,25 +189,6 @@ static uint16_t bytes_to_blocks(struct mbox_flash_data *mbox_flash,
> return bytes >> mbox_flash->shift;
> }
>
> -static struct bmc_mbox_msg *msg_alloc(struct mbox_flash_data *mbox_flash,
> - uint8_t command)
> -{
> - /*
> - * Yes this causes *slow*.
> - * 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(&mbox_flash->msg_mem, 0, sizeof(mbox_flash->msg_mem));
> - mbox_flash->msg_mem.command = command;
> - return &mbox_flash->msg_mem;
> -}
> -
> -static void msg_free_memory(struct bmc_mbox_msg *mem __unused)
> -{
> - /* Allocation is so simple this isn't required */
> -}
> -
> /*
> * The BMC may send is an out of band message to say that it doesn't
> * own the flash anymore.
> @@ -296,26 +278,22 @@ static int wait_for_bmc(struct mbox_flash_data *mbox_flash, unsigned int timeout
>
> static int mbox_flash_ack(struct mbox_flash_data *mbox_flash, uint8_t reg)
> {
> - struct bmc_mbox_msg *msg;
> + struct bmc_mbox_msg msg = MSG_CREATE(MBOX_C_BMC_EVENT_ACK);
> int rc;
>
> - msg = msg_alloc(mbox_flash, MBOX_C_BMC_EVENT_ACK);
> - if (!msg)
> - return FLASH_ERR_MALLOC_FAILED;
> -
> - msg_put_u8(msg, 0, reg);
> + msg_put_u8(&msg, 0, reg);
>
> /* Clear this first so msg_send() doesn't freak out */
> mbox_flash->reboot = false;
>
> - rc = msg_send(mbox_flash, msg, MBOX_DEFAULT_TIMEOUT);
> + rc = msg_send(mbox_flash, &msg, MBOX_DEFAULT_TIMEOUT);
>
> /* Still need to deal with it, we've only acked it now. */
> mbox_flash->reboot = true;
>
> if (rc) {
> prlog(PR_ERR, "Failed to enqueue/send BMC MBOX message\n");
> - goto out;
> + return rc;
> }
>
> /*
> @@ -327,8 +305,6 @@ static int mbox_flash_ack(struct mbox_flash_data *mbox_flash, uint8_t reg)
> if (rc)
> prlog(PR_ERR, "Error waiting for BMC\n");
>
> -out:
> - msg_free_memory(msg);
> return rc;
> }
>
> @@ -474,21 +450,17 @@ static bool do_delayed_work(struct mbox_flash_data *mbox_flash)
> static int mbox_flash_mark_write(struct mbox_flash_data *mbox_flash,
> uint64_t pos, uint64_t len, int type)
> {
> - struct bmc_mbox_msg *msg;
> + struct bmc_mbox_msg msg = MSG_CREATE(type);
> int rc;
>
> - msg = msg_alloc(mbox_flash, type);
> - if (!msg)
> - return FLASH_ERR_MALLOC_FAILED;
> -
> if (mbox_flash->version == 1) {
> uint32_t start = ALIGN_DOWN(pos, 1 << mbox_flash->shift);
> - msg_put_u16(msg, 0, bytes_to_blocks(mbox_flash, pos));
> + msg_put_u16(&msg, 0, bytes_to_blocks(mbox_flash, pos));
> /*
> * We need to make sure that we mark dirty until up to atleast
> * pos + len.
> */
> - msg_put_u32(msg, 2, pos + len - start);
> + msg_put_u32(&msg, 2, pos + len - start);
> } else {
> uint64_t window_pos = pos - mbox_flash->write.cur_pos;
> uint16_t start = bytes_to_blocks(mbox_flash, window_pos);
> @@ -496,24 +468,20 @@ static int mbox_flash_mark_write(struct mbox_flash_data *mbox_flash,
> ALIGN_UP(window_pos + len,
> 1 << mbox_flash->shift));
>
> - msg_put_u16(msg, 0, start);
> - msg_put_u16(msg, 2, end - start); /* Total Length */
> + msg_put_u16(&msg, 0, start);
> + msg_put_u16(&msg, 2, end - start); /* Total Length */
> }
>
> - rc = msg_send(mbox_flash, msg, MBOX_DEFAULT_TIMEOUT);
> + rc = msg_send(mbox_flash, &msg, MBOX_DEFAULT_TIMEOUT);
> if (rc) {
> prlog(PR_ERR, "Failed to enqueue/send BMC MBOX message\n");
> - goto out;
> + return rc;
> }
>
> rc = wait_for_bmc(mbox_flash, MBOX_DEFAULT_TIMEOUT);
> - if (rc) {
> + if (rc)
> prlog(PR_ERR, "Error waiting for BMC\n");
> - goto out;
> - }
>
> -out:
> - msg_free_memory(msg);
> return rc;
> }
>
> @@ -543,7 +511,7 @@ static int mbox_flash_erase(struct mbox_flash_data *mbox_flash, uint64_t pos,
>
> static int mbox_flash_flush(struct mbox_flash_data *mbox_flash)
> {
> - struct bmc_mbox_msg *msg;
> + struct bmc_mbox_msg msg = MSG_CREATE(MBOX_C_WRITE_FLUSH);
> int rc;
>
> if (!mbox_flash->write.open) {
> @@ -551,22 +519,16 @@ static int mbox_flash_flush(struct mbox_flash_data *mbox_flash)
> return FLASH_ERR_DEVICE_GONE;
> }
>
> - msg = msg_alloc(mbox_flash, MBOX_C_WRITE_FLUSH);
> - if (!msg)
> - return FLASH_ERR_MALLOC_FAILED;
> -
> - rc = msg_send(mbox_flash, msg, MBOX_DEFAULT_TIMEOUT);
> + rc = msg_send(mbox_flash, &msg, MBOX_DEFAULT_TIMEOUT);
> if (rc) {
> prlog(PR_ERR, "Failed to enqueue/send BMC MBOX message\n");
> - goto out;
> + return rc;
> }
>
> rc = wait_for_bmc(mbox_flash, MBOX_DEFAULT_TIMEOUT);
> if (rc)
> prlog(PR_ERR, "Error waiting for BMC\n");
>
> -out:
> - msg_free_memory(msg);
> return rc;
> }
>
> @@ -587,7 +549,7 @@ static int mbox_window_move(struct mbox_flash_data *mbox_flash,
> struct lpc_window *win, uint8_t command,
> uint64_t pos, uint64_t len, uint64_t *size)
> {
> - struct bmc_mbox_msg *msg;
> + struct bmc_mbox_msg msg = MSG_CREATE(command);
> int rc;
>
> /* Is the window currently open valid */
> @@ -605,15 +567,11 @@ static int mbox_window_move(struct mbox_flash_data *mbox_flash,
> */
> win->cur_pos = pos & ~mbox_flash_mask(mbox_flash);
>
> - msg = msg_alloc(mbox_flash, command);
> - if (!msg)
> - return FLASH_ERR_MALLOC_FAILED;
> -
> - msg_put_u16(msg, 0, bytes_to_blocks(mbox_flash, pos));
> - rc = msg_send(mbox_flash, msg, MBOX_DEFAULT_TIMEOUT);
> + msg_put_u16(&msg, 0, bytes_to_blocks(mbox_flash, pos));
> + rc = msg_send(mbox_flash, &msg, MBOX_DEFAULT_TIMEOUT);
> if (rc) {
> prlog(PR_ERR, "Failed to enqueue/send BMC MBOX message\n");
> - goto out;
> + return rc;
> }
>
> mbox_flash->read.open = false;
> @@ -622,7 +580,7 @@ static int mbox_window_move(struct mbox_flash_data *mbox_flash,
> rc = wait_for_bmc(mbox_flash, MBOX_DEFAULT_TIMEOUT);
> if (rc) {
> prlog(PR_ERR, "Error waiting for BMC\n");
> - goto out;
> + return rc;
> }
>
> *size = len;
> @@ -645,8 +603,6 @@ static int mbox_window_move(struct mbox_flash_data *mbox_flash,
> prlog(PR_ERR, "win pos: 0x%08x win size: 0x%08x\n", win->cur_pos, win->size);
> }
>
> -out:
> - msg_free_memory(msg);
> return rc;
> }
>
> @@ -750,8 +706,8 @@ static int mbox_flash_read(struct blocklevel_device *bl, uint64_t pos,
> static int mbox_flash_get_info(struct blocklevel_device *bl, const char **name,
> uint64_t *total_size, uint32_t *erase_granule)
> {
> + struct bmc_mbox_msg msg = MSG_CREATE(MBOX_C_GET_FLASH_INFO);
> struct mbox_flash_data *mbox_flash;
> - struct bmc_mbox_msg *msg;
> int rc;
>
> mbox_flash = container_of(bl, struct mbox_flash_data, bl);
> @@ -759,10 +715,6 @@ static int mbox_flash_get_info(struct blocklevel_device *bl, const char **name,
> if (do_delayed_work(mbox_flash))
> return FLASH_ERR_AGAIN;
>
> - msg = msg_alloc(mbox_flash, MBOX_C_GET_FLASH_INFO);
> - if (!msg)
> - return FLASH_ERR_MALLOC_FAILED;
> -
> /*
> * We want to avoid runtime mallocs in skiboot. The expected
> * behavour to uses of libflash is that one can free() the memory
> @@ -773,15 +725,15 @@ static int mbox_flash_get_info(struct blocklevel_device *bl, const char **name,
> *name = NULL;
>
> mbox_flash->busy = true;
> - rc = msg_send(mbox_flash, msg, MBOX_DEFAULT_TIMEOUT);
> + rc = msg_send(mbox_flash, &msg, MBOX_DEFAULT_TIMEOUT);
> if (rc) {
> prlog(PR_ERR, "Failed to enqueue/send BMC MBOX message\n");
> - goto out;
> + return rc;
> }
>
> if (wait_for_bmc(mbox_flash, MBOX_DEFAULT_TIMEOUT)) {
> prlog(PR_ERR, "Error waiting for BMC\n");
> - goto out;
> + return rc;
> }
>
> mbox_flash->bl.erase_mask = mbox_flash->erase_granule - 1;
> @@ -791,8 +743,6 @@ static int mbox_flash_get_info(struct blocklevel_device *bl, const char **name,
> if (erase_granule)
> *erase_granule = mbox_flash->erase_granule;
>
> -out:
> - msg_free_memory(msg);
> return rc;
> }
>
> @@ -922,7 +872,7 @@ out:
>
> static int protocol_init(struct mbox_flash_data *mbox_flash)
> {
> - struct bmc_mbox_msg *msg;
> + struct bmc_mbox_msg msg = MSG_CREATE(MBOX_C_GET_MBOX_INFO);
> int rc;
>
> /* Assume V2 */
> @@ -961,25 +911,19 @@ static int protocol_init(struct mbox_flash_data *mbox_flash)
> */
> mbox_flash->version = 2;
>
> - msg = msg_alloc(mbox_flash, MBOX_C_GET_MBOX_INFO);
> - if (!msg)
> - return FLASH_ERR_MALLOC_FAILED;
> -
> - msg_put_u8(msg, 0, mbox_flash->version);
> - rc = msg_send(mbox_flash, msg, MBOX_DEFAULT_TIMEOUT);
> + msg_put_u8(&msg, 0, mbox_flash->version);
> + rc = msg_send(mbox_flash, &msg, MBOX_DEFAULT_TIMEOUT);
> if (rc) {
> prlog(PR_ERR, "Failed to enqueue/send BMC MBOX message\n");
> - goto out;
> + return rc;
> }
>
> rc = wait_for_bmc(mbox_flash, MBOX_DEFAULT_TIMEOUT);
> if (rc) {
> prlog(PR_ERR, "Error waiting for BMC\n");
> - goto out;
> + return rc;
> }
>
> - msg_free_memory(msg);
> -
> prlog(PR_INFO, "Detected mbox protocol version %d\n", mbox_flash->version);
> if (mbox_flash->version == 1) {
> mbox_flash->bl.erase = &mbox_flash_erase_v1;
> @@ -998,13 +942,8 @@ static int protocol_init(struct mbox_flash_data *mbox_flash)
> */
> prlog(PR_CRIT, "Bad version: %u\n", mbox_flash->version);
> rc = FLASH_ERR_PARM_ERROR;
> - goto out;
> }
>
> -
> - return 0;
> -out:
> - msg_free_memory(msg);
> return rc;
> }
>
More information about the Skiboot
mailing list