From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mauro Condarelli Date: Thu, 1 Oct 2020 12:17:05 +0200 Subject: [RFC PATCH v2 1/1] Fix missing __udivdi3 in SquashFS implementation. In-Reply-To: <20201001105600.006e298f@xps13> References: <20200930154511.630103-1-mc5686@mclink.it> <20200930154511.630103-2-mc5686@mclink.it> <20201001092841.55b93d5c@windsurf.home> <20201001095943.3e73cbd9@xps13> <9340dc9e-e06c-0a8f-bc66-a758d234f6c9@mclink.it> <5d7cdbf1-742e-1fbe-61f6-ac1804fc827a@mclink.it> <20201001105600.006e298f@xps13> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Ok, Thanks. Patch is ready, I'll send it after some extended testing on my system (vocore2). Regards Mauro On 10/1/20 10:56 AM, Miquel Raynal wrote: > Hi Mauro, > > Mauro Condarelli wrote on Thu, 1 Oct 2020 10:53:30 > +0200: > >> 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 wrote on Thu, 1 Oct >>>> 2020 09:28:41 +0200: >>>> >>>>> Hello, >>>>> >>>>> On Wed, 30 Sep 2020 17:45:11 +0200 >>>>> Mauro Condarelli 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 >>>>> 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. > I think we all agree using these macros is much nicer, readable and > probably almost as fast. > > Thanks, > Miqu?l >