[PATCH 3/9] lib/flash: Add support for platform versions

Cyril Bur cyril.bur at au1.ibm.com
Tue Dec 15 17:41:01 AEDT 2015


On Tue, 15 Dec 2015 14:15:24 +1100
Samuel Mendoza-Jonas <sam.mj at au1.ibm.com> wrote:

> Add basic libflash support to read the VERSION partition on BMC
> machines.
> 

Comments inline.

> Signed-off-by: Samuel Mendoza-Jonas <sam.mj at au1.ibm.com>
> ---
>  lib/Makefile.am    |  19 ++++++-
>  lib/flash/config.h |  19 +++++++
>  lib/flash/flash.c  | 164 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/flash/flash.h  |  26 +++++++++
>  4 files changed, 227 insertions(+), 1 deletion(-)
>  create mode 100644 lib/flash/config.h
>  create mode 100644 lib/flash/flash.c
>  create mode 100644 lib/flash/flash.h
> 
> diff --git a/lib/Makefile.am b/lib/Makefile.am
> index a2421a5..a3ae943 100644
> --- a/lib/Makefile.am
> +++ b/lib/Makefile.am
> @@ -48,4 +48,21 @@ lib_libpbcore_la_SOURCES = \
>  	lib/url/url.c \
>  	lib/url/url.h \
>  	lib/util/util.c \
> -	lib/util/util.h
> +	lib/util/util.h \
> +	lib/flash/config.h \
> +	lib/flash/flash.h
> +
> +if ENABLE_MTD
> +lib_libpbcore_la_SOURCES += \
> +	lib/flash/flash.c
> +
> +lib_libpbcore_la_CPPFLAGS += \
> +	$(AM_CPPFLAGS)
> +
> +lib_libpbcore_la_LDFLAGS = \
> +	$(AM_LDFLAGS) \
> +	-l:libflash.so
> +
> +lib_libpbcore_la_SOURCES += \
> +	lib/flash/flash.c
> +endif
> diff --git a/lib/flash/config.h b/lib/flash/config.h
> new file mode 100644
> index 0000000..a132a01
> --- /dev/null
> +++ b/lib/flash/config.h
> @@ -0,0 +1,19 @@
> +/* For CCAN */
> +
> +#include <endian.h>
> +#include <byteswap.h>
> +
> +#define HAVE_TYPEOF			1
> +#define HAVE_BUILTIN_TYPES_COMPATIBLE_P	1
> +
> +
> +#if __BYTE_ORDER == __LITTLE_ENDIAN
> +#define HAVE_BIG_ENDIAN         0
> +#define HAVE_LITTLE_ENDIAN      1
> +#else
> +#define HAVE_BIG_ENDIAN         1
> +#define HAVE_LITTLE_ENDIAN      0
> +#endif
> +
> +#define HAVE_BYTESWAP_H 1
> +#define HAVE_BSWAP_64	1
> diff --git a/lib/flash/flash.c b/lib/flash/flash.c
> new file mode 100644
> index 0000000..6705776
> --- /dev/null
> +++ b/lib/flash/flash.c

You have a Copyright on flash.h?

> @@ -0,0 +1,164 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <getopt.h>
> +#include <errno.h>
> +#include <asm/byteorder.h>
> +
> +#include <log/log.h>
> +#include <talloc/talloc.h>
> +#include <flash/flash.h>
> +#include <ccan/endian/endian.h>
> +
> +#include <libflash/arch_flash.h>
> +#include <libflash/blocklevel.h>
> +#include <libflash/libflash.h>
> +#include <libflash/libffs.h>
> +#include <libflash/file.h>
> +#include <libflash/ecc.h>
> +
> +
> +struct flash_info {
> +	/* Device information */
> +	struct blocklevel_device	*bl;
> +	struct ffs_handle		*ffs;
> +	uint32_t			size;	/* raw size of partition */
> +	const char			*path;
> +	bool				ecc;
> +	uint32_t			erase_granule;
> +
> +	/* Partition information */
> +	uint32_t			attr_part_idx;
> +	uint32_t			attr_data_pos;
> +	uint32_t			attr_data_len; /* Includes ECC bytes */
> +
> +	/* Buffer information */
> +	char				*buffer;
> +	uint32_t			len;
> +};
> +
> +//TODO May need to consider flash sides

Indeed I suspect you will.

However, the logic that I wrote for pflash really is tool independent, I'll
whip something up tomorrow to see if I can get it into external/common code
before you have a problem here...

On second thought, an ffs method might be the go...

> +static struct flash_info *flash_setup(void *ctx, const char *partition,
> +				      bool ecc)
> +{
> +	struct flash_info *info;
> +	int rc = 0;
> +	const char *filename = NULL;
> +	//DEBUG: set filename/partition if local
> +
> +	info = talloc_zero(ctx, struct flash_info);

General theme here, I assume you're cool with segfaults? I think we have this
chat every time you send patches, I'm just obligated to point it out but
yeah....

Having said that, your code later on seems to care about flash_setup possibly
failing. If you can handle failures then I would say handle the ('never going
to happen') ENOMEM...

> +
> +	rc = arch_flash_init(&info->bl, filename);
> +	if (rc) {
> +		pb_log("Failed to init %s\n",
> +		       filename ? "%s" : "mtd device");
> +		return NULL;
> +	}
> +
> +	rc = blocklevel_get_info(info->bl, &info->path, &info->size,
> +				 &info->erase_granule);
> +	if (rc) {
> +		pb_log("Failed to retrieve blocklevel info\n");
> +		return NULL;
> +	}
> +
> +	if (partition) {
> +		rc = ffs_init(0, info->size, info->bl, &info->ffs, 1);
> +		if (rc) {
> +			pb_log("%s: Failed to init ffs\n", __func__);
> +			goto out;
> +		}
> +		rc = ffs_lookup_part(info->ffs, partition, &info->attr_part_idx);
> +		if (rc) {
> +			pb_log("Failed to find %s partition\n", partition);
> +			goto out;
> +		}
> +		rc = ffs_part_info(info->ffs, info->attr_part_idx, NULL,
> +				   &info->attr_data_pos, &info->attr_data_len,
> +				   NULL, &info->ecc);
> +		if (rc) {
> +			pb_log("Failed to retrive partition info\n");
> +			goto out;
> +		}
> +	} else  {
> +		/* If no partition name provided, assume the specified file
> +		 * points directly to the partition */
> +		if (ecc) {
> +			rc = blocklevel_ecc_protect(info->bl, 0, info->size);
> +			if (rc)
> +				pb_debug("Partition is not ECC protected, "
> +				       "or error checking ECC\n");
> +			else
> +				info->ecc = true;
> +		}
> +		info->attr_data_pos = 0;
> +		info->attr_data_len = info->size;
> +	}
> +
> +	pb_debug("%s Details\n", partition ? partition : "Partition");
> +	pb_debug("\tName\t\t%s\n", info->path);
> +	pb_debug("\tFlash Size\t%u\n", info->size);
> +	pb_debug("\tGranule\t\t%u\n", info->erase_granule);
> +	pb_debug("\tECC\t\t%s\n", info->ecc ? "Protected" : "Unprotected");
> +	pb_debug("\tIndex\t\t%u\n", info->attr_part_idx);
> +	pb_debug("\tOffset\t\t%u\n", info->attr_data_pos);
> +	pb_debug("\tPart. Size\t%u\n", info->attr_data_len);
> +
> +	info->len = info->attr_data_len -  ecc_size(info->attr_data_len);
> +	info->buffer = talloc_array(info, char, info->len);
> +	if (!info->buffer) {
> +		pb_log("%s: Failed to init buffer!\n", __func__);
> +		goto out;
> +	}
> +
> +	rc = blocklevel_read(info->bl, info->attr_data_pos,
> +			     info->buffer, info->len);
> +	if (rc) {
> +		pb_log("Failed to read %s\n",
> +		       partition ? partition : "Partition");
> +		goto out;
> +	}
> +
> +	return info;
> +out:
> +	arch_flash_close(info->bl, filename);
> +	talloc_free(info);
> +	return NULL;
> +}
> +

So question, why does flash_setup() do the actual ummm reading and
flash_read_version() does the formatting...

Couldn't flash_setup() just return a struct blocklevel?

> +int flash_read_version(void *ctx, char ***versions)
> +{
> +	char *saveptr, *tok,  **tmp;
> +	const char *delim = "\n";
> +	struct flash_info *info;
> +	int n = 0;
> +
> +	saveptr = tok = NULL;
> +	tmp = NULL;
> +
> +	info = flash_setup(ctx, "VERSION", false);
> +	if (!info)
> +		return 0;
> +
> +	/* open-power-platform */
> +	tok = strtok_r(info->buffer, delim, &saveptr);
> +	if (tok) {
> +		tmp = talloc_array(ctx, char *, 1);
> +		tmp[n++] = talloc_strdup(ctx, tok);
> +	}
> +
> +	tok = strtok_r(NULL, delim, &saveptr);
> +	while (tok) {
> +		/* Ignore leading tab from subsequent lines */
> +		tmp = talloc_realloc(ctx, tmp, char *, n + 1);
> +		tmp[n++] = talloc_strdup(ctx, tok + 1);
> +		tok = strtok_r(NULL, delim, &saveptr);
> +	}
> +
> +	arch_flash_close(info->bl, NULL);
> +	talloc_free(info);
> +
> +	*versions = tmp;
> +	return n;
> +}
> diff --git a/lib/flash/flash.h b/lib/flash/flash.h
> new file mode 100644
> index 0000000..103f1e4
> --- /dev/null
> +++ b/lib/flash/flash.h
> @@ -0,0 +1,26 @@
> +/*
> + *  Copyright (C) 2013 IBM Corporation
> + *

2013?

> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA

I think newage wisdom is to drop the address because it changes no?

> + */
> +
> +#ifndef FLASH_H
> +#define FLASH_H
> +
> +#include <flash/config.h>
> +#include <types/types.h>
> +
> +int flash_read_version(void *ctx, char ***versions);
> +
> +#endif /* FLASH_H */



More information about the Petitboot mailing list