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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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:

...

> > > + * It might be useful if we could support also inversely
> > > proportional ranges?
> >
> > Looks like remark that should not be here, rather in commit message
> > or even in cover letter.
>
> I think this is a good place so that anyone who opens this file will
> see what could be done to improve this. No one is looking at the old
> commit messages unless they face some problems. And cover letters fade
> away even faster - although the cover letter and commit messages may
> catch some attention during the patch submissions.
>
> So maybe you would agree with me if I added this also in cover letter
> and commit message :)

OK

...

> > > +int linear_range_get_selector_low(const struct linear_range *r,
> > > +                             unsigned int val, unsigned int
> > > *selector,
> > > +                             bool *found)

> > As far as I can see this is a bit different from _high counterpart.
>
> Yes. Because the logic is different. _low accepts selector for a
> closest value which is smaller or equal to given. _high accepts
> selector for a closest value which is higher or equal to given. This
> mean that both the _high and _low accept also input value which is not
> in the range - _low accepts input which is higher than range, _high
> accepts input which is lower than the range.

Thanks for elaboration.

> > > +   if (!r->step)
> >
> > Why not positive conditional?
>
> because we really want to test if step is zero.

> I can write it as
> if (r->step == 0) if that is preferred.

Yes, please. Then we will see that 0 has special meaning here.

>  But we definitely want to chek
> if r->step is zero and handle it as exception case. (We want to protect
> from division by zero - and in that case all selectors are Ok as range
> is constant)
>
>
> > > +           *selector = r->min_sel;
> > > +   else
> > > +           *selector = (val - r->min) / r->step + r->min_sel;
> > > +   return 0;
> > > +}

...

> > > +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.
Is this logic described in the description? Sorry, I forgot, although
I had read it.
If no, perhaps it needs to be added either to description or as a
comment to the loop.

> > 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.

-- 
With Best Regards,
Andy Shevchenko

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