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

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

[Steve Rae]

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] [PATCH v2 1/5] fastboot: sparse: remove session-id logic

[Steve Rae]

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);
-- 
1.8.5

[U-Boot] [PATCH v2 2/5] fastboot: sparse: resync common/image-sparse.c (part 1)

[Steve Rae]

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=LE.BR.1.2.1

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);
-- 
1.8.5

[U-Boot] [PATCH v2 3/5] fastboot: sparse: resync common/image-sparse.c (part 2)

[Steve Rae]

- 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=LE.BR.1.2.1

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);
-- 
1.8.5

[U-Boot] [PATCH v2 4/5] fastboot: sparse: implement reserve()

[Steve Rae]

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)
-- 
1.8.5

[U-Boot] [PATCH v2 5/5] fastboot: sparse: improve CHUNK_TYPE_FILL write performance

[Steve Rae]

- 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;
-- 
1.8.5

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

[Sergey Kubushyn]

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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************

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

[Steve Rae]

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 at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>
> ---
> ******************************************************************
> *  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
> *  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
> ******************************************************************

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

[Steve Rae]

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

[U-Boot] [PATCH v2 2/5] fastboot: sparse: resync common/image-sparse.c (part 1)

[Maxime Ripard]

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=LE.BR.1.2.1
> 
> 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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160615/12d0ce18/attachment.sig>

[U-Boot] [PATCH v2 4/5] fastboot: sparse: implement reserve()

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160615/2094daf9/attachment.sig>

[U-Boot] [PATCH v2 5/5] fastboot: sparse: improve CHUNK_TYPE_FILL write performance

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160615/660a659d/attachment.sig>

[U-Boot] [PATCH v2 2/5] fastboot: sparse: resync common/image-sparse.c (part 1)

[Steve Rae]

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=LE.BR.1.2.1
> >
> > 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

[U-Boot] [PATCH v2 5/5] fastboot: sparse: improve CHUNK_TYPE_FILL write performance

[Steve Rae]

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

[U-Boot] [PATCH v2 5/5] fastboot: sparse: improve CHUNK_TYPE_FILL write performance

[Steve Rae]

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

[U-Boot] [PATCH v2 2/5] fastboot: sparse: resync common/image-sparse.c (part 1)

[Maxime Ripard]

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=LE.BR.1.2.1
> > >
> > > 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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160622/3ac02f06/attachment.sig>

[U-Boot] [PATCH v2 5/5] fastboot: sparse: improve CHUNK_TYPE_FILL write performance

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160622/2779844c/attachment.sig>

[U-Boot] [PATCH v2 2/5] fastboot: sparse: resync common/image-sparse.c (part 1)

[Tom Rini]

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=LE.BR.1.2.1
> > > >
> > > > 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160622/425c3afc/attachment.sig>

[U-Boot] [U-Boot, v2, 1/5] fastboot: sparse: remove session-id logic

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160627/a2082351/attachment.sig>

[U-Boot] [U-Boot, v2, 2/5] fastboot: sparse: resync common/image-sparse.c (part 1)

[Tom Rini]

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=LE.BR.1.2.1
> 
> Signed-off-by: Steve Rae <srae@broadcom.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160627/055148dc/attachment.sig>

[U-Boot] [U-Boot, v2, 3/5] fastboot: sparse: resync common/image-sparse.c (part 2)

[Tom Rini]

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=LE.BR.1.2.1
> 
> Signed-off-by: Steve Rae <srae@broadcom.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160627/72f42899/attachment.sig>

[U-Boot] [U-Boot,v2,4/5] fastboot: sparse: implement reserve()

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160627/169c5695/attachment.sig>

[U-Boot] [U-Boot, v2, 5/5] fastboot: sparse: improve CHUNK_TYPE_FILL write performance

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160627/a0e55ee4/attachment.sig>