Correcting myself. See below. On 10/1/20 10:41 AM, Mauro Condarelli wrote: > Thanks for Your review. > > On 10/1/20 9:59 AM, Miquel Raynal wrote: >> 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. > This fails on mips32, specifically for the vocore2 board. > Problem here is __udivdi3 is not defined for this architecture > while it is for ARM32. > My (limited) understanding suggests it should be removed for ARM > since its usage has been (is being?) weeded out from both kernel > and u-boot. This is not my call, though. > > I will add a note to v3. > >>>> --- >>>> >>>> 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. > Will do in v3. > > >>>> - 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. > I did not do this because DIV_ROUND_UP() is in a global > (non-architecture-specific) > header and it unconditionally uses division; I did not know how to handle > this. > Would a comment suffice to clarify intent? Something like: > > n_blks = (table_size + table_offset + ctxt.cur_dev->blksz - 1) >> > ctxt.cur_dev->log2blksz; /* ROUND_UP division */ > > Note: this problem stays even if we roll-back to use do_div(); see below. actually include/linux/kernel.h defines both DIV_ROUND_DOWN_ULL() and DIV_ROUND_UP_ULL() I suppose we should use those in all cases.
>>>> else >>>> - blk_list_size = file_size / blk_size; >>>> + blk_list_size = file_size > LOG2(blk_size); >>> Very bad mistake here: > should be >>. > Sorry, my bad. > Corrected for v3. > >> 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. > Please make up your mind about this. > I personally tend to agree with Miquèl and my v1 patch used > do_div() exclusively (i did not use even the lldiv() wrapper), but > I will not insist either way... just let me know what's considered > better. As said above: it seems using the macros is both "standard" and safer than using shifts. If I get a go-ahead I'll use those macros in v3. > >> Thanks, >> Miquèl > Many thanks > Mauro
