Message ID | e80dacb85f58bea2597c4dcc9fbc57af0ba489c6.1435791392.git.marcel.ziswiler@toradex.com |
---|---|
State | Superseded |
Headers | show |
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
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
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
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
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
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
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 --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); }