[U-Boot] [PATCH 01/10] usb: gadget: don't leak configs when unbinding

[U-Boot] [PATCH 01/10] usb: gadget: don't leak configs when unbinding

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

By the time g_dnl_unbind() is run, cdev->config has been set to NULL,
so the free() there does nothing, and the config struct is leaked.
Equally, struct usb_gadget contains a linked list of config structs, so
the code should iterate over them all and free each one, rather than
freeing one particular config struct.

composite_unbind() already iterates over the list of config structs, and
unlinks each from the linked list. Fix this loop to free() each struct as
it's unlinked and otherwise forgotten.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/gadget/composite.c | 1 +
 drivers/usb/gadget/g_dnl.c     | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index d96296cd73b1..a13b21d0a0f2 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -948,6 +948,7 @@ static void composite_unbind(struct usb_gadget *gadget)
 			debug("unbind config '%s'/%p\n", c->label, c);
 			c->unbind(c);
 		}
+		free(c);
 	}
 	if (composite->unbind)
 		composite->unbind(cdev);
diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
index ad89a0d2e670..2fa6da4091e1 100644
--- a/drivers/usb/gadget/g_dnl.c
+++ b/drivers/usb/gadget/g_dnl.c
@@ -93,8 +93,6 @@ static int g_dnl_unbind(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget *gadget = cdev->gadget;
 
-	free(cdev->config);
-	cdev->config = NULL;
 	debug("%s: calling usb_gadget_disconnect for "
 			"controller '%s'\n", __func__, gadget->name);
 	usb_gadget_disconnect(gadget);
-- 
1.9.1

[U-Boot] [PATCH 02/10] ext4: avoid calling ext4fs_mount() twice, which leaks

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

ext4_write_file() is only called from the "fs" layer, which calls both
ext4fs_mount() and ext4fs_close() before/after calling ext4_write_file().
Fix ext4_write_file() not to call ext4fs_mount() again, since the mount
operation malloc()s some RAM which is leaked when a second mount call
over-writes the pointer to that data, if no intervening close call is
made.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 fs/ext4/ext4_write.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index fbc4c4b1cc1a..fa67eb6a56b4 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -986,26 +986,17 @@ int ext4_write_file(const char *filename, void *buf, loff_t offset,
 		return -1;
 	}
 
-	/* mount the filesystem */
-	if (!ext4fs_mount(0)) {
-		printf("** Error Bad ext4 partition **\n");
-		goto fail;
-	}
-
 	ret = ext4fs_write(filename, buf, len);
-
 	if (ret) {
 		printf("** Error ext4fs_write() **\n");
 		goto fail;
 	}
-	ext4fs_close();
 
 	*actwrite = len;
 
 	return 0;
 
 fail:
-	ext4fs_close();
 	*actwrite = 0;
 
 	return -1;
-- 
1.9.1

[U-Boot] [PATCH 03/10] ext4: free allocations by parse_path()

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

parse_path() malloc()s the entries in the array it's passed. Those
allocations must be free()d by the caller, ext4fs_get_parent_inode_num().
Add code to do this.

For this to work, all the array entries must be dynamically allocated,
rather than a mix of dynamic and static allocations. Fix parse_path() not
to over-write arr[0] with a pointer to statically allocated data.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 fs/ext4/ext4_common.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index cab5465b9d4f..b09f23aa5b83 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -614,8 +614,7 @@ static int parse_path(char **arr, char *dirname)
 	arr[i] = zalloc(strlen("/") + 1);
 	if (!arr[i])
 		return -ENOMEM;
-
-	arr[i++] = "/";
+	memcpy(arr[i++], "/", strlen("/"));
 
 	/* add each path entry after root */
 	while (token != NULL) {
@@ -745,6 +744,11 @@ end:
 fail:
 	free(depth_dirname);
 	free(parse_dirname);
+	for (i = 0; i < depth; i++) {
+		if (!ptr[i])
+			break;
+		free(ptr[i]);
+	}
 	free(ptr);
 	free(parent_inode);
 	free(first_inode);
-- 
1.9.1

[U-Boot] [PATCH 04/10] ext4: fix leak in check_filename()

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

root_first_block_buffer should be free()d in all cases, not just when an
error occurs. Fix the success exit path of the function to do this.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 fs/ext4/ext4_common.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index b09f23aa5b83..7d7609a91fa8 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -769,6 +769,7 @@ static int check_filename(char *filename, unsigned int blknr)
 	struct ext2_dirent *previous_dir = NULL;
 	char *ptr = NULL;
 	struct ext_filesystem *fs = get_fs();
+	int ret = -1;
 
 	/* get the first block of root */
 	first_block_no_of_root = blknr;
@@ -822,12 +823,12 @@ static int check_filename(char *filename, unsigned int blknr)
 		if (ext4fs_put_metadata(root_first_block_addr,
 					first_block_no_of_root))
 			goto fail;
-		return inodeno;
+		ret = inodeno;
 	}
 fail:
 	free(root_first_block_buffer);
 
-	return -1;
+	return ret;
 }
 
 int ext4fs_filename_check(char *filename)
-- 
1.9.1

[U-Boot] [PATCH 05/10] dfu: don't keep freeing/reallocating

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

DFU currently allocates buffer memory at the start of each data transfer
operation and frees it at the end. Especially since memalign() is used to
allocate the buffer, and various other allocations happen during the
transfer, this can expose the code to heap fragmentation, which prevents
the allocation from succeeding on subsequent transfers.

Fix the code to allocate the buffer once when DFU mode is initialized,
and free the buffer once when DFU mode is exited, to reduce the exposure
to heap fragmentation.

The failure mode is:

// Internally to memalign(), this allocates a lot more than s to guarantee
// that alignment can occur, then returns chunks of memory at the start/
// end of the allocated buffer to the heap.
p = memalign(a, s);
// Various other malloc()s occur here, some of which allocate the RAM
// immediately before/after "p".
//
// DFU transfer is complete, so buffer is released.
free(p);
// By chance, no other malloc()/free() here, in DFU at least.
//
// A new DFU transfer starts, so the buffer is allocated again.
// In theory this should succeed since we just free()d a buffer of the
// same size. However, this fails because memalign() internally attempts
// to allocate much more than "s", yet free(p) above only free()d a
// little more than "s".
p = memalign(a, s);

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/dfu/dfu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index 675162d927d8..d85d3f507a7b 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -164,7 +164,6 @@ static int dfu_write_buffer_drain(struct dfu_entity *dfu)
 void dfu_write_transaction_cleanup(struct dfu_entity *dfu)
 {
 	/* clear everything */
-	dfu_free_buf();
 	dfu->crc = 0;
 	dfu->offset = 0;
 	dfu->i_blk_seq_num = 0;
@@ -385,7 +384,6 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 			      dfu_hash_algo->name, dfu->crc);
 		puts("\nUPLOAD ... done\nCtrl+C to exit ...\n");
 
-		dfu_free_buf();
 		dfu->i_blk_seq_num = 0;
 		dfu->crc = 0;
 		dfu->offset = 0;
@@ -433,6 +431,7 @@ static int dfu_fill_entity(struct dfu_entity *dfu, char *s, int alt,
 		       __func__,  interface);
 		return -1;
 	}
+	dfu_get_buf(dfu);
 
 	return 0;
 }
@@ -441,6 +440,7 @@ void dfu_free_entities(void)
 {
 	struct dfu_entity *dfu, *p, *t = NULL;
 
+	dfu_free_buf();
 	list_for_each_entry_safe_reverse(dfu, p, &dfu_list, list) {
 		list_del(&dfu->list);
 		if (dfu->free_entity)
-- 
1.9.1

[U-Boot] [PATCH 06/10] dfu: mmc: buffer file reads too

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

When writing to files in a filesystem on MMC, dfu_mmc.c buffers up the
entire file content until the end of the transaction, at which point the
file is written in one go. This allows writing files larger than the USB
transfer size (CONFIG_SYS_DFU_DATA_BUF_SIZE); the maximum written file
size is CONFIG_SYS_DFU_MAX_FILE_SIZE (the size of the temporary buffer).

The current file reading code does not do any buffering, and so limits
the maximum read file size to the USB transfer size. Enhance the code to
do the same kind of buffering as the write path, so the same file size
limits apply.

Remove the size checking code from dfu_read() since all read paths now
support larger files than the USB transfer buffer.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/dfu/dfu.c     | 11 -----------
 drivers/dfu/dfu_mmc.c | 27 ++++++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index d85d3f507a7b..d1e465aa7695 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -338,17 +338,6 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 		dfu->r_left = dfu->get_medium_size(dfu);
 		if (dfu->r_left < 0)
 			return dfu->r_left;
-		switch (dfu->layout) {
-		case DFU_RAW_ADDR:
-		case DFU_RAM_ADDR:
-			break;
-		default:
-			if (dfu->r_left > dfu_buf_size) {
-				printf("%s: File too big for buffer\n",
-				       __func__);
-				return -EOVERFLOW;
-			}
-		}
 
 		debug("%s: %s %ld [B]\n", __func__, dfu->name, dfu->r_left);
 
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index 2a780f7b5d31..5a9fb4a6e247 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -18,6 +18,7 @@
 
 static unsigned char *dfu_file_buf;
 static long dfu_file_buf_len;
+static long dfu_file_buf_filled;
 
 static int mmc_access_part(struct dfu_entity *dfu, struct mmc *mmc, int part)
 {
@@ -230,9 +231,12 @@ long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
 		return dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size;
 	case DFU_FS_FAT:
 	case DFU_FS_EXT4:
+		dfu_file_buf_filled = -1;
 		ret = mmc_file_op(DFU_OP_SIZE, dfu, NULL, &len);
 		if (ret < 0)
 			return ret;
+		if (len > CONFIG_SYS_DFU_MAX_FILE_SIZE)
+			return -1;
 		return len;
 	default:
 		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
@@ -241,6 +245,27 @@ long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
 	}
 }
 
+static int mmc_file_unbuffer(struct dfu_entity *dfu, u64 offset, void *buf,
+			     long *len)
+{
+	int ret;
+	long file_len;
+
+	if (dfu_file_buf_filled == -1) {
+		ret = mmc_file_op(DFU_OP_READ, dfu, dfu_file_buf, &file_len);
+		if (ret < 0)
+			return ret;
+		dfu_file_buf_filled = file_len;
+	}
+	if (offset + *len > dfu_file_buf_filled)
+		return -EINVAL;
+
+	/* Add to the current buffer. */
+	memcpy(buf, dfu_file_buf + offset, *len);
+
+	return 0;
+}
+
 int dfu_read_medium_mmc(struct dfu_entity *dfu, u64 offset, void *buf,
 		long *len)
 {
@@ -252,7 +277,7 @@ int dfu_read_medium_mmc(struct dfu_entity *dfu, u64 offset, void *buf,
 		break;
 	case DFU_FS_FAT:
 	case DFU_FS_EXT4:
-		ret = mmc_file_op(DFU_OP_READ, dfu, buf, len);
+		ret = mmc_file_unbuffer(dfu, offset, buf, len);
 		break;
 	default:
 		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
-- 
1.9.1

[U-Boot] [PATCH 07/10] ARM: tegra: fix malloc region sizing

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Commit 52a7c98a1772 "tegra-common: increase malloc pool len by dfu mmc
file buffer size" updated the definition of CONFIG_SYS_MALLOC_LEN for
Tegra to take account of the DFU buffer size. However, this change had
no effect, since typical Tegra board config headers don't set the DFU-
related defines until after tegra-common.h is included. Fix this by
moving the affected conditional code to tegra-common-post.h, which is
included last. Also move the definition of SYS_NONCACHED_MEMORY since
it's a related and adjacent definition.

Fix the condition to test for the DFU feature, rather than specifically
MMC DFU support, so it applies in all cases.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 include/configs/tegra-common-post.h | 14 ++++++++++++++
 include/configs/tegra-common.h      | 14 --------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h
index e67ff7b95715..594fa456d17f 100644
--- a/include/configs/tegra-common-post.h
+++ b/include/configs/tegra-common-post.h
@@ -8,6 +8,20 @@
 #ifndef __TEGRA_COMMON_POST_H
 #define __TEGRA_COMMON_POST_H
 
+/*
+ * Size of malloc() pool
+ */
+#ifdef CONFIG_USB_FUNCTION_DFU
+#define CONFIG_SYS_MALLOC_LEN		((4 << 20) + \
+						CONFIG_SYS_DFU_DATA_BUF_SIZE)
+#else
+#define CONFIG_SYS_MALLOC_LEN		(4 << 20)	/* 4MB  */
+#endif
+
+#ifndef CONFIG_ARM64
+#define CONFIG_SYS_NONCACHED_MEMORY	(1 << 20)	/* 1 MiB */
+#endif
+
 #ifndef CONFIG_SPL_BUILD
 #define BOOT_TARGET_DEVICES(func) \
 	func(MMC, mmc, 1) \
diff --git a/include/configs/tegra-common.h b/include/configs/tegra-common.h
index 6fe5f2ce6543..b886c6450763 100644
--- a/include/configs/tegra-common.h
+++ b/include/configs/tegra-common.h
@@ -37,20 +37,6 @@
 #define CONFIG_ENV_SIZE			0x2000	/* Total Size Environment */
 
 /*
- * Size of malloc() pool
- */
-#ifdef CONFIG_DFU_MMC
-#define CONFIG_SYS_MALLOC_LEN		((4 << 20) + \
-					CONFIG_SYS_DFU_DATA_BUF_SIZE)
-#else
-#define CONFIG_SYS_MALLOC_LEN		(4 << 20)	/* 4MB  */
-#endif
-
-#ifndef CONFIG_ARM64
-#define CONFIG_SYS_NONCACHED_MEMORY	(1 << 20)       /* 1 MiB */
-#endif
-
-/*
  * NS16550 Configuration
  */
 #define CONFIG_TEGRA_SERIAL
-- 
1.9.1

[U-Boot] [PATCH 08/10] ARM: tegra: tweak DFU buffer sizes

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

CONFIG_SYS_DFU_DATA_BUF_SIZE defines the size of chunks transferred
across USB. This doesn't need to be particularly large, since it doesn't
limit the overall transfer size.

CONFIG_SYS_DFU_MAX_FILE_SIZE is used to buffer an entire file before
writing it to a filesystem. This define limits the maximum file size that
may be transferred. Bump this up to 32MiB in order to support large
uncompressed kernel images.

Both of these buffers are dynamically allocated, and so the size of both
needs to be taken into account when calculating the required malloc
region size.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 include/configs/tegra-common-post.h       | 5 +++--
 include/configs/tegra-common-usb-gadget.h | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h
index 594fa456d17f..651c4c493536 100644
--- a/include/configs/tegra-common-post.h
+++ b/include/configs/tegra-common-post.h
@@ -12,8 +12,9 @@
  * Size of malloc() pool
  */
 #ifdef CONFIG_USB_FUNCTION_DFU
-#define CONFIG_SYS_MALLOC_LEN		((4 << 20) + \
-						CONFIG_SYS_DFU_DATA_BUF_SIZE)
+#define CONFIG_SYS_MALLOC_LEN	(SZ_4M + \
+					CONFIG_SYS_DFU_DATA_BUF_SIZE + \
+					CONFIG_SYS_DFU_MAX_FILE_SIZE)
 #else
 #define CONFIG_SYS_MALLOC_LEN		(4 << 20)	/* 4MB  */
 #endif
diff --git a/include/configs/tegra-common-usb-gadget.h b/include/configs/tegra-common-usb-gadget.h
index e51da3f40571..b1a14cbe73b5 100644
--- a/include/configs/tegra-common-usb-gadget.h
+++ b/include/configs/tegra-common-usb-gadget.h
@@ -30,7 +30,8 @@
 #define CONFIG_CMD_USB_MASS_STORAGE
 /* DFU protocol */
 #define CONFIG_USB_FUNCTION_DFU
-#define CONFIG_SYS_DFU_DATA_BUF_SIZE (1 * 1024 * 1024)
+#define CONFIG_SYS_DFU_DATA_BUF_SIZE SZ_1M
+#define CONFIG_SYS_DFU_MAX_FILE_SIZE SZ_32M
 #define CONFIG_CMD_DFU
 #ifdef CONFIG_MMC
 #define CONFIG_DFU_MMC
-- 
1.9.1

[U-Boot] [PATCH 09/10] ARM: tegra: enable filesystem writing

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Writing to files is a useful feature in general, so enable it everywhere.
The primary purpose is to make DFU useful on filesystems in addition to
raw devices.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 include/configs/tegra-common.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/configs/tegra-common.h b/include/configs/tegra-common.h
index b886c6450763..1c469d092e8c 100644
--- a/include/configs/tegra-common.h
+++ b/include/configs/tegra-common.h
@@ -140,6 +140,8 @@
 
 #ifndef CONFIG_SPL_BUILD
 #include <config_distro_defaults.h>
+#define CONFIG_CMD_EXT4_WRITE
+#define CONFIG_FAT_WRITE
 #endif
 
 #endif /* _TEGRA_COMMON_H_ */
-- 
1.9.1

[U-Boot] [PATCH 10/10] ARM: tegra: enable DFU for RAM

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This allows transferring data directly to/from RAM. For example, one
could create a boot script that starts DFU on a RAM region, then once
DFU exits (which is under the control of the attached USB host, via a
USB bus reset), uses the code/data that was received over DFU.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 include/configs/tegra-common-usb-gadget.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/configs/tegra-common-usb-gadget.h b/include/configs/tegra-common-usb-gadget.h
index b1a14cbe73b5..f6e1d5c4db2d 100644
--- a/include/configs/tegra-common-usb-gadget.h
+++ b/include/configs/tegra-common-usb-gadget.h
@@ -39,6 +39,7 @@
 #ifdef CONFIG_SPI_FLASH
 #define CONFIG_DFU_SF
 #endif
+#define CONFIG_DFU_RAM
 #endif
 
 #endif /* _TEGRA_COMMON_USB_GADGET_H_ */
-- 
1.9.1

[U-Boot] [PATCH 01/10] usb: gadget: don't leak configs when unbinding

[Lukasz Majewski]

Hi Stephen,

> From: Stephen Warren <swarren@nvidia.com>
> 
> By the time g_dnl_unbind() is run, cdev->config has been set to NULL,
> so the free() there does nothing, and the config struct is leaked.
> Equally, struct usb_gadget contains a linked list of config structs,
> so the code should iterate over them all and free each one, rather
> than freeing one particular config struct.
> 
> composite_unbind() already iterates over the list of config structs,
> and unlinks each from the linked list. Fix this loop to free() each
> struct as it's unlinked and otherwise forgotten.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/usb/gadget/composite.c | 1 +
>  drivers/usb/gadget/g_dnl.c     | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/composite.c
> b/drivers/usb/gadget/composite.c index d96296cd73b1..a13b21d0a0f2
> 100644 --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -948,6 +948,7 @@ static void composite_unbind(struct usb_gadget
> *gadget) debug("unbind config '%s'/%p\n", c->label, c);
>  			c->unbind(c);
>  		}
> +		free(c);
>  	}
>  	if (composite->unbind)
>  		composite->unbind(cdev);
> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
> index ad89a0d2e670..2fa6da4091e1 100644
> --- a/drivers/usb/gadget/g_dnl.c
> +++ b/drivers/usb/gadget/g_dnl.c
> @@ -93,8 +93,6 @@ static int g_dnl_unbind(struct usb_composite_dev
> *cdev) {
>  	struct usb_gadget *gadget = cdev->gadget;
>  
> -	free(cdev->config);
> -	cdev->config = NULL;
>  	debug("%s: calling usb_gadget_disconnect for "
>  			"controller '%s'\n", __func__, gadget->name);
>  	usb_gadget_disconnect(gadget);

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

Tested-by: Lukasz Majewski <l.majewski@samsung.com>
Test HW: Odroid XU3 - Exynos5433.

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 05/10] dfu: don't keep freeing/reallocating

[Lukasz Majewski]

Hi Stephen,

> From: Stephen Warren <swarren@nvidia.com>
> 
> DFU currently allocates buffer memory at the start of each data
> transfer operation and frees it at the end. Especially since
> memalign() is used to allocate the buffer, and various other
> allocations happen during the transfer, this can expose the code to
> heap fragmentation, which prevents the allocation from succeeding on
> subsequent transfers.
> 
> Fix the code to allocate the buffer once when DFU mode is initialized,
> and free the buffer once when DFU mode is exited, to reduce the
> exposure to heap fragmentation.
> 
> The failure mode is:
> 
> // Internally to memalign(), this allocates a lot more than s to
> guarantee // that alignment can occur, then returns chunks of memory
> at the start/ // end of the allocated buffer to the heap.
> p = memalign(a, s);
> // Various other malloc()s occur here, some of which allocate the RAM
> // immediately before/after "p".
> //
> // DFU transfer is complete, so buffer is released.
> free(p);
> // By chance, no other malloc()/free() here, in DFU at least.
> //
> // A new DFU transfer starts, so the buffer is allocated again.
> // In theory this should succeed since we just free()d a buffer of the
> // same size. However, this fails because memalign() internally
> attempts // to allocate much more than "s", yet free(p) above only
> free()d a // little more than "s".
> p = memalign(a, s);
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/dfu/dfu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index 675162d927d8..d85d3f507a7b 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -164,7 +164,6 @@ static int dfu_write_buffer_drain(struct
> dfu_entity *dfu) void dfu_write_transaction_cleanup(struct dfu_entity
> *dfu) {
>  	/* clear everything */
> -	dfu_free_buf();
>  	dfu->crc = 0;
>  	dfu->offset = 0;
>  	dfu->i_blk_seq_num = 0;
> @@ -385,7 +384,6 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) dfu_hash_algo->name, dfu->crc);
>  		puts("\nUPLOAD ... done\nCtrl+C to exit ...\n");
>  
> -		dfu_free_buf();
>  		dfu->i_blk_seq_num = 0;
>  		dfu->crc = 0;
>  		dfu->offset = 0;
> @@ -433,6 +431,7 @@ static int dfu_fill_entity(struct dfu_entity
> *dfu, char *s, int alt, __func__,  interface);
>  		return -1;
>  	}
> +	dfu_get_buf(dfu);
>  
>  	return 0;
>  }
> @@ -441,6 +440,7 @@ void dfu_free_entities(void)
>  {
>  	struct dfu_entity *dfu, *p, *t = NULL;
>  
> +	dfu_free_buf();
>  	list_for_each_entry_safe_reverse(dfu, p, &dfu_list, list) {
>  		list_del(&dfu->list);
>  		if (dfu->free_entity)

Acked-by: Lukasz Majewski <l.majewski@samsung.com>
Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Test HW: Odroid XU3 - Exynos5433
[DFU tests]

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 06/10] dfu: mmc: buffer file reads too

[Lukasz Majewski]

Hi Stephen,

> From: Stephen Warren <swarren@nvidia.com>
> 
> When writing to files in a filesystem on MMC, dfu_mmc.c buffers up the
> entire file content until the end of the transaction, at which point
> the file is written in one go. This allows writing files larger than
> the USB transfer size (CONFIG_SYS_DFU_DATA_BUF_SIZE); the maximum
> written file size is CONFIG_SYS_DFU_MAX_FILE_SIZE (the size of the
> temporary buffer).
> 
> The current file reading code does not do any buffering, and so limits
> the maximum read file size to the USB transfer size. Enhance the code
> to do the same kind of buffering as the write path, so the same file
> size limits apply.
> 
> Remove the size checking code from dfu_read() since all read paths now
> support larger files than the USB transfer buffer.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/dfu/dfu.c     | 11 -----------
>  drivers/dfu/dfu_mmc.c | 27 ++++++++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index d85d3f507a7b..d1e465aa7695 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -338,17 +338,6 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
> int size, int blk_seq_num) dfu->r_left = dfu->get_medium_size(dfu);
>  		if (dfu->r_left < 0)
>  			return dfu->r_left;
> -		switch (dfu->layout) {
> -		case DFU_RAW_ADDR:
> -		case DFU_RAM_ADDR:
> -			break;
> -		default:
> -			if (dfu->r_left > dfu_buf_size) {
> -				printf("%s: File too big for
> buffer\n",
> -				       __func__);
> -				return -EOVERFLOW;
> -			}
> -		}
>  
>  		debug("%s: %s %ld [B]\n", __func__, dfu->name,
> dfu->r_left); 
> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
> index 2a780f7b5d31..5a9fb4a6e247 100644
> --- a/drivers/dfu/dfu_mmc.c
> +++ b/drivers/dfu/dfu_mmc.c
> @@ -18,6 +18,7 @@
>  
>  static unsigned char *dfu_file_buf;
>  static long dfu_file_buf_len;
> +static long dfu_file_buf_filled;
>  
>  static int mmc_access_part(struct dfu_entity *dfu, struct mmc *mmc,
> int part) {
> @@ -230,9 +231,12 @@ long dfu_get_medium_size_mmc(struct dfu_entity
> *dfu) return dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size;
>  	case DFU_FS_FAT:
>  	case DFU_FS_EXT4:
> +		dfu_file_buf_filled = -1;
>  		ret = mmc_file_op(DFU_OP_SIZE, dfu, NULL, &len);
>  		if (ret < 0)
>  			return ret;
> +		if (len > CONFIG_SYS_DFU_MAX_FILE_SIZE)
> +			return -1;
>  		return len;
>  	default:
>  		printf("%s: Layout (%s) not (yet) supported!\n",
> __func__, @@ -241,6 +245,27 @@ long dfu_get_medium_size_mmc(struct
> dfu_entity *dfu) }
>  }
>  
> +static int mmc_file_unbuffer(struct dfu_entity *dfu, u64 offset,
> void *buf,
> +			     long *len)
> +{
> +	int ret;
> +	long file_len;
> +
> +	if (dfu_file_buf_filled == -1) {
> +		ret = mmc_file_op(DFU_OP_READ, dfu, dfu_file_buf,
> &file_len);
> +		if (ret < 0)
> +			return ret;
> +		dfu_file_buf_filled = file_len;
> +	}
> +	if (offset + *len > dfu_file_buf_filled)
> +		return -EINVAL;
> +
> +	/* Add to the current buffer. */
> +	memcpy(buf, dfu_file_buf + offset, *len);
> +
> +	return 0;
> +}
> +
>  int dfu_read_medium_mmc(struct dfu_entity *dfu, u64 offset, void
> *buf, long *len)
>  {
> @@ -252,7 +277,7 @@ int dfu_read_medium_mmc(struct dfu_entity *dfu,
> u64 offset, void *buf, break;
>  	case DFU_FS_FAT:
>  	case DFU_FS_EXT4:
> -		ret = mmc_file_op(DFU_OP_READ, dfu, buf, len);
> +		ret = mmc_file_unbuffer(dfu, offset, buf, len);
>  		break;
>  	default:
>  		printf("%s: Layout (%s) not (yet) supported!\n",
> __func__,

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

Tested-by: Lukasz Majewski <l.majewski@samsung.com>
Test HW: Odroid XU3 - Exynos5433
[DFU Tests]

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 04/10] ext4: fix leak in check_filename()

[Lukasz Majewski]

Hi Stephen,

> From: Stephen Warren <swarren@nvidia.com>
> 
> root_first_block_buffer should be free()d in all cases, not just when
> an error occurs. Fix the success exit path of the function to do this.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  fs/ext4/ext4_common.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index b09f23aa5b83..7d7609a91fa8 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -769,6 +769,7 @@ static int check_filename(char *filename,
> unsigned int blknr) struct ext2_dirent *previous_dir = NULL;
>  	char *ptr = NULL;
>  	struct ext_filesystem *fs = get_fs();
> +	int ret = -1;
>  
>  	/* get the first block of root */
>  	first_block_no_of_root = blknr;
> @@ -822,12 +823,12 @@ static int check_filename(char *filename,
> unsigned int blknr) if (ext4fs_put_metadata(root_first_block_addr,
>  					first_block_no_of_root))
>  			goto fail;
> -		return inodeno;
> +		ret = inodeno;
>  	}
>  fail:
>  	free(root_first_block_buffer);
>  
> -	return -1;
> +	return ret;
>  }
>  
>  int ext4fs_filename_check(char *filename)

Acked-by: Lukasz Majewski <l.majewski@samsung.com>
Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Test HW: Odroid XU3 - Exynos5433
[DFU tests]

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 03/10] ext4: free allocations by parse_path()

[Lukasz Majewski]

Hi Stephen,

> From: Stephen Warren <swarren@nvidia.com>
> 
> parse_path() malloc()s the entries in the array it's passed. Those
> allocations must be free()d by the caller,
> ext4fs_get_parent_inode_num(). Add code to do this.
> 
> For this to work, all the array entries must be dynamically allocated,
> rather than a mix of dynamic and static allocations. Fix parse_path()
> not to over-write arr[0] with a pointer to statically allocated data.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  fs/ext4/ext4_common.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index cab5465b9d4f..b09f23aa5b83 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -614,8 +614,7 @@ static int parse_path(char **arr, char *dirname)
>  	arr[i] = zalloc(strlen("/") + 1);
>  	if (!arr[i])
>  		return -ENOMEM;
> -
> -	arr[i++] = "/";
> +	memcpy(arr[i++], "/", strlen("/"));
>  
>  	/* add each path entry after root */
>  	while (token != NULL) {
> @@ -745,6 +744,11 @@ end:
>  fail:
>  	free(depth_dirname);
>  	free(parse_dirname);
> +	for (i = 0; i < depth; i++) {
> +		if (!ptr[i])
> +			break;
> +		free(ptr[i]);
> +	}
>  	free(ptr);
>  	free(parent_inode);
>  	free(first_inode);

Acked-by: Lukasz Majewski <l.majewski@samsung.com>
Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Test HW: Odroid XU3 - Exynos5433
[DFU tests]

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 02/10] ext4: avoid calling ext4fs_mount() twice, which leaks

[Lukasz Majewski]

Hi Stephen,

> From: Stephen Warren <swarren@nvidia.com>
> 
> ext4_write_file() is only called from the "fs" layer, which calls both
> ext4fs_mount() and ext4fs_close() before/after calling
> ext4_write_file(). Fix ext4_write_file() not to call ext4fs_mount()
> again, since the mount operation malloc()s some RAM which is leaked
> when a second mount call over-writes the pointer to that data, if no
> intervening close call is made.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  fs/ext4/ext4_write.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> index fbc4c4b1cc1a..fa67eb6a56b4 100644
> --- a/fs/ext4/ext4_write.c
> +++ b/fs/ext4/ext4_write.c
> @@ -986,26 +986,17 @@ int ext4_write_file(const char *filename, void
> *buf, loff_t offset, return -1;
>  	}
>  
> -	/* mount the filesystem */
> -	if (!ext4fs_mount(0)) {
> -		printf("** Error Bad ext4 partition **\n");
> -		goto fail;
> -	}
> -
>  	ret = ext4fs_write(filename, buf, len);
> -
>  	if (ret) {
>  		printf("** Error ext4fs_write() **\n");
>  		goto fail;
>  	}
> -	ext4fs_close();
>  
>  	*actwrite = len;
>  
>  	return 0;
>  
>  fail:
> -	ext4fs_close();
>  	*actwrite = 0;
>  
>  	return -1;


Acked-by: Lukasz Majewski <l.majewski@samsung.com>
Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Test HW: Odroid XU3 - Exynos5433
[DFU tests]


-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 07/10] ARM: tegra: fix malloc region sizing

[Tom Warren]

Stephen,

> -----Original Message-----
> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> Sent: Friday, September 04, 2015 9:04 PM
> To: Tom Rini
> Cc: u-boot at lists.denx.de; Simon Glass; Tom Warren; Stephen Warren; Lukasz
> Majewski; Przemyslaw Marczak
> Subject: [PATCH 07/10] ARM: tegra: fix malloc region sizing
> 
> From: Stephen Warren <swarren@nvidia.com>
> 
> Commit 52a7c98a1772 "tegra-common: increase malloc pool len by dfu mmc file
> buffer size" updated the definition of CONFIG_SYS_MALLOC_LEN for Tegra to
> take account of the DFU buffer size. However, this change had no effect, since
> typical Tegra board config headers don't set the DFU- related defines until after
> tegra-common.h is included. Fix this by moving the affected conditional code to
> tegra-common-post.h, which is included last. Also move the definition of
> SYS_NONCACHED_MEMORY since it's a related and adjacent definition.
> 
> Fix the condition to test for the DFU feature, rather than specifically MMC DFU
> support, so it applies in all cases.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
Do you want me to take these last four in to u-boot-tegra for the pending PR, or do you expect them to go in another way?

Thanks,

Tom
--
nvpublic
> ---
>  include/configs/tegra-common-post.h | 14 ++++++++++++++
>  include/configs/tegra-common.h      | 14 --------------
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-
> common-post.h
> index e67ff7b95715..594fa456d17f 100644
> --- a/include/configs/tegra-common-post.h
> +++ b/include/configs/tegra-common-post.h
> @@ -8,6 +8,20 @@
>  #ifndef __TEGRA_COMMON_POST_H
>  #define __TEGRA_COMMON_POST_H
> 
> +/*
> + * Size of malloc() pool
> + */
> +#ifdef CONFIG_USB_FUNCTION_DFU
> +#define CONFIG_SYS_MALLOC_LEN		((4 << 20) + \
> +
> 	CONFIG_SYS_DFU_DATA_BUF_SIZE)
> +#else
> +#define CONFIG_SYS_MALLOC_LEN		(4 << 20)	/* 4MB  */
> +#endif
> +
> +#ifndef CONFIG_ARM64
> +#define CONFIG_SYS_NONCACHED_MEMORY	(1 << 20)	/* 1 MiB
> */
> +#endif
> +
>  #ifndef CONFIG_SPL_BUILD
>  #define BOOT_TARGET_DEVICES(func) \
>  	func(MMC, mmc, 1) \
> diff --git a/include/configs/tegra-common.h b/include/configs/tegra-
> common.h index 6fe5f2ce6543..b886c6450763 100644
> --- a/include/configs/tegra-common.h
> +++ b/include/configs/tegra-common.h
> @@ -37,20 +37,6 @@
>  #define CONFIG_ENV_SIZE			0x2000	/* Total Size
> Environment */
> 
>  /*
> - * Size of malloc() pool
> - */
> -#ifdef CONFIG_DFU_MMC
> -#define CONFIG_SYS_MALLOC_LEN		((4 << 20) + \
> -					CONFIG_SYS_DFU_DATA_BUF_SIZE)
> -#else
> -#define CONFIG_SYS_MALLOC_LEN		(4 << 20)	/* 4MB  */
> -#endif
> -
> -#ifndef CONFIG_ARM64
> -#define CONFIG_SYS_NONCACHED_MEMORY	(1 << 20)       /* 1 MiB */
> -#endif
> -
> -/*
>   * NS16550 Configuration
>   */
>  #define CONFIG_TEGRA_SERIAL
> --
> 1.9.1

[U-Boot] [PATCH 07/10] ARM: tegra: fix malloc region sizing

[Stephen Warren]

On 09/08/2015 09:53 AM, Tom Warren wrote:
> Stephen,
> 
> Stephen Warren wrote at Friday, September 04, 2015 9:04 PM:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Commit 52a7c98a1772 "tegra-common: increase malloc pool len by dfu mmc file
>> buffer size" updated the definition of CONFIG_SYS_MALLOC_LEN for Tegra to
>> take account of the DFU buffer size. However, this change had no effect, since
>> typical Tegra board config headers don't set the DFU- related defines until after
>> tegra-common.h is included. Fix this by moving the affected conditional code to
>> tegra-common-post.h, which is included last. Also move the definition of
>> SYS_NONCACHED_MEMORY since it's a related and adjacent definition.
>>
>> Fix the condition to test for the DFU feature, rather than specifically MMC DFU
>> support, so it applies in all cases.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>
> Do you want me to take these last four in to u-boot-tegra for the pending PR, or do you expect them to go in another way?

I believe the 4 "ARM: tegra:" patches can go through the Tegra tree
since they're independent from the other patches in the series. Thanks.

[U-Boot] [PATCH 01/10] usb: gadget: don't leak configs when unbinding

[Stephen Warren]

On 09/08/2015 05:00 AM, Lukasz Majewski wrote:
> Hi Stephen,
> 
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> By the time g_dnl_unbind() is run, cdev->config has been set to NULL,
>> so the free() there does nothing, and the config struct is leaked.
>> Equally, struct usb_gadget contains a linked list of config structs,
>> so the code should iterate over them all and free each one, rather
>> than freeing one particular config struct.
>>
>> composite_unbind() already iterates over the list of config structs,
>> and unlinks each from the linked list. Fix this loop to free() each
>> struct as it's unlinked and otherwise forgotten.
...
> 
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> 
> Tested-by: Lukasz Majewski <l.majewski@samsung.com>
> Test HW: Odroid XU3 - Exynos5433.

Thanks for all the tests/acks.

[U-Boot] [PATCH 07/10] ARM: tegra: fix malloc region sizing

[Stephen Warren]

On 09/08/2015 02:45 PM, Stephen Warren wrote:
> On 09/08/2015 09:53 AM, Tom Warren wrote:
>> Stephen,
>>
>> Stephen Warren wrote at Friday, September 04, 2015 9:04 PM:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> Commit 52a7c98a1772 "tegra-common: increase malloc pool len by dfu mmc file
>>> buffer size" updated the definition of CONFIG_SYS_MALLOC_LEN for Tegra to
>>> take account of the DFU buffer size. However, this change had no effect, since
>>> typical Tegra board config headers don't set the DFU- related defines until after
>>> tegra-common.h is included. Fix this by moving the affected conditional code to
>>> tegra-common-post.h, which is included last. Also move the definition of
>>> SYS_NONCACHED_MEMORY since it's a related and adjacent definition.
>>>
>>> Fix the condition to test for the DFU feature, rather than specifically MMC DFU
>>> support, so it applies in all cases.
>>>
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>
>> Do you want me to take these last four in to u-boot-tegra for the pending PR, or do you expect them to go in another way?
> 
> I believe the 4 "ARM: tegra:" patches can go through the Tegra tree
> since they're independent from the other patches in the series. Thanks.

I note that Lukasz has ack'd all the other patches, so perhaps you can
just take the whole series through the Tegra tree? At least the DFU
patches since he's maintainer there. Perhaps TomR can ack the ext4
patches since they don't seem to have a maintainer.

[U-Boot] [PATCH 07/10] ARM: tegra: fix malloc region sizing

[Stephen Warren]

On 09/08/2015 02:56 PM, Tom Rini wrote:
> On Tue, Sep 08, 2015 at 02:52:06PM -0700, Stephen Warren wrote:
>> On 09/08/2015 02:45 PM, Stephen Warren wrote:
>>> On 09/08/2015 09:53 AM, Tom Warren wrote:
>>>> Stephen,
>>>> 
>>>> Stephen Warren wrote at Friday, September 04, 2015 9:04 PM:
>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>> 
>>>>> Commit 52a7c98a1772 "tegra-common: increase malloc pool len
>>>>> by dfu mmc file buffer size" updated the definition of
>>>>> CONFIG_SYS_MALLOC_LEN for Tegra to take account of the DFU
>>>>> buffer size. However, this change had no effect, since 
>>>>> typical Tegra board config headers don't set the DFU-
>>>>> related defines until after tegra-common.h is included. Fix
>>>>> this by moving the affected conditional code to 
>>>>> tegra-common-post.h, which is included last. Also move the
>>>>> definition of SYS_NONCACHED_MEMORY since it's a related and
>>>>> adjacent definition.
>>>>> 
>>>>> Fix the condition to test for the DFU feature, rather than
>>>>> specifically MMC DFU support, so it applies in all cases.
>>>>> 
>>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>>> 
>>>> Do you want me to take these last four in to u-boot-tegra for
>>>> the pending PR, or do you expect them to go in another way?
>>> 
>>> I believe the 4 "ARM: tegra:" patches can go through the Tegra
>>> tree since they're independent from the other patches in the
>>> series. Thanks.
>> 
>> I note that Lukasz has ack'd all the other patches, so perhaps
>> you can just take the whole series through the Tegra tree? At
>> least the DFU patches since he's maintainer there. Perhaps TomR
>> can ack the ext4 patches since they don't seem to have a
>> maintainer.
> 
> I think I snagged them all in patchwork for myself.  Do the Tegra
> ones depend on anything else?

I don't believe the Tegra patches depend on anything to compile or
run; they're just all conceptually related to DFU support on Tegra.

[U-Boot] [PATCH 07/10] ARM: tegra: fix malloc region sizing

[Tom Rini]

On Tue, Sep 08, 2015 at 02:52:06PM -0700, Stephen Warren wrote:
> On 09/08/2015 02:45 PM, Stephen Warren wrote:
> > On 09/08/2015 09:53 AM, Tom Warren wrote:
> >> Stephen,
> >>
> >> Stephen Warren wrote at Friday, September 04, 2015 9:04 PM:
> >>> From: Stephen Warren <swarren@nvidia.com>
> >>>
> >>> Commit 52a7c98a1772 "tegra-common: increase malloc pool len by dfu mmc file
> >>> buffer size" updated the definition of CONFIG_SYS_MALLOC_LEN for Tegra to
> >>> take account of the DFU buffer size. However, this change had no effect, since
> >>> typical Tegra board config headers don't set the DFU- related defines until after
> >>> tegra-common.h is included. Fix this by moving the affected conditional code to
> >>> tegra-common-post.h, which is included last. Also move the definition of
> >>> SYS_NONCACHED_MEMORY since it's a related and adjacent definition.
> >>>
> >>> Fix the condition to test for the DFU feature, rather than specifically MMC DFU
> >>> support, so it applies in all cases.
> >>>
> >>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> >>
> >> Do you want me to take these last four in to u-boot-tegra for the pending PR, or do you expect them to go in another way?
> > 
> > I believe the 4 "ARM: tegra:" patches can go through the Tegra tree
> > since they're independent from the other patches in the series. Thanks.
> 
> I note that Lukasz has ack'd all the other patches, so perhaps you can
> just take the whole series through the Tegra tree? At least the DFU
> patches since he's maintainer there. Perhaps TomR can ack the ext4
> patches since they don't seem to have a maintainer.

I think I snagged them all in patchwork for myself.  Do the Tegra ones
depend on anything else?

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

[U-Boot] [PATCH 07/10] ARM: tegra: fix malloc region sizing

[Lukasz Majewski]

Hi Stephen,

> On 09/08/2015 02:45 PM, Stephen Warren wrote:
> > On 09/08/2015 09:53 AM, Tom Warren wrote:
> >> Stephen,
> >>
> >> Stephen Warren wrote at Friday, September 04, 2015 9:04 PM:
> >>> From: Stephen Warren <swarren@nvidia.com>
> >>>
> >>> Commit 52a7c98a1772 "tegra-common: increase malloc pool len by
> >>> dfu mmc file buffer size" updated the definition of
> >>> CONFIG_SYS_MALLOC_LEN for Tegra to take account of the DFU buffer
> >>> size. However, this change had no effect, since typical Tegra
> >>> board config headers don't set the DFU- related defines until
> >>> after tegra-common.h is included. Fix this by moving the affected
> >>> conditional code to tegra-common-post.h, which is included last.
> >>> Also move the definition of SYS_NONCACHED_MEMORY since it's a
> >>> related and adjacent definition.
> >>>
> >>> Fix the condition to test for the DFU feature, rather than
> >>> specifically MMC DFU support, so it applies in all cases.
> >>>
> >>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> >>
> >> Do you want me to take these last four in to u-boot-tegra for the
> >> pending PR, or do you expect them to go in another way?
> > 
> > I believe the 4 "ARM: tegra:" patches can go through the Tegra tree
> > since they're independent from the other patches in the series.
> > Thanks.
> 
> I note that Lukasz has ack'd all the other patches, so perhaps you can
> just take the whole series through the Tegra tree? At least the DFU
> patches since he's maintainer there. 

I personally would opt for applying this series to one tree. I've
already ack'ed those patches, so those can go to any other tree (or
even -dfu one when nobody wants to pick them :-)).

> Perhaps TomR can ack the ext4
> patches since they don't seem to have a maintainer.

Lack of FS (FAT, EXT4) maintainer is PITTA. I hope that somebody would
step up for this position.

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 07/10] ARM: tegra: fix malloc region sizing

[Tom Warren]

> -----Original Message-----
> From: Lukasz Majewski [mailto:l.majewski at samsung.com]
> Sent: Tuesday, September 08, 2015 11:51 PM
> To: Stephen Warren
> Cc: Tom Warren; Tom Rini; Stephen Warren; u-boot at lists.denx.de; Simon
> Glass; Przemyslaw Marczak
> Subject: Re: [PATCH 07/10] ARM: tegra: fix malloc region sizing
> 
> Hi Stephen,
> 
> > On 09/08/2015 02:45 PM, Stephen Warren wrote:
> > > On 09/08/2015 09:53 AM, Tom Warren wrote:
> > >> Stephen,
> > >>
> > >> Stephen Warren wrote at Friday, September 04, 2015 9:04 PM:
> > >>> From: Stephen Warren <swarren@nvidia.com>
> > >>>
> > >>> Commit 52a7c98a1772 "tegra-common: increase malloc pool len by dfu
> > >>> mmc file buffer size" updated the definition of
> > >>> CONFIG_SYS_MALLOC_LEN for Tegra to take account of the DFU buffer
> > >>> size. However, this change had no effect, since typical Tegra
> > >>> board config headers don't set the DFU- related defines until
> > >>> after tegra-common.h is included. Fix this by moving the affected
> > >>> conditional code to tegra-common-post.h, which is included last.
> > >>> Also move the definition of SYS_NONCACHED_MEMORY since it's a
> > >>> related and adjacent definition.
> > >>>
> > >>> Fix the condition to test for the DFU feature, rather than
> > >>> specifically MMC DFU support, so it applies in all cases.
> > >>>
> > >>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> > >>
> > >> Do you want me to take these last four in to u-boot-tegra for the
> > >> pending PR, or do you expect them to go in another way?
> > >
> > > I believe the 4 "ARM: tegra:" patches can go through the Tegra tree
> > > since they're independent from the other patches in the series.
> > > Thanks.
> >
> > I note that Lukasz has ack'd all the other patches, so perhaps you can
> > just take the whole series through the Tegra tree? At least the DFU
> > patches since he's maintainer there.
> 
> I personally would opt for applying this series to one tree. I've already ack'ed
> those patches, so those can go to any other tree (or even -dfu one when
> nobody wants to pick them :-)).
I believe TomR has taken over all of these patches in Patchwork, so perhaps he's going to take them in to u-boot/master.

Tom
--
nvpublic
> 
> > Perhaps TomR can ack the ext4
> > patches since they don't seem to have a maintainer.
> 
> Lack of FS (FAT, EXT4) maintainer is PITTA. I hope that somebody would step up
> for this position.
> 
> --
> Best regards,
> 
> Lukasz Majewski
> 
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [U-Boot, 01/10] usb: gadget: don't leak configs when unbinding

[Tom Rini]

On Fri, Sep 04, 2015 at 10:03:42PM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> By the time g_dnl_unbind() is run, cdev->config has been set to NULL,
> so the free() there does nothing, and the config struct is leaked.
> Equally, struct usb_gadget contains a linked list of config structs, so
> the code should iterate over them all and free each one, rather than
> freeing one particular config struct.
> 
> composite_unbind() already iterates over the list of config structs, and
> unlinks each from the linked list. Fix this loop to free() each struct as
> it's unlinked and otherwise forgotten.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 02/10] ext4: avoid calling ext4fs_mount() twice, which leaks

[Tom Rini]

On Fri, Sep 04, 2015 at 10:03:43PM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> ext4_write_file() is only called from the "fs" layer, which calls both
> ext4fs_mount() and ext4fs_close() before/after calling ext4_write_file().
> Fix ext4_write_file() not to call ext4fs_mount() again, since the mount
> operation malloc()s some RAM which is leaked when a second mount call
> over-writes the pointer to that data, if no intervening close call is
> made.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot,03/10] ext4: free allocations by parse_path()

[Tom Rini]

On Fri, Sep 04, 2015 at 10:03:44PM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> parse_path() malloc()s the entries in the array it's passed. Those
> allocations must be free()d by the caller, ext4fs_get_parent_inode_num().
> Add code to do this.
> 
> For this to work, all the array entries must be dynamically allocated,
> rather than a mix of dynamic and static allocations. Fix parse_path() not
> to over-write arr[0] with a pointer to statically allocated data.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot,04/10] ext4: fix leak in check_filename()

[Tom Rini]

On Fri, Sep 04, 2015 at 10:03:45PM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> root_first_block_buffer should be free()d in all cases, not just when an
> error occurs. Fix the success exit path of the function to do this.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot,05/10] dfu: don't keep freeing/reallocating

[Tom Rini]

On Fri, Sep 04, 2015 at 10:03:46PM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> DFU currently allocates buffer memory at the start of each data transfer
> operation and frees it at the end. Especially since memalign() is used to
> allocate the buffer, and various other allocations happen during the
> transfer, this can expose the code to heap fragmentation, which prevents
> the allocation from succeeding on subsequent transfers.
> 
> Fix the code to allocate the buffer once when DFU mode is initialized,
> and free the buffer once when DFU mode is exited, to reduce the exposure
> to heap fragmentation.
> 
> The failure mode is:
> 
> // Internally to memalign(), this allocates a lot more than s to guarantee
> // that alignment can occur, then returns chunks of memory at the start/
> // end of the allocated buffer to the heap.
> p = memalign(a, s);
> // Various other malloc()s occur here, some of which allocate the RAM
> // immediately before/after "p".
> //
> // DFU transfer is complete, so buffer is released.
> free(p);
> // By chance, no other malloc()/free() here, in DFU at least.
> //
> // A new DFU transfer starts, so the buffer is allocated again.
> // In theory this should succeed since we just free()d a buffer of the
> // same size. However, this fails because memalign() internally attempts
> // to allocate much more than "s", yet free(p) above only free()d a
> // little more than "s".
> p = memalign(a, s);
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot,06/10] dfu: mmc: buffer file reads too

[Tom Rini]

On Fri, Sep 04, 2015 at 10:03:47PM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> When writing to files in a filesystem on MMC, dfu_mmc.c buffers up the
> entire file content until the end of the transaction, at which point the
> file is written in one go. This allows writing files larger than the USB
> transfer size (CONFIG_SYS_DFU_DATA_BUF_SIZE); the maximum written file
> size is CONFIG_SYS_DFU_MAX_FILE_SIZE (the size of the temporary buffer).
> 
> The current file reading code does not do any buffering, and so limits
> the maximum read file size to the USB transfer size. Enhance the code to
> do the same kind of buffering as the write path, so the same file size
> limits apply.
> 
> Remove the size checking code from dfu_read() since all read paths now
> support larger files than the USB transfer buffer.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Lukasz Majewski <l.majewski@samsung.com>
> Tested-by: Lukasz Majewski <l.majewski@samsung.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot,07/10] ARM: tegra: fix malloc region sizing

[Tom Rini]

On Fri, Sep 04, 2015 at 10:03:48PM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> Commit 52a7c98a1772 "tegra-common: increase malloc pool len by dfu mmc
> file buffer size" updated the definition of CONFIG_SYS_MALLOC_LEN for
> Tegra to take account of the DFU buffer size. However, this change had
> no effect, since typical Tegra board config headers don't set the DFU-
> related defines until after tegra-common.h is included. Fix this by
> moving the affected conditional code to tegra-common-post.h, which is
> included last. Also move the definition of SYS_NONCACHED_MEMORY since
> it's a related and adjacent definition.
> 
> Fix the condition to test for the DFU feature, rather than specifically
> MMC DFU support, so it applies in all cases.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot,08/10] ARM: tegra: tweak DFU buffer sizes

[Tom Rini]

On Fri, Sep 04, 2015 at 10:03:49PM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> CONFIG_SYS_DFU_DATA_BUF_SIZE defines the size of chunks transferred
> across USB. This doesn't need to be particularly large, since it doesn't
> limit the overall transfer size.
> 
> CONFIG_SYS_DFU_MAX_FILE_SIZE is used to buffer an entire file before
> writing it to a filesystem. This define limits the maximum file size that
> may be transferred. Bump this up to 32MiB in order to support large
> uncompressed kernel images.
> 
> Both of these buffers are dynamically allocated, and so the size of both
> needs to be taken into account when calculating the required malloc
> region size.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot,09/10] ARM: tegra: enable filesystem writing

[Tom Rini]

On Fri, Sep 04, 2015 at 10:03:50PM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> Writing to files is a useful feature in general, so enable it everywhere.
> The primary purpose is to make DFU useful on filesystems in addition to
> raw devices.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot,10/10] ARM: tegra: enable DFU for RAM

[Tom Rini]

On Fri, Sep 04, 2015 at 10:03:51PM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> This allows transferring data directly to/from RAM. For example, one
> could create a boot script that starts DFU on a RAM region, then once
> DFU exits (which is under the control of the attached USB host, via a
> USB bus reset), uses the code/data that was received over DFU.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Applied to u-boot/master, thanks!

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