Received-SPF: pass (google.com: domain of linux-kernel+bounces-184953-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <3f3b75df-3488-4915-bc21-54cb6a6e2a74@linaro.org>
Date: Tue, 21 May 2024 07:33:46 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] bitfield.h: add FIELD_MAX_CONST
To: Yury Norov <yury.norov@gmail.com>, Michal Schmidt <mschmidt@redhat.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>,
 linux-kernel@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>,
 "David S. Miller" <davem@davemloft.net>
References: <20240515172732.288391-1-mschmidt@redhat.com>
 <Zkuksm3K+pKugjgF@yury-ThinkPad>
Content-Language: en-US
From: Alex Elder <elder@linaro.org>
In-Reply-To: <Zkuksm3K+pKugjgF@yury-ThinkPad>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 5/20/24 2:29 PM, Yury Norov wrote:
> + Alex Elder <elder@linaro.org>, Jakub Kicinski <kuba@kernel.org> and
> David S. Miller <davem@davemloft.net>

Thanks for adding me to this.

My bottom line response is that I don't understand exactly
what problem this is solving (but I trust it solves a
problem for you).  It *seems* like the existing macro(s)
should work for you, and if they don't, you might not be
using it (them) correctly.  And... if a fix is needed, it
should be made to the existing macro if possible.

> On Wed, May 15, 2024 at 07:27:31PM +0200, Michal Schmidt wrote:
>> FIELD_MAX_CONST is like FIELD_MAX, but it can be used where statement
>> expressions are forbidden. For example, using FIELD_MAX in a
>> static_assert gives:
>>    error: braced-group within expression allowed only inside a function

So you want to use FIELD_MAX() in the outer scope in a file,
not within a function?  And the way it's currently defined
doesn't permit that?

>> It can be used also in array declarations, where using FIELD_MAX would
>> trigger a warning :
>>    warning: ISO C90 forbids variable length array ‘buf’ [-Wvla]

Are you passing a constant to FIELD_MAX() when computing the
array size?  If so, I don't understand this warning.

>> (It's a bit surprising, because despite the warning, gcc calculated
>> the array size correctly at compile time.)
>>
>> A simplified example of what I actually want to use in a driver:
>>    #define DATA_SIZE_M GENMASK(3, 0)
>>    #define MAX_DATA_SIZE FIELD_MAX_CONST(DATA_SIZE_M)

FIELD_MAX() returns the maximum representable value, not the
number of possible values.

>>    static void f(void) {
>>    	char buf[MAX_DATA_SIZE];
>>    	/* ... */
>>    }
>>
>> In the implementation, reuse the existing compile-time checks from
>> FIELD_PREP_CONST.
>>
>> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> 
> Hi Michal,
> 
> So... FIELD_MAX() already requires the _mask to be a const value.
> Now you add a FIELD_MAX_CONST(), which makes it more confusing.

Yes, it's not clear when you would use one rather than the other.
I think a better fix is to fix the existing FIELD_MAX() (and
field_max(), defined later in that file).

> It looks like your new _CONST() macro would work anywhere where the
> existing FIELD_MAX() works. At least for me, if I apply your patch
> and do:
> 
>          #define FIELD_MAX FIELD_MAX_CONST
> 
> The implementation of the 'const' version looks the same as the
> 'variable' one, except for that sanity checking business.
> 
> I think the right path to go would be making the __BF_FIELD_CHECK()
> a structure initializers friendly by using the BUILD_BUG_ON_ZERO(),
> just like you did with the __BF_FIELD_CHECK_CONST(), so that the
> FIELD_MAX() would work in all cases.

I haven't investigated this much further, but yes, I agree that
you should fix what's there to work the way you like if possible,
while preserving all guarantees provided before.

Still, I'll provide some comments on the patch below.

> Thanks,
> Yury
> 
>> ---
>>   include/linux/bitfield.h | 34 +++++++++++++++++++++++++++-------
>>   1 file changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
>> index 63928f173223..50bbab317319 100644
>> --- a/include/linux/bitfield.h
>> +++ b/include/linux/bitfield.h
>> @@ -76,6 +76,16 @@
>>   					      (1ULL << __bf_shf(_mask))); \
>>   	})
>>   
>> +#define __BF_FIELD_CHECK_CONST(_mask, _val)				\
>> +	(								\
>> +		/* mask must be non-zero */				\
>> +		BUILD_BUG_ON_ZERO((_mask) == 0) +			\

This ^^^ implements the opposite of what the comment says.  Use:
		BUILD_BUG_ON_ZERO(_mask);

Also, why are you adding?  The way this macro is defined, it
really should return Boolean, but you're returning an integer
sum.

>> +		/* check if value fits */				\
>> +		BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \
>> +		/* check if mask is contiguous */			\
>> +		__BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask)))	\

I don't really see why __BUILD_BUG_ON_NOT_POWER_OF_2() isn't used
here (and in FIELD_PREP_CONST() for that matter--other than line
length).

Each of the above checks can stand alone, and if they all pass,
you can simply return true (or return the result of the last
check, but I really think they should be treated as void type).

>> +	)
>> +
>>   /**
>>    * FIELD_MAX() - produce the maximum value representable by a field
>>    * @_mask: shifted mask defining the field's length and position
>> @@ -89,6 +99,22 @@
>>   		(typeof(_mask))((_mask) >> __bf_shf(_mask));		\
>>   	})
>>   
>> +/**
>> + * FIELD_MAX_CONST() - produce the maximum value representable by a field
>> + * @_mask: shifted mask defining the field's length and position
>> + *
>> + * FIELD_MAX_CONST() returns the maximum value that can be held in
>> + * the field specified by @_mask.

I don't think this part of the description adds much; it
basically restates what the one-liner does.

					-Alex

>> + *
>> + * Unlike FIELD_MAX(), it can be used where statement expressions can't.
>> + * Error checking is less comfortable for this version.
>> + */
>> +#define FIELD_MAX_CONST(_mask)						\
>> +	(								\
>> +		__BF_FIELD_CHECK_CONST(_mask, 0ULL) +			\
>> +		(typeof(_mask))((_mask) >> __bf_shf(_mask))		\
>> +	)
>> +
>>   /**
>>    * FIELD_FIT() - check if value fits in the field
>>    * @_mask: shifted mask defining the field's length and position
>> @@ -132,13 +158,7 @@
>>    */
>>   #define FIELD_PREP_CONST(_mask, _val)					\
>>   	(								\
>> -		/* mask must be non-zero */				\
>> -		BUILD_BUG_ON_ZERO((_mask) == 0) +			\
>> -		/* check if value fits */				\
>> -		BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \
>> -		/* check if mask is contiguous */			\
>> -		__BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))) +	\
>> -		/* and create the value */				\
>> +		__BF_FIELD_CHECK_CONST(_mask, _val) +			\
>>   		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
>>   	)
>>   
>> -- 
>> 2.44.0