[U-Boot] [PATCH v2 0/5] Update fastboot sparse image handling

While retaining the storage abstraction feature implemented in U-Boot, this series updates the fastboot sparse image handling by (1) fixing broken code, (2) resync'ing with the upstream code, and (3) improving performance when writing CHUNK_TYPE_FILL
Changes in v2: - series rebased onto v2016.07-rc1
Steve Rae (5): fastboot: sparse: remove session-id logic fastboot: sparse: resync common/image-sparse.c (part 1) fastboot: sparse: resync common/image-sparse.c (part 2) fastboot: sparse: implement reserve() fastboot: sparse: improve CHUNK_TYPE_FILL write performance
common/fb_mmc.c | 79 +++---- common/fb_nand.c | 106 +++++---- common/image-sparse.c | 478 +++++++++++++++------------------------- drivers/usb/gadget/f_fastboot.c | 47 ++-- include/fastboot.h | 4 +- include/fb_mmc.h | 7 +- include/fb_nand.h | 7 +- include/image-sparse.h | 29 ++- 8 files changed, 312 insertions(+), 445 deletions(-)

This "session-id" alogrithm is not required, and currently corrupts the stored image whenever more the one "session" is required.
Signed-off-by: Steve Rae srae@broadcom.com --- for more information, see the thread starting at [1] [1] http://lists.denx.de/pipermail/u-boot/2016-April/251889.html
Changes in v2: - series rebased onto v2016.07-rc1
common/fb_mmc.c | 8 +++----- common/fb_nand.c | 4 ++-- common/image-sparse.c | 21 ++++----------------- drivers/usb/gadget/f_fastboot.c | 16 ++-------------- include/fb_mmc.h | 5 ++--- include/fb_nand.h | 5 ++--- include/image-sparse.h | 2 +- 7 files changed, 16 insertions(+), 45 deletions(-)
diff --git a/common/fb_mmc.c b/common/fb_mmc.c index e3abcc8..9e53adb 100644 --- a/common/fb_mmc.c +++ b/common/fb_mmc.c @@ -97,9 +97,8 @@ static void write_raw_image(struct blk_desc *dev_desc, disk_partition_t *info, fastboot_okay(response_str, ""); }
-void fb_mmc_flash_write(const char *cmd, unsigned int session_id, - void *download_buffer, unsigned int download_bytes, - char *response) +void fb_mmc_flash_write(const char *cmd, void *download_buffer, + unsigned int download_bytes, char *response) { struct blk_desc *dev_desc; disk_partition_t info; @@ -153,8 +152,7 @@ void fb_mmc_flash_write(const char *cmd, unsigned int session_id, printf("Flashing sparse image at offset " LBAFU "\n", info.start);
- store_sparse_image(&sparse, &sparse_priv, session_id, - download_buffer); + store_sparse_image(&sparse, &sparse_priv, download_buffer); } else { write_raw_image(dev_desc, &info, cmd, download_buffer, download_bytes); diff --git a/common/fb_nand.c b/common/fb_nand.c index e55ea38..c17e2f0 100644 --- a/common/fb_nand.c +++ b/common/fb_nand.c @@ -126,7 +126,7 @@ static int fb_nand_sparse_write(struct sparse_storage *storage, return written / storage->block_sz; }
-void fb_nand_flash_write(const char *partname, unsigned int session_id, +void fb_nand_flash_write(const char *partname, void *download_buffer, unsigned int download_bytes, char *response) { @@ -161,7 +161,7 @@ void fb_nand_flash_write(const char *partname, unsigned int session_id, sparse.name = part->name; sparse.write = fb_nand_sparse_write;
- ret = store_sparse_image(&sparse, &sparse_priv, session_id, + ret = store_sparse_image(&sparse, &sparse_priv, download_buffer); } else { printf("Flashing raw image at offset 0x%llx\n", diff --git a/common/image-sparse.c b/common/image-sparse.c index 2bf737b..893c68b 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -52,8 +52,6 @@ typedef struct sparse_buffer { u16 type; } sparse_buffer_t;
-static uint32_t last_offset; - static unsigned int sparse_get_chunk_data_size(sparse_header_t *sparse, chunk_header_t *chunk) { @@ -267,8 +265,8 @@ static void sparse_put_data_buffer(sparse_buffer_t *buffer) free(buffer); }
-int store_sparse_image(sparse_storage_t *storage, void *storage_priv, - unsigned int session_id, void *data) +int store_sparse_image(sparse_storage_t *storage, + void *storage_priv, void *data) { unsigned int chunk, offset; sparse_header_t *sparse_header; @@ -303,19 +301,10 @@ int store_sparse_image(sparse_storage_t *storage, void *storage_priv, return -EINVAL; }
- /* - * If it's a new flashing session, start at the beginning of - * the partition. If not, then simply resume where we were. - */ - if (session_id > 0) - start = last_offset; - else - start = storage->start; - - printf("Flashing sparse image on partition %s at offset 0x%x (ID: %d)\n", - storage->name, start * storage->block_sz, session_id); + puts("Flashing Sparse Image\n");
/* Start processing chunks */ + start = storage->start; for (chunk = 0; chunk < sparse_header->total_chunks; chunk++) { uint32_t blkcnt;
@@ -390,7 +379,5 @@ int store_sparse_image(sparse_storage_t *storage, void *storage_priv, return -EIO; }
- last_offset = start + total_blocks; - return 0; } diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 28b244a..ddf989c 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -59,7 +59,6 @@ static inline struct f_fastboot *func_to_fastboot(struct usb_function *f) }
static struct f_fastboot *fastboot_func; -static unsigned int fastboot_flash_session_id; static unsigned int download_size; static unsigned int download_bytes;
@@ -424,15 +423,6 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req)
sprintf(str_num, "0x%08x", CONFIG_FASTBOOT_BUF_SIZE); strncat(response, str_num, chars_left); - - /* - * This also indicates the start of a new flashing - * "session", in which we could have 1-N buffers to - * write to a partition. - * - * Reset our session counter. - */ - fastboot_flash_session_id = 0; } else if (!strcmp_l1("serialno", cmd)) { s = getenv("serial#"); if (s) @@ -600,16 +590,14 @@ static void cb_flash(struct usb_ep *ep, struct usb_request *req)
strcpy(response, "FAILno flash device defined"); #ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV - fb_mmc_flash_write(cmd, fastboot_flash_session_id, - (void *)CONFIG_FASTBOOT_BUF_ADDR, + fb_mmc_flash_write(cmd, (void *)CONFIG_FASTBOOT_BUF_ADDR, download_bytes, response); #endif #ifdef CONFIG_FASTBOOT_FLASH_NAND_DEV - fb_nand_flash_write(cmd, fastboot_flash_session_id, + fb_nand_flash_write(cmd, (void *)CONFIG_FASTBOOT_BUF_ADDR, download_bytes, response); #endif - fastboot_flash_session_id++; fastboot_tx_write_str(response); } #endif diff --git a/include/fb_mmc.h b/include/fb_mmc.h index 978a139..402ba9b 100644 --- a/include/fb_mmc.h +++ b/include/fb_mmc.h @@ -4,7 +4,6 @@ * SPDX-License-Identifier: GPL-2.0+ */
-void fb_mmc_flash_write(const char *cmd, unsigned int session_id, - void *download_buffer, unsigned int download_bytes, - char *response); +void fb_mmc_flash_write(const char *cmd, void *download_buffer, + unsigned int download_bytes, char *response); void fb_mmc_erase(const char *cmd, char *response); diff --git a/include/fb_nand.h b/include/fb_nand.h index 80ddef5..88bdf36 100644 --- a/include/fb_nand.h +++ b/include/fb_nand.h @@ -5,7 +5,6 @@ * SPDX-License-Identifier: GPL-2.0+ */
-void fb_nand_flash_write(const char *cmd, unsigned int session_id, - void *download_buffer, unsigned int download_bytes, - char *response); +void fb_nand_flash_write(const char *cmd, void *download_buffer, + unsigned int download_bytes, char *response); void fb_nand_erase(const char *cmd, char *response); diff --git a/include/image-sparse.h b/include/image-sparse.h index 0382f5b..a2b0694 100644 --- a/include/image-sparse.h +++ b/include/image-sparse.h @@ -32,4 +32,4 @@ static inline int is_sparse_image(void *buf) }
int store_sparse_image(sparse_storage_t *storage, void *storage_priv, - unsigned int session_id, void *data); + void *data);

On Tue, Jun 07, 2016 at 11:19:35AM -0700, Steve Rae wrote:
This "session-id" alogrithm is not required, and currently corrupts the stored image whenever more the one "session" is required.
Signed-off-by: Steve Rae srae@broadcom.com
Applied to u-boot/master, thanks!

This file originally came from upstream code.
While retaining the storage abstraction feature, this is the first set of the changes required to resync with the cmd_flash_mmc_sparse_img() in the file aboot.c from https://us.codeaurora.org/cgit/quic/la/kernel/lk/plain/app/aboot/aboot.c?h=L...
Signed-off-by: Steve Rae srae@broadcom.com ---
Changes in v2: None
common/fb_mmc.c | 32 ++-- common/fb_nand.c | 58 ++++--- common/image-sparse.c | 449 +++++++++++++++++-------------------------------- include/image-sparse.h | 25 +-- 4 files changed, 211 insertions(+), 353 deletions(-)
diff --git a/common/fb_mmc.c b/common/fb_mmc.c index 9e53adb..3dad0ea 100644 --- a/common/fb_mmc.c +++ b/common/fb_mmc.c @@ -7,12 +7,10 @@ #include <config.h> #include <common.h> #include <blk.h> -#include <errno.h> #include <fastboot.h> #include <fb_mmc.h> #include <image-sparse.h> #include <part.h> -#include <sparse_format.h> #include <mmc.h> #include <div64.h>
@@ -48,22 +46,13 @@ static int part_get_info_efi_by_name_or_alias(struct blk_desc *dev_desc, return ret; }
- -static int fb_mmc_sparse_write(struct sparse_storage *storage, - void *priv, - unsigned int offset, - unsigned int size, - char *data) +static lbaint_t fb_mmc_sparse_write(struct sparse_storage *info, + lbaint_t blk, lbaint_t blkcnt, const void *buffer) { - struct fb_mmc_sparse *sparse = priv; + struct fb_mmc_sparse *sparse = info->priv; struct blk_desc *dev_desc = sparse->dev_desc; - int ret; - - ret = blk_dwrite(dev_desc, offset, size, data); - if (!ret) - return -EIO;
- return ret; + return blk_dwrite(dev_desc, blk, blkcnt, buffer); }
static void write_raw_image(struct blk_desc *dev_desc, disk_partition_t *info, @@ -139,26 +128,25 @@ void fb_mmc_flash_write(const char *cmd, void *download_buffer,
if (is_sparse_image(download_buffer)) { struct fb_mmc_sparse sparse_priv; - sparse_storage_t sparse; + struct sparse_storage sparse;
sparse_priv.dev_desc = dev_desc;
- sparse.block_sz = info.blksz; + sparse.blksz = info.blksz; sparse.start = info.start; sparse.size = info.size; - sparse.name = cmd; sparse.write = fb_mmc_sparse_write;
printf("Flashing sparse image at offset " LBAFU "\n", - info.start); + sparse.start);
- store_sparse_image(&sparse, &sparse_priv, download_buffer); + sparse.priv = &sparse_priv; + write_sparse_image(&sparse, cmd, download_buffer, + download_bytes, response_str); } else { write_raw_image(dev_desc, &info, cmd, download_buffer, download_bytes); } - - fastboot_okay(response_str, ""); }
void fb_mmc_erase(const char *cmd, char *response) diff --git a/common/fb_nand.c b/common/fb_nand.c index c17e2f0..bdfad17 100644 --- a/common/fb_nand.c +++ b/common/fb_nand.c @@ -10,7 +10,6 @@
#include <fastboot.h> #include <image-sparse.h> -#include <sparse_format.h>
#include <linux/mtd/mtd.h> #include <jffs2/jffs2.h> @@ -19,7 +18,7 @@ static char *response_str;
struct fb_nand_sparse { - struct mtd_info *nand; + struct mtd_info *mtd; struct part_info *part; };
@@ -34,7 +33,7 @@ __weak int board_fastboot_write_partition_setup(char *name) }
static int fb_nand_lookup(const char *partname, char *response, - struct mtd_info **nand, + struct mtd_info **mtd, struct part_info **part) { struct mtd_device *dev; @@ -105,30 +104,32 @@ static int _fb_nand_write(struct mtd_info *mtd, struct part_info *part, buffer, flags); }
-static int fb_nand_sparse_write(struct sparse_storage *storage, - void *priv, - unsigned int offset, - unsigned int size, - char *data) +static lbaint_t fb_nand_sparse_write(struct sparse_storage *info, + lbaint_t blk, lbaint_t blkcnt, const void *buffer) { - struct fb_nand_sparse *sparse = priv; + struct fb_nand_sparse *sparse = info->priv; size_t written; int ret;
- ret = _fb_nand_write(sparse->nand, sparse->part, data, - offset * storage->block_sz, - size * storage->block_sz, &written); + ret = _fb_nand_write(sparse->mtd, sparse->part, (void *)buffer, + blk * info->blksz, + blkcnt * info->blksz, &written); if (ret < 0) { printf("Failed to write sparse chunk\n"); return ret; }
- return written / storage->block_sz; +/* TODO - verify that the value "written" includes the "bad-blocks" ... */ + + /* + * the return value must be 'blkcnt' ("good-blocks") plus the + * number of "bad-blocks" encountered within this space... + */ + return written / info->blksz; }
-void fb_nand_flash_write(const char *partname, - void *download_buffer, unsigned int download_bytes, - char *response) +void fb_nand_flash_write(const char *cmd, void *download_buffer, + unsigned int download_bytes, char *response) { struct part_info *part; struct mtd_info *mtd = NULL; @@ -137,7 +138,7 @@ void fb_nand_flash_write(const char *partname, /* initialize the response buffer */ response_str = response;
- ret = fb_nand_lookup(partname, response, &mtd, &part); + ret = fb_nand_lookup(cmd, response, &mtd, &part); if (ret) { error("invalid NAND device"); fastboot_fail(response_str, "invalid NAND device"); @@ -150,19 +151,22 @@ void fb_nand_flash_write(const char *partname,
if (is_sparse_image(download_buffer)) { struct fb_nand_sparse sparse_priv; - sparse_storage_t sparse; + struct sparse_storage sparse;
- sparse_priv.nand = mtd; + sparse_priv.mtd = mtd; sparse_priv.part = part;
- sparse.block_sz = mtd->writesize; - sparse.start = part->offset / sparse.block_sz; - sparse.size = part->size / sparse.block_sz; - sparse.name = part->name; + sparse.blksz = mtd->writesize; + sparse.start = part->offset / sparse.blksz; + sparse.size = part->size / sparse.blksz; sparse.write = fb_nand_sparse_write;
- ret = store_sparse_image(&sparse, &sparse_priv, - download_buffer); + printf("Flashing sparse image at offset " LBAFU "\n", + sparse.start); + + sparse.priv = &sparse_priv; + write_sparse_image(&sparse, cmd, download_buffer, + download_bytes, response_str); } else { printf("Flashing raw image at offset 0x%llx\n", part->offset); @@ -182,7 +186,7 @@ void fb_nand_flash_write(const char *partname, fastboot_okay(response_str, ""); }
-void fb_nand_erase(const char *partname, char *response) +void fb_nand_erase(const char *cmd, char *response) { struct part_info *part; struct mtd_info *mtd = NULL; @@ -191,7 +195,7 @@ void fb_nand_erase(const char *partname, char *response) /* initialize the response buffer */ response_str = response;
- ret = fb_nand_lookup(partname, response, &mtd, &part); + ret = fb_nand_lookup(cmd, response, &mtd, &part); if (ret) { error("invalid NAND device"); fastboot_fail(response_str, "invalid NAND device"); diff --git a/common/image-sparse.c b/common/image-sparse.c index 893c68b..924cc63 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -36,54 +36,44 @@
#include <config.h> #include <common.h> -#include <div64.h> -#include <errno.h> #include <image-sparse.h> +#include <div64.h> #include <malloc.h> #include <part.h> #include <sparse_format.h> +#include <fastboot.h>
#include <linux/math64.h>
-typedef struct sparse_buffer { - void *data; - u32 length; - u32 repeat; - u16 type; -} sparse_buffer_t; - -static unsigned int sparse_get_chunk_data_size(sparse_header_t *sparse, - chunk_header_t *chunk) +void write_sparse_image( + struct sparse_storage *info, const char *part_name, + void *data, unsigned sz, char *response_str) { - return chunk->total_sz - sparse->chunk_hdr_sz; -} - -static unsigned int sparse_block_size_to_storage(unsigned int size, - sparse_storage_t *storage, - sparse_header_t *sparse) -{ - return (unsigned int)lldiv((uint64_t)size * sparse->blk_sz, - storage->block_sz); -} - -static bool sparse_chunk_has_buffer(chunk_header_t *chunk) -{ - switch (chunk->chunk_type) { - case CHUNK_TYPE_RAW: - case CHUNK_TYPE_FILL: - return true; - - default: - return false; - } -} + lbaint_t blk; + lbaint_t blkcnt; + lbaint_t blks; + uint32_t bytes_written = 0; + unsigned int chunk; + unsigned int offset; + unsigned int chunk_data_sz; + uint32_t *fill_buf = NULL; + uint32_t fill_val; + sparse_header_t *sparse_header; + chunk_header_t *chunk_header; + uint32_t total_blocks = 0; + int i;
-static sparse_header_t *sparse_parse_header(void **data) -{ /* Read and skip over sparse image header */ - sparse_header_t *sparse_header = (sparse_header_t *) *data; + sparse_header = (sparse_header_t *)data;
- *data += sparse_header->file_hdr_sz; + data += sparse_header->file_hdr_sz; + if (sparse_header->file_hdr_sz > sizeof(sparse_header_t)) { + /* + * Skip the remaining bytes in a header that is longer than + * we expected. + */ + data += (sparse_header->file_hdr_sz - sizeof(sparse_header_t)); + }
debug("=== Sparse Image Header ===\n"); debug("magic: 0x%x\n", sparse_header->magic); @@ -95,289 +85,164 @@ static sparse_header_t *sparse_parse_header(void **data) debug("total_blks: %d\n", sparse_header->total_blks); debug("total_chunks: %d\n", sparse_header->total_chunks);
- return sparse_header; -} - -static int sparse_parse_fill_chunk(sparse_header_t *sparse, - chunk_header_t *chunk) -{ - unsigned int chunk_data_sz = sparse_get_chunk_data_size(sparse, chunk); - - if (chunk_data_sz != sizeof(uint32_t)) - return -EINVAL; - - return 0; -} - -static int sparse_parse_raw_chunk(sparse_header_t *sparse, - chunk_header_t *chunk) -{ - unsigned int chunk_data_sz = sparse_get_chunk_data_size(sparse, chunk); - - /* Check if the data size is a multiple of the main block size */ - if (chunk_data_sz % sparse->blk_sz) - return -EINVAL; - - /* Check that the chunk size is consistent */ - if ((chunk_data_sz / sparse->blk_sz) != chunk->chunk_sz) - return -EINVAL; - - return 0; -} - -static chunk_header_t *sparse_parse_chunk(sparse_header_t *sparse, - void **image) -{ - chunk_header_t *chunk = (chunk_header_t *) *image; - int ret; - - debug("=== Chunk Header ===\n"); - debug("chunk_type: 0x%x\n", chunk->chunk_type); - debug("chunk_data_sz: 0x%x\n", chunk->chunk_sz); - debug("total_size: 0x%x\n", chunk->total_sz); - - switch (chunk->chunk_type) { - case CHUNK_TYPE_RAW: - ret = sparse_parse_raw_chunk(sparse, chunk); - if (ret) - return NULL; - break; - - case CHUNK_TYPE_FILL: - ret = sparse_parse_fill_chunk(sparse, chunk); - if (ret) - return NULL; - break; - - case CHUNK_TYPE_DONT_CARE: - case CHUNK_TYPE_CRC32: - debug("Ignoring chunk\n"); - break; - - default: - printf("%s: Unknown chunk type: %x\n", __func__, - chunk->chunk_type); - return NULL; - } - - *image += sparse->chunk_hdr_sz; - - return chunk; -} - -static int sparse_get_fill_buffer(sparse_header_t *sparse, - chunk_header_t *chunk, - sparse_buffer_t *buffer, - unsigned int blk_sz, - void *data) -{ - int i; - - buffer->type = CHUNK_TYPE_FILL; - - /* - * We create a buffer of one block, and ask it to be - * repeated as many times as needed. - */ - buffer->length = blk_sz; - buffer->repeat = (chunk->chunk_sz * sparse->blk_sz) / blk_sz; - - buffer->data = memalign(ARCH_DMA_MINALIGN, - ROUNDUP(blk_sz, - ARCH_DMA_MINALIGN)); - if (!buffer->data) - return -ENOMEM; - - for (i = 0; i < (buffer->length / sizeof(uint32_t)); i++) - ((uint32_t *)buffer->data)[i] = *(uint32_t *)(data); - - return 0; -} - -static int sparse_get_raw_buffer(sparse_header_t *sparse, - chunk_header_t *chunk, - sparse_buffer_t *buffer, - unsigned int blk_sz, - void *data) -{ - unsigned int chunk_data_sz = sparse_get_chunk_data_size(sparse, chunk); - - buffer->type = CHUNK_TYPE_RAW; - buffer->length = chunk_data_sz; - buffer->data = data; - buffer->repeat = 1; - - return 0; -} - -static sparse_buffer_t *sparse_get_data_buffer(sparse_header_t *sparse, - chunk_header_t *chunk, - unsigned int blk_sz, - void **image) -{ - unsigned int chunk_data_sz = sparse_get_chunk_data_size(sparse, chunk); - sparse_buffer_t *buffer; - void *data = *image; - int ret; - - *image += chunk_data_sz; - - if (!sparse_chunk_has_buffer(chunk)) - return NULL; - - buffer = calloc(sizeof(sparse_buffer_t), 1); - if (!buffer) - return NULL; - - switch (chunk->chunk_type) { - case CHUNK_TYPE_RAW: - ret = sparse_get_raw_buffer(sparse, chunk, buffer, blk_sz, - data); - if (ret) - return NULL; - break; - - case CHUNK_TYPE_FILL: - ret = sparse_get_fill_buffer(sparse, chunk, buffer, blk_sz, - data); - if (ret) - return NULL; - break; - - default: - return NULL; - } - - debug("=== Buffer ===\n"); - debug("length: 0x%x\n", buffer->length); - debug("repeat: 0x%x\n", buffer->repeat); - debug("type: 0x%x\n", buffer->type); - debug("data: 0x%p\n", buffer->data); - - return buffer; -} - -static void sparse_put_data_buffer(sparse_buffer_t *buffer) -{ - if (buffer->type == CHUNK_TYPE_FILL) - free(buffer->data); - - free(buffer); -} - -int store_sparse_image(sparse_storage_t *storage, - void *storage_priv, void *data) -{ - unsigned int chunk, offset; - sparse_header_t *sparse_header; - chunk_header_t *chunk_header; - sparse_buffer_t *buffer; - uint32_t start; - uint32_t total_blocks = 0; - int i; - - debug("=== Storage ===\n"); - debug("name: %s\n", storage->name); - debug("block_size: 0x%x\n", storage->block_sz); - debug("start: 0x%x\n", storage->start); - debug("size: 0x%x\n", storage->size); - debug("write: 0x%p\n", storage->write); - debug("priv: 0x%p\n", storage_priv); - - sparse_header = sparse_parse_header(&data); - if (!sparse_header) { - printf("sparse header issue\n"); - return -EINVAL; - } - /* * Verify that the sparse block size is a multiple of our * storage backend block size */ - div_u64_rem(sparse_header->blk_sz, storage->block_sz, &offset); + div_u64_rem(sparse_header->blk_sz, info->blksz, &offset); if (offset) { printf("%s: Sparse image block size issue [%u]\n", __func__, sparse_header->blk_sz); - return -EINVAL; + fastboot_fail(response_str, "sparse image block size issue"); + return; }
puts("Flashing Sparse Image\n");
/* Start processing chunks */ - start = storage->start; + blk = info->start; for (chunk = 0; chunk < sparse_header->total_chunks; chunk++) { - uint32_t blkcnt; - - chunk_header = sparse_parse_chunk(sparse_header, &data); - if (!chunk_header) { - printf("Unknown chunk type"); - return -EINVAL; + /* Read and skip over chunk header */ + chunk_header = (chunk_header_t *)data; + data += sizeof(chunk_header_t); + + if (chunk_header->chunk_type != CHUNK_TYPE_RAW) { + debug("=== Chunk Header ===\n"); + debug("chunk_type: 0x%x\n", chunk_header->chunk_type); + debug("chunk_data_sz: 0x%x\n", chunk_header->chunk_sz); + debug("total_size: 0x%x\n", chunk_header->total_sz); }
- /* - * If we have a DONT_CARE type, just skip the blocks - * and go on parsing the rest of the chunks - */ - if (chunk_header->chunk_type == CHUNK_TYPE_DONT_CARE) { - blkcnt = sparse_block_size_to_storage(chunk_header->chunk_sz, - storage, - sparse_header); -#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV - total_blocks += blkcnt; -#endif - continue; + if (sparse_header->chunk_hdr_sz > sizeof(chunk_header_t)) { + /* + * Skip the remaining bytes in a header that is longer + * than we expected. + */ + data += (sparse_header->chunk_hdr_sz - + sizeof(chunk_header_t)); }
- /* Retrieve the buffer we're going to write */ - buffer = sparse_get_data_buffer(sparse_header, chunk_header, - storage->block_sz, &data); - if (!buffer) - continue; + chunk_data_sz = sparse_header->blk_sz * chunk_header->chunk_sz; + blkcnt = chunk_data_sz / info->blksz; + switch (chunk_header->chunk_type) { + case CHUNK_TYPE_RAW: + if (chunk_header->total_sz != + (sparse_header->chunk_hdr_sz + chunk_data_sz)) { + fastboot_fail(response_str, + "Bogus chunk size for chunk type Raw"); + return; + } + + if (blk + blkcnt > info->start + info->size) { + printf( + "%s: Request would exceed partition size!\n", + __func__); + fastboot_fail(response_str, + "Request would exceed partition size!"); + return; + }
- blkcnt = (buffer->length / storage->block_sz) * buffer->repeat; + blks = info->write(info, blk, blkcnt, data); + /* blks might be > blkcnt (eg. NAND bad-blocks) */ + if (blks < blkcnt) { + printf("%s: %s" LBAFU " [" LBAFU "]\n", + __func__, "Write failed, block #", + blk, blks); + fastboot_fail(response_str, + "flash write failure"); + return; + } + blk += blks; + bytes_written += blkcnt * info->blksz; + total_blocks += chunk_header->chunk_sz; + data += chunk_data_sz; + break; + + case CHUNK_TYPE_FILL: + if (chunk_header->total_sz != + (sparse_header->chunk_hdr_sz + sizeof(uint32_t))) { + fastboot_fail(response_str, + "Bogus chunk size for chunk type FILL"); + return; + }
- if ((start + total_blocks + blkcnt) > - (storage->start + storage->size)) { - printf("%s: Request would exceed partition size!\n", - __func__); - return -EINVAL; - } + fill_buf = (uint32_t *) + memalign(ARCH_DMA_MINALIGN, + ROUNDUP(info->blksz, + ARCH_DMA_MINALIGN)); + if (!fill_buf) { + fastboot_fail(response_str, + "Malloc failed for: CHUNK_TYPE_FILL"); + return; + }
- for (i = 0; i < buffer->repeat; i++) { - unsigned long buffer_blk_cnt; - int ret; + fill_val = *(uint32_t *)data; + data = (char *)data + sizeof(uint32_t);
- buffer_blk_cnt = buffer->length / storage->block_sz; + for (i = 0; i < (info->blksz / sizeof(fill_val)); i++) + fill_buf[i] = fill_val;
- ret = storage->write(storage, storage_priv, - start + total_blocks, - buffer_blk_cnt, - buffer->data); - if (ret < 0) { - printf("%s: Write %d failed %d\n", - __func__, i, ret); - return ret; + if (blk + blkcnt > info->start + info->size) { + printf( + "%s: Request would exceed partition size!\n", + __func__); + fastboot_fail(response_str, + "Request would exceed partition size!"); + return; }
- total_blocks += ret; - } + for (i = 0; i < blkcnt; i++) { + blks = info->write(info, blk, 1, fill_buf); + /* blks might be > 1 (eg. NAND bad-blocks) */ + if (blks < 1) { + printf("%s: %s, block # " LBAFU "\n", + __func__, "Write failed", blk); + fastboot_fail(response_str, + "flash write failure"); + free(fill_buf); + return; + } + blk += blks; + } + bytes_written += blkcnt * info->blksz; + total_blocks += chunk_data_sz / sparse_header->blk_sz; + free(fill_buf); + break;
- sparse_put_data_buffer(buffer); + case CHUNK_TYPE_DONT_CARE: +#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV + blk += blkcnt; + total_blocks += chunk_header->chunk_sz; +#endif + break; + + case CHUNK_TYPE_CRC32: + if (chunk_header->total_sz != + sparse_header->chunk_hdr_sz) { + fastboot_fail(response_str, + "Bogus chunk size for chunk type Dont Care"); + return; + } + total_blocks += chunk_header->chunk_sz; + data += chunk_data_sz; + break; + + default: + printf("%s: Unknown chunk type: %x\n", __func__, + chunk_header->chunk_type); + fastboot_fail(response_str, "Unknown chunk type"); + return; + } }
debug("Wrote %d blocks, expected to write %d blocks\n", - total_blocks, - sparse_block_size_to_storage(sparse_header->total_blks, - storage, sparse_header)); - printf("........ wrote %d blocks to '%s'\n", total_blocks, - storage->name); + total_blocks, sparse_header->total_blks); + printf("........ wrote %u bytes to '%s'\n", bytes_written, part_name);
- if (total_blocks != - sparse_block_size_to_storage(sparse_header->total_blks, - storage, sparse_header)) { - printf("sparse image write failure\n"); - return -EIO; - } + if (total_blocks != sparse_header->total_blks) + fastboot_fail(response_str, "sparse image write failure"); + else + fastboot_okay(response_str, "");
- return 0; + return; } diff --git a/include/image-sparse.h b/include/image-sparse.h index a2b0694..4e9e784 100644 --- a/include/image-sparse.h +++ b/include/image-sparse.h @@ -9,16 +9,17 @@
#define ROUNDUP(x, y) (((x) + ((y) - 1)) & ~((y) - 1))
-typedef struct sparse_storage { - unsigned int block_sz; - unsigned int start; - unsigned int size; - const char *name; - - int (*write)(struct sparse_storage *storage, void *priv, - unsigned int offset, unsigned int size, - char *data); -} sparse_storage_t; +struct sparse_storage { + lbaint_t blksz; + lbaint_t start; + lbaint_t size; + void *priv; + + lbaint_t (*write)(struct sparse_storage *info, + lbaint_t blk, + lbaint_t blkcnt, + const void *buffer); +};
static inline int is_sparse_image(void *buf) { @@ -31,5 +32,5 @@ static inline int is_sparse_image(void *buf) return 0; }
-int store_sparse_image(sparse_storage_t *storage, void *storage_priv, - void *data); +void write_sparse_image(struct sparse_storage *info, const char *part_name, + void *data, unsigned sz, char *response_str);

On Tue, Jun 07, 2016 at 11:19:36AM -0700, Steve Rae wrote:
This file originally came from upstream code.
While retaining the storage abstraction feature, this is the first set of the changes required to resync with the cmd_flash_mmc_sparse_img() in the file aboot.c from https://us.codeaurora.org/cgit/quic/la/kernel/lk/plain/app/aboot/aboot.c?h=L...
Signed-off-by: Steve Rae srae@broadcom.com
Again, please split that in several patches to have one patch per-change you're doing.
This is just impossible to review.
Maxime

On Wed, Jun 15, 2016 at 1:18 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
On Tue, Jun 07, 2016 at 11:19:36AM -0700, Steve Rae wrote:
This file originally came from upstream code.
While retaining the storage abstraction feature, this is the first set of the changes required to resync with the cmd_flash_mmc_sparse_img() in the file aboot.c from https://us.codeaurora.org/cgit/quic/la/kernel/lk/plain/app/aboot/aboot.c?h=L...
Signed-off-by: Steve Rae srae@broadcom.com
Again, please split that in several patches to have one patch per-change you're doing.
This is just impossible to review.
And I think you just reinforced the point: this code was so far away from the original upstream code that it is not even recognizable anymore.... Furthermore, I have attempted to split this into several commits -- however, I kept breaking bi-sect so I abandoned that idea. Instead, I extensively reviewed: git log --follow -p -- common/image-sparse.c to determine which changes needed to be retained in U-Boot. This series, therefore, provides working code that is resync'ed with the upstream code, and becomes a platform for planning any future changes. Thanks, Steve
Maxime
-- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com

On Thu, Jun 16, 2016 at 10:29:39AM -0700, Steve Rae wrote:
On Wed, Jun 15, 2016 at 1:18 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
On Tue, Jun 07, 2016 at 11:19:36AM -0700, Steve Rae wrote:
This file originally came from upstream code.
While retaining the storage abstraction feature, this is the first set of the changes required to resync with the cmd_flash_mmc_sparse_img() in the file aboot.c from https://us.codeaurora.org/cgit/quic/la/kernel/lk/plain/app/aboot/aboot.c?h=L...
Signed-off-by: Steve Rae srae@broadcom.com
Again, please split that in several patches to have one patch per-change you're doing.
This is just impossible to review.
And I think you just reinforced the point: this code was so far away from the original upstream code that it is not even recognizable anymore....
I think the only point that was made is that a different bootloader has a different implementation of the same protocol, just like for any other protocol.
An implementation relying on a 120 lines switch statement, and a 250 lines functions, that hardcodes the backing storage device.
I'm not sure this is such a good inspiration.
Maxime

On Wed, Jun 22, 2016 at 09:36:23PM +0200, Maxime Ripard wrote:
On Thu, Jun 16, 2016 at 10:29:39AM -0700, Steve Rae wrote:
On Wed, Jun 15, 2016 at 1:18 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
On Tue, Jun 07, 2016 at 11:19:36AM -0700, Steve Rae wrote:
This file originally came from upstream code.
While retaining the storage abstraction feature, this is the first set of the changes required to resync with the cmd_flash_mmc_sparse_img() in the file aboot.c from https://us.codeaurora.org/cgit/quic/la/kernel/lk/plain/app/aboot/aboot.c?h=L...
Signed-off-by: Steve Rae srae@broadcom.com
Again, please split that in several patches to have one patch per-change you're doing.
This is just impossible to review.
And I think you just reinforced the point: this code was so far away from the original upstream code that it is not even recognizable anymore....
I think the only point that was made is that a different bootloader has a different implementation of the same protocol, just like for any other protocol.
An implementation relying on a 120 lines switch statement, and a 250 lines functions, that hardcodes the backing storage device.
I'm not sure this is such a good inspiration.
True. But do we want to have a compatible implementation, or match the canonical implementation? Especially since there's many existing 3rd party tools that will test against the upstream version of things.

On Tue, Jun 07, 2016 at 11:19:36AM -0700, Steve Rae wrote:
This file originally came from upstream code.
While retaining the storage abstraction feature, this is the first set of the changes required to resync with the cmd_flash_mmc_sparse_img() in the file aboot.c from https://us.codeaurora.org/cgit/quic/la/kernel/lk/plain/app/aboot/aboot.c?h=L...
Signed-off-by: Steve Rae srae@broadcom.com
Applied to u-boot/master, thanks!

- update fastboot_okay() and fastboot_fail()
This file originally came from upstream code.
While retaining the storage abstraction feature, this is the second set of the changes required to resync with the cmd_flash_mmc_sparse_img() in the file aboot.c from https://us.codeaurora.org/cgit/quic/la/kernel/lk/plain/app/aboot/aboot.c?h=L...
Signed-off-by: Steve Rae srae@broadcom.com ---
Changes in v2: None
common/fb_mmc.c | 40 ++++++++++++++++------------------------ common/fb_nand.c | 38 +++++++++++++++----------------------- common/image-sparse.c | 26 +++++++++++++------------- drivers/usb/gadget/f_fastboot.c | 31 +++++++++++++++++++------------ include/fastboot.h | 4 ++-- include/fb_mmc.h | 4 ++-- include/fb_nand.h | 4 ++-- include/image-sparse.h | 2 +- 8 files changed, 70 insertions(+), 79 deletions(-)
diff --git a/common/fb_mmc.c b/common/fb_mmc.c index 3dad0ea..6bafbc6 100644 --- a/common/fb_mmc.c +++ b/common/fb_mmc.c @@ -18,8 +18,6 @@ #define CONFIG_FASTBOOT_GPT_NAME GPT_ENTRY_NAME #endif
-static char *response_str; - struct fb_mmc_sparse { struct blk_desc *dev_desc; }; @@ -68,7 +66,7 @@ static void write_raw_image(struct blk_desc *dev_desc, disk_partition_t *info,
if (blkcnt > info->size) { error("too large for partition: '%s'\n", part_name); - fastboot_fail(response_str, "too large for partition"); + fastboot_fail("too large for partition"); return; }
@@ -77,28 +75,25 @@ static void write_raw_image(struct blk_desc *dev_desc, disk_partition_t *info, blks = blk_dwrite(dev_desc, info->start, blkcnt, buffer); if (blks != blkcnt) { error("failed writing to device %d\n", dev_desc->devnum); - fastboot_fail(response_str, "failed writing to device"); + fastboot_fail("failed writing to device"); return; }
printf("........ wrote " LBAFU " bytes to '%s'\n", blkcnt * info->blksz, part_name); - fastboot_okay(response_str, ""); + fastboot_okay(""); }
void fb_mmc_flash_write(const char *cmd, void *download_buffer, - unsigned int download_bytes, char *response) + unsigned int download_bytes) { struct blk_desc *dev_desc; disk_partition_t info;
- /* initialize the response buffer */ - response_str = response; - dev_desc = blk_get_dev("mmc", CONFIG_FASTBOOT_FLASH_MMC_DEV); if (!dev_desc || dev_desc->type == DEV_TYPE_UNKNOWN) { error("invalid mmc device\n"); - fastboot_fail(response_str, "invalid mmc device"); + fastboot_fail("invalid mmc device"); return; }
@@ -108,21 +103,21 @@ void fb_mmc_flash_write(const char *cmd, void *download_buffer, if (is_valid_gpt_buf(dev_desc, download_buffer)) { printf("%s: invalid GPT - refusing to write to flash\n", __func__); - fastboot_fail(response_str, "invalid GPT partition"); + fastboot_fail("invalid GPT partition"); return; } if (write_mbr_and_gpt_partitions(dev_desc, download_buffer)) { printf("%s: writing GPT partitions failed\n", __func__); - fastboot_fail(response_str, + fastboot_fail( "writing GPT partitions failed"); return; } printf("........ success\n"); - fastboot_okay(response_str, ""); + fastboot_okay(""); return; } else if (part_get_info_efi_by_name_or_alias(dev_desc, cmd, &info)) { error("cannot find partition: '%s'\n", cmd); - fastboot_fail(response_str, "cannot find partition"); + fastboot_fail("cannot find partition"); return; }
@@ -142,14 +137,14 @@ void fb_mmc_flash_write(const char *cmd, void *download_buffer,
sparse.priv = &sparse_priv; write_sparse_image(&sparse, cmd, download_buffer, - download_bytes, response_str); + download_bytes); } else { write_raw_image(dev_desc, &info, cmd, download_buffer, download_bytes); } }
-void fb_mmc_erase(const char *cmd, char *response) +void fb_mmc_erase(const char *cmd) { int ret; struct blk_desc *dev_desc; @@ -159,24 +154,21 @@ void fb_mmc_erase(const char *cmd, char *response)
if (mmc == NULL) { error("invalid mmc device"); - fastboot_fail(response_str, "invalid mmc device"); + fastboot_fail("invalid mmc device"); return; }
- /* initialize the response buffer */ - response_str = response; - dev_desc = blk_get_dev("mmc", CONFIG_FASTBOOT_FLASH_MMC_DEV); if (!dev_desc || dev_desc->type == DEV_TYPE_UNKNOWN) { error("invalid mmc device"); - fastboot_fail(response_str, "invalid mmc device"); + fastboot_fail("invalid mmc device"); return; }
ret = part_get_info_efi_by_name_or_alias(dev_desc, cmd, &info); if (ret) { error("cannot find partition: '%s'", cmd); - fastboot_fail(response_str, "cannot find partition"); + fastboot_fail("cannot find partition"); return; }
@@ -195,11 +187,11 @@ void fb_mmc_erase(const char *cmd, char *response) blks = dev_desc->block_erase(dev_desc, blks_start, blks_size); if (blks != blks_size) { error("failed erasing from device %d", dev_desc->devnum); - fastboot_fail(response_str, "failed erasing from device"); + fastboot_fail("failed erasing from device"); return; }
printf("........ erased " LBAFU " bytes from '%s'\n", blks_size * info.blksz, cmd); - fastboot_okay(response_str, ""); + fastboot_okay(""); } diff --git a/common/fb_nand.c b/common/fb_nand.c index bdfad17..77ca55d 100644 --- a/common/fb_nand.c +++ b/common/fb_nand.c @@ -15,8 +15,6 @@ #include <jffs2/jffs2.h> #include <nand.h>
-static char *response_str; - struct fb_nand_sparse { struct mtd_info *mtd; struct part_info *part; @@ -32,7 +30,7 @@ __weak int board_fastboot_write_partition_setup(char *name) return 0; }
-static int fb_nand_lookup(const char *partname, char *response, +static int fb_nand_lookup(const char *partname, struct mtd_info **mtd, struct part_info **part) { @@ -43,21 +41,21 @@ static int fb_nand_lookup(const char *partname, char *response, ret = mtdparts_init(); if (ret) { error("Cannot initialize MTD partitions\n"); - fastboot_fail(response_str, "cannot init mtdparts"); + fastboot_fail("cannot init mtdparts"); return ret; }
ret = find_dev_and_part(partname, &dev, &pnum, part); if (ret) { error("cannot find partition: '%s'", partname); - fastboot_fail(response_str, "cannot find partition"); + fastboot_fail("cannot find partition"); return ret; }
if (dev->id->type != MTD_DEV_TYPE_NAND) { error("partition '%s' is not stored on a NAND device", partname); - fastboot_fail(response_str, "not a NAND device"); + fastboot_fail("not a NAND device"); return -EINVAL; }
@@ -129,19 +127,16 @@ static lbaint_t fb_nand_sparse_write(struct sparse_storage *info, }
void fb_nand_flash_write(const char *cmd, void *download_buffer, - unsigned int download_bytes, char *response) + unsigned int download_bytes) { struct part_info *part; struct mtd_info *mtd = NULL; int ret;
- /* initialize the response buffer */ - response_str = response; - - ret = fb_nand_lookup(cmd, response, &mtd, &part); + ret = fb_nand_lookup(cmd, &mtd, &part); if (ret) { error("invalid NAND device"); - fastboot_fail(response_str, "invalid NAND device"); + fastboot_fail("invalid NAND device"); return; }
@@ -166,7 +161,7 @@ void fb_nand_flash_write(const char *cmd, void *download_buffer,
sparse.priv = &sparse_priv; write_sparse_image(&sparse, cmd, download_buffer, - download_bytes, response_str); + download_bytes); } else { printf("Flashing raw image at offset 0x%llx\n", part->offset); @@ -179,26 +174,23 @@ void fb_nand_flash_write(const char *cmd, void *download_buffer, }
if (ret) { - fastboot_fail(response_str, "error writing the image"); + fastboot_fail("error writing the image"); return; }
- fastboot_okay(response_str, ""); + fastboot_okay(""); }
-void fb_nand_erase(const char *cmd, char *response) +void fb_nand_erase(const char *cmd) { struct part_info *part; struct mtd_info *mtd = NULL; int ret;
- /* initialize the response buffer */ - response_str = response; - - ret = fb_nand_lookup(cmd, response, &mtd, &part); + ret = fb_nand_lookup(cmd, &mtd, &part); if (ret) { error("invalid NAND device"); - fastboot_fail(response_str, "invalid NAND device"); + fastboot_fail("invalid NAND device"); return; }
@@ -209,9 +201,9 @@ void fb_nand_erase(const char *cmd, char *response) ret = _fb_nand_erase(mtd, part); if (ret) { error("failed erasing from device %s", mtd->name); - fastboot_fail(response_str, "failed erasing from device"); + fastboot_fail("failed erasing from device"); return; }
- fastboot_okay(response_str, ""); + fastboot_okay(""); } diff --git a/common/image-sparse.c b/common/image-sparse.c index 924cc63..b36703b 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -47,7 +47,7 @@
void write_sparse_image( struct sparse_storage *info, const char *part_name, - void *data, unsigned sz, char *response_str) + void *data, unsigned sz) { lbaint_t blk; lbaint_t blkcnt; @@ -93,7 +93,7 @@ void write_sparse_image( if (offset) { printf("%s: Sparse image block size issue [%u]\n", __func__, sparse_header->blk_sz); - fastboot_fail(response_str, "sparse image block size issue"); + fastboot_fail("sparse image block size issue"); return; }
@@ -128,7 +128,7 @@ void write_sparse_image( case CHUNK_TYPE_RAW: if (chunk_header->total_sz != (sparse_header->chunk_hdr_sz + chunk_data_sz)) { - fastboot_fail(response_str, + fastboot_fail( "Bogus chunk size for chunk type Raw"); return; } @@ -137,7 +137,7 @@ void write_sparse_image( printf( "%s: Request would exceed partition size!\n", __func__); - fastboot_fail(response_str, + fastboot_fail( "Request would exceed partition size!"); return; } @@ -148,7 +148,7 @@ void write_sparse_image( printf("%s: %s" LBAFU " [" LBAFU "]\n", __func__, "Write failed, block #", blk, blks); - fastboot_fail(response_str, + fastboot_fail( "flash write failure"); return; } @@ -161,7 +161,7 @@ void write_sparse_image( case CHUNK_TYPE_FILL: if (chunk_header->total_sz != (sparse_header->chunk_hdr_sz + sizeof(uint32_t))) { - fastboot_fail(response_str, + fastboot_fail( "Bogus chunk size for chunk type FILL"); return; } @@ -171,7 +171,7 @@ void write_sparse_image( ROUNDUP(info->blksz, ARCH_DMA_MINALIGN)); if (!fill_buf) { - fastboot_fail(response_str, + fastboot_fail( "Malloc failed for: CHUNK_TYPE_FILL"); return; } @@ -186,7 +186,7 @@ void write_sparse_image( printf( "%s: Request would exceed partition size!\n", __func__); - fastboot_fail(response_str, + fastboot_fail( "Request would exceed partition size!"); return; } @@ -197,7 +197,7 @@ void write_sparse_image( if (blks < 1) { printf("%s: %s, block # " LBAFU "\n", __func__, "Write failed", blk); - fastboot_fail(response_str, + fastboot_fail( "flash write failure"); free(fill_buf); return; @@ -219,7 +219,7 @@ void write_sparse_image( case CHUNK_TYPE_CRC32: if (chunk_header->total_sz != sparse_header->chunk_hdr_sz) { - fastboot_fail(response_str, + fastboot_fail( "Bogus chunk size for chunk type Dont Care"); return; } @@ -230,7 +230,7 @@ void write_sparse_image( default: printf("%s: Unknown chunk type: %x\n", __func__, chunk_header->chunk_type); - fastboot_fail(response_str, "Unknown chunk type"); + fastboot_fail("Unknown chunk type"); return; } } @@ -240,9 +240,9 @@ void write_sparse_image( printf("........ wrote %u bytes to '%s'\n", bytes_written, part_name);
if (total_blocks != sparse_header->total_blks) - fastboot_fail(response_str, "sparse image write failure"); + fastboot_fail("sparse image write failure"); else - fastboot_okay(response_str, ""); + fastboot_okay("");
return; } diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index ddf989c..2160b1c 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -151,16 +151,18 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req); static int strcmp_l1(const char *s1, const char *s2);
-void fastboot_fail(char *response, const char *reason) +static char *fb_response_str; + +void fastboot_fail(const char *reason) { - strncpy(response, "FAIL\0", 5); - strncat(response, reason, FASTBOOT_RESPONSE_LEN - 4 - 1); + strncpy(fb_response_str, "FAIL\0", 5); + strncat(fb_response_str, reason, FASTBOOT_RESPONSE_LEN - 4 - 1); }
-void fastboot_okay(char *response, const char *reason) +void fastboot_okay(const char *reason) { - strncpy(response, "OKAY\0", 5); - strncat(response, reason, FASTBOOT_RESPONSE_LEN - 4 - 1); + strncpy(fb_response_str, "OKAY\0", 5); + strncat(fb_response_str, reason, FASTBOOT_RESPONSE_LEN - 4 - 1); }
static void fastboot_complete(struct usb_ep *ep, struct usb_request *req) @@ -588,15 +590,18 @@ static void cb_flash(struct usb_ep *ep, struct usb_request *req) return; }
- strcpy(response, "FAILno flash device defined"); + /* initialize the response buffer */ + fb_response_str = response; + + fastboot_fail("no flash device defined"); #ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV fb_mmc_flash_write(cmd, (void *)CONFIG_FASTBOOT_BUF_ADDR, - download_bytes, response); + download_bytes); #endif #ifdef CONFIG_FASTBOOT_FLASH_NAND_DEV fb_nand_flash_write(cmd, (void *)CONFIG_FASTBOOT_BUF_ADDR, - download_bytes, response); + download_bytes); #endif fastboot_tx_write_str(response); } @@ -637,13 +642,15 @@ static void cb_erase(struct usb_ep *ep, struct usb_request *req) return; }
- strcpy(response, "FAILno flash device defined"); + /* initialize the response buffer */ + fb_response_str = response;
+ fastboot_fail("no flash device defined"); #ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV - fb_mmc_erase(cmd, response); + fb_mmc_erase(cmd); #endif #ifdef CONFIG_FASTBOOT_FLASH_NAND_DEV - fb_nand_erase(cmd, response); + fb_nand_erase(cmd); #endif fastboot_tx_write_str(response); } diff --git a/include/fastboot.h b/include/fastboot.h index db826d2..616631e 100644 --- a/include/fastboot.h +++ b/include/fastboot.h @@ -16,7 +16,7 @@ /* The 64 defined bytes plus \0 */ #define FASTBOOT_RESPONSE_LEN (64 + 1)
-void fastboot_fail(char *response, const char *reason); -void fastboot_okay(char *response, const char *reason); +void fastboot_fail(const char *reason); +void fastboot_okay(const char *reason);
#endif /* _FASTBOOT_H_ */ diff --git a/include/fb_mmc.h b/include/fb_mmc.h index 402ba9b..12b99cb 100644 --- a/include/fb_mmc.h +++ b/include/fb_mmc.h @@ -5,5 +5,5 @@ */
void fb_mmc_flash_write(const char *cmd, void *download_buffer, - unsigned int download_bytes, char *response); -void fb_mmc_erase(const char *cmd, char *response); + unsigned int download_bytes); +void fb_mmc_erase(const char *cmd); diff --git a/include/fb_nand.h b/include/fb_nand.h index 88bdf36..aaf7cf7 100644 --- a/include/fb_nand.h +++ b/include/fb_nand.h @@ -6,5 +6,5 @@ */
void fb_nand_flash_write(const char *cmd, void *download_buffer, - unsigned int download_bytes, char *response); -void fb_nand_erase(const char *cmd, char *response); + unsigned int download_bytes); +void fb_nand_erase(const char *cmd); diff --git a/include/image-sparse.h b/include/image-sparse.h index 4e9e784..f6869d6 100644 --- a/include/image-sparse.h +++ b/include/image-sparse.h @@ -33,4 +33,4 @@ static inline int is_sparse_image(void *buf) }
void write_sparse_image(struct sparse_storage *info, const char *part_name, - void *data, unsigned sz, char *response_str); + void *data, unsigned sz);

On Tue, Jun 07, 2016 at 11:19:37AM -0700, Steve Rae wrote:
- update fastboot_okay() and fastboot_fail()
This file originally came from upstream code.
While retaining the storage abstraction feature, this is the second set of the changes required to resync with the cmd_flash_mmc_sparse_img() in the file aboot.c from https://us.codeaurora.org/cgit/quic/la/kernel/lk/plain/app/aboot/aboot.c?h=L...
Signed-off-by: Steve Rae srae@broadcom.com
Applied to u-boot/master, thanks!

In order to process the CHUNK_TYPE_DONT_CARE properly, there is a requirement to be able to 'reserve' a specified number of blocks in the storage media. Because of the special handling of "bad blocks" in NAND devices, this is implemented in a storage abstraction function.
Signed-off-by: Steve Rae srae@broadcom.com ---
Changes in v2: None
common/fb_mmc.c | 7 +++++++ common/fb_nand.c | 20 ++++++++++++++++++++ common/image-sparse.c | 5 ++--- include/image-sparse.h | 4 ++++ 4 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/common/fb_mmc.c b/common/fb_mmc.c index 6bafbc6..c739651 100644 --- a/common/fb_mmc.c +++ b/common/fb_mmc.c @@ -53,6 +53,12 @@ static lbaint_t fb_mmc_sparse_write(struct sparse_storage *info, return blk_dwrite(dev_desc, blk, blkcnt, buffer); }
+static lbaint_t fb_mmc_sparse_reserve(struct sparse_storage *info, + lbaint_t blk, lbaint_t blkcnt) +{ + return blkcnt; +} + static void write_raw_image(struct blk_desc *dev_desc, disk_partition_t *info, const char *part_name, void *buffer, unsigned int download_bytes) @@ -131,6 +137,7 @@ void fb_mmc_flash_write(const char *cmd, void *download_buffer, sparse.start = info.start; sparse.size = info.size; sparse.write = fb_mmc_sparse_write; + sparse.reserve = fb_mmc_sparse_reserve;
printf("Flashing sparse image at offset " LBAFU "\n", sparse.start); diff --git a/common/fb_nand.c b/common/fb_nand.c index 77ca55d..c8c79e9 100644 --- a/common/fb_nand.c +++ b/common/fb_nand.c @@ -126,6 +126,25 @@ static lbaint_t fb_nand_sparse_write(struct sparse_storage *info, return written / info->blksz; }
+static lbaint_t fb_nand_sparse_reserve(struct sparse_storage *info, + lbaint_t blk, lbaint_t blkcnt) +{ + int bad_blocks = 0; + +/* + * TODO - implement a function to determine the total number + * of blocks which must be used in order to reserve the specified + * number ("blkcnt") of "good-blocks", starting at "blk"... + * ( possibly something like the "check_skip_len()" function ) + */ + + /* + * the return value must be 'blkcnt' ("good-blocks") plus the + * number of "bad-blocks" encountered within this space... + */ + return blkcnt + bad_blocks; +} + void fb_nand_flash_write(const char *cmd, void *download_buffer, unsigned int download_bytes) { @@ -155,6 +174,7 @@ void fb_nand_flash_write(const char *cmd, void *download_buffer, sparse.start = part->offset / sparse.blksz; sparse.size = part->size / sparse.blksz; sparse.write = fb_nand_sparse_write; + sparse.reserve = fb_nand_sparse_reserve;
printf("Flashing sparse image at offset " LBAFU "\n", sparse.start); diff --git a/common/image-sparse.c b/common/image-sparse.c index b36703b..9632c6f 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -1,3 +1,4 @@ + /* * Copyright (c) 2009, Google Inc. * All rights reserved. @@ -210,10 +211,8 @@ void write_sparse_image( break;
case CHUNK_TYPE_DONT_CARE: -#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV - blk += blkcnt; + blk += info->reserve(info, blk, blkcnt); total_blocks += chunk_header->chunk_sz; -#endif break;
case CHUNK_TYPE_CRC32: diff --git a/include/image-sparse.h b/include/image-sparse.h index f6869d6..b0cc500 100644 --- a/include/image-sparse.h +++ b/include/image-sparse.h @@ -19,6 +19,10 @@ struct sparse_storage { lbaint_t blk, lbaint_t blkcnt, const void *buffer); + + lbaint_t (*reserve)(struct sparse_storage *info, + lbaint_t blk, + lbaint_t blkcnt); };
static inline int is_sparse_image(void *buf)

On Tue, Jun 07, 2016 at 11:19:38AM -0700, Steve Rae wrote:
In order to process the CHUNK_TYPE_DONT_CARE properly, there is a requirement to be able to 'reserve' a specified number of blocks in the storage media. Because of the special handling of "bad blocks" in NAND devices, this is implemented in a storage abstraction function.
Signed-off-by: Steve Rae srae@broadcom.com
That looks fine,
Reviewed-by: Maxime Ripard maxime.ripard@free-electrons.com
Thanks, Maxime

On Tue, Jun 07, 2016 at 11:19:38AM -0700, Steve Rae wrote:
In order to process the CHUNK_TYPE_DONT_CARE properly, there is a requirement to be able to 'reserve' a specified number of blocks in the storage media. Because of the special handling of "bad blocks" in NAND devices, this is implemented in a storage abstraction function.
Signed-off-by: Steve Rae srae@broadcom.com Reviewed-by: Maxime Ripard maxime.ripard@free-electrons.com
Applied to u-boot/master, thanks!

- increase the size of the fill buffer - testing has shown a 10x improvement when the sparse image has large CHUNK_TYPE_FILL chunks
Signed-off-by: Steve Rae srae@broadcom.com ---
Changes in v2: None
common/image-sparse.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index 9632c6f..ddf5772 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -1,4 +1,3 @@ - /* * Copyright (c) 2009, Google Inc. * All rights reserved. @@ -46,6 +45,10 @@
#include <linux/math64.h>
+#ifndef CONFIG_FASTBOOT_FLASH_FILLBUF_SIZE +#define CONFIG_FASTBOOT_FLASH_FILLBUF_SIZE (1024 * 512) +#endif + void write_sparse_image( struct sparse_storage *info, const char *part_name, void *data, unsigned sz) @@ -62,7 +65,11 @@ void write_sparse_image( sparse_header_t *sparse_header; chunk_header_t *chunk_header; uint32_t total_blocks = 0; + int fill_buf_num_blks; int i; + int j; + + fill_buf_num_blks = CONFIG_FASTBOOT_FLASH_FILLBUF_SIZE / info->blksz;
/* Read and skip over sparse image header */ sparse_header = (sparse_header_t *)data; @@ -169,8 +176,9 @@ void write_sparse_image(
fill_buf = (uint32_t *) memalign(ARCH_DMA_MINALIGN, - ROUNDUP(info->blksz, - ARCH_DMA_MINALIGN)); + ROUNDUP( + info->blksz * fill_buf_num_blks, + ARCH_DMA_MINALIGN)); if (!fill_buf) { fastboot_fail( "Malloc failed for: CHUNK_TYPE_FILL"); @@ -180,7 +188,10 @@ void write_sparse_image( fill_val = *(uint32_t *)data; data = (char *)data + sizeof(uint32_t);
- for (i = 0; i < (info->blksz / sizeof(fill_val)); i++) + for (i = 0; + i < (info->blksz * fill_buf_num_blks / + sizeof(fill_val)); + i++) fill_buf[i] = fill_val;
if (blk + blkcnt > info->start + info->size) { @@ -192,18 +203,24 @@ void write_sparse_image( return; }
- for (i = 0; i < blkcnt; i++) { - blks = info->write(info, blk, 1, fill_buf); - /* blks might be > 1 (eg. NAND bad-blocks) */ - if (blks < 1) { - printf("%s: %s, block # " LBAFU "\n", - __func__, "Write failed", blk); + for (i = 0; i < blkcnt;) { + j = blkcnt - i; + if (j > fill_buf_num_blks) + j = fill_buf_num_blks; + blks = info->write(info, blk, j, fill_buf); + /* blks might be > j (eg. NAND bad-blocks) */ + if (blks < j) { + printf("%s: %s " LBAFU " [%d]\n", + __func__, + "Write failed, block #", + blk, j); fastboot_fail( "flash write failure"); free(fill_buf); return; } blk += blks; + i += j; } bytes_written += blkcnt * info->blksz; total_blocks += chunk_data_sz / sparse_header->blk_sz;

On Tue, Jun 07, 2016 at 11:19:39AM -0700, Steve Rae wrote:
- increase the size of the fill buffer
- testing has shown a 10x improvement when the sparse image has large CHUNK_TYPE_FILL chunks
Signed-off-by: Steve Rae srae@broadcom.com
Changes in v2: None
common/image-sparse.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index 9632c6f..ddf5772 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -1,4 +1,3 @@
/*
- Copyright (c) 2009, Google Inc.
- All rights reserved.
@@ -46,6 +45,10 @@
#include <linux/math64.h>
+#ifndef CONFIG_FASTBOOT_FLASH_FILLBUF_SIZE +#define CONFIG_FASTBOOT_FLASH_FILLBUF_SIZE (1024 * 512)
I wonder whether that would be better to just put the number of blocks there.
NAND blocks are much larger than MMC's, so the gain benefit might not be even.
Maxime

On Wed, Jun 15, 2016 at 1:36 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
On Tue, Jun 07, 2016 at 11:19:39AM -0700, Steve Rae wrote:
- increase the size of the fill buffer
- testing has shown a 10x improvement when the sparse image has large CHUNK_TYPE_FILL chunks
Signed-off-by: Steve Rae srae@broadcom.com
Changes in v2: None
common/image-sparse.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index 9632c6f..ddf5772 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -1,4 +1,3 @@
/*
- Copyright (c) 2009, Google Inc.
- All rights reserved.
@@ -46,6 +45,10 @@
#include <linux/math64.h>
+#ifndef CONFIG_FASTBOOT_FLASH_FILLBUF_SIZE +#define CONFIG_FASTBOOT_FLASH_FILLBUF_SIZE (1024 * 512)
I wonder whether that would be better to just put the number of blocks there.
I wanted to imply that this is not just a random value (even though the code does values which are not a multiple of the info->blksz) Thanks, Steve
NAND blocks are much larger than MMC's, so the gain benefit might not be even.
Maxime
-- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com

On Thu, Jun 16, 2016 at 10:34 AM, Steve Rae srae@broadcom.com wrote:
On Wed, Jun 15, 2016 at 1:36 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
On Tue, Jun 07, 2016 at 11:19:39AM -0700, Steve Rae wrote:
- increase the size of the fill buffer
- testing has shown a 10x improvement when the sparse image has large CHUNK_TYPE_FILL chunks
Signed-off-by: Steve Rae srae@broadcom.com
Changes in v2: None
common/image-sparse.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index 9632c6f..ddf5772 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -1,4 +1,3 @@
/*
- Copyright (c) 2009, Google Inc.
- All rights reserved.
@@ -46,6 +45,10 @@
#include <linux/math64.h>
+#ifndef CONFIG_FASTBOOT_FLASH_FILLBUF_SIZE +#define CONFIG_FASTBOOT_FLASH_FILLBUF_SIZE (1024 * 512)
I wonder whether that would be better to just put the number of blocks there.
I wanted to imply that this is not just a random value (even though the code does
handle values which are not a multiple of the info->blksz)
Thanks, Steve
NAND blocks are much larger than MMC's, so the gain benefit might not be even.
Maxime
-- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com

On Thu, Jun 16, 2016 at 10:34:37AM -0700, Steve Rae wrote:
On Wed, Jun 15, 2016 at 1:36 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:
On Tue, Jun 07, 2016 at 11:19:39AM -0700, Steve Rae wrote:
- increase the size of the fill buffer
- testing has shown a 10x improvement when the sparse image has large CHUNK_TYPE_FILL chunks
Signed-off-by: Steve Rae srae@broadcom.com
Changes in v2: None
common/image-sparse.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index 9632c6f..ddf5772 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -1,4 +1,3 @@
/*
- Copyright (c) 2009, Google Inc.
- All rights reserved.
@@ -46,6 +45,10 @@
#include <linux/math64.h>
+#ifndef CONFIG_FASTBOOT_FLASH_FILLBUF_SIZE +#define CONFIG_FASTBOOT_FLASH_FILLBUF_SIZE (1024 * 512)
I wonder whether that would be better to just put the number of blocks there.
I wanted to imply that this is not just a random value (even though the code does values which are not a multiple of the info->blksz)
I know, but what I was saying is that NAND block size are usually a couple of kilobytes, while the benefit here probably comes from the number of blocks, not the actual size they take, which, for the same benefits, will probably be the same number of blocks, but implies a bigger size.
Maxime

On Tue, Jun 07, 2016 at 11:19:39AM -0700, Steve Rae wrote:
- increase the size of the fill buffer
- testing has shown a 10x improvement when the sparse image has large CHUNK_TYPE_FILL chunks
Signed-off-by: Steve Rae srae@broadcom.com
Applied to u-boot/master, thanks!

On Tue, 7 Jun 2016, Steve Rae wrote:
Quick question before diving in -- does anybody work on making fastboot able to flash multiple devices?
There are some braindead designs (e.g. Variscite SOMs) that have both eMMC and NAND on board and only able to boot off of NAND. Android FS layout uses insane number of partitions so it would be logical to put some of those onto NAND while keeping others on eMMC. However at present time the target device for "flash" command is a compile-time option that only allows writing to one device.
Is anybody working on this currently? I don't want to re-invent the wheel. I will come up with initial RFC if nobody work on this...
While retaining the storage abstraction feature implemented in U-Boot, this series updates the fastboot sparse image handling by (1) fixing broken code, (2) resync'ing with the upstream code, and (3) improving performance when writing CHUNK_TYPE_FILL
Changes in v2:
- series rebased onto v2016.07-rc1
Steve Rae (5): fastboot: sparse: remove session-id logic fastboot: sparse: resync common/image-sparse.c (part 1) fastboot: sparse: resync common/image-sparse.c (part 2) fastboot: sparse: implement reserve() fastboot: sparse: improve CHUNK_TYPE_FILL write performance
common/fb_mmc.c | 79 +++---- common/fb_nand.c | 106 +++++---- common/image-sparse.c | 478 +++++++++++++++------------------------- drivers/usb/gadget/f_fastboot.c | 47 ++-- include/fastboot.h | 4 +- include/fb_mmc.h | 7 +- include/fb_nand.h | 7 +- include/image-sparse.h | 29 ++- 8 files changed, 312 insertions(+), 445 deletions(-)
-- 1.8.5
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
--- ****************************************************************** * KSI@home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************

On Tue, Jun 7, 2016 at 11:36 AM, Sergey Kubushyn ksi@koi8.net wrote:
On Tue, 7 Jun 2016, Steve Rae wrote:
Quick question before diving in -- does anybody work on making fastboot able to flash multiple devices?
There are some braindead designs (e.g. Variscite SOMs) that have both eMMC and NAND on board and only able to boot off of NAND. Android FS layout uses insane number of partitions so it would be logical to put some of those onto NAND while keeping others on eMMC. However at present time the target device for "flash" command is a compile-time option that only allows writing to one device.
Is anybody working on this currently? I don't want to re-invent the wheel. I will come up with initial RFC if nobody work on this...
I am not aware of anyone working on this -- I look forward to your RFC! Thanks, Steve
While retaining the storage abstraction feature implemented in U-Boot, this series updates the fastboot sparse image handling by (1) fixing broken code, (2) resync'ing with the upstream code, and (3) improving performance when writing CHUNK_TYPE_FILL
Changes in v2:
- series rebased onto v2016.07-rc1
Steve Rae (5): fastboot: sparse: remove session-id logic fastboot: sparse: resync common/image-sparse.c (part 1) fastboot: sparse: resync common/image-sparse.c (part 2) fastboot: sparse: implement reserve() fastboot: sparse: improve CHUNK_TYPE_FILL write performance
common/fb_mmc.c | 79 +++---- common/fb_nand.c | 106 +++++---- common/image-sparse.c | 478 +++++++++++++++------------------------- drivers/usb/gadget/f_fastboot.c | 47 ++-- include/fastboot.h | 4 +- include/fb_mmc.h | 7 +- include/fb_nand.h | 7 +- include/image-sparse.h | 29 ++- 8 files changed, 312 insertions(+), 445 deletions(-)
-- 1.8.5
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
- KSI@home KOI8 Net < > The impossible we do immediately. *
- Las Vegas NV, USA < > Miracles require 24-hour notice. *

Tom,
On Tue, Jun 7, 2016 at 11:19 AM, Steve Rae srae@broadcom.com wrote:
While retaining the storage abstraction feature implemented in U-Boot, this series updates the fastboot sparse image handling by (1) fixing broken code, (2) resync'ing with the upstream code, and (3) improving performance when writing CHUNK_TYPE_FILL
Changes in v2:
- series rebased onto v2016.07-rc1
Steve Rae (5): fastboot: sparse: remove session-id logic fastboot: sparse: resync common/image-sparse.c (part 1) fastboot: sparse: resync common/image-sparse.c (part 2) fastboot: sparse: implement reserve() fastboot: sparse: improve CHUNK_TYPE_FILL write performance
common/fb_mmc.c | 79 +++---- common/fb_nand.c | 106 +++++---- common/image-sparse.c | 478 +++++++++++++++------------------------- drivers/usb/gadget/f_fastboot.c | 47 ++-- include/fastboot.h | 4 +- include/fb_mmc.h | 7 +- include/fb_nand.h | 7 +- include/image-sparse.h | 29 ++- 8 files changed, 312 insertions(+), 445 deletions(-)
-- 1.8.5
This series need to be rebased in order to apply onto v2016.07-rc1 .... Thanks, Steve
participants (4)
-
Maxime Ripard
-
Sergey Kubushyn
-
Steve Rae
-
Tom Rini