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

[Mauro Condarelli]

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