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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 22 Jun 2023 14:32:33 +0200
From:   Herve Codina <herve.codina@...tlin.com>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        "Kuninori Morimoto" <kuninori.morimoto.gx@...esas.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v5 07/13] minmax: Introduce {min,max}_array()

Hi David, Andy

On Tue, 20 Jun 2023 11:45:01 +0000
David Laight <David.Laight@...LAB.COM> wrote:

> From: Herve Codina
> > Sent: 15 June 2023 16:26
> > 
> > Introduce min_array() (resp max_array()) in order to get the
> > minimal (resp maximum) of values present in an array.
> > 
> > Signed-off-by: Herve Codina <herve.codina@...tlin.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
> > ---
> >  include/linux/minmax.h | 64 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> > 
> > diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> > index 396df1121bff..1672985b02a3 100644
> > --- a/include/linux/minmax.h
> > +++ b/include/linux/minmax.h
> > @@ -133,6 +133,70 @@
> >   */
> >  #define max_t(type, x, y)	__careful_cmp((type)(x), (type)(y), >)
> > 
> > +/*
> > + * Remove a const qualifier from integer types
> > + * _Generic(foo, type-name: association, ..., default: association) performs a
> > + * comparison against the foo type (not the qualified type).
> > + * Do not use the const keyword in the type-name as it will not match the
> > + * unqualified type of foo.
> > + */
> > +#define __unconst_integer_type_cases(type)	\
> > +	unsigned type:  (unsigned type)0,	\
> > +	signed type:    (signed type)0
> > +
> > +#define __unconst_integer_typeof(x) typeof(			\
> > +	_Generic((x),						\
> > +		char: (char)0,					\
> > +		__unconst_integer_type_cases(char),		\
> > +		__unconst_integer_type_cases(short),		\
> > +		__unconst_integer_type_cases(int),		\
> > +		__unconst_integer_type_cases(long),		\
> > +		__unconst_integer_type_cases(long long),	\
> > +		default: (x)))  
> 
> Those are probably more generally useful and belong elsewhere.

Yes but it is only used here.
It can be move somewhere, in a common place, when necessary.

> 
> > +
> > +/*
> > + * Do not check the array parameter using __must_be_array().
> > + * In the following legit use-case where the "array" passed is a simple pointer,
> > + * __must_be_array() will return a failure.
> > + * --- 8< ---
> > + * int *buff
> > + * ...
> > + * min = min_array(buff, nb_items);
> > + * --- 8< ---  
> 
> Is that needed in the .h file?
> 
> > + *
> > + * The first typeof(&(array)[0]) is needed in order to support arrays of both
> > + * 'int *buff' and 'int buf[N]' types.
> > + *
> > + * The array can be an array of const items.
> > + * typeof() keeps the const qualifier. Use __unconst_typeof() in order to
> > + * discard the const qualifier for the __element variable.
> > + */
> > +#define __minmax_array(op, array, len) ({				\
> > +	typeof(&(array)[0]) __array = (array);				\
> > +	typeof(len) __len = (len);					\
> > +	__unconst_integer_typeof(__array[0]) __element = __array[--__len]; \  
> 
> s/__element/__bound/
> 
> > +	while (__len--)							\
> > +		__element = op(__element, __array[__len]);		\
> > +	__element; })  
> 
> I'm not all sure that all the shenanigans required to use min()
> is really needed here.
> 
> It would also be generally better to process the array forwards.
> So something like:
> 	typeof (&array[0]) __ptr = array, __limit = array + len;
> 	typeof (array[0] + 0) __element, __bound = *__ptr++;
> 	while (ptr < __limit) {
> 		__element = *__ptr++;
> 		if (__element > __bound)
> 			__bound = __element;
> 	}
> 	(typeof (array[0]))__bound; })
> seems fine to me.
> The final cast is there to convert 'int' back to un/signed char|short.
> Not really needed and might generate worse code.
> 
> But if you insist on using min/max ignore this bit.

I didn't plan to change the {min,max}_array() macros in this series as you
suggest.

Maybe min()/max() is too strict but it's a way to be sure about the type
used. Also the current version doesn't need any extra cast to get rid of
the integer promotion as the integer promotion doesn't occur.

Is it ok for you if we keep as it ?

Thanks for your feedback,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ