linux-kernel - Re: [PATCH v5 1/3] lib: add linear range get selector within

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8d5eb59bf3a2054e945d6c3c0203be529b61f8ec.camel@fi.rohmeurope.com>
Date:   Fri, 28 May 2021 09:02:42 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     "gene.chen.richtek@...il.com" <gene.chen.richtek@...il.com>,
        "sre@...nel.org" <sre@...nel.org>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>
CC:     "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "inux-pm@...r.kernel.org" <inux-pm@...r.kernel.org>,
        "shufan_lee@...htek.com" <shufan_lee@...htek.com>,
        "cy_huang@...htek.com" <cy_huang@...htek.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "gene_chen@...htek.com" <gene_chen@...htek.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "Wilma.Wu@...iatek.com" <Wilma.Wu@...iatek.com>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "benjamin.chao@...iatek.com" <benjamin.chao@...iatek.com>
Subject: Re: [PATCH v5 1/3] lib: add linear range get selector within

On Fri, 2021-05-28 at 16:12 +0800, Gene Chen wrote:
> From: Gene Chen <gene_chen@...htek.com>
> 
> Add linear range get selector within for choose closest selector
> between minimum and maximum selector.
> 
> Signed-off-by: Gene Chen <gene_chen@...htek.com>
> ---
>  include/linux/linear_range.h |  2 ++
>  lib/linear_ranges.c          | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/include/linux/linear_range.h
> b/include/linux/linear_range.h
> index 17b5943727d5..fd3d0b358f22 100644
> --- a/include/linux/linear_range.h
> +++ b/include/linux/linear_range.h
> @@ -41,6 +41,8 @@ int linear_range_get_selector_low(const struct
> linear_range *r,
>  int linear_range_get_selector_high(const struct linear_range *r,
>  				   unsigned int val, unsigned int
> *selector,
>  				   bool *found);
> +void linear_range_get_selector_within(const struct linear_range *r,
> +				      unsigned int val, unsigned int
> *selector);
>  int linear_range_get_selector_low_array(const struct linear_range
> *r,
>  					int ranges, unsigned int val,
>  					unsigned int *selector, bool
> *found);
> diff --git a/lib/linear_ranges.c b/lib/linear_ranges.c
> index ced5c15d3f04..a1a7dfa881de 100644
> --- a/lib/linear_ranges.c
> +++ b/lib/linear_ranges.c
> @@ -241,5 +241,36 @@ int linear_range_get_selector_high(const struct
> linear_range *r,
>  }
>  EXPORT_SYMBOL_GPL(linear_range_get_selector_high);
>  
> +/**
> + * linear_range_get_selector_within - return linear range selector
> for value
> + * @r:		pointer to linear range where selector is
> looked from
> + * @val:	value for which the selector is searched
> + * @selector:	address where found selector value is updated
> + *
> + * Return selector for which range value is closest match for given
> + * input value. Value is matching if it is equal or lower than given
> + * value. But return maximum selector if given value is higher than
> + * maximum value.
> + */
> +void linear_range_get_selector_within(const struct linear_range *r,
> +				      unsigned int val, unsigned int
> *selector)

I like the naming! The "within" sounds good to me :)
It slightly bothers my "style eye" to see this not returning a value
(void) - but still returning the value via parameter (selector). It may
be slightly more complicated to read when used.

Please consider for first time reading code like:
unsigned int do_something(const struct linear_range *r, unsigned int
val, int *myvalue)
{
	int ret;

	...

	linear_range_get_selector_within(r, val, myvalue);

	...

	return ret;
}

to reading code like:
unsigned int do_something(const struct linear_range *r, unsigned int
val, int *myvalue)
{
	int ret;

	...

	*myvalue = linear_range_get_selector_within(r, val);

	...

	return ret;
}

For me reading the latter shows better where the myvalue is really set.

OTOH, Your suggestion is consistent with the other functions so I am
not asking for a change if this is Ok with others :) Thanks a lot!

Reviewed-by: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>

> +{
> +	if (r->min > val) {
> +		*selector = r->min_sel;
> +		return;
> +	}
> +
> +	if (linear_range_get_max_value(r) < val) {
> +		*selector = r->max_sel;
> +		return;
> +	}
> +
> +	if (r->step == 0)
> +		*selector = r->min_sel;
> +	else
> +		*selector = (val - r->min) / r->step + r->min_sel;
> +}
> +EXPORT_SYMBOL_GPL(linear_range_get_selector_within);
> +
>  MODULE_DESCRIPTION("linear-ranges helper");
>  MODULE_LICENSE("GPL");