Hello, Thomas Petazzoni <thomas.petazz...@bootlin.com> wrote on Thu, 1 Oct 2020 09:28:41 +0200:
> Hello, > > On Wed, 30 Sep 2020 17:45:11 +0200 > Mauro Condarelli <mc5...@mclink.it> wrote: > > > Use right shift to avoid 64-bit divisions. > > > > These divisions are needed to convert from file length (potentially > > over 32-bit range) to block number, so result and remainder are > > guaranteed to fit in 32-bit integers. > > > > Signed-off-by: Mauro Condarelli <mc5...@mclink.it> > > Perhaps the commit log should contain an example U-Boot > configuration/platform where it fails to build. Indeed, we did test the > SquashFS code on ARM 32-bit, and it built and worked fine. > > > --- > > > > Changes in v2: > > - replace division with right shift (Daniel Schwierzeck). > > - remove vocore2-specific change (Daniel Schwierzeck). > > - add warning to Kconfig about CONFIG_SYS_MALLOC_LEN (Tom Rini). > > > > fs/squashfs/Kconfig | 2 ++ > > fs/squashfs/sqfs.c | 30 +++++++++++++++--------------- > > fs/squashfs/sqfs_inode.c | 6 +++--- > > 3 files changed, 20 insertions(+), 18 deletions(-) > > > > diff --git a/fs/squashfs/Kconfig b/fs/squashfs/Kconfig > > index 54ab1618f1..7c3f83d007 100644 > > --- a/fs/squashfs/Kconfig > > +++ b/fs/squashfs/Kconfig > > @@ -9,3 +9,5 @@ config FS_SQUASHFS > > filesystem use, for archival use (i.e. in cases where a .tar.gz file > > may be used), and in constrained block device/memory systems (e.g. > > embedded systems) where low overhead is needed. > > + WARNING: if compression is enabled SquashFS needs a large amount > > + of dynamic memory; make sure CONFIG_SYS_MALLOC_LEN >= 0x4000. > > This change is completely unrelated, and should be in a separate patch. I was about to tell you the same thing, this warning is useful but should definitely lay into its own commit. > > - n_blks = DIV_ROUND_UP(table_size + table_offset, > > - ctxt.cur_dev->blksz); > > + n_blks = (table_size + table_offset + ctxt.cur_dev->blksz - 1) > > >> > > + ctxt.cur_dev->log2blksz; > > I understand why you have to add blksz - 1 before doing the shift, but > I find that it's overall a lot less readable/clear. Is there a way to > do better ? > > We could use DIV_ROUND_UP_ULL() I guess. > > > > else > > - blk_list_size = file_size / blk_size; > > + blk_list_size = file_size > LOG2(blk_size); > > Very bad mistake here: > should be >>. I personally highly dislike replacing divisions into shifts. I think it's too much effort when trying to understand code you did not write yourself. Is it possible to use something like do_div? plus, you can check the remainder to be 0 in this case. Thanks, Miquèl
