| Message ID | 20200920162901.442748-4-mc5686@mclink.it |
|---|---|
| State | Superseded |
| Delegated to: | Daniel Schwierzeck |
| Headers | show |
| Series | Patches to allow running u-boot on vocore2 board | expand |
On Sun, Sep 20, 2020 at 06:29:01PM +0200, Mauro Condarelli wrote: > Signed-off-by: Mauro Condarelli <mc5686@mclink.it> > --- > fs/squashfs/sqfs.c | 45 +++++++++++++++++++++++++-------------- > fs/squashfs/sqfs_inode.c | 8 +++---- > include/configs/vocore2.h | 2 +- > 3 files changed, 34 insertions(+), 21 deletions(-) > > diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c > index 15208b4dab..b49331ce93 100644 > --- a/fs/squashfs/sqfs.c > +++ b/fs/squashfs/sqfs.c > @@ -18,6 +18,8 @@ > #include <string.h> > #include <squashfs.h> > #include <part.h> > +#include <div64.h> > +#include <stdio.h> > > #include "sqfs_decompressor.h" > #include "sqfs_filesystem.h" > @@ -82,13 +84,16 @@ static int sqfs_count_tokens(const char *filename) > */ > static int sqfs_calc_n_blks(__le64 start, __le64 end, u64 *offset) > { > - u64 start_, table_size; > + u64 start_, table_size, blks; > > table_size = le64_to_cpu(end) - le64_to_cpu(start); > - start_ = le64_to_cpu(start) / ctxt.cur_dev->blksz; > + start_ = le64_to_cpu(start); > + do_div(start_, ctxt.cur_dev->blksz); > *offset = le64_to_cpu(start) - (start_ * ctxt.cur_dev->blksz); > > - return DIV_ROUND_UP(table_size + *offset, ctxt.cur_dev->blksz); > + blks = table_size + *offset; > + if (do_div(blks, ctxt.cur_dev->blksz)) blks++; > + return blks; > } > > /* > @@ -109,8 +114,8 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, > if (inode_fragment_index >= get_unaligned_le32(&sblk->fragments)) > return -EINVAL; > > - start = get_unaligned_le64(&sblk->fragment_table_start) / > - ctxt.cur_dev->blksz; > + start = get_unaligned_le64(&sblk->fragment_table_start); > + do_div(start, ctxt.cur_dev->blksz); > n_blks = sqfs_calc_n_blks(sblk->fragment_table_start, > sblk->export_table_start, > &table_offset); > @@ -135,7 +140,8 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, > start_block = get_unaligned_le64(table + table_offset + block * > sizeof(u64)); > > - start = start_block / ctxt.cur_dev->blksz; > + start = start_block; > + do_div(start, ctxt.cur_dev->blksz); > n_blks = sqfs_calc_n_blks(cpu_to_le64(start_block), > sblk->fragment_table_start, &table_offset); > > @@ -641,8 +647,8 @@ static int sqfs_read_inode_table(unsigned char **inode_table) > > table_size = get_unaligned_le64(&sblk->directory_table_start) - > get_unaligned_le64(&sblk->inode_table_start); > - start = get_unaligned_le64(&sblk->inode_table_start) / > - ctxt.cur_dev->blksz; > + start = get_unaligned_le64(&sblk->inode_table_start); > + do_div(start, ctxt.cur_dev->blksz); > n_blks = sqfs_calc_n_blks(sblk->inode_table_start, > sblk->directory_table_start, &table_offset); > > @@ -725,8 +731,8 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list) > /* DIRECTORY TABLE */ > table_size = get_unaligned_le64(&sblk->fragment_table_start) - > get_unaligned_le64(&sblk->directory_table_start); > - start = get_unaligned_le64(&sblk->directory_table_start) / > - ctxt.cur_dev->blksz; > + start = get_unaligned_le64(&sblk->directory_table_start); > + do_div(start, ctxt.cur_dev->blksz); > n_blks = sqfs_calc_n_blks(sblk->directory_table_start, > sblk->fragment_table_start, &table_offset); > > @@ -1158,6 +1164,7 @@ static int sqfs_get_regfile_info(struct squashfs_reg_inode *reg, > fentry); > if (ret < 0) > return -EINVAL; > + > finfo->comp = true; > if (fentry->size < 1 || fentry->start == 0x7FFFFFFF) > return -EINVAL; > @@ -1328,17 +1335,19 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, > data_offset = finfo.start; > datablock = malloc(get_unaligned_le32(&sblk->block_size)); > if (!datablock) { > + printf("Error: malloc(%u) failed.\n", get_unaligned_le32(&sblk->block_size)); > ret = -ENOMEM; > goto free_paths; > } > } > > for (j = 0; j < datablk_count; j++) { > - start = data_offset / ctxt.cur_dev->blksz; > + start = data_offset; > + do_div(start, ctxt.cur_dev->blksz); > table_size = SQFS_BLOCK_SIZE(finfo.blk_sizes[j]); > table_offset = data_offset - (start * ctxt.cur_dev->blksz); > - n_blks = DIV_ROUND_UP(table_size + table_offset, > - ctxt.cur_dev->blksz); > + n_blks = table_size + table_offset; > + if (do_div(n_blks, ctxt.cur_dev->blksz)) n_blks++; > > data_buffer = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz); > > @@ -1365,8 +1374,10 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, > dest_len = get_unaligned_le32(&sblk->block_size); > ret = sqfs_decompress(&ctxt, datablock, &dest_len, > data, table_size); > - if (ret) > + if (ret) { > + printf("Errrt: block decompress failed.\n"); > goto free_buffer; > + } > > memcpy(buf + offset + *actread, datablock, dest_len); > *actread += dest_len; > @@ -1388,10 +1399,12 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, > goto free_buffer; > } > > - start = frag_entry.start / ctxt.cur_dev->blksz; > + start = frag_entry.start; > + do_div(start, ctxt.cur_dev->blksz); > table_size = SQFS_BLOCK_SIZE(frag_entry.size); > table_offset = frag_entry.start - (start * ctxt.cur_dev->blksz); > - n_blks = DIV_ROUND_UP(table_size + table_offset, ctxt.cur_dev->blksz); > + n_blks = table_size + table_offset; > + if (do_div(n_blks, ctxt.cur_dev->blksz)) n_blks++; > > fragment = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz); > > diff --git a/fs/squashfs/sqfs_inode.c b/fs/squashfs/sqfs_inode.c > index 1368f3063c..c330e32a0e 100644 > --- a/fs/squashfs/sqfs_inode.c > +++ b/fs/squashfs/sqfs_inode.c > @@ -11,6 +11,7 @@ > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > +#include <div64.h> > > #include "sqfs_decompressor.h" > #include "sqfs_filesystem.h" > @@ -67,10 +68,9 @@ int sqfs_inode_size(struct squashfs_base_inode *inode, u32 blk_size) > u64 file_size = get_unaligned_le64(&lreg->file_size); > unsigned int blk_list_size; > > - if (fragment == 0xFFFFFFFF) > - blk_list_size = DIV_ROUND_UP(file_size, blk_size); > - else > - blk_list_size = file_size / blk_size; > + if (do_div(file_size, blk_size) && (fragment == 0xFFFFFFFF)) > + file_size++; > + blk_list_size = file_size; > > return sizeof(*lreg) + blk_list_size * sizeof(u32); > } > diff --git a/include/configs/vocore2.h b/include/configs/vocore2.h > index 29a57ad233..dfdb8fcc04 100644 > --- a/include/configs/vocore2.h > +++ b/include/configs/vocore2.h > @@ -41,7 +41,7 @@ > > /* Memory usage */ > #define CONFIG_SYS_MAXARGS 64 > -#define CONFIG_SYS_MALLOC_LEN (1024 * 1024) > +#define CONFIG_SYS_MALLOC_LEN (16 * 1024 * 1024) > #define CONFIG_SYS_BOOTPARAMS_LEN (128 * 1024) > #define CONFIG_SYS_CBSIZE 512 Add in maintainers..
Am Sonntag, den 20.09.2020, 21:21 -0400 schrieb Tom Rini: > On Sun, Sep 20, 2020 at 06:29:01PM +0200, Mauro Condarelli wrote: > > > Signed-off-by: Mauro Condarelli <mc5686@mclink.it> > > --- > > fs/squashfs/sqfs.c | 45 +++++++++++++++++++++++++-------------- > > fs/squashfs/sqfs_inode.c | 8 +++---- > > include/configs/vocore2.h | 2 +- remove that file which is unrelated to this patch > > 3 files changed, 34 insertions(+), 21 deletions(-) > > > > diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c > > index 15208b4dab..b49331ce93 100644 > > --- a/fs/squashfs/sqfs.c > > +++ b/fs/squashfs/sqfs.c > > @@ -18,6 +18,8 @@ > > #include <string.h> > > #include <squashfs.h> > > #include <part.h> > > +#include <div64.h> > > +#include <stdio.h> > > > > #include "sqfs_decompressor.h" > > #include "sqfs_filesystem.h" > > @@ -82,13 +84,16 @@ static int sqfs_count_tokens(const char *filename) > > */ > > static int sqfs_calc_n_blks(__le64 start, __le64 end, u64 *offset) > > { > > - u64 start_, table_size; > > + u64 start_, table_size, blks; > > > > table_size = le64_to_cpu(end) - le64_to_cpu(start); > > - start_ = le64_to_cpu(start) / ctxt.cur_dev->blksz; > > + start_ = le64_to_cpu(start); > > + do_div(start_, ctxt.cur_dev->blksz); have you tried with lldiv() which returns the 64bit result? Also it would be a little cleaner: start_ = lldiv(le64_to_cpu(start), ctxt.cur_dev->blksz); > > *offset = le64_to_cpu(start) - (start_ * ctxt.cur_dev->blksz); > > > > - return DIV_ROUND_UP(table_size + *offset, ctxt.cur_dev->blksz); > > + blks = table_size + *offset; > > + if (do_div(blks, ctxt.cur_dev->blksz)) blks++; > > + return blks; maybe define something like this and use that instead of DIV_ROUND_UP: #define lldiv_round_up(n, d) lldiv((n) + (d) - 1, (d)) > > } > > > > /* > > @@ -109,8 +114,8 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, > > if (inode_fragment_index >= get_unaligned_le32(&sblk->fragments)) > > return -EINVAL; > > > > - start = get_unaligned_le64(&sblk->fragment_table_start) / > > - ctxt.cur_dev->blksz; > > + start = get_unaligned_le64(&sblk->fragment_table_start); > > + do_div(start, ctxt.cur_dev->blksz); > > n_blks = sqfs_calc_n_blks(sblk->fragment_table_start, > > sblk->export_table_start, > > &table_offset); > > @@ -135,7 +140,8 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, > > start_block = get_unaligned_le64(table + table_offset + block * > > sizeof(u64)); > > > > - start = start_block / ctxt.cur_dev->blksz; > > + start = start_block; > > + do_div(start, ctxt.cur_dev->blksz); > > n_blks = sqfs_calc_n_blks(cpu_to_le64(start_block), > > sblk->fragment_table_start, &table_offset); > > > > @@ -641,8 +647,8 @@ static int sqfs_read_inode_table(unsigned char **inode_table) > > > > table_size = get_unaligned_le64(&sblk->directory_table_start) - > > get_unaligned_le64(&sblk->inode_table_start); > > - start = get_unaligned_le64(&sblk->inode_table_start) / > > - ctxt.cur_dev->blksz; > > + start = get_unaligned_le64(&sblk->inode_table_start); > > + do_div(start, ctxt.cur_dev->blksz); > > n_blks = sqfs_calc_n_blks(sblk->inode_table_start, > > sblk->directory_table_start, &table_offset); > > > > @@ -725,8 +731,8 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list) > > /* DIRECTORY TABLE */ > > table_size = get_unaligned_le64(&sblk->fragment_table_start) - > > get_unaligned_le64(&sblk->directory_table_start); > > - start = get_unaligned_le64(&sblk->directory_table_start) / > > - ctxt.cur_dev->blksz; > > + start = get_unaligned_le64(&sblk->directory_table_start); > > + do_div(start, ctxt.cur_dev->blksz); > > n_blks = sqfs_calc_n_blks(sblk->directory_table_start, > > sblk->fragment_table_start, &table_offset); > > > > @@ -1158,6 +1164,7 @@ static int sqfs_get_regfile_info(struct squashfs_reg_inode *reg, > > fentry); > > if (ret < 0) > > return -EINVAL; > > + > > finfo->comp = true; > > if (fentry->size < 1 || fentry->start == 0x7FFFFFFF) > > return -EINVAL; > > @@ -1328,17 +1335,19 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, > > data_offset = finfo.start; > > datablock = malloc(get_unaligned_le32(&sblk->block_size)); > > if (!datablock) { > > + printf("Error: malloc(%u) failed.\n", get_unaligned_le32(&sblk->block_size)); > > ret = -ENOMEM; > > goto free_paths; > > } > > } > > > > for (j = 0; j < datablk_count; j++) { > > - start = data_offset / ctxt.cur_dev->blksz; > > + start = data_offset; > > + do_div(start, ctxt.cur_dev->blksz); > > table_size = SQFS_BLOCK_SIZE(finfo.blk_sizes[j]); > > table_offset = data_offset - (start * ctxt.cur_dev->blksz); > > - n_blks = DIV_ROUND_UP(table_size + table_offset, > > - ctxt.cur_dev->blksz); > > + n_blks = table_size + table_offset; > > + if (do_div(n_blks, ctxt.cur_dev->blksz)) n_blks++; > > > > data_buffer = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz); > > > > @@ -1365,8 +1374,10 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, > > dest_len = get_unaligned_le32(&sblk->block_size); > > ret = sqfs_decompress(&ctxt, datablock, &dest_len, > > data, table_size); > > - if (ret) > > + if (ret) { > > + printf("Errrt: block decompress failed.\n"); > > goto free_buffer; > > + } > > > > memcpy(buf + offset + *actread, datablock, dest_len); > > *actread += dest_len; > > @@ -1388,10 +1399,12 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, > > goto free_buffer; > > } > > > > - start = frag_entry.start / ctxt.cur_dev->blksz; > > + start = frag_entry.start; > > + do_div(start, ctxt.cur_dev->blksz); > > table_size = SQFS_BLOCK_SIZE(frag_entry.size); > > table_offset = frag_entry.start - (start * ctxt.cur_dev->blksz); > > - n_blks = DIV_ROUND_UP(table_size + table_offset, ctxt.cur_dev->blksz); > > + n_blks = table_size + table_offset; > > + if (do_div(n_blks, ctxt.cur_dev->blksz)) n_blks++; > > > > fragment = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz); > > > > diff --git a/fs/squashfs/sqfs_inode.c b/fs/squashfs/sqfs_inode.c > > index 1368f3063c..c330e32a0e 100644 > > --- a/fs/squashfs/sqfs_inode.c > > +++ b/fs/squashfs/sqfs_inode.c > > @@ -11,6 +11,7 @@ > > #include <stdio.h> > > #include <stdlib.h> > > #include <string.h> > > +#include <div64.h> > > > > #include "sqfs_decompressor.h" > > #include "sqfs_filesystem.h" > > @@ -67,10 +68,9 @@ int sqfs_inode_size(struct squashfs_base_inode *inode, u32 blk_size) > > u64 file_size = get_unaligned_le64(&lreg->file_size); > > unsigned int blk_list_size; > > > > - if (fragment == 0xFFFFFFFF) > > - blk_list_size = DIV_ROUND_UP(file_size, blk_size); > > - else > > - blk_list_size = file_size / blk_size; > > + if (do_div(file_size, blk_size) && (fragment == 0xFFFFFFFF)) > > + file_size++; > > + blk_list_size = file_size; > > > > return sizeof(*lreg) + blk_list_size * sizeof(u32); > > } > > diff --git a/include/configs/vocore2.h b/include/configs/vocore2.h > > index 29a57ad233..dfdb8fcc04 100644 > > --- a/include/configs/vocore2.h > > +++ b/include/configs/vocore2.h > > @@ -41,7 +41,7 @@ > > > > /* Memory usage */ > > #define CONFIG_SYS_MAXARGS 64 > > -#define CONFIG_SYS_MALLOC_LEN (1024 * 1024) > > +#define CONFIG_SYS_MALLOC_LEN (16 * 1024 * 1024) > > #define CONFIG_SYS_BOOTPARAMS_LEN (128 * 1024) > > #define CONFIG_SYS_CBSIZE 512 > > Add in maintainers.. >
Thanks for the review, I'll prepare a v2 ASAP. On 9/23/20 12:05 AM, Daniel Schwierzeck wrote: > Am Sonntag, den 20.09.2020, 21:21 -0400 schrieb Tom Rini: >> On Sun, Sep 20, 2020 at 06:29:01PM +0200, Mauro Condarelli wrote: >> >>> Signed-off-by: Mauro Condarelli <mc5686@mclink.it> >>> --- >>> fs/squashfs/sqfs.c | 45 +++++++++++++++++++++++++-------------- >>> fs/squashfs/sqfs_inode.c | 8 +++---- >>> include/configs/vocore2.h | 2 +- > remove that file which is unrelated to this patch I will as this is fixing things just for my target and that is clearly wrong. OTOH I feel some provision should be implemented (probably at Config.in level) to ensure SquashFS has enough malloc space for its needs. What are the best practices to handle this? >>> 3 files changed, 34 insertions(+), 21 deletions(-) >>> >>> diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c >>> index 15208b4dab..b49331ce93 100644 >>> --- a/fs/squashfs/sqfs.c >>> +++ b/fs/squashfs/sqfs.c >>> @@ -18,6 +18,8 @@ >>> #include <string.h> >>> #include <squashfs.h> >>> #include <part.h> >>> +#include <div64.h> >>> +#include <stdio.h> >>> >>> #include "sqfs_decompressor.h" >>> #include "sqfs_filesystem.h" >>> @@ -82,13 +84,16 @@ static int sqfs_count_tokens(const char *filename) >>> */ >>> static int sqfs_calc_n_blks(__le64 start, __le64 end, u64 *offset) >>> { >>> - u64 start_, table_size; >>> + u64 start_, table_size, blks; >>> >>> table_size = le64_to_cpu(end) - le64_to_cpu(start); >>> - start_ = le64_to_cpu(start) / ctxt.cur_dev->blksz; >>> + start_ = le64_to_cpu(start); >>> + do_div(start_, ctxt.cur_dev->blksz); > have you tried with lldiv() which returns the 64bit result? Also it > would be a little cleaner: > > start_ = lldiv(le64_to_cpu(start), ctxt.cur_dev->blksz); I thought of that (actually my first attempt was quite similar, but I noticed that lldiv() actually uses do_div() internally and so I decided to go directly for the lower level (and presumably faster) solution. If You (or the maintainers) feel otherwise I can revert with no problems. >>> *offset = le64_to_cpu(start) - (start_ * ctxt.cur_dev->blksz); >>> >>> - return DIV_ROUND_UP(table_size + *offset, ctxt.cur_dev->blksz); >>> + blks = table_size + *offset; >>> + if (do_div(blks, ctxt.cur_dev->blksz)) blks++; >>> + return blks; > maybe define something like this and use that instead of DIV_ROUND_UP: > > #define lldiv_round_up(n, d) lldiv((n) + (d) - 1, (d)) Again: IMHO having a macro does not add much value here and using lldiv() only adds a further level of call nesting, but I'm open to switch; I would like to understand the rationale behind these requests, though. >>> } >>> >>> /* >>> @@ -109,8 +114,8 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, >>> if (inode_fragment_index >= get_unaligned_le32(&sblk->fragments)) >>> return -EINVAL; >>> >>> - start = get_unaligned_le64(&sblk->fragment_table_start) / >>> - ctxt.cur_dev->blksz; >>> + start = get_unaligned_le64(&sblk->fragment_table_start); >>> + do_div(start, ctxt.cur_dev->blksz); >>> n_blks = sqfs_calc_n_blks(sblk->fragment_table_start, >>> sblk->export_table_start, >>> &table_offset); >>> @@ -135,7 +140,8 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, >>> start_block = get_unaligned_le64(table + table_offset + block * >>> sizeof(u64)); >>> >>> - start = start_block / ctxt.cur_dev->blksz; >>> + start = start_block; >>> + do_div(start, ctxt.cur_dev->blksz); >>> n_blks = sqfs_calc_n_blks(cpu_to_le64(start_block), >>> sblk->fragment_table_start, &table_offset); >>> >>> @@ -641,8 +647,8 @@ static int sqfs_read_inode_table(unsigned char **inode_table) >>> >>> table_size = get_unaligned_le64(&sblk->directory_table_start) - >>> get_unaligned_le64(&sblk->inode_table_start); >>> - start = get_unaligned_le64(&sblk->inode_table_start) / >>> - ctxt.cur_dev->blksz; >>> + start = get_unaligned_le64(&sblk->inode_table_start); >>> + do_div(start, ctxt.cur_dev->blksz); >>> n_blks = sqfs_calc_n_blks(sblk->inode_table_start, >>> sblk->directory_table_start, &table_offset); >>> >>> @@ -725,8 +731,8 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list) >>> /* DIRECTORY TABLE */ >>> table_size = get_unaligned_le64(&sblk->fragment_table_start) - >>> get_unaligned_le64(&sblk->directory_table_start); >>> - start = get_unaligned_le64(&sblk->directory_table_start) / >>> - ctxt.cur_dev->blksz; >>> + start = get_unaligned_le64(&sblk->directory_table_start); >>> + do_div(start, ctxt.cur_dev->blksz); >>> n_blks = sqfs_calc_n_blks(sblk->directory_table_start, >>> sblk->fragment_table_start, &table_offset); >>> >>> @@ -1158,6 +1164,7 @@ static int sqfs_get_regfile_info(struct squashfs_reg_inode *reg, >>> fentry); >>> if (ret < 0) >>> return -EINVAL; >>> + >>> finfo->comp = true; >>> if (fentry->size < 1 || fentry->start == 0x7FFFFFFF) >>> return -EINVAL; >>> @@ -1328,17 +1335,19 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, >>> data_offset = finfo.start; >>> datablock = malloc(get_unaligned_le32(&sblk->block_size)); >>> if (!datablock) { >>> + printf("Error: malloc(%u) failed.\n", get_unaligned_le32(&sblk->block_size)); >>> ret = -ENOMEM; >>> goto free_paths; >>> } >>> } >>> >>> for (j = 0; j < datablk_count; j++) { >>> - start = data_offset / ctxt.cur_dev->blksz; >>> + start = data_offset; >>> + do_div(start, ctxt.cur_dev->blksz); >>> table_size = SQFS_BLOCK_SIZE(finfo.blk_sizes[j]); >>> table_offset = data_offset - (start * ctxt.cur_dev->blksz); >>> - n_blks = DIV_ROUND_UP(table_size + table_offset, >>> - ctxt.cur_dev->blksz); >>> + n_blks = table_size + table_offset; >>> + if (do_div(n_blks, ctxt.cur_dev->blksz)) n_blks++; >>> >>> data_buffer = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz); >>> >>> @@ -1365,8 +1374,10 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, >>> dest_len = get_unaligned_le32(&sblk->block_size); >>> ret = sqfs_decompress(&ctxt, datablock, &dest_len, >>> data, table_size); >>> - if (ret) >>> + if (ret) { >>> + printf("Errrt: block decompress failed.\n"); >>> goto free_buffer; >>> + } >>> >>> memcpy(buf + offset + *actread, datablock, dest_len); >>> *actread += dest_len; >>> @@ -1388,10 +1399,12 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, >>> goto free_buffer; >>> } >>> >>> - start = frag_entry.start / ctxt.cur_dev->blksz; >>> + start = frag_entry.start; >>> + do_div(start, ctxt.cur_dev->blksz); >>> table_size = SQFS_BLOCK_SIZE(frag_entry.size); >>> table_offset = frag_entry.start - (start * ctxt.cur_dev->blksz); >>> - n_blks = DIV_ROUND_UP(table_size + table_offset, ctxt.cur_dev->blksz); >>> + n_blks = table_size + table_offset; >>> + if (do_div(n_blks, ctxt.cur_dev->blksz)) n_blks++; >>> >>> fragment = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz); >>> >>> diff --git a/fs/squashfs/sqfs_inode.c b/fs/squashfs/sqfs_inode.c >>> index 1368f3063c..c330e32a0e 100644 >>> --- a/fs/squashfs/sqfs_inode.c >>> +++ b/fs/squashfs/sqfs_inode.c >>> @@ -11,6 +11,7 @@ >>> #include <stdio.h> >>> #include <stdlib.h> >>> #include <string.h> >>> +#include <div64.h> >>> >>> #include "sqfs_decompressor.h" >>> #include "sqfs_filesystem.h" >>> @@ -67,10 +68,9 @@ int sqfs_inode_size(struct squashfs_base_inode *inode, u32 blk_size) >>> u64 file_size = get_unaligned_le64(&lreg->file_size); >>> unsigned int blk_list_size; >>> >>> - if (fragment == 0xFFFFFFFF) >>> - blk_list_size = DIV_ROUND_UP(file_size, blk_size); >>> - else >>> - blk_list_size = file_size / blk_size; >>> + if (do_div(file_size, blk_size) && (fragment == 0xFFFFFFFF)) >>> + file_size++; >>> + blk_list_size = file_size; >>> >>> return sizeof(*lreg) + blk_list_size * sizeof(u32); >>> } >>> diff --git a/include/configs/vocore2.h b/include/configs/vocore2.h >>> index 29a57ad233..dfdb8fcc04 100644 >>> --- a/include/configs/vocore2.h >>> +++ b/include/configs/vocore2.h >>> @@ -41,7 +41,7 @@ >>> >>> /* Memory usage */ >>> #define CONFIG_SYS_MAXARGS 64 >>> -#define CONFIG_SYS_MALLOC_LEN (1024 * 1024) >>> +#define CONFIG_SYS_MALLOC_LEN (16 * 1024 * 1024) >>> #define CONFIG_SYS_BOOTPARAMS_LEN (128 * 1024) >>> #define CONFIG_SYS_CBSIZE 512 >> Add in maintainers.. done ;) I just rebased on later master (some patches did not apply cleanly). I will check again before submitting v2. Regards Mauro
On Wed, Sep 23, 2020 at 08:27:24PM +0200, Mauro Condarelli wrote: > Thanks for the review, > I'll prepare a v2 ASAP. > > On 9/23/20 12:05 AM, Daniel Schwierzeck wrote: > > Am Sonntag, den 20.09.2020, 21:21 -0400 schrieb Tom Rini: > >> On Sun, Sep 20, 2020 at 06:29:01PM +0200, Mauro Condarelli wrote: > >> > >>> Signed-off-by: Mauro Condarelli <mc5686@mclink.it> > >>> --- > >>> fs/squashfs/sqfs.c | 45 +++++++++++++++++++++++++-------------- > >>> fs/squashfs/sqfs_inode.c | 8 +++---- > >>> include/configs/vocore2.h | 2 +- > > remove that file which is unrelated to this patch > I will as this is fixing things just for my target and that is clearly wrong. > OTOH I feel some provision should be implemented (probably at Config.in > level) to ensure SquashFS has enough malloc space for its needs. > What are the best practices to handle this? I don't recall why CONFIG_SYS_MALLOC_LEN hasn't been fully moved to Kconfig right now, there may be a good reason, or it just may be hard. I don't think Kconfig logic supports something like "depends on FOO > 0x4000" so the only way is to note in the squashfs help how much space may be used for malloc in worst cases.
Am Mittwoch, den 23.09.2020, 20:27 +0200 schrieb Mauro Condarelli: > Thanks for the review, > I'll prepare a v2 ASAP. > > On 9/23/20 12:05 AM, Daniel Schwierzeck wrote: > > Am Sonntag, den 20.09.2020, 21:21 -0400 schrieb Tom Rini: > > > On Sun, Sep 20, 2020 at 06:29:01PM +0200, Mauro Condarelli wrote: > > > > > > > Signed-off-by: Mauro Condarelli <mc5686@mclink.it> > > > > --- > > > > fs/squashfs/sqfs.c | 45 +++++++++++++++++++++++++-------------- > > > > fs/squashfs/sqfs_inode.c | 8 +++---- > > > > include/configs/vocore2.h | 2 +- > > remove that file which is unrelated to this patch > I will as this is fixing things just for my target and that is clearly wrong. > OTOH I feel some provision should be implemented (probably at Config.in > level) to ensure SquashFS has enough malloc space for its needs. > What are the best practices to handle this? > > > > > > 3 files changed, 34 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c > > > > index 15208b4dab..b49331ce93 100644 > > > > --- a/fs/squashfs/sqfs.c > > > > +++ b/fs/squashfs/sqfs.c > > > > @@ -18,6 +18,8 @@ > > > > #include <string.h> > > > > #include <squashfs.h> > > > > #include <part.h> > > > > +#include <div64.h> > > > > +#include <stdio.h> > > > > > > > > #include "sqfs_decompressor.h" > > > > #include "sqfs_filesystem.h" > > > > @@ -82,13 +84,16 @@ static int sqfs_count_tokens(const char *filename) > > > > */ > > > > static int sqfs_calc_n_blks(__le64 start, __le64 end, u64 *offset) > > > > { > > > > - u64 start_, table_size; > > > > + u64 start_, table_size, blks; > > > > > > > > table_size = le64_to_cpu(end) - le64_to_cpu(start); > > > > - start_ = le64_to_cpu(start) / ctxt.cur_dev->blksz; > > > > + start_ = le64_to_cpu(start); > > > > + do_div(start_, ctxt.cur_dev->blksz); > > have you tried with lldiv() which returns the 64bit result? Also it > > would be a little cleaner: > > > > start_ = lldiv(le64_to_cpu(start), ctxt.cur_dev->blksz); > I thought of that (actually my first attempt was quite similar, > but I noticed that lldiv() actually uses do_div() internally and > so I decided to go directly for the lower level (and presumably > faster) solution. > If You (or the maintainers) feel otherwise I can revert with > no problems. because do_div() replaces the dividend with the division remainder and lldiv() returns the division result without modifying the dividend. Anyway I had a look at the Linux code and other U-Boot file systems and there is a better solution. "struct blk_desc" has a member "int log2blksz" which is initialized as "desc->log2blksz = LOG2(desc- >blksz) in blk-uclass.c. With that you can replace all divisions with "desc->blksz" as divisor with a simple right bit shift like so: start_ = le64_to_cpu(start) >> ctxt.cur_dev->log2blksz; Likewise all multiplications can be replaced with a left bit shift, e.g.: *offset = le64_to_cpu(start) - (start_ << ctxt.cur_dev->log2blksz); Also the Linux code never uses DIV_ROUND_UP() for any block related calculations. So maybe all DIV_ROUND_UP() calls can be replaced with a bit shift too.
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 15208b4dab..b49331ce93 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -18,6 +18,8 @@ #include <string.h> #include <squashfs.h> #include <part.h> +#include <div64.h> +#include <stdio.h> #include "sqfs_decompressor.h" #include "sqfs_filesystem.h" @@ -82,13 +84,16 @@ static int sqfs_count_tokens(const char *filename) */ static int sqfs_calc_n_blks(__le64 start, __le64 end, u64 *offset) { - u64 start_, table_size; + u64 start_, table_size, blks; table_size = le64_to_cpu(end) - le64_to_cpu(start); - start_ = le64_to_cpu(start) / ctxt.cur_dev->blksz; + start_ = le64_to_cpu(start); + do_div(start_, ctxt.cur_dev->blksz); *offset = le64_to_cpu(start) - (start_ * ctxt.cur_dev->blksz); - return DIV_ROUND_UP(table_size + *offset, ctxt.cur_dev->blksz); + blks = table_size + *offset; + if (do_div(blks, ctxt.cur_dev->blksz)) blks++; + return blks; } /* @@ -109,8 +114,8 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, if (inode_fragment_index >= get_unaligned_le32(&sblk->fragments)) return -EINVAL; - start = get_unaligned_le64(&sblk->fragment_table_start) / - ctxt.cur_dev->blksz; + start = get_unaligned_le64(&sblk->fragment_table_start); + do_div(start, ctxt.cur_dev->blksz); n_blks = sqfs_calc_n_blks(sblk->fragment_table_start, sblk->export_table_start, &table_offset); @@ -135,7 +140,8 @@ static int sqfs_frag_lookup(u32 inode_fragment_index, start_block = get_unaligned_le64(table + table_offset + block * sizeof(u64)); - start = start_block / ctxt.cur_dev->blksz; + start = start_block; + do_div(start, ctxt.cur_dev->blksz); n_blks = sqfs_calc_n_blks(cpu_to_le64(start_block), sblk->fragment_table_start, &table_offset); @@ -641,8 +647,8 @@ static int sqfs_read_inode_table(unsigned char **inode_table) table_size = get_unaligned_le64(&sblk->directory_table_start) - get_unaligned_le64(&sblk->inode_table_start); - start = get_unaligned_le64(&sblk->inode_table_start) / - ctxt.cur_dev->blksz; + start = get_unaligned_le64(&sblk->inode_table_start); + do_div(start, ctxt.cur_dev->blksz); n_blks = sqfs_calc_n_blks(sblk->inode_table_start, sblk->directory_table_start, &table_offset); @@ -725,8 +731,8 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list) /* DIRECTORY TABLE */ table_size = get_unaligned_le64(&sblk->fragment_table_start) - get_unaligned_le64(&sblk->directory_table_start); - start = get_unaligned_le64(&sblk->directory_table_start) / - ctxt.cur_dev->blksz; + start = get_unaligned_le64(&sblk->directory_table_start); + do_div(start, ctxt.cur_dev->blksz); n_blks = sqfs_calc_n_blks(sblk->directory_table_start, sblk->fragment_table_start, &table_offset); @@ -1158,6 +1164,7 @@ static int sqfs_get_regfile_info(struct squashfs_reg_inode *reg, fentry); if (ret < 0) return -EINVAL; + finfo->comp = true; if (fentry->size < 1 || fentry->start == 0x7FFFFFFF) return -EINVAL; @@ -1328,17 +1335,19 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, data_offset = finfo.start; datablock = malloc(get_unaligned_le32(&sblk->block_size)); if (!datablock) { + printf("Error: malloc(%u) failed.\n", get_unaligned_le32(&sblk->block_size)); ret = -ENOMEM; goto free_paths; } } for (j = 0; j < datablk_count; j++) { - start = data_offset / ctxt.cur_dev->blksz; + start = data_offset; + do_div(start, ctxt.cur_dev->blksz); table_size = SQFS_BLOCK_SIZE(finfo.blk_sizes[j]); table_offset = data_offset - (start * ctxt.cur_dev->blksz); - n_blks = DIV_ROUND_UP(table_size + table_offset, - ctxt.cur_dev->blksz); + n_blks = table_size + table_offset; + if (do_div(n_blks, ctxt.cur_dev->blksz)) n_blks++; data_buffer = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz); @@ -1365,8 +1374,10 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, dest_len = get_unaligned_le32(&sblk->block_size); ret = sqfs_decompress(&ctxt, datablock, &dest_len, data, table_size); - if (ret) + if (ret) { + printf("Errrt: block decompress failed.\n"); goto free_buffer; + } memcpy(buf + offset + *actread, datablock, dest_len); *actread += dest_len; @@ -1388,10 +1399,12 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len, goto free_buffer; } - start = frag_entry.start / ctxt.cur_dev->blksz; + start = frag_entry.start; + do_div(start, ctxt.cur_dev->blksz); table_size = SQFS_BLOCK_SIZE(frag_entry.size); table_offset = frag_entry.start - (start * ctxt.cur_dev->blksz); - n_blks = DIV_ROUND_UP(table_size + table_offset, ctxt.cur_dev->blksz); + n_blks = table_size + table_offset; + if (do_div(n_blks, ctxt.cur_dev->blksz)) n_blks++; fragment = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz); diff --git a/fs/squashfs/sqfs_inode.c b/fs/squashfs/sqfs_inode.c index 1368f3063c..c330e32a0e 100644 --- a/fs/squashfs/sqfs_inode.c +++ b/fs/squashfs/sqfs_inode.c @@ -11,6 +11,7 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <div64.h> #include "sqfs_decompressor.h" #include "sqfs_filesystem.h" @@ -67,10 +68,9 @@ int sqfs_inode_size(struct squashfs_base_inode *inode, u32 blk_size) u64 file_size = get_unaligned_le64(&lreg->file_size); unsigned int blk_list_size; - if (fragment == 0xFFFFFFFF) - blk_list_size = DIV_ROUND_UP(file_size, blk_size); - else - blk_list_size = file_size / blk_size; + if (do_div(file_size, blk_size) && (fragment == 0xFFFFFFFF)) + file_size++; + blk_list_size = file_size; return sizeof(*lreg) + blk_list_size * sizeof(u32); } diff --git a/include/configs/vocore2.h b/include/configs/vocore2.h index 29a57ad233..dfdb8fcc04 100644 --- a/include/configs/vocore2.h +++ b/include/configs/vocore2.h @@ -41,7 +41,7 @@ /* Memory usage */ #define CONFIG_SYS_MAXARGS 64 -#define CONFIG_SYS_MALLOC_LEN (1024 * 1024) +#define CONFIG_SYS_MALLOC_LEN (16 * 1024 * 1024) #define CONFIG_SYS_BOOTPARAMS_LEN (128 * 1024) #define CONFIG_SYS_CBSIZE 512
Signed-off-by: Mauro Condarelli <mc5686@mclink.it> --- fs/squashfs/sqfs.c | 45 +++++++++++++++++++++++++-------------- fs/squashfs/sqfs_inode.c | 8 +++---- include/configs/vocore2.h | 2 +- 3 files changed, 34 insertions(+), 21 deletions(-)