| Message ID | ZZZ7pJBGkTdFFqiY@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: gts-helper: Fix division loop | expand |
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
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!
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
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
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
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)
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)
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
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
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
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; }
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(-) base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab