[RFC PATCH v2 1/1] Fix missing __udivdi3 in SquashFS implementation.

[Thomas Petazzoni <thomas.petazzoni@bootlin.com>]

Hello,

On Wed, 30 Sep 2020 17:45:11 +0200
Mauro Condarelli <mc5686@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 <mc5686@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.


> -		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