[Skiboot] [PATCH] blocklevel_info: Refactor the passing of output data

Cyril Bur cyrilbur at gmail.com
Tue Aug 29 15:37:53 AEST 2017


On Mon, 2017-08-28 at 21:19 -0700, William A. Kennington III wrote:
> This changes refactors all of the output variables from flash_get_info
> into a new output struct. Callers now supply the info struct, and always
> get all of the info with each call. There are never any allocations done
> by this function so it shouldn't be much slower to always populate all
> of the information.
> 

Hi William,

As nice as this refactor is (thanks for the effort!), unfortunately
we're a bit stuck with the interface as is, both openbmc and petitboot
projects link against a dynamic libflash.

Ironically I'm about to send a series which which does rework the
libffs api a bit - although I'm quite sure only external/ffspart
actually uses what I've changed. blocklevel_get_info() though is pretty
core and changing that won't work.

Perhaps you've prompted a discussion that now might be a good time to
do a significant rewrite of libflash/libffs which are now being used
for a completely different purpose for which they were first written,
hopefully Stewart will weigh in.


Cyril

> Signed-off-by: William A. Kennington III <wak at google.com>
> ---
>  core/flash.c                           | 15 +++++------
>  external/common/arch_flash_arm.c       |  6 ++---
>  external/common/arch_flash_common.c    |  6 ++---
>  external/gard/gard.c                   | 10 +++----
>  external/opal-prd/pnor.c               | 12 ++++-----
>  external/opal-prd/pnor.h               |  3 +--
>  external/opal-prd/test/test_pnor_ops.c |  7 +++--
>  external/pflash/pflash.c               | 46 ++++++++++++++++----------------
>  libflash/blocklevel.c                  | 35 ++++++++++++-------------
>  libflash/blocklevel.h                  | 16 +++++++++---
>  libflash/file.c                        | 48 ++++++++++++++--------------------
>  libflash/libffs.c                      |  8 +++---
>  libflash/libflash.c                    | 15 +++++------
>  libflash/mbox-flash.c                  | 19 ++++----------
>  libflash/test/test-flash.c             |  6 ++---
>  platforms/mambo/mambo.c                | 15 ++++-------
>  16 files changed, 125 insertions(+), 142 deletions(-)
> 
> diff --git a/core/flash.c b/core/flash.c
> index 53e6eba08..35a0c4c32 100644
> --- a/core/flash.c
> +++ b/core/flash.c
> @@ -254,21 +254,20 @@ static int num_flashes(void)
>  
>  int flash_register(struct blocklevel_device *bl)
>  {
> -	uint64_t size;
> -	uint32_t block_size;
> +	struct blocklevel_info bl_info;
>  	struct ffs_handle *ffs;
>  	struct dt_node *node;
>  	struct flash *flash;
> -	const char *name;
>  	int rc;
>  
> -	rc = blocklevel_get_info(bl, &name, &size, &block_size);
> +	rc = blocklevel_get_info(bl, &bl_info);
>  	if (rc)
>  		return rc;
>  
>  	prlog(PR_INFO, "FLASH: registering flash device %s "
>  			"(size 0x%llx, blocksize 0x%x)\n",
> -			name ?: "(unnamed)", size, block_size);
> +			bl_info.name ? bl_info.name : "(unnamed)", bl_info.size,
> +			bl_info.erase_granule);
>  
>  	lock(&flash_lock);
>  
> @@ -281,8 +280,8 @@ int flash_register(struct blocklevel_device *bl)
>  
>  	flash->busy = false;
>  	flash->bl = bl;
> -	flash->size = size;
> -	flash->block_size = block_size;
> +	flash->size = bl_info.size;
> +	flash->block_size = bl_info.erase_granule;
>  	flash->id = num_flashes();
>  
>  	list_add(&flashes, &flash->list);
> @@ -302,7 +301,7 @@ int flash_register(struct blocklevel_device *bl)
>  
>  	node = flash_add_dt_node(flash, flash->id);
>  
> -	setup_system_flash(flash, node, name, ffs);
> +	setup_system_flash(flash, node, bl_info.name, ffs);
>  
>  	if (ffs)
>  		ffs_close(ffs);
> diff --git a/external/common/arch_flash_arm.c b/external/common/arch_flash_arm.c
> index 3cdd41ded..ac43de0c4 100644
> --- a/external/common/arch_flash_arm.c
> +++ b/external/common/arch_flash_arm.c
> @@ -303,13 +303,13 @@ int arch_flash_erase_chip(struct blocklevel_device *bl)
>  	if (!arch_data.flash_chip) {
>  		/* Just assume its a regular erase */
>  		int rc;
> -		uint64_t total_size;
> +		struct blocklevel_info bl_info;
>  
> -		rc = blocklevel_get_info(bl, NULL, &total_size, NULL);
> +		rc = blocklevel_get_info(bl, &bl_info);
>  		if (rc)
>  			return rc;
>  
> -		return blocklevel_erase(bl, 0, total_size);
> +		return blocklevel_erase(bl, 0, bl_info.size);
>  	}
>  
>  	return flash_erase_chip(arch_data.flash_chip);
> diff --git a/external/common/arch_flash_common.c b/external/common/arch_flash_common.c
> index 6bce7e1ba..41239662a 100644
> --- a/external/common/arch_flash_common.c
> +++ b/external/common/arch_flash_common.c
> @@ -31,13 +31,13 @@
>  int __attribute__((weak)) arch_flash_erase_chip(struct blocklevel_device *bl)
>  {
>  	int rc;
> -	uint64_t total_size;
> +	struct blocklevel_info bl_info;
>  
> -	rc = blocklevel_get_info(bl, NULL, &total_size, NULL);
> +	rc = blocklevel_get_info(bl, &bl_info);
>  	if (rc)
>  		return rc;
>  
> -	return blocklevel_erase(bl, 0, total_size);
> +	return blocklevel_erase(bl, 0, bl_info.size);
>  }
>  
>  int __attribute__((weak,const)) arch_flash_4b_mode(struct blocklevel_device *bl, int set_4b)
> diff --git a/external/gard/gard.c b/external/gard/gard.c
> index c5cb93b88..3946cad6f 100644
> --- a/external/gard/gard.c
> +++ b/external/gard/gard.c
> @@ -596,7 +596,7 @@ int main(int argc, char **argv)
>  	const char *action, *progname;
>  	char *filename = NULL;
>  	struct gard_ctx _ctx, *ctx;
> -	uint64_t bl_size;
> +	struct blocklevel_info bl_info;
>  	int rc, i = 0;
>  	bool part = 0;
>  	bool ecc = 0;
> @@ -665,17 +665,17 @@ int main(int argc, char **argv)
>  		goto out_free;
>  	}
>  
> -	rc = blocklevel_get_info(ctx->bl, NULL, &bl_size, NULL);
> +	rc = blocklevel_get_info(ctx->bl, &bl_info);
>  	if (rc)
>  		goto out;
>  
> -	if (bl_size > UINT_MAX) {
> +	if (bl_info.size > UINT_MAX) {
>  		fprintf(stderr, "MTD device bigger than %i: size: %" PRIu64 "\n",
> -			UINT_MAX, bl_size);
> +			UINT_MAX, bl_info.size);
>  		rc = EXIT_FAILURE;
>  		goto out;
>  	}
> -	ctx->f_size = bl_size;
> +	ctx->f_size = bl_info.size;
>  
>  	if (!part) {
>  		rc = ffs_init(0, ctx->f_size, ctx->bl, &ctx->ffs, 1);
> diff --git a/external/opal-prd/pnor.c b/external/opal-prd/pnor.c
> index c032421d1..afb6ccd1f 100644
> --- a/external/opal-prd/pnor.c
> +++ b/external/opal-prd/pnor.c
> @@ -64,13 +64,13 @@ int pnor_init(struct pnor *pnor)
>  		return -1;
>  	}
>  
> -	rc = blocklevel_get_info(pnor->bl, NULL, &(pnor->size), &(pnor->erasesize));
> +	rc = blocklevel_get_info(pnor->bl, &pnor->bl_info);
>  	if (rc) {
>  		pr_log(LOG_ERR, "PNOR: blocklevel_get_info() failed. Can't use PNOR");
>  		goto out;
>  	}
>  
> -	rc = ffs_init(0, pnor->size, pnor->bl, &pnor->ffsh, 0);
> +	rc = ffs_init(0, pnor->bl_info.size, pnor->bl, &pnor->ffsh, 0);
>  	if (rc) {
>  		pr_log(LOG_ERR, "PNOR: Failed to open pnor partition table");
>  		goto out;
> @@ -121,8 +121,8 @@ static int mtd_write(struct pnor *pnor, void *data, uint64_t offset,
>  {
>  	int rc;
>  
> -	if (len > pnor->size || offset > pnor->size ||
> -	    len + offset > pnor->size)
> +	if (len > pnor->bl_info.size || offset > pnor->bl_info.size ||
> +	    len + offset > pnor->bl_info.size)
>  		return -ERANGE;
>  
>  	rc = blocklevel_smart_write(pnor->bl, offset, data, len);
> @@ -137,8 +137,8 @@ static int mtd_read(struct pnor *pnor, void *data, uint64_t offset,
>  {
>  	int rc;
>  
> -	if (len > pnor->size || offset > pnor->size ||
> -	    len + offset > pnor->size)
> +	if (len > pnor->bl_info.size || offset > pnor->bl_info.size ||
> +	    len + offset > pnor->bl_info.size)
>  		return -ERANGE;
>  
>  	rc = blocklevel_read(pnor->bl, offset, data, len);
> diff --git a/external/opal-prd/pnor.h b/external/opal-prd/pnor.h
> index 28571af64..1ee033659 100644
> --- a/external/opal-prd/pnor.h
> +++ b/external/opal-prd/pnor.h
> @@ -7,8 +7,7 @@
>  struct pnor {
>  	char			*path;
>  	struct ffs_handle	*ffsh;
> -	uint64_t		size;
> -	uint32_t		erasesize;
> +	struct blocklevel_info bl_info;
>  	struct blocklevel_device *bl;
>  };
>  
> diff --git a/external/opal-prd/test/test_pnor_ops.c b/external/opal-prd/test/test_pnor_ops.c
> index fd5e2c22a..3743362e1 100644
> --- a/external/opal-prd/test/test_pnor_ops.c
> +++ b/external/opal-prd/test/test_pnor_ops.c
> @@ -130,11 +130,14 @@ int main(int argc, char **argv)
>  	for (i = 0; i < 2; i++)
>  		write(fd, data, 16);
>  
> +	/* Unused */
> +	pnor.bl_info.name = NULL;
> +
>  	/* Adjust this if making the file smaller */
> -	pnor.size = 32;
> +	pnor.bl_info.size = 32;
>  
>  	/* This is fake. Make it smaller than the size */
> -	pnor.erasesize = 4;
> +	pnor.bl_info.erase_granule = 4;
>  
>  	printf("Write: ");
>  	memset(data, 'A', sizeof(data));
> diff --git a/external/pflash/pflash.c b/external/pflash/pflash.c
> index bfc975fe5..3f501e583 100644
> --- a/external/pflash/pflash.c
> +++ b/external/pflash/pflash.c
> @@ -43,11 +43,9 @@
>  
>  struct flash_details {
>  	struct blocklevel_device *bl;
> +	struct blocklevel_info bl_info;
>  	int need_relock;
> -	const char *name;
>  	uint64_t toc;
> -	uint64_t total_size;
> -	uint32_t erase_granule;
>  };
>  
>  /* Full pflash version number (possibly includes gitid). */
> @@ -144,7 +142,7 @@ static struct ffs_handle *open_ffs(struct flash_details *flash)
>  	struct ffs_handle *ffsh;
>  	int rc;
>  
> -	rc = ffs_init(flash->toc, flash->total_size,
> +	rc = ffs_init(flash->toc, flash->bl_info.size,
>  			flash->bl, &ffsh, 0);
>  	if (rc) {
>  		fprintf(stderr, "Error %d opening ffs !\n", rc);
> @@ -166,11 +164,11 @@ static void print_flash_info(struct flash_details *flash)
>  
>  	printf("Flash info:\n");
>  	printf("-----------\n");
> -	printf("Name          = %s\n", flash->name);
> +	printf("Name          = %s\n", flash->bl_info.name);
>  	printf("Total size    = %" PRIu64 "MB\t Flags E:ECC, P:PRESERVED, R:READONLY\n",
> -			flash->total_size >> 20);
> +			flash->bl_info.size >> 20);
>  	printf("Erase granule = %2d%-13sB:BACKUP, F:REPROVISION\n",
> -			flash->erase_granule >> 10, "KB");
> +			flash->bl_info.erase_granule >> 10, "KB");
>  
>  	if (bmc_flash)
>  		return;
> @@ -282,9 +280,11 @@ static int erase_chip(struct flash_details *flash)
>  	 * likes the progress bars.
>  	 * Lets do an erase block at a time erase then...
>  	 */
> -	progress_init(flash->total_size);
> -	for (pos = 0; pos < flash->total_size; pos += flash->erase_granule) {
> -		rc = blocklevel_erase(flash->bl, pos, flash->erase_granule);
> +	progress_init(flash->bl_info.size);
> +	for (pos = 0; pos < flash->bl_info.size;
> +			pos += flash->bl_info.erase_granule) {
> +		rc = blocklevel_erase(flash->bl, pos,
> +				flash->bl_info.erase_granule);
>  		if (rc)
>  			break;
>  		progress_tick(pos);
> @@ -303,7 +303,7 @@ static int erase_range(struct flash_details *flash,
>  		uint32_t start, uint32_t size, bool will_program,
>  		struct ffs_handle *ffsh, int ffs_index)
>  {
> -	uint32_t done = 0, erase_mask = flash->erase_granule - 1;
> +	uint32_t done = 0, erase_mask = flash->bl_info.erase_granule - 1;
>  	bool confirm;
>  	int rc;
>  
> @@ -326,25 +326,26 @@ static int erase_range(struct flash_details *flash,
>  	if (start & erase_mask) {
>  		/* Align to next erase block */
>  		rc = blocklevel_smart_erase(flash->bl, start,
> -				flash->erase_granule - (start & erase_mask));
> +				flash->bl_info.erase_granule - (start & erase_mask));
>  		if (rc) {
>  			fprintf(stderr, "Failed to blocklevel_smart_erase(): %d\n", rc);
>  			return 1;
>  		}
> -		start += flash->erase_granule - (start & erase_mask);
> -		size -= flash->erase_granule - (start & erase_mask);
> -		done = flash->erase_granule - (start & erase_mask);
> +		start += flash->bl_info.erase_granule - (start & erase_mask);
> +		size -= flash->bl_info.erase_granule - (start & erase_mask);
> +		done = flash->bl_info.erase_granule - (start & erase_mask);
>  	}
>  	progress_tick(done);
>  	while (size & ~(erase_mask)) {
> -		rc = blocklevel_smart_erase(flash->bl, start, flash->erase_granule);
> +		rc = blocklevel_smart_erase(flash->bl, start,
> +				flash->bl_info.erase_granule);
>  		if (rc) {
>  			fprintf(stderr, "Failed to blocklevel_smart_erase(): %d\n", rc);
>  			return 1;
>  		}
> -		start += flash->erase_granule;
> -		size -= flash->erase_granule;
> -		done += flash->erase_granule;
> +		start += flash->bl_info.erase_granule;
> +		size -= flash->bl_info.erase_granule;
> +		done += flash->bl_info.erase_granule;
>  		progress_tick(done);
>  	}
>  	if (size) {
> @@ -1003,8 +1004,7 @@ int main(int argc, char *argv[])
>  		goto out;
>  	}
>  
> -	rc = blocklevel_get_info(flash.bl, &flash.name,
> -			    &flash.total_size, &flash.erase_granule);
> +	rc = blocklevel_get_info(flash.bl, &flash.bl_info);
>  	if (rc) {
>  		fprintf(stderr, "Error %d getting flash info\n", rc);
>  		rc = 1;
> @@ -1025,7 +1025,7 @@ int main(int argc, char *argv[])
>  
>  	/* If read specified and no read_size, use flash size */
>  	if (do_read && !read_size && !part_name)
> -		read_size = flash.total_size;
> +		read_size = flash.bl_info.size;
>  
>  	/* We have a partition, adjust read/write size if needed */
>  	if (part_name || print_detail) {
> @@ -1087,7 +1087,7 @@ int main(int argc, char *argv[])
>  		/* Set address */
>  		address = pstart;
>  	} else if (erase) {
> -		if ((address | write_size) & (flash.erase_granule - 1)) {
> +		if ((address | write_size) & (flash.bl_info.erase_granule - 1)) {
>  			if (must_confirm) {
>  				printf("ERROR: Erase at 0x%08x for 0x%08x isn't erase block aligned\n",
>  						address, write_size);
> diff --git a/libflash/blocklevel.c b/libflash/blocklevel.c
> index 33d5c5d2f..3955be823 100644
> --- a/libflash/blocklevel.c
> +++ b/libflash/blocklevel.c
> @@ -229,12 +229,11 @@ int blocklevel_erase(struct blocklevel_device *bl, uint64_t pos, uint64_t len)
>  	return rc;
>  }
>  
> -int blocklevel_get_info(struct blocklevel_device *bl, const char **name, uint64_t *total_size,
> -		uint32_t *erase_granule)
> +int blocklevel_get_info(struct blocklevel_device *bl, struct blocklevel_info *bl_info)
>  {
>  	int rc;
>  
> -	if (!bl || !bl->get_info) {
> +	if (!bl || !bl->get_info || !bl_info) {
>  		errno = EINVAL;
>  		return FLASH_ERR_PARM_ERROR;
>  	}
> @@ -243,12 +242,12 @@ int blocklevel_get_info(struct blocklevel_device *bl, const char **name, uint64_
>  	if (rc)
>  		return rc;
>  
> -	rc = bl->get_info(bl, name, total_size, erase_granule);
> +	rc = bl->get_info(bl, bl_info);
>  
>  	/* Check the validity of what we are being told */
> -	if (erase_granule && *erase_granule != bl->erase_mask + 1)
> +	if (bl_info->erase_granule != bl->erase_mask + 1)
>  		FL_ERR("blocklevel_get_info: WARNING: erase_granule (0x%08x) and erase_mask"
> -				" (0x%08x) don't match\n", *erase_granule, bl->erase_mask + 1);
> +				" (0x%08x) don't match\n", bl_info->erase_granule, bl->erase_mask + 1);
>  
>  	release(bl);
>  
> @@ -417,7 +416,7 @@ out:
>  
>  int blocklevel_smart_write(struct blocklevel_device *bl, uint64_t pos, const void *buf, uint64_t len)
>  {
> -	uint32_t erase_size;
> +	struct blocklevel_info bl_info;
>  	const void *write_buf = buf;
>  	void *write_buf_start = NULL;
>  	void *erase_buf;
> @@ -435,7 +434,7 @@ int blocklevel_smart_write(struct blocklevel_device *bl, uint64_t pos, const voi
>  		return blocklevel_write(bl, pos, buf, len);
>  	}
>  
> -	rc = blocklevel_get_info(bl, NULL, NULL, &erase_size);
> +	rc = blocklevel_get_info(bl, &bl_info);
>  	if (rc)
>  		return rc;
>  
> @@ -458,7 +457,7 @@ int blocklevel_smart_write(struct blocklevel_device *bl, uint64_t pos, const voi
>  		write_buf = write_buf_start;
>  	}
>  
> -	erase_buf = malloc(erase_size);
> +	erase_buf = malloc(bl_info.erase_granule);
>  	if (!erase_buf) {
>  		errno = ENOMEM;
>  		rc = FLASH_ERR_MALLOC_FAILED;
> @@ -470,32 +469,32 @@ int blocklevel_smart_write(struct blocklevel_device *bl, uint64_t pos, const voi
>  		goto out_free;
>  
>  	while (len > 0) {
> -		uint32_t erase_block = pos & ~(erase_size - 1);
> -		uint32_t block_offset = pos & (erase_size - 1);
> -		uint32_t size = erase_size > len ? len : erase_size;
> +		uint32_t erase_block = pos & ~(bl_info.erase_granule - 1);
> +		uint32_t block_offset = pos & (bl_info.erase_granule - 1);
> +		uint32_t size = bl_info.erase_granule > len ? len : bl_info.erase_granule;
>  		int cmp;
>  
>  		/* Write crosses an erase boundary, shrink the write to the boundary */
> -		if (erase_size < block_offset + size) {
> -			size = erase_size - block_offset;
> +		if (bl_info.erase_granule < block_offset + size) {
> +			size = bl_info.erase_granule - block_offset;
>  		}
>  
> -		rc = bl->read(bl, erase_block, erase_buf, erase_size);
> +		rc = bl->read(bl, erase_block, erase_buf, bl_info.erase_granule);
>  		if (rc)
>  			goto out;
>  
>  		cmp = blocklevel_flashcmp(erase_buf + block_offset, write_buf, size);
>  		FL_DBG("%s: region 0x%08x..0x%08x ", __func__,
> -				erase_block, erase_size);
> +				erase_block, bl_info.erase_granule);
>  		if (cmp != 0) {
>  			FL_DBG("needs ");
>  			if (cmp == -1) {
>  				FL_DBG("erase and ");
> -				bl->erase(bl, erase_block, erase_size);
> +				bl->erase(bl, erase_block, bl_info.erase_granule);
>  			}
>  			FL_DBG("write\n");
>  			memcpy(erase_buf + block_offset, write_buf, size);
> -			rc = bl->write(bl, erase_block, erase_buf, erase_size);
> +			rc = bl->write(bl, erase_block, erase_buf, bl_info.erase_granule);
>  			if (rc)
>  				goto out;
>  		}
> diff --git a/libflash/blocklevel.h b/libflash/blocklevel.h
> index ba42c83d0..d2d269054 100644
> --- a/libflash/blocklevel.h
> +++ b/libflash/blocklevel.h
> @@ -34,6 +34,15 @@ enum blocklevel_flags {
>  	WRITE_NEED_ERASE = 1,
>  };
>  
> +struct blocklevel_info {
> +	/* name is not owned by this struct and must persist for the
> +	 * liftime of blocklevel_info
> +	 */
> +	const char *name;
> +	uint64_t size;
> +	uint32_t erase_granule;
> +};
> +
>  /*
>   * libffs may be used with different backends, all should provide these for
>   * libflash to get the information it needs
> @@ -45,8 +54,7 @@ struct blocklevel_device {
>  	int (*read)(struct blocklevel_device *bl, uint64_t pos, void *buf, uint64_t len);
>  	int (*write)(struct blocklevel_device *bl, uint64_t pos, const void *buf, uint64_t len);
>  	int (*erase)(struct blocklevel_device *bl, uint64_t pos, uint64_t len);
> -	int (*get_info)(struct blocklevel_device *bl, const char **name, uint64_t *total_size,
> -			uint32_t *erase_granule);
> +	int (*get_info)(struct blocklevel_device *bl, struct blocklevel_info *bl_info);
>  
>  	/*
>  	 * Keep the erase mask so that blocklevel_erase() can do sanity checking
> @@ -62,8 +70,8 @@ int blocklevel_read(struct blocklevel_device *bl, uint64_t pos, void *buf, uint6
>  int blocklevel_raw_write(struct blocklevel_device *bl, uint64_t pos, const void *buf, uint64_t len);
>  int blocklevel_write(struct blocklevel_device *bl, uint64_t pos, const void *buf, uint64_t len);
>  int blocklevel_erase(struct blocklevel_device *bl, uint64_t pos, uint64_t len);
> -int blocklevel_get_info(struct blocklevel_device *bl, const char **name, uint64_t *total_size,
> -		uint32_t *erase_granule);
> +
> +int blocklevel_get_info(struct blocklevel_device *bl, struct blocklevel_info *bl_info);
>  
>  /*
>   * blocklevel_smart_write() performs reads on the data to see if it
> diff --git a/libflash/file.c b/libflash/file.c
> index 7065eb4fa..b87679cd1 100644
> --- a/libflash/file.c
> +++ b/libflash/file.c
> @@ -212,6 +212,7 @@ static int get_info_name(struct file_data *file_data, char **name)
>  	}
>  	lpath[len] = '\0';
>  
> +	free(*name);
>  	*name = lpath;
>  
>  	free(path);
> @@ -219,8 +220,8 @@ static int get_info_name(struct file_data *file_data, char **name)
>  }
>  
>  
> -static int mtd_get_info(struct blocklevel_device *bl, const char **name,
> -		uint64_t *total_size, uint32_t *erase_granule)
> +static int mtd_get_info(struct blocklevel_device *bl,
> +		struct blocklevel_info *bl_info)
>  {
>  	struct file_data *file_data = container_of(bl, struct file_data, bl);
>  	struct mtd_info_user mtd_info;
> @@ -230,24 +231,19 @@ static int mtd_get_info(struct blocklevel_device *bl, const char **name,
>  	if (rc == -1)
>  		return FLASH_ERR_BAD_READ;
>  
> -	if (total_size)
> -		*total_size = mtd_info.size;
> -
> -	if (erase_granule)
> -		*erase_granule = mtd_info.erasesize;
> +	rc = get_info_name(file_data, &(file_data->name));
> +	if (rc)
> +		return rc;
>  
> -	if (name) {
> -		rc = get_info_name(file_data, &(file_data->name));
> -		if (rc)
> -			return rc;
> -		*name = file_data->name;
> -	}
> +	bl_info->name = file_data->name;
> +	bl_info->size = mtd_info.size;
> +	bl_info->erase_granule = mtd_info.erasesize;
>  
>  	return 0;
>  }
>  
> -static int file_get_info(struct blocklevel_device *bl, const char **name,
> -		uint64_t *total_size, uint32_t *erase_granule)
> +static int file_get_info(struct blocklevel_device *bl,
> +		struct blocklevel_info *bl_info)
>  {
>  	struct file_data *file_data = container_of(bl, struct file_data, bl);
>  	struct stat st;
> @@ -256,18 +252,13 @@ static int file_get_info(struct blocklevel_device *bl, const char **name,
>  	if (fstat(file_data->fd, &st))
>  		return FLASH_ERR_PARM_ERROR;
>  
> -	if (total_size)
> -		*total_size = st.st_size;
> -
> -	if (erase_granule)
> -		*erase_granule = 1;
> +	rc = get_info_name(file_data, &(file_data->name));
> +	if (rc)
> +		return rc;
>  
> -	if (name) {
> -		rc = get_info_name(file_data, &(file_data->name));
> -		if (rc)
> -			return rc;
> -		*name = file_data->name;
> -	}
> +	bl_info->name = file_data->name;
> +	bl_info->size = st.st_size;
> +	bl_info->erase_granule = 1;
>  
>  	return 0;
>  }
> @@ -315,8 +306,9 @@ int file_init(int fd, struct blocklevel_device **bl)
>  		file_data->bl.erase = &mtd_erase;
>  		file_data->bl.get_info = &mtd_get_info;
>  		file_data->bl.flags = WRITE_NEED_ERASE;
> -		mtd_get_info(&file_data->bl, NULL, NULL, &(file_data->bl.erase_mask));
> -		file_data->bl.erase_mask--;
> +		struct blocklevel_info bl_info;
> +		mtd_get_info(&file_data->bl, &bl_info);
> +		file_data->bl.erase_mask = bl_info.erase_granule - 1;
>  	} else if (!S_ISREG(sbuf.st_mode)) {
>  		/* If not a char device or a regular file something went wrong */
>  		goto out;
> diff --git a/libflash/libffs.c b/libflash/libffs.c
> index 038f59425..6fc23eee9 100644
> --- a/libflash/libffs.c
> +++ b/libflash/libffs.c
> @@ -216,25 +216,25 @@ int ffs_init(uint32_t offset, uint32_t max_size, struct blocklevel_device *bl,
>  {
>  	struct __ffs_hdr blank_hdr;
>  	struct __ffs_hdr raw_hdr;
> +	struct blocklevel_info bl_info;
>  	struct ffs_handle *f;
> -	uint64_t total_size;
>  	int rc, i;
>  
>  	if (!ffs || !bl)
>  		return FLASH_ERR_PARM_ERROR;
>  	*ffs = NULL;
>  
> -	rc = blocklevel_get_info(bl, NULL, &total_size, NULL);
> +	rc = blocklevel_get_info(bl, &bl_info);
>  	if (rc) {
>  		FL_ERR("FFS: Error %d retrieving flash info\n", rc);
>  		return rc;
>  	}
> -	if (total_size > UINT_MAX)
> +	if (bl_info.size > UINT_MAX)
>  		return FLASH_ERR_VERIFY_FAILURE;
>  	if ((offset + max_size) < offset)
>  		return FLASH_ERR_PARM_ERROR;
>  
> -	if ((max_size > total_size))
> +	if ((max_size > bl_info.size))
>  		return FLASH_ERR_PARM_ERROR;
>  
>  	/* Read flash header */
> diff --git a/libflash/libflash.c b/libflash/libflash.c
> index 38f87b86e..82a2baecc 100644
> --- a/libflash/libflash.c
> +++ b/libflash/libflash.c
> @@ -792,16 +792,15 @@ static int flash_configure(struct flash_chip *c)
>  	return 0;
>  }
>  
> -static int flash_get_info(struct blocklevel_device *bl, const char **name,
> -		   uint64_t *total_size, uint32_t *erase_granule)
> +static int flash_get_info(struct blocklevel_device *bl,
> +		struct blocklevel_info *bl_info)
>  {
>  	struct flash_chip *c = container_of(bl, struct flash_chip, bl);
> -	if (name)
> -		*name = c->info.name;
> -	if (total_size)
> -		*total_size = c->tsize;
> -	if (erase_granule)
> -		*erase_granule = c->min_erase_mask + 1;
> +
> +	bl_info->name = c->info.name;
> +	bl_info->size = c->tsize;
> +	bl_info->erase_granule = c->min_erase_mask + 1;
> +
>  	return 0;
>  }
>  
> diff --git a/libflash/mbox-flash.c b/libflash/mbox-flash.c
> index 950e24f05..d4200ce53 100644
> --- a/libflash/mbox-flash.c
> +++ b/libflash/mbox-flash.c
> @@ -745,8 +745,8 @@ static int mbox_flash_read(struct blocklevel_device *bl, uint64_t pos,
>  	return rc;
>  }
>  
> -static int mbox_flash_get_info(struct blocklevel_device *bl, const char **name,
> -		uint64_t *total_size, uint32_t *erase_granule)
> +static int mbox_flash_get_info(struct blocklevel_device *bl,
> +		struct blocklevel_info *bl_info)
>  {
>  	struct mbox_flash_data *mbox_flash;
>  	struct bmc_mbox_msg *msg;
> @@ -761,14 +761,7 @@ static int mbox_flash_get_info(struct blocklevel_device *bl, const char **name,
>  	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
> -	 * returned.
> -	 * NULL will do for now.
> -	 */
> -	if (name)
> -		*name = NULL;
> +	bl_info->name = "pnor_mbox";
>  
>  	mbox_flash->busy = true;
>  	rc = msg_send(mbox_flash, msg);
> @@ -784,10 +777,8 @@ static int mbox_flash_get_info(struct blocklevel_device *bl, const char **name,
>  
>  	mbox_flash->bl.erase_mask = mbox_flash->erase_granule - 1;
>  
> -	if (total_size)
> -		*total_size = mbox_flash->total_size;
> -	if (erase_granule)
> -		*erase_granule = mbox_flash->erase_granule;
> +	bl_info->size = mbox_flash->total_size;
> +	bl_info->erase_granule = mbox_flash->erase_granule;
>  
>  out:
>  	msg_free_memory(msg);
> diff --git a/libflash/test/test-flash.c b/libflash/test/test-flash.c
> index 3f77d6f3e..1ca467bb0 100644
> --- a/libflash/test/test-flash.c
> +++ b/libflash/test/test-flash.c
> @@ -367,9 +367,7 @@ struct spi_flash_ctrl sim_ctrl = {
>  int main(void)
>  {
>  	struct blocklevel_device *bl;
> -	uint64_t total_size;
> -	uint32_t erase_granule;
> -	const char *name;
> +	struct blocklevel_info bl_info;
>  	uint16_t *test;
>  	struct ecc64 *ecc_test;
>  	uint64_t *test64;
> @@ -384,7 +382,7 @@ int main(void)
>  		ERR("flash_init failed with err %d\n", rc);
>  		exit(1);
>  	}
> -	rc = flash_get_info(bl, &name, &total_size, &erase_granule);
> +	rc = flash_get_info(bl, &bl_info);
>  	if (rc) {
>  		ERR("flash_get_info failed with err %d\n", rc);
>  		exit(1);
> diff --git a/platforms/mambo/mambo.c b/platforms/mambo/mambo.c
> index cb6e103cc..dd7872a8e 100644
> --- a/platforms/mambo/mambo.c
> +++ b/platforms/mambo/mambo.c
> @@ -125,19 +125,14 @@ static int bogus_disk_erase(struct blocklevel_device *bl __unused,
>  	return 0; /* NOP */
>  }
>  
> -static int bogus_disk_get_info(struct blocklevel_device *bl, const char **name,
> -			      uint64_t *total_size, uint32_t *erase_granule)
> +static int bogus_disk_get_info(struct blocklevel_device *bl,
> +			struct blocklevel_info *bl_info)
>  {
>  	struct bogus_disk_info *bdi = bl->priv;
>  
> -	if (total_size)
> -		*total_size = bdi->size;
> -
> -	if (erase_granule)
> -		*erase_granule = BD_SECT_SZ;
> -
> -	if (name)
> -		*name = "mambobogus";
> +	bl_info->name = "mambobogus";
> +	bl_info->size = bdi->size;
> +	bl_info->erase_granule = BD_SECT_SZ;
>  
>  	return 0;
>  }


More information about the Skiboot mailing list