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

[Mauro Condarelli]

Thanks for Your review.

On 10/1/20 9:59 AM, Miquel Raynal wrote:
> Hello,
>
> Thomas Petazzoni <thomas.petazzoni at bootlin.com> wrote on Thu, 1 Oct
> 2020 09:28:41 +0200:
>
>> Hello,
>>
>> On Wed, 30 Sep 2020 17:45:11 +0200
>> Mauro Condarelli <mc5686 at 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 at 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.

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

>
> Thanks,
> Miquèl
Many thanks
Mauro