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

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

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;
> > > > +}
> > > 
> > > Can we refactor this?
> > > 
> > >       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;
> > >       }
> > > 
> > >       return break;
> > > 
> > > This will unshadow the error code returned by the loop body.
> > 
> > What is the return break; doing here? I don't understand the
> > syntax.
> 
> Typo. return ret; should be.
> 
> > And we want the logic to be such that we return zero from this
> > function
> > if _any_ of the calls to linear_range_get_selector_low returned
> > zero.
> > (Even if further calls would return non-zero value). But we wan't
> > to
> > keep looking for better (in-range) match if *found is not true.
> 
> won't or want to? I'm confused with the last.

want :) Typo here too.

> Is this logic described in the description? Sorry, I forgot, although
> I had read it.

 * Scan array of ranges for selector which which range value matches
given
 * input value. Value is matching if it is equal or smaller than
given
 * value. If given value is found to be in a range scannins is
stopped and
 * @found is set true. If a range with values smaller than
given value is found
 * but the range max is being smaller than given
value, then the ranges
 * biggest selector is updated to @selector but
scanning ranges is continued
 * and @found is set to false.

I think it is but I am open to all suggestions how to improve doc!

> > > Or if ranges is guaranteed to be always positive number, convert
> > > this
> > > to do {} while.
> > 
> > I don't see the benefit :/
> 
> I see.
>  do {
>    ret = ...
>  } while (++i < ranges && *found == false);
> 
> much better to read.

Huh?
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).


Br,
	Matti Vaittinen
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek