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 <[email protected]>
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/[email protected]/
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
--
2.43.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 =]
On Mon, 12 Feb 2024 13:20:09 +0200
Matti Vaittinen <[email protected]> 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 <[email protected]>
> 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/[email protected]/
>
> 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 <[email protected]> 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 <[email protected]>
>> 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/[email protected]/
>>
>> 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 <[email protected]> wrote:
> On 17/2/24 00:28, Jonathan Cameron wrote:
> > On Mon, 12 Feb 2024 13:20:09 +0200
> > Matti Vaittinen <[email protected]> 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 <[email protected]>
> >> 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/[email protected]/
> >>
> >> 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 <[email protected]> wrote:
>
>> On 17/2/24 00:28, Jonathan Cameron wrote:
>>> On Mon, 12 Feb 2024 13:20:09 +0200
>>> Matti Vaittinen <[email protected]> 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 <[email protected]>
>>>> 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/[email protected]/
>>>>
>>>> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>> 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
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
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 <[email protected]> wrote:
>>
>>> On 17/2/24 00:28, Jonathan Cameron wrote:
>>>> On Mon, 12 Feb 2024 13:20:09 +0200
>>>> Matti Vaittinen <[email protected]> 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 <[email protected]>
>>>>> 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 <[email protected]>
Thank you Subhajit! Your effort is _very much_ appreciated! :)
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Mon, 19 Feb 2024 09:22:24 +0200
Matti Vaittinen <[email protected]> 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 <[email protected]> wrote:
> >>
> >>> On 17/2/24 00:28, Jonathan Cameron wrote:
> >>>> On Mon, 12 Feb 2024 13:20:09 +0200
> >>>> Matti Vaittinen <[email protected]> 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 <[email protected]>
> >>>>> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]> wrote:
>>>>
>>>>> On 17/2/24 00:28, Jonathan Cameron wrote:
>>>>>> On Mon, 12 Feb 2024 13:20:09 +0200
>>>>>> Matti Vaittinen <[email protected]> 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 <[email protected]>
>>>>>>> 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 <[email protected]>
>>
>> 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