[PATCH] iio: gts-helper: Fix division loop

[PATCH] iio: gts-helper: Fix division loop

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]

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

I've implemented also a fixup series for supporting rounding of
gains/scales:
https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gmail.com/

That series does also remove the offending loop.

We don't currently have any in-tree users of GTS helpers which would
need the rounding support so pushing the rounding is not urgent (and I
haven't heard of Subjahit whose driver required the rounding). Hence, we
may want to only take this loop fix in for now (?) and reconsider
rounding when someone need that.

Jonathan, what's your take on this?

 drivers/iio/industrialio-gts-helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
index 7653261d2dc2..abcab2d38589 100644
--- a/drivers/iio/industrialio-gts-helper.c
+++ b/drivers/iio/industrialio-gts-helper.c
@@ -34,7 +34,7 @@
 static int iio_gts_get_gain(const u64 max, const u64 scale)
 {
 	u64 full = max;
-	int tmp = 1;
+	int tmp = 0;
 
 	if (scale > full || !scale)
 		return -EINVAL;
@@ -48,8 +48,7 @@ static int iio_gts_get_gain(const u64 max, const u64 scale)
 		tmp++;
 	}
 
-	while (full > scale * (u64)tmp)
-		tmp++;
+	tmp += div64_u64(full, scale);
 
 	return tmp;
 }

base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab
-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] iio: gts-helper: Fix division loop

[Jonathan Cameron]

On Thu, 4 Jan 2024 11:34:28 +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")

Hmm. Fix or not perf improvement?  I'm going to take the middle ground
and leave the fixes tag, but not rush this in.

So applied to the togreg branch of iio.git and for now just pushed out
as testing for 0-day etc to take a look before I rebase that tree after
rc1.



> ---
> 
> I've implemented also a fixup series for supporting rounding of
> gains/scales:
> https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gmail.com/
> 
> That series does also remove the offending loop.
> 
> We don't currently have any in-tree users of GTS helpers which would
> need the rounding support so pushing the rounding is not urgent (and I
> haven't heard of Subjahit whose driver required the rounding). Hence, we
> may want to only take this loop fix in for now (?) and reconsider
> rounding when someone need that.
> 
> Jonathan, what's your take on this?
Agreed - let us wait for the rounding to have a user, but makes sense
to tidy this corner up in the meantime.

Thanks,

Jonathan

> 
>  drivers/iio/industrialio-gts-helper.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> index 7653261d2dc2..abcab2d38589 100644
> --- a/drivers/iio/industrialio-gts-helper.c
> +++ b/drivers/iio/industrialio-gts-helper.c
> @@ -34,7 +34,7 @@
>  static int iio_gts_get_gain(const u64 max, const u64 scale)
>  {
>  	u64 full = max;
> -	int tmp = 1;
> +	int tmp = 0;
>  
>  	if (scale > full || !scale)
>  		return -EINVAL;
> @@ -48,8 +48,7 @@ static int iio_gts_get_gain(const u64 max, const u64 scale)
>  		tmp++;
>  	}
>  
> -	while (full > scale * (u64)tmp)
> -		tmp++;
> +	tmp += div64_u64(full, scale);
>  
>  	return tmp;
>  }
> 
> base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab

Re: [PATCH] iio: gts-helper: Fix division loop

[Matti Vaittinen]

On 1/7/24 18:22, Jonathan Cameron wrote:
> On Thu, 4 Jan 2024 11:34:28 +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")
> 
> Hmm. Fix or not perf improvement?

Ha. I don't fancy calling my own doings as bugs - but when a code can 
keep kernel busy looping for several seconds... :|

>  I'm going to take the middle ground
> and leave the fixes tag, but not rush this in.

I think this is fine. It is nice to get this fix in stable if we have 
longer living stables with GTS helpers, because that's where many people 
do "real work" - and because it is likely some devices with new sensors 
are being built on products using these stable releases.

Still, the current users (ROHM light sensors) don't use that big 
dividends. Hence I don't think this needs any high speed fixup process :)

Thanks!


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH] iio: gts-helper: Fix division loop

[Subhajit Ghosh]

On 8/1/24 02:52, Jonathan Cameron wrote:
> On Thu, 4 Jan 2024 11:34:28 +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")
> 
> Hmm. Fix or not perf improvement?  I'm going to take the middle ground
> and leave the fixes tag, but not rush this in.
> 
> So applied to the togreg branch of iio.git and for now just pushed out
> as testing for 0-day etc to take a look before I rebase that tree after
> rc1.
> 
> 
> 
>> ---
>>
>> I've implemented also a fixup series for supporting rounding of
>> gains/scales:
>> https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gmail.com/
>>
>> That series does also remove the offending loop.
>>
>> We don't currently have any in-tree users of GTS helpers which would
>> need the rounding support so pushing the rounding is not urgent (and I
>> haven't heard of Subjahit whose driver required the rounding). Hence, we
>> may want to only take this loop fix in for now (?) and reconsider
>> rounding when someone need that.
>>
>> Jonathan, what's your take on this?
> Agreed - let us wait for the rounding to have a user, but makes sense
> to tidy this corner up in the meantime.
> 
> Thanks,
> 
> Jonathan
> 
>>
>>   drivers/iio/industrialio-gts-helper.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
>> index 7653261d2dc2..abcab2d38589 100644
>> --- a/drivers/iio/industrialio-gts-helper.c
>> +++ b/drivers/iio/industrialio-gts-helper.c
>> @@ -34,7 +34,7 @@
>>   static int iio_gts_get_gain(const u64 max, const u64 scale)
>>   {
>>   	u64 full = max;
>> -	int tmp = 1;
>> +	int tmp = 0;
>>   
>>   	if (scale > full || !scale)
>>   		return -EINVAL;
>> @@ -48,8 +48,7 @@ static int iio_gts_get_gain(const u64 max, const u64 scale)
>>   		tmp++;
>>   	}
>>   
>> -	while (full > scale * (u64)tmp)
>> -		tmp++;
>> +	tmp += div64_u64(full, scale);
>>   
>>   	return tmp;
>>   }
>>
>> base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab
> 
> 
Hi Matti,

Your fix works beautifully with the latest version of apds9306 driver which I am working on.
All available scale values can be set without any errors. Thank you.

Moving to a new city with a new full time job with the assumption of getting more time
for my list of opensource projects and contributions proved to be utterly wrong!

Regards,
Subhajit Ghosh

Re: [PATCH] iio: gts-helper: Fix division loop

[Matti Vaittinen]

On 1/19/24 13:56, Subhajit Ghosh wrote:
> On 8/1/24 02:52, Jonathan Cameron wrote:
>> On Thu, 4 Jan 2024 11:34:28 +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")
>>
>> Hmm. Fix or not perf improvement?  I'm going to take the middle ground
>> and leave the fixes tag, but not rush this in.
>>
>> So applied to the togreg branch of iio.git and for now just pushed out
>> as testing for 0-day etc to take a look before I rebase that tree after
>> rc1.
>>
>>
>>
>>> ---
>>>
>>> I've implemented also a fixup series for supporting rounding of
>>> gains/scales:
>>> https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gmail.com/
>>>
>>> That series does also remove the offending loop.
>>>
>>> We don't currently have any in-tree users of GTS helpers which would
>>> need the rounding support so pushing the rounding is not urgent (and I
>>> haven't heard of Subjahit whose driver required the rounding). Hence, we
>>> may want to only take this loop fix in for now (?) and reconsider
>>> rounding when someone need that.
>>>
>>> Jonathan, what's your take on this?
>> Agreed - let us wait for the rounding to have a user, but makes sense
>> to tidy this corner up in the meantime.
>>
>> Thanks,
>>
>> Jonathan
>>
>>>
>>>   drivers/iio/industrialio-gts-helper.c | 5 ++---
>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iio/industrialio-gts-helper.c 
>>> b/drivers/iio/industrialio-gts-helper.c
>>> index 7653261d2dc2..abcab2d38589 100644
>>> --- a/drivers/iio/industrialio-gts-helper.c
>>> +++ b/drivers/iio/industrialio-gts-helper.c
>>> @@ -34,7 +34,7 @@
>>>   static int iio_gts_get_gain(const u64 max, const u64 scale)
>>>   {
>>>       u64 full = max;
>>> -    int tmp = 1;
>>> +    int tmp = 0;
>>>       if (scale > full || !scale)
>>>           return -EINVAL;
>>> @@ -48,8 +48,7 @@ static int iio_gts_get_gain(const u64 max, const 
>>> u64 scale)
>>>           tmp++;
>>>       }
>>> -    while (full > scale * (u64)tmp)
>>> -        tmp++;
>>> +    tmp += div64_u64(full, scale);
>>>       return tmp;
>>>   }
>>>
>>> base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab
>>
>>
> Hi Matti,
> 
> Your fix works beautifully with the latest version of apds9306 driver 
> which I am working on.
> All available scale values can be set without any errors. Thank you.

Thanks for testing Subhajit! Just to ensure we have no miscommunication 
- did you test just this division fix, or the rounding fix here:
https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gmail.com/

> Moving to a new city with a new full time job with the assumption of 
> getting more time
> for my list of opensource projects and contributions proved to be 
> utterly wrong!

Well, I can't blame you :) Being in a new work at new city sounds like 
you have a lot on your plate right now. Give it half a year and things 
will stabilize though :) Oh, and falsely assuming that "when XXX, I will 
have the time to do YYY" - been there done that :)

Good luck on the new work and city!

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH] iio: gts-helper: Fix division loop

[Subhajit Ghosh]

On 22/1/24 17:20, Matti Vaittinen wrote:
> On 1/19/24 13:56, Subhajit Ghosh wrote:
>> On 8/1/24 02:52, Jonathan Cameron wrote:
>>> On Thu, 4 Jan 2024 11:34:28 +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")
>>>
>>> Hmm. Fix or not perf improvement?  I'm going to take the middle ground
>>> and leave the fixes tag, but not rush this in.
>>>
>>> So applied to the togreg branch of iio.git and for now just pushed out
>>> as testing for 0-day etc to take a look before I rebase that tree after
>>> rc1.
>>>
>>>
>>>
>>>> ---
>>>>
>>>> I've implemented also a fixup series for supporting rounding of
>>>> gains/scales:
>>>> https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gmail.com/
>>>>
>>>> That series does also remove the offending loop.
>>>>
>>>> We don't currently have any in-tree users of GTS helpers which would
>>>> need the rounding support so pushing the rounding is not urgent (and I
>>>> haven't heard of Subjahit whose driver required the rounding). Hence, we
>>>> may want to only take this loop fix in for now (?) and reconsider
>>>> rounding when someone need that.
>>>>
>>>> Jonathan, what's your take on this?
>>> Agreed - let us wait for the rounding to have a user, but makes sense
>>> to tidy this corner up in the meantime.
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>
>>>>
>>>>   drivers/iio/industrialio-gts-helper.c | 5 ++---
>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
>>>> index 7653261d2dc2..abcab2d38589 100644
>>>> --- a/drivers/iio/industrialio-gts-helper.c
>>>> +++ b/drivers/iio/industrialio-gts-helper.c
>>>> @@ -34,7 +34,7 @@
>>>>   static int iio_gts_get_gain(const u64 max, const u64 scale)
>>>>   {
>>>>       u64 full = max;
>>>> -    int tmp = 1;
>>>> +    int tmp = 0;
>>>>       if (scale > full || !scale)
>>>>           return -EINVAL;
>>>> @@ -48,8 +48,7 @@ static int iio_gts_get_gain(const u64 max, const u64 scale)
>>>>           tmp++;
>>>>       }
>>>> -    while (full > scale * (u64)tmp)
>>>> -        tmp++;
>>>> +    tmp += div64_u64(full, scale);
>>>>       return tmp;
>>>>   }
>>>>
>>>> base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab
>>>
>>>
>> Hi Matti,
>>
>> Your fix works beautifully with the latest version of apds9306 driver which I am working on.
>> All available scale values can be set without any errors. Thank you.
> 
> Thanks for testing Subhajit! Just to ensure we have no miscommunication - did you test just this division fix, or the rounding fix here:
> https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gmail.com/
You are most welcome. I did not check the above rounding fix pointed out by the link. I will be happy to check it and let you know by the end of this month.
I checked this division fix.

> 
>> Moving to a new city with a new full time job with the assumption of getting more time
>> for my list of opensource projects and contributions proved to be utterly wrong!
> 
> Well, I can't blame you :) Being in a new work at new city sounds like you have a lot on your plate right now. Give it half a year and things will stabilize though :) Oh, and falsely assuming that "when XXX, I will have the time to do YYY" - been there done that :)
> 
> Good luck on the new work and city!
Thank you Matti.
> 
> Yours,
>      -- Matti
> 
Regards,
Subhajit Ghosh

Re: [PATCH] iio: gts-helper: Fix division loop

[Subhajit Ghosh]

Hi Matti,
>>>>>   drivers/iio/industrialio-gts-helper.c | 5 ++---
>>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
>>>>> index 7653261d2dc2..abcab2d38589 100644
>>>>> --- a/drivers/iio/industrialio-gts-helper.c
>>>>> +++ b/drivers/iio/industrialio-gts-helper.c
>>>>> @@ -34,7 +34,7 @@
>>>>>   static int iio_gts_get_gain(const u64 max, const u64 scale)
>>>>>   {
>>>>>       u64 full = max;
>>>>> -    int tmp = 1;
>>>>> +    int tmp = 0;
>>>>>       if (scale > full || !scale)
>>>>>           return -EINVAL;
>>>>> @@ -48,8 +48,7 @@ static int iio_gts_get_gain(const u64 max, const u64 scale)
>>>>>           tmp++;
>>>>>       }
>>>>> -    while (full > scale * (u64)tmp)
>>>>> -        tmp++;
>>>>> +    tmp += div64_u64(full, scale);
>>>>>       return tmp;
>>>>>   }
>>>>>
>>>>> base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab
>>>>
>>>>
>>> Hi Matti,
>>>
>>> Your fix works beautifully with the latest version of apds9306 driver which I am working on.
>>> All available scale values can be set without any errors. Thank you.
>>
>> Thanks for testing Subhajit! Just to ensure we have no miscommunication - did you test just this division fix, or the rounding fix here:
>> https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gmail.com/
> You are most welcome. I did not check the above rounding fix pointed out by the link. I will be happy to check it and let you know by the end of this month.
> I checked this division fix.
I tested the patch in the above link with adps9306 driver which I am working on and it seems to work well without any issues.

Regards,
Subhajit Ghosh

Re: [PATCH] iio: gts-helper: Fix division loop

[Matti Vaittinen]

On 2/4/24 15:49, Subhajit Ghosh wrote:
> Hi Matti,
>>>>>>   drivers/iio/industrialio-gts-helper.c | 5 ++---
>>>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/industrialio-gts-helper.c 
>>>>>> b/drivers/iio/industrialio-gts-helper.c
>>>>>> index 7653261d2dc2..abcab2d38589 100644
>>>>>> --- a/drivers/iio/industrialio-gts-helper.c
>>>>>> +++ b/drivers/iio/industrialio-gts-helper.c
>>>>>> @@ -34,7 +34,7 @@
>>>>>>   static int iio_gts_get_gain(const u64 max, const u64 scale)
>>>>>>   {
>>>>>>       u64 full = max;
>>>>>> -    int tmp = 1;
>>>>>> +    int tmp = 0;
>>>>>>       if (scale > full || !scale)
>>>>>>           return -EINVAL;
>>>>>> @@ -48,8 +48,7 @@ static int iio_gts_get_gain(const u64 max, const 
>>>>>> u64 scale)
>>>>>>           tmp++;
>>>>>>       }
>>>>>> -    while (full > scale * (u64)tmp)
>>>>>> -        tmp++;
>>>>>> +    tmp += div64_u64(full, scale);
>>>>>>       return tmp;
>>>>>>   }
>>>>>>
>>>>>> base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab
>>>>>
>>>>>
>>>> Hi Matti,
>>>>
>>>> Your fix works beautifully with the latest version of apds9306 
>>>> driver which I am working on.
>>>> All available scale values can be set without any errors. Thank you.
>>>
>>> Thanks for testing Subhajit! Just to ensure we have no 
>>> miscommunication - did you test just this division fix, or the 
>>> rounding fix here:
>>> https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gmail.com/
>> You are most welcome. I did not check the above rounding fix pointed 
>> out by the link. I will be happy to check it and let you know by the 
>> end of this month.
>> I checked this division fix.
> I tested the patch in the above link with adps9306 driver which I am 
> working on and it seems to work well without any issues.

Great! Thanks a lot Subhajit! Your testing is very much appreciated :)

I think you sent another version of your APDS9306 driver. AFAIR, this 
driver could have benefited from the above rounding fix. I will see if I 
find the time and energy to see if I can dive into this again and 
re-spin this rounding fix. No promises but I'll try :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

RE: [PATCH] iio: gts-helper: Fix division loop

[David Laight]

From: Matti Vaittinen
> Sent: 22 January 2024 06:51
> 
> On 1/19/24 13:56, Subhajit Ghosh wrote:
> > On 8/1/24 02:52, Jonathan Cameron wrote:
> >> On Thu, 4 Jan 2024 11:34:28 +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")
> >>
> >> Hmm. Fix or not perf improvement?  I'm going to take the middle ground
> >> and leave the fixes tag, but not rush this in.
> >>
> >> So applied to the togreg branch of iio.git and for now just pushed out
> >> as testing for 0-day etc to take a look before I rebase that tree after
> >> rc1.
> >>
> >>
> >>
> >>> ---
> >>>
> >>> I've implemented also a fixup series for supporting rounding of
> >>> gains/scales:
> >>>
> https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gm
> ail.com/
> >>>
> >>> That series does also remove the offending loop.
> >>>
> >>> We don't currently have any in-tree users of GTS helpers which would
> >>> need the rounding support so pushing the rounding is not urgent (and I
> >>> haven't heard of Subjahit whose driver required the rounding). Hence, we
> >>> may want to only take this loop fix in for now (?) and reconsider
> >>> rounding when someone need that.

Why did I look as this crappy code :-)
I think the change breaks the rounding.
For 'normal' values I think you just want:
	return 1 + (max - 1)/scale.

The 'avoid overflow' test isn't needed if you subtract 1 from max.
(Rather than return (max + scale - 1)/scale; where the add can overflow.
But you do need something to return 1 (or error) if max is zero.

	David

> >>>
> >>> Jonathan, what's your take on this?
> >> Agreed - let us wait for the rounding to have a user, but makes sense
> >> to tidy this corner up in the meantime.
> >>
> >> Thanks,
> >>
> >> Jonathan
> >>
> >>>
> >>>   drivers/iio/industrialio-gts-helper.c | 5 ++---
> >>>   1 file changed, 2 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/industrialio-gts-helper.c
> >>> b/drivers/iio/industrialio-gts-helper.c
> >>> index 7653261d2dc2..abcab2d38589 100644
> >>> --- a/drivers/iio/industrialio-gts-helper.c
> >>> +++ b/drivers/iio/industrialio-gts-helper.c
> >>> @@ -34,7 +34,7 @@
> >>>   static int iio_gts_get_gain(const u64 max, const u64 scale)
> >>>   {
> >>>       u64 full = max;
> >>> -    int tmp = 1;
> >>> +    int tmp = 0;
> >>>       if (scale > full || !scale)
> >>>           return -EINVAL;
> >>> @@ -48,8 +48,7 @@ static int iio_gts_get_gain(const u64 max, const
> >>> u64 scale)
> >>>           tmp++;
> >>>       }
> >>> -    while (full > scale * (u64)tmp)
> >>> -        tmp++;
> >>> +    tmp += div64_u64(full, scale);
> >>>       return tmp;
> >>>   }
> >>>
> >>> base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab
> >>
> >>
> > Hi Matti,
> >
> > Your fix works beautifully with the latest version of apds9306 driver
> > which I am working on.
> > All available scale values can be set without any errors. Thank you.
> 
> Thanks for testing Subhajit! Just to ensure we have no miscommunication
> - did you test just this division fix, or the rounding fix here:
> https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gm
> ail.com/
> 
> > Moving to a new city with a new full time job with the assumption of
> > getting more time
> > for my list of opensource projects and contributions proved to be
> > utterly wrong!
> 
> Well, I can't blame you :) Being in a new work at new city sounds like
> you have a lot on your plate right now. Give it half a year and things
> will stabilize though :) Oh, and falsely assuming that "when XXX, I will
> have the time to do YYY" - been there done that :)
> 
> Good luck on the new work and city!
> 
> Yours,
> 	-- Matti
> 
> --
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
> 
> ~~ When things go utterly wrong vim users can always type :help! ~~
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] iio: gts-helper: Fix division loop

[Jonathan Cameron]

On Mon, 22 Jan 2024 16:27:13 +0000
David Laight <David.Laight@ACULAB.COM> wrote:

> From: Matti Vaittinen
> > Sent: 22 January 2024 06:51
> > 
> > On 1/19/24 13:56, Subhajit Ghosh wrote:  
> > > On 8/1/24 02:52, Jonathan Cameron wrote:  
> > >> On Thu, 4 Jan 2024 11:34:28 +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")  
> > >>
> > >> Hmm. Fix or not perf improvement?  I'm going to take the middle ground
> > >> and leave the fixes tag, but not rush this in.
> > >>
> > >> So applied to the togreg branch of iio.git and for now just pushed out
> > >> as testing for 0-day etc to take a look before I rebase that tree after
> > >> rc1.
> > >>
> > >>
> > >>  
> > >>> ---
> > >>>
> > >>> I've implemented also a fixup series for supporting rounding of
> > >>> gains/scales:
> > >>>  
> > https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gm
> > ail.com/  
> > >>>
> > >>> That series does also remove the offending loop.
> > >>>
> > >>> We don't currently have any in-tree users of GTS helpers which would
> > >>> need the rounding support so pushing the rounding is not urgent (and I
> > >>> haven't heard of Subjahit whose driver required the rounding). Hence, we
> > >>> may want to only take this loop fix in for now (?) and reconsider
> > >>> rounding when someone need that.  
> 
> Why did I look as this crappy code :-)
> I think the change breaks the rounding.
> For 'normal' values I think you just want:
> 	return 1 + (max - 1)/scale.
> 
> The 'avoid overflow' test isn't needed if you subtract 1 from max.
> (Rather than return (max + scale - 1)/scale; where the add can overflow.
> But you do need something to return 1 (or error) if max is zero.
> 
> 	David
Too late for my brain to process this, so with an abundance
of caution I've dropped it for now (I'm going to push out as hopefully not
rebasing in a few mins)

J

> 
> > >>>
> > >>> Jonathan, what's your take on this?  
> > >> Agreed - let us wait for the rounding to have a user, but makes sense
> > >> to tidy this corner up in the meantime.
> > >>
> > >> Thanks,
> > >>
> > >> Jonathan
> > >>  
> > >>>
> > >>>   drivers/iio/industrialio-gts-helper.c | 5 ++---
> > >>>   1 file changed, 2 insertions(+), 3 deletions(-)
> > >>>
> > >>> diff --git a/drivers/iio/industrialio-gts-helper.c
> > >>> b/drivers/iio/industrialio-gts-helper.c
> > >>> index 7653261d2dc2..abcab2d38589 100644
> > >>> --- a/drivers/iio/industrialio-gts-helper.c
> > >>> +++ b/drivers/iio/industrialio-gts-helper.c
> > >>> @@ -34,7 +34,7 @@
> > >>>   static int iio_gts_get_gain(const u64 max, const u64 scale)
> > >>>   {
> > >>>       u64 full = max;
> > >>> -    int tmp = 1;
> > >>> +    int tmp = 0;
> > >>>       if (scale > full || !scale)
> > >>>           return -EINVAL;
> > >>> @@ -48,8 +48,7 @@ static int iio_gts_get_gain(const u64 max, const
> > >>> u64 scale)
> > >>>           tmp++;
> > >>>       }
> > >>> -    while (full > scale * (u64)tmp)
> > >>> -        tmp++;
> > >>> +    tmp += div64_u64(full, scale);
> > >>>       return tmp;
> > >>>   }
> > >>>
> > >>> base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab  
> > >>
> > >>  
> > > Hi Matti,
> > >
> > > Your fix works beautifully with the latest version of apds9306 driver
> > > which I am working on.
> > > All available scale values can be set without any errors. Thank you.  
> > 
> > Thanks for testing Subhajit! Just to ensure we have no miscommunication
> > - did you test just this division fix, or the rounding fix here:
> > https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gm
> > ail.com/
> >   
> > > Moving to a new city with a new full time job with the assumption of
> > > getting more time
> > > for my list of opensource projects and contributions proved to be
> > > utterly wrong!  
> > 
> > Well, I can't blame you :) Being in a new work at new city sounds like
> > you have a lot on your plate right now. Give it half a year and things
> > will stabilize though :) Oh, and falsely assuming that "when XXX, I will
> > have the time to do YYY" - been there done that :)
> > 
> > Good luck on the new work and city!
> > 
> > Yours,
> > 	-- Matti
> > 
> > --
> > Matti Vaittinen
> > Linux kernel developer at ROHM Semiconductors
> > Oulu Finland
> > 
> > ~~ When things go utterly wrong vim users can always type :help! ~~
> >   
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

Re: [PATCH] iio: gts-helper: Fix division loop

[Matti Vaittinen]

On 1/22/24 18:27, David Laight wrote:
> From: Matti Vaittinen
>> Sent: 22 January 2024 06:51
>>
>> On 1/19/24 13:56, Subhajit Ghosh wrote:
>>> On 8/1/24 02:52, Jonathan Cameron wrote:
>>>> On Thu, 4 Jan 2024 11:34:28 +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")
>>>>
>>>> Hmm. Fix or not perf improvement?  I'm going to take the middle ground
>>>> and leave the fixes tag, but not rush this in.
>>>>
>>>> So applied to the togreg branch of iio.git and for now just pushed out
>>>> as testing for 0-day etc to take a look before I rebase that tree after
>>>> rc1.
>>>>
>>>>
>>>>
>>>>> ---
>>>>>
>>>>> I've implemented also a fixup series for supporting rounding of
>>>>> gains/scales:
>>>>>
>> https://lore.kernel.org/lkml/37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gm
>> ail.com/
>>>>>
>>>>> That series does also remove the offending loop.
>>>>>
>>>>> We don't currently have any in-tree users of GTS helpers which would
>>>>> need the rounding support so pushing the rounding is not urgent (and I
>>>>> haven't heard of Subjahit whose driver required the rounding). Hence, we
>>>>> may want to only take this loop fix in for now (?) and reconsider
>>>>> rounding when someone need that.
> 
> Why did I look as this crappy code :-)

Well, we all make mistakes, don't we? ;) Thanks for looking at it though!

> I think the change breaks the rounding.

This I do disagree with. AFAICS, the original code does not do any 
rounding either. This specific patch was just intended for removing the 
loop. It did not add or remove the rounding.

> For 'normal' values I think you just want:
> 	return 1 + (max - 1)/scale.

I am sorry but I think I got a bit lost. What we currently have in-tree 
is just an integer division. 64bit / 64bit, where no rounding whatsoever 
is done. It is implemented with a loop - hence the "funny" if - condition.

What I wanted to do with this patch was to just replace the loop.

I think the "return 1 + (max - 1)/scale" is not equivalent?

The driver submitted by Subjahit was first user which used scale values 
which did not match exact integer gains. This is because he used 
illuminance channel with the real world Lux values :) So, in order to 
avoid manual 'change scale values to be exact multiplies of the gains' 
work in driver, I thought of doing the regular rounding in these 
helpers. This, however, is not done in this specific patch.

Even if it was, I am not sure "return 1 + (max - 1)/scale" would be 
correct? If we have for example max = 7, scale = 6, division
7/6 = 1.166666..., which rounds down to 1.

However, 1 + (7 - 1) / 6
=> 1 + 6 / 6
=> 2.

> The 'avoid overflow' test isn't needed if you subtract 1 from max.

Ah, yes. I think our confusion comes from that "overflow"-test. It 
indeed is no longer necessary as we no longer need to add anything to 
the 'max'. This whole function renders to simple:

if (scale > full || !scale)
	return -EINVAL;

return div64_u64(full, scale);

unless we implement the proper rounding. should've noticed this. Thanks 
for the heads-up!

> (Rather than return (max + scale - 1)/scale; where the add can overflow.
> But you do need something to return 1 (or error) if max is zero.
> 
> 	David



-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~