[RESEND,v2] iio: gts-helper: Fix division loop

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

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


base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de
  

On Mon, 12 Feb 2024 13:20:09 +0200
Matti Vaittinen <mazziesaccount@gmail.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@gmail.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);
>  }
>  
>  /**
> 
> base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de
  

On 17/2/24 00:28, Jonathan Cameron wrote:
> On Mon, 12 Feb 2024 13:20:09 +0200
> Matti Vaittinen <mazziesaccount@gmail.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@gmail.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?

Regards,
Subhajit Ghosh

>>
>> base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de
>
  

On Sun, 18 Feb 2024 01:09:33 +1030
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> On 17/2/24 00:28, Jonathan Cameron wrote:
> > On Mon, 12 Feb 2024 13:20:09 +0200
> > Matti Vaittinen <mazziesaccount@gmail.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@gmail.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  
> >   
>
  

On 18/2/24 02:57, Jonathan Cameron wrote:
> On Sun, 18 Feb 2024 01:09:33 +1030
> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
> 
>> On 17/2/24 00:28, Jonathan Cameron wrote:
>>> On Mon, 12 Feb 2024 13:20:09 +0200
>>> Matti Vaittinen <mazziesaccount@gmail.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@gmail.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@tweaklogic.com>

Regards,
Subhajit Ghosh
  

Hi Jonathan,

On 2/16/24 15:58, Jonathan Cameron wrote:
> On Mon, 12 Feb 2024 13:20:09 +0200
> Matti Vaittinen <mazziesaccount@gmail.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@gmail.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).

Oh, I didn't think about it. Just thought I'll rebase to the most recent 
tag. I see the point now that you mentioned it, thanks.

> Would have been fine to send a ping response to the first posting of it.

Ok. Some maintainers like Mark prefer getting full resend instead of a 
ping because they don't keep the old messages/patches around. Reacting 
to ping would require them to go and fetch the patch from lore - while 
having full resend allows them to apply patch using their normal 
work-flow. Or, at least I think this is how Mark told me couple of years 
ago. I must admit that plenty of water has flown through the Oulu-river 
since that, so maybe this has changed also for them.

Anyways, good to know your preference, thanks!

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

Ok. This is perfectly fine. I just thought that maybe the patch fell 
through the cracks and decided to re-send before I forget ... :)

> Applied to the togreg branch of iio.git and pushed out as testing for
> all the normal reasons.

Thanks!

Yours,
	-- Matti
  

On 2/18/24 07:26, Subhajit Ghosh wrote:
> On 18/2/24 02:57, Jonathan Cameron wrote:
>> On Sun, 18 Feb 2024 01:09:33 +1030
>> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
>>
>>> On 17/2/24 00:28, Jonathan Cameron wrote:
>>>> On Mon, 12 Feb 2024 13:20:09 +0200
>>>> Matti Vaittinen <mazziesaccount@gmail.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@gmail.com>
>>>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
>>>>>
>>>>> ---


> As I understand that you have already applied this patch but still,
> 
> Tested-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>

Thank you Subhajit! Your effort is _very much_ appreciated! :)

Yours,
	-- Matti
  

On Mon, 19 Feb 2024 09:22:24 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 2/18/24 07:26, Subhajit Ghosh wrote:
> > On 18/2/24 02:57, Jonathan Cameron wrote:  
> >> On Sun, 18 Feb 2024 01:09:33 +1030
> >> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
> >>  
> >>> On 17/2/24 00:28, Jonathan Cameron wrote:  
> >>>> On Mon, 12 Feb 2024 13:20:09 +0200
> >>>> Matti Vaittinen <mazziesaccount@gmail.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@gmail.com>
> >>>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
> >>>>>
> >>>>> ---  
> 
> 
> > As I understand that you have already applied this patch but still,
> > 
> > Tested-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>  
> 
> Thank you Subhajit! Your effort is _very much_ appreciated! :)
I had to rebase the tree anyway to squash an unrelated issue, so
I added the tag whilst doing so.

Thanks,

Jonathan

> 
> Yours,
> 	-- Matti
>
  

On 20/2/24 06:02, Jonathan Cameron wrote:
> On Mon, 19 Feb 2024 09:22:24 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 2/18/24 07:26, Subhajit Ghosh wrote:
>>> On 18/2/24 02:57, Jonathan Cameron wrote:
>>>> On Sun, 18 Feb 2024 01:09:33 +1030
>>>> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
>>>>   
>>>>> On 17/2/24 00:28, Jonathan Cameron wrote:
>>>>>> On Mon, 12 Feb 2024 13:20:09 +0200
>>>>>> Matti Vaittinen <mazziesaccount@gmail.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@gmail.com>
>>>>>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
>>>>>>>
>>>>>>> ---
>>
>>
>>> As I understand that you have already applied this patch but still,
>>>
>>> Tested-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
>>
>> Thank you Subhajit! Your effort is _very much_ appreciated! :)
> I had to rebase the tree anyway to squash an unrelated issue, so
> I added the tag whilst doing so.
> 
> Thanks,
> 
> Jonathan
> 
>>
>> Yours,
>> 	-- Matti
>>
> 
You are welcome Matti and thank you Jonathan.

Regards,
Subhajit Ghosh