[Skiboot] [RFC PATCH 6/6] libflash: add blocklevel_raw_prepare()

[Vasant Hegde]

On 02/28/2019 11:48 AM, Stewart Smith wrote:
> The logic of a prepare call is to move the window (or at least start the
> process of moving the window) so that when a read/write/erase call is
> made, we don't have to wait for a IPMI round trip. >
> For non-hiomap backends, one could use similar logic.
> 
> ---
> Not Signed-off-by yet as I'm totally not convinced

Even I'm not convinced about this approach. Its still error prone.
May be best is to split read/write/erase call to two parts (one initiates 
request to BMC
and second one does post processing after getting data from BMC).
That way we can remove synchronous message completely from these paths.

Few comments based on current code.


> ---
>   core/flash.c           |  9 +++++++--
>   libflash/blocklevel.c  | 22 +++++++++++++++++++++
>   libflash/blocklevel.h  |  2 ++
>   libflash/ipmi-hiomap.c | 44 ++++++++++++++++++++++++++++++++++++++++--
>   4 files changed, 73 insertions(+), 4 deletions(-)
> 
> diff --git a/core/flash.c b/core/flash.c
> index 04d99348515e..e267aaa0373d 100644
> --- a/core/flash.c
> +++ b/core/flash.c
> @@ -1,4 +1,4 @@
> -/* Copyright 2013-2018 IBM Corp.
> +/* Copyright 2013-2019 IBM Corp.
>    *
>    * Licensed under the Apache License, Version 2.0 (the "License");
>    * you may not use this file except in compliance with the License.
> @@ -239,7 +239,7 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus
>   {
>   	struct flash *flash = data;
>   	uint64_t offset, buf, len;
> -	int rc;
> +	int rc = 0;
> 
>   	offset = flash->async.pos;
>   	buf = flash->async.buf;
> @@ -254,6 +254,10 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus
>   	len = MIN(flash->async.len, flash->block_size);
>   	prlog(PR_TRACE, "Flash poll op %d len %llu\n", flash->async.op, len);
> 
> +	/* If we've done work to prepare, come back later */
> +	if (blocklevel_raw_prepare(flash->bl, (flash->async.op!=FLASH_OP_READ), offset, len))
> +		goto schedule;

Need a way to differentiate errors vs waiting to get response from BMC.

> +
>   	switch (flash->async.op) {
>   	case FLASH_OP_READ:
>   		rc = blocklevel_raw_read(flash->bl, offset, (void *)buf, len);
> @@ -274,6 +278,7 @@ static void flash_poll(struct timer *t __unused, void *data, uint64_t now __unus
>   	flash->async.pos += len;
>   	flash->async.buf += len;
>   	flash->async.len -= len;
> +schedule:
>   	if (!rc && flash->async.len) {
>   		/*
>   		 * We want to get called pretty much straight away, just have
> diff --git a/libflash/blocklevel.c b/libflash/blocklevel.c
> index 5b57d3c6194f..9659c7fd68af 100644

.../...


> 
> +static int ipmi_hiomap_prepare(struct blocklevel_device *bl, bool rw,
> +			       uint64_t pos, uint64_t len)
> +{
> +	struct ipmi_hiomap *ctx;
> +	enum lpc_window_state want_state;
> +	uint64_t size;
> +	uint8_t command = HIOMAP_C_CREATE_READ_WINDOW;
> +	bool valid_state;
> +	bool is_read;
> +	int rc = 0;
> +
> +	/* LPC is only 32bit */
> +	if (pos > UINT_MAX || len > UINT_MAX)
> +		return FLASH_ERR_PARM_ERROR;
> +
> +	if (rw)
> +		command = HIOMAP_C_CREATE_WRITE_WINDOW;
> +
> +	ctx = container_of(bl, struct ipmi_hiomap, bl);
> +
> +	is_read = (command == HIOMAP_C_CREATE_READ_WINDOW);
> +	want_state = is_read ? read_window : write_window;
> +
> +	lock(&ctx->lock);
> +	valid_state = want_state == ctx->window_state;
> +	rc = hiomap_window_valid(ctx, pos, len);
> +	if (valid_state && !rc) {
> +		unlock(&ctx->lock);
> +		return 0;
> +	}
> +	unlock(&ctx->lock);
> +
> +	rc = ipmi_hiomap_handle_events(ctx);
> +	if (rc)
> +		return rc;
> +
> +	rc = hiomap_window_move(ctx, command, pos, len, &size);

This still uses synchronous path. So not going to help much.

-Vasant