diff mbox

[U-Boot,7/8] mtd/nand/ubi: assortment of alignment fixes

Message ID e80dacb85f58bea2597c4dcc9fbc57af0ba489c6.1435791392.git.marcel.ziswiler@toradex.com
State Superseded
Headers show

Commit Message

Marcel Ziswiler July 1, 2015, 11:04 p.m. UTC 
From: Marcel Ziswiler <marcel.ziswiler@toradex.com>

Various U-Boot adoptions/extensions to MTD/NAND/UBI did not take buffer
alignment into account which led to failures of the following form:

ERROR: v7_dcache_inval_range - start address is not aligned - 0x1f7f0108
ERROR: v7_dcache_inval_range - stop address is not aligned - 0x1f7f1108

Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
---
 common/cmd_ubi.c             | 2 +-
 drivers/mtd/nand/nand_util.c | 2 +-
 fs/ubifs/super.c             | 4 ++--
 fs/ubifs/ubifs.c             | 4 ++--
 lib/gzip.c                   | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

Comments

Marek Vasut July 2, 2015, 5:53 a.m. UTC | #1 
On Thursday, July 02, 2015 at 01:04:52 AM, Marcel Ziswiler wrote:
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> Various U-Boot adoptions/extensions to MTD/NAND/UBI did not take buffer
> alignment into account which led to failures of the following form:
> 
> ERROR: v7_dcache_inval_range - start address is not aligned - 0x1f7f0108
> ERROR: v7_dcache_inval_range - stop address is not aligned - 0x1f7f1108
> 
> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

What about using ALLOC_CACHE_ALIGN_BUFFER() and friends instead ? See
include/common.h for their definition, this is what those functions are
exactly for.

Best regards,
Marek Vasut
Scott Wood July 2, 2015, 9:35 p.m. UTC | #2 
On Thu, 2015-07-02 at 07:53 +0200, Marek Vasut wrote:
> On Thursday, July 02, 2015 at 01:04:52 AM, Marcel Ziswiler wrote:
> > From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > 
> > Various U-Boot adoptions/extensions to MTD/NAND/UBI did not take buffer
> > alignment into account which led to failures of the following form:
> > 
> > ERROR: v7_dcache_inval_range - start address is not aligned - 0x1f7f0108
> > ERROR: v7_dcache_inval_range - stop address is not aligned - 0x1f7f1108
> > 
> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> What about using ALLOC_CACHE_ALIGN_BUFFER() and friends instead ? See
> include/common.h for their definition, this is what those functions are
> exactly for.

ALLOC_CACHE_ALIGN_BUFFER() is for statically allocating an aligned buffer.  
Dynamically allocating an aligned buffer is exactly what memalign() is for.

-Scott
Marek Vasut July 3, 2015, 1:44 p.m. UTC | #3 
On Thursday, July 02, 2015 at 11:35:19 PM, Scott Wood wrote:
> On Thu, 2015-07-02 at 07:53 +0200, Marek Vasut wrote:
> > On Thursday, July 02, 2015 at 01:04:52 AM, Marcel Ziswiler wrote:
> > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > 
> > > Various U-Boot adoptions/extensions to MTD/NAND/UBI did not take buffer
> > > alignment into account which led to failures of the following form:
> > > 
> > > ERROR: v7_dcache_inval_range - start address is not aligned -
> > > 0x1f7f0108 ERROR: v7_dcache_inval_range - stop address is not aligned
> > > - 0x1f7f1108
> > > 
> > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > 
> > What about using ALLOC_CACHE_ALIGN_BUFFER() and friends instead ? See
> > include/common.h for their definition, this is what those functions are
> > exactly for.
> 
> ALLOC_CACHE_ALIGN_BUFFER() is for statically allocating an aligned buffer.

You're confusing this with DEFINE_ALIGN_BUFFER, no ?

> Dynamically allocating an aligned buffer is exactly what memalign() is for.

Isn't memalign()ed memory aligned only to the start address, while the end
address (and thus the length) is not aligned ? This is what memalign(3) has
to say:

"
The function posix_memalign() allocates size bytes and places the
address of the allocated memory in *memptr.  The address of the
allo‐ cated memory will be a multiple of alignment, which must
be a power of two and a multiple of sizeof(void *).  If size is  0,
then  the value placed in *memptr is either NULL, or a unique pointer
value that can later be successfully passed to free(3).

The obsolete function memalign() allocates size bytes and returns a
pointer to the allocated memory.  The memory address will be a mul‐
tiple of alignment, which must be a power of two.
"

Best regards,
Marek Vasut
Scott Wood July 6, 2015, 9:25 p.m. UTC | #4 
On Fri, 2015-07-03 at 15:44 +0200, Marek Vasut wrote:
> On Thursday, July 02, 2015 at 11:35:19 PM, Scott Wood wrote:
> > On Thu, 2015-07-02 at 07:53 +0200, Marek Vasut wrote:
> > > On Thursday, July 02, 2015 at 01:04:52 AM, Marcel Ziswiler wrote:
> > > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > > 
> > > > Various U-Boot adoptions/extensions to MTD/NAND/UBI did not take 
> > > > buffer
> > > > alignment into account which led to failures of the following form:
> > > > 
> > > > ERROR: v7_dcache_inval_range - start address is not aligned -
> > > > 0x1f7f0108 ERROR: v7_dcache_inval_range - stop address is not aligned
> > > > - 0x1f7f1108
> > > > 
> > > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > 
> > > What about using ALLOC_CACHE_ALIGN_BUFFER() and friends instead ? See
> > > include/common.h for their definition, this is what those functions are
> > > exactly for.
> > 
> > ALLOC_CACHE_ALIGN_BUFFER() is for statically allocating an aligned buffer.
> 
> You're confusing this with DEFINE_ALIGN_BUFFER, no ?

OK, not "statically", but on the stack.  It is not appropriate to turn 
dynamic allocations into stack allocations without considering how large the 
allocation can be.  It'd also be more intrusive a change than necessary, even 
if the sizes were small enough.

> > Dynamically allocating an aligned buffer is exactly what memalign() is 
> > for.
> 
> Isn't memalign()ed memory aligned only to the start address, while the end
> address (and thus the length) is not aligned ?

The end address is aligned if the size passed to memalign is aligned.  Maybe 
add a wrapper that calls memalign() with the size rounded up to 
ARCH_DMA_MINALIGN?

>  This is what memalign(3) has
> to say:
> 
> "
> The function posix_memalign() allocates size bytes and places the
> address of the allocated memory in *memptr.  The address of the
> allo‐ cated memory will be a multiple of alignment, which must
> be a power of two and a multiple of sizeof(void *).  If size is  0,
> then  the value placed in *memptr is either NULL, or a unique pointer
> value that can later be successfully passed to free(3).
> 
> The obsolete function memalign() allocates size bytes and returns a
> pointer to the allocated memory.  The memory address will be a mul‐
> tiple of alignment, which must be a power of two.
> "

posix_memalign() does not exist in U-Boot, and it's not clear to me why 
memalign() should be considered obsolete.  Is the difference just the ability 
to return -EINVAL?

-Scott
Marek Vasut July 6, 2015, 9:45 p.m. UTC | #5 
On Monday, July 06, 2015 at 11:25:35 PM, Scott Wood wrote:
> On Fri, 2015-07-03 at 15:44 +0200, Marek Vasut wrote:
> > On Thursday, July 02, 2015 at 11:35:19 PM, Scott Wood wrote:
> > > On Thu, 2015-07-02 at 07:53 +0200, Marek Vasut wrote:
> > > > On Thursday, July 02, 2015 at 01:04:52 AM, Marcel Ziswiler wrote:
> > > > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > > > 
> > > > > Various U-Boot adoptions/extensions to MTD/NAND/UBI did not take
> > > > > buffer
> > > > > alignment into account which led to failures of the following form:
> > > > > 
> > > > > ERROR: v7_dcache_inval_range - start address is not aligned -
> > > > > 0x1f7f0108 ERROR: v7_dcache_inval_range - stop address is not
> > > > > aligned - 0x1f7f1108
> > > > > 
> > > > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > > 
> > > > What about using ALLOC_CACHE_ALIGN_BUFFER() and friends instead ? See
> > > > include/common.h for their definition, this is what those functions
> > > > are exactly for.
> > > 
> > > ALLOC_CACHE_ALIGN_BUFFER() is for statically allocating an aligned
> > > buffer.
> > 
> > You're confusing this with DEFINE_ALIGN_BUFFER, no ?
> 
> OK, not "statically", but on the stack.  It is not appropriate to turn
> dynamic allocations into stack allocations without considering how large
> the allocation can be.  It'd also be more intrusive a change than
> necessary, even if the sizes were small enough.

Fine.

> > > Dynamically allocating an aligned buffer is exactly what memalign() is
> > > for.
> > 
> > Isn't memalign()ed memory aligned only to the start address, while the
> > end address (and thus the length) is not aligned ?
> 
> The end address is aligned if the size passed to memalign is aligned. 
> Maybe add a wrapper that calls memalign() with the size rounded up to
> ARCH_DMA_MINALIGN?

I agree.

> >  This is what memalign(3) has
> > 
> > to say:
> > 
> > "
> > The function posix_memalign() allocates size bytes and places the
> > address of the allocated memory in *memptr.  The address of the
> > allo‐ cated memory will be a multiple of alignment, which must
> > be a power of two and a multiple of sizeof(void *).  If size is  0,
> > then  the value placed in *memptr is either NULL, or a unique pointer
> > value that can later be successfully passed to free(3).
> > 
> > The obsolete function memalign() allocates size bytes and returns a
> > pointer to the allocated memory.  The memory address will be a mul‐
> > tiple of alignment, which must be a power of two.
> > "
> 
> posix_memalign() does not exist in U-Boot, and it's not clear to me why
> memalign() should be considered obsolete.  Is the difference just the
> ability to return -EINVAL?

The args are also totally different.

Best regards,
Marek Vasut
Scott Wood July 6, 2015, 9:50 p.m. UTC | #6 
On Mon, 2015-07-06 at 23:45 +0200, Marek Vasut wrote:
> On Monday, July 06, 2015 at 11:25:35 PM, Scott Wood wrote:
> > On Fri, 2015-07-03 at 15:44 +0200, Marek Vasut wrote:
> > > 
> > > This is what memalign(3) has
> > > 
> > > to say:
> > > 
> > > "
> > > The function posix_memalign() allocates size bytes and places the
> > > address of the allocated memory in *memptr.  The address of the
> > > allo‐ cated memory will be a multiple of alignment, which must
> > > be a power of two and a multiple of sizeof(void *).  If size is  0,
> > > then  the value placed in *memptr is either NULL, or a unique pointer
> > > value that can later be successfully passed to free(3).
> > > 
> > > The obsolete function memalign() allocates size bytes and returns a
> > > pointer to the allocated memory.  The memory address will be a mul‐
> > > tiple of alignment, which must be a power of two.
> > > "
> > 
> > posix_memalign() does not exist in U-Boot, and it's not clear to me why
> > memalign() should be considered obsolete.  Is the difference just the
> > ability to return -EINVAL?
> 
> The args are also totally different.

...in order to accommodate returning an error value.

-Scott
Marek Vasut July 6, 2015, 9:57 p.m. UTC | #7 
On Monday, July 06, 2015 at 11:50:39 PM, Scott Wood wrote:
> On Mon, 2015-07-06 at 23:45 +0200, Marek Vasut wrote:
> > On Monday, July 06, 2015 at 11:25:35 PM, Scott Wood wrote:
> > > On Fri, 2015-07-03 at 15:44 +0200, Marek Vasut wrote:
> > > > This is what memalign(3) has
> > > > 
> > > > to say:
> > > > 
> > > > "
> > > > The function posix_memalign() allocates size bytes and places the
> > > > address of the allocated memory in *memptr.  The address of the
> > > > allo‐ cated memory will be a multiple of alignment, which must
> > > > be a power of two and a multiple of sizeof(void *).  If size is  0,
> > > > then  the value placed in *memptr is either NULL, or a unique pointer
> > > > value that can later be successfully passed to free(3).
> > > > 
> > > > The obsolete function memalign() allocates size bytes and returns a
> > > > pointer to the allocated memory.  The memory address will be a mul‐
> > > > tiple of alignment, which must be a power of two.
> > > > "
> > > 
> > > posix_memalign() does not exist in U-Boot, and it's not clear to me why
> > > memalign() should be considered obsolete.  Is the difference just the
> > > ability to return -EINVAL?
> > 
> > The args are also totally different.
> 
> ...in order to accommodate returning an error value.

... of course.

Best regards,
Marek Vasut
diff mbox

Patch

diff --git a/common/cmd_ubi.c b/common/cmd_ubi.c
index cbc10c5..30a1259 100644
--- a/common/cmd_ubi.c
+++ b/common/cmd_ubi.c
@@ -363,7 +363,7 @@  int ubi_volume_read(char *volume, char *buf, size_t size)
 	tbuf_size = vol->usable_leb_size;
 	if (size < tbuf_size)
 		tbuf_size = ALIGN(size, ubi->min_io_size);
-	tbuf = malloc(tbuf_size);
+	tbuf = memalign(ARCH_DMA_MINALIGN, tbuf_size);
 	if (!tbuf) {
 		printf("NO MEM\n");
 		return ENOMEM;
diff --git a/drivers/mtd/nand/nand_util.c b/drivers/mtd/nand/nand_util.c
index ee2c24d..395ba2d 100644
--- a/drivers/mtd/nand/nand_util.c
+++ b/drivers/mtd/nand/nand_util.c
@@ -839,7 +839,7 @@  int nand_torture(nand_info_t *nand, loff_t offset)
 
 	patt_count = ARRAY_SIZE(patterns);
 
-	buf = malloc(nand->erasesize);
+	buf = memalign(ARCH_DMA_MINALIGN, nand->erasesize);
 	if (buf == NULL) {
 		puts("Out of memory for erase block buffer\n");
 		return -ENOMEM;
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 10f8fff..ac0d13f 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -57,7 +57,7 @@  struct inode *iget_locked(struct super_block *sb, unsigned long ino)
 {
 	struct inode *inode;
 
-	inode = (struct inode *)malloc(sizeof(struct ubifs_inode));
+	inode = (struct inode *)memalign(ARCH_DMA_MINALIGN, sizeof(struct ubifs_inode));
 	if (inode) {
 		inode->i_ino = ino;
 		inode->i_sb = sb;
@@ -104,7 +104,7 @@  void iput(struct inode *inode)
 	/*
 	 * Allocate and use new inode
 	 */
-	ino = (struct inode *)malloc(sizeof(struct ubifs_inode));
+	ino = (struct inode *)memalign(ARCH_DMA_MINALIGN, sizeof(struct ubifs_inode));
 	memcpy(ino, inode, sizeof(struct ubifs_inode));
 
 	/*
diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 6dd6174..03f9f79 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -108,7 +108,7 @@  static inline struct crypto_comp *crypto_alloc_comp(const char *alg_name,
 	struct crypto_comp *ptr;
 	int i = 0;
 
-	ptr = malloc(sizeof(struct crypto_comp));
+	ptr = memalign(ARCH_DMA_MINALIGN, sizeof(struct crypto_comp));
 	while (i < UBIFS_COMPR_TYPES_CNT) {
 		comp = ubifs_compressors[i];
 		if (!comp) {
@@ -723,7 +723,7 @@  static int do_readpage(struct ubifs_info *c, struct inode *inode,
 				 * destination area to a multiple of
 				 * UBIFS_BLOCK_SIZE.
 				 */
-				buff = malloc(UBIFS_BLOCK_SIZE);
+				buff = memalign(ARCH_DMA_MINALIGN, UBIFS_BLOCK_SIZE);
 				if (!buff) {
 					printf("%s: Error, malloc fails!\n",
 					       __func__);
diff --git a/lib/gzip.c b/lib/gzip.c
index ff37d4f..2a74411 100644
--- a/lib/gzip.c
+++ b/lib/gzip.c
@@ -25,7 +25,7 @@  static void *zalloc(void *x, unsigned items, unsigned size)
 	size *= items;
 	size = (size + ZALLOC_ALIGNMENT - 1) & ~(ZALLOC_ALIGNMENT - 1);
 
-	p = malloc (size);
+	p = memalign(ARCH_DMA_MINALIGN, size);
 
 	return (p);
 }