From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Petazzoni Date: Thu, 1 Oct 2020 09:28:41 +0200 Subject: [RFC PATCH v2 1/1] Fix missing __udivdi3 in SquashFS implementation. In-Reply-To: <20200930154511.630103-2-mc5686@mclink.it> References: <20200930154511.630103-1-mc5686@mclink.it> <20200930154511.630103-2-mc5686@mclink.it> Message-ID: <20201001092841.55b93d5c@windsurf.home> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. > --- > > 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. > - 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 >>. Best regards, Thomas -- Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com