[PATCH 3/9] lib/flash: Add support for platform versions
Sam Mendoza-Jonas
sam.mj at au1.ibm.com
Wed Dec 16 11:41:32 AEDT 2015
On Tue, Dec 15, 2015 at 05:41:01PM +1100, Cyril Bur wrote:
> 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.
Thanks for the review!
>
> > 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?
Yep this is included in flash.h, but it reminds me to go double check a
few other places...
>
> > @@ -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...
I'm postponing dealing with sides at the moment - from machines I've
used during testing I haven't seen a separate ATTR_PERM for the golden
side, but looking now I *do* see a separate one for VERSION.
>
> > +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...
>
No you're right in this case, I should be handling it and trying to
continue without retrieving VERSION info. I'll pick through
the rest of the series and check similar calls.
> > +
> > + 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?
>
True, if nothing else the naming could be better. The original rationale
(when ATTR_PERM handling was interleaved with these changes) was to find
a given partition and read it out into a usable buffer in one step to
avoid having to deal with ECC during the rest of the attribute logic.
I *could* move the actual reading to flash_read_version() but as soon as
I add in the attribute code I'll be repeating a similar process 1-2 more
times elsewhere.
> > +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?
>
s/2013/2015 (or 2016 depending on when I merge this?)
> > + * 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?
>
Ah yes
> > + */
> > +
> > +#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