linux-kernel - Re: [RESEND PATCH v2] iio: gts-helper: Fix division loop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 18 Feb 2024 15:56:12 +1030
From: Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Matti Vaittinen <mazziesaccount@...il.com>,
 Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Lars-Peter Clausen <lars@...afoo.de>, linux-kernel@...r.kernel.org,
 David Laight <David.Laight@...lab.com>, linux-iio@...r.kernel.org
Subject: Re: [RESEND PATCH v2] iio: gts-helper: Fix division loop

On 18/2/24 02:57, Jonathan Cameron wrote:
> On Sun, 18 Feb 2024 01:09:33 +1030
> Subhajit Ghosh <subhajit.ghosh@...aklogic.com> wrote:
> 
>> On 17/2/24 00:28, Jonathan Cameron wrote:
>>> On Mon, 12 Feb 2024 13:20:09 +0200
>>> Matti Vaittinen <mazziesaccount@...il.com> wrote:
>>>    
>>>> The loop based 64bit division may run for a long time when dividend is a
>>>> lot bigger than the divider. Replace the division loop by the
>>>> div64_u64() which implementation may be significantly faster.
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
>>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
>>>>
>>>> ---
>>>> This is a resend. Only change is the base which is now the v6.8-rc4 and
>>>> not the v6.8-rc1
>>> Given I'm not rushing this in, it is going via my togreg tree, so the
>>> rebase wasn't really helpful (thankfully didn't stop it applying).
>>> Would have been fine to send a ping response to the first posting of it.
>>>
>>> I was leaving some time for David or Subhajit to have time to take
>>> another look, but guess they are either happy with this or busy.
>>>
>>> Applied to the togreg branch of iio.git and pushed out as testing for
>>> all the normal reasons.
>>>
>>> Jonathan
>>>    
>>>>
>>>> This change was earlier applied and reverted as it confusingly lacked of
>>>> the removal of the overflow check (which is only needed when we do
>>>> looping "while (full > scale * (u64)tmp)". As this loop got removed, the
>>>> check got also obsolete and leaving it to the code caused some
>>>> confusion.
>>>>
>>>> So, I marked this as a v2, where v1 is the reverted change discussed
>>>> here:
>>>> https://lore.kernel.org/linux-iio/ZZZ7pJBGkTdFFqiY@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi/
>>>>
>>>> Revision history:
>>>> v1 => v2:
>>>>    - Drop the obsolete overflow check
>>>>    - Rebased on top of the v6.8-rc4
>>>>
>>>> iio: gts: loop fix fix
>>>> ---
>>>>    drivers/iio/industrialio-gts-helper.c | 15 +--------------
>>>>    1 file changed, 1 insertion(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
>>>> index 7653261d2dc2..b51eb6cb766f 100644
>>>> --- a/drivers/iio/industrialio-gts-helper.c
>>>> +++ b/drivers/iio/industrialio-gts-helper.c
>>>> @@ -34,24 +34,11 @@
>>>>    static int iio_gts_get_gain(const u64 max, const u64 scale)
>>>>    {
>>>>    	u64 full = max;
>>>> -	int tmp = 1;
>>>>    
>>>>    	if (scale > full || !scale)
>>>>    		return -EINVAL;
>>>>    
>>>> -	if (U64_MAX - full < scale) {
>>>> -		/* Risk of overflow */
>>>> -		if (full - scale < scale)
>>>> -			return 1;
>>>> -
>>>> -		full -= scale;
>>>> -		tmp++;
>>>> -	}
>>>> -
>>>> -	while (full > scale * (u64)tmp)
>>>> -		tmp++;
>>>> -
>>>> -	return tmp;
>>>> +	return div64_u64(full, scale);
>>>>    }
>>>>    
>>>>    /**
>> Hi Matti and Jonathan,
>>
>> I somehow missed testing this patch earlier. The above patch works fine with apds9306 v7 driver(which work in progress!).
>> There are no errors.
>> My test script is simple:
>> #!/bin/bash
>> D=0
>> S=`cat /sys/bus/iio/devices/iio:device${D}/in_illuminance_scale_available`
>>
>> for s in $S; do
>> 	echo $s
>> 	echo $s > /sys/bus/iio/devices/iio:device${D}/in_illuminance_scale
>> 	sleep 5
>> done
>>
>> One question - if I test a patch like this, do I put a "Tested-by" tag or just mention that I have tested it?
> Both are useful - so thanks for this email.
> 
> Preference for a formal tag though as that goes in the git commit and we have
> a convenient record that both says you tested it + that we should make sure
> to cc you on related changes as you may well be in a position to test those
> as well!
> 
> Thanks,
> 
> Jonathan
> 
>>
>> Regards,
>> Subhajit Ghosh
>>
>>>>
>>>> base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de
>>>    
>>
> 
Thank you Jonathan for explaining the above.
I forgot to mention that the above test is run in parallel with continuous raw reads
from another script and event monitoring.
As I understand that you have already applied this patch but still,

Tested-by: Subhajit Ghosh <subhajit.ghosh@...aklogic.com>

Regards,
Subhajit Ghosh