Re: [SPF Softfail] Re: [PATCH v7 03/10] lib: add linear ranges helpers

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

Hello Andy,

On Wed, 2020-04-01 at 17:42 +0300, Andy Shevchenko wrote:
> On Wed, Apr 1, 2020 at 4:18 PM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > On Wed, 2020-04-01 at 15:39 +0300, Andy Shevchenko wrote:
> > > On Wed, Apr 1, 2020 at 1:09 PM Vaittinen, Matti
> > > <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > > > On Tue, 2020-03-31 at 17:05 +0300, Andy Shevchenko wrote:
> > > > > On Tue, Mar 31, 2020 at 03:23:03PM +0300, Matti Vaittinen
> > > > > wrote:
> > > > > > +int linear_range_get_selector_low_array(const struct
> > > > > > linear_range
> > > > > > *r,
> > > > > > +                                   int ranges, unsigned
> > > > > > int
> > > > > > val,
> > > > > > +                                   unsigned int *selector,
> > > > > > bool
> > > > > > *found)
> > > > > > +{
> > > > > > +   int i;
> > > > > > +   int ret = -EINVAL;
> > > > > > +
> > > > > > +   for (i = 0; i < ranges; i++) {
> > > > > > +           int tmpret;
> > > > > > +
> > > > > > +           tmpret = linear_range_get_selector_low(&r[i],
> > > > > > val,
> > > > > > selector,
> > > > > > +                                                  found);
> > > > > > +
> > > > > > +           if (!tmpret)
> > > > > > +                   ret = 0;
> > > > > > +
> > > > > > +           if (*found)
> > > > > > +                   break;
> > > > > > +   }
> > > > > > +
> > > > > > +   return ret;
> > > > > > +}
> 
> Looked again at the code of the callee.
> So, *found becomes true if and only if the return is 0

Yes. If found is true, then ret is 0.

>  (or other way
> around if you prefer).

No. Not other way around. Ret can be 0 and found false. This is what we
need to handle. Ret means we found range value smaller than given
input. Found means also the given input was in range.

> Now I'm wondering why you need 'found' at all?

To separate case where given input was in range.

> It means above may be as simple as
> 
>       int i;
>       int ret = -EINVAL;
> 
>       for (i = 0; i < ranges; i++) {
>                ret = linear_range_get_selector_low(&r[i], val,
> selector, found);
>                if (*found)
>                        break;
>       }

No. It can't. Here we get wrong return value if one of the calls to
linear_range_get_selector_low() return zero with found being false and
subsequent calls return non zero ret. Then we actually found lower-
than-given-input value from one of the ranges and updated the selector
(that would be usable for example for regulator voltage control) but
end up returning non zero ret as the subsequent calls to
linear_range_get_selector_low() do overwrite the ret.

OTOH, we should not stop for first range having zero ret if found is
false because some of the ranges may have range where given input is
in-range (and this is considered better value).

> 
>       return ret;
> 
> or assuming 'found' will gone

No. We should not drop the 'found'.

> 
>   int i
>   int ret = -EINVAL;
> 
>   for (i = 0; i < ranges && ret; i++) {
>       ret = linear_range_get_selector_low(&r[i], val, selector);
>   }
>   return ret;
> 
> ...
> 
> >  * value. If given value is found to be in a range scannins is
> > I think it is but I am open to all suggestions how to improve doc!
> 
> Thanks. At least fix a typo: scannins -> scannings
> 

Thanks :) I'll fix it

> ...
> 
> > Compared to:
> > for (i = 0; i < ranges; i++) {
> >         ret = ...
> > }
> > 
> > I wouldn't say so.
> > 
> > As I explained, we need to have "temporary" return value in any
> > case
> > because we need to return 0 if any of the calls to
> > linear_range_get_selector_low() returned 0. Return value 0 from
> > linear_range_get_selector_low() means we found "matching" value
> > (lower
> > than input) and selector was updated (although input value was not
> > in
> > range).
> 
> See above.


Best Regards
	--Matti
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek