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
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]
(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)
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 <[email protected]>
---
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) + \
+ /* 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))) \
+ )
+
/**
* 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.
+ *
+ * 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
+ Alex Elder <[email protected]>, Jakub Kicinski <[email protected]> and
David S. Miller <[email protected]>
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
>
> 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]
> (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)
> 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 <[email protected]>
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.
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.
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) + \
> + /* 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))) \
> + )
> +
> /**
> * 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.
> + *
> + * 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
On Mon, 20 May 2024 12:29:54 -0700 Yury Norov wrote:
> > 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)
> > static void f(void) {
> > char buf[MAX_DATA_SIZE];
You need a "+ 1" somewhere here, right?
> > }
> >
> > In the implementation, reuse the existing compile-time checks from
> > FIELD_PREP_CONST.
> >
> > Signed-off-by: Michal Schmidt <[email protected]>
>
> 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.
+1, I think this is doing too much in general.
On 5/20/24 2:29 PM, Yury Norov wrote:
> + Alex Elder <[email protected]>, Jakub Kicinski <[email protected]> and
> David S. Miller <[email protected]>
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 <[email protected]>
>
> 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
On Tue, May 21, 2024 at 2:33 PM Alex Elder <[email protected]> wrote:
> On 5/20/24 2:29 PM, Yury Norov wrote:
> > + Alex Elder <[email protected]>, Jakub Kicinski <[email protected]> and
> > David S. Miller <[email protected]>
>
> 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.
Yury, Jakub, Alex,
thanks for your reviews so far.
All of you want to avoid adding another macro. I agree and I will be back
with v2.
To clarify where exactly I ran into the current limitations of FIELD_MAX:
I am reworking drivers/net/ethernet/intel/ice/ice_gnss.c:ice_gnss_read().
There, I will be changing "buf" to a small on-stack array:
char buf[ICE_MAX_I2C_DATA_SIZE];
where ICE_MAX_I2C_DATA_SIZE is defined using FIELD_MAX.
There are a few more issues. I extracted them into this test case that
I would like to make compilable:
=======================================================
#define TEST_REG_1 GENMASK(3,0)
#define TEST_MAX_1 FIELD_MAX(TEST_REG_1)
#define TEST_REG_2 GENMASK(BITS_PER_LONG - 1, BITS_PER_LONG - 2)
#define TEST_MAX_2 FIELD_MAX(TEST_REG_2)
/* Using FIELD_MAX inside a static_assert yields:
* error: braced-group within expression allowed only inside a function
*/
static_assert(TEST_MAX_1 == 15);
/* Even after the above is solved, using a mask that has the highest bit set
* will expose another issue:
* error: bit-field ‘<anonymous>’ width not an integer constant
* This one *might* be a gcc bug triggered by -fsanitize=shift.
* It does not appear with clang.
* Defining __bf_shf as __builtin_ctzll(x) instead of __builtin_ffsll(x)-1 can
* work around it, apparently.
*/
static_assert(TEST_MAX_2 == 3);
int test_field_max_array(void);
int test_field_max_array(void)
{
/* Using FIELD_MAX for sizing a local array yields:
* error: ISO C90 forbids variable length array ‘buf’ [-Werror=vla]
*/
char buf[TEST_MAX_1];
/* This line compiles OK in the current implementation and must keep
* working.
*/
WARN_ON(TEST_MAX_2 > 3);
return sizeof(buf);
}
=======================================================
> > 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?
Right. Although, I don't *really* need to use it in outer scope.
It would be just nice to have. I just need the next thing:
> >> 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.
Yes, the array is an automatic variable and its size is calculated with
FIELD_MAX from a constant GENMASK. Indeed, the warning is surprising.
> >> (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 <[email protected]>
> >
> > 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);
No, I think you're misunderstanding what BUILD_BUG_ON_ZERO does.
It does not mean "BUG if the argument is zero".
It means "BUG if the argument is true. Otherwise, the value is 0."
> 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).
Note that I did not come up with the addition pattern. I just moved it
from FIELD_PREP_CONST. Its purpose is to avoid using a statement expression,
because that has certains limitations on where it can be used.
> >> + )
> >> +
> >> /**
> >> * 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.
Right.
Anyway, v2 won't add the new macro.
Thanks!
Michal
> -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
>
On Tue, May 21, 2024 at 04:44:33PM +0200, Michal Schmidt wrote:
> On Tue, May 21, 2024 at 2:33 PM Alex Elder <[email protected]> wrote:
> > On 5/20/24 2:29 PM, Yury Norov wrote:
> > > + Alex Elder <[email protected]>, Jakub Kicinski <[email protected]> and
> > > David S. Miller <[email protected]>
> >
> > 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.
>
> Yury, Jakub, Alex,
> thanks for your reviews so far.
>
> All of you want to avoid adding another macro. I agree and I will be back
> with v2.
>
> To clarify where exactly I ran into the current limitations of FIELD_MAX:
> I am reworking drivers/net/ethernet/intel/ice/ice_gnss.c:ice_gnss_read().
> There, I will be changing "buf" to a small on-stack array:
> char buf[ICE_MAX_I2C_DATA_SIZE];
> where ICE_MAX_I2C_DATA_SIZE is defined using FIELD_MAX.
>
> There are a few more issues. I extracted them into this test case that
> I would like to make compilable:
Whatever you make with this, please add a 2nd patch with a test(s)
covering your cases. You can refer the test_bitmap_const_eval() in
lib/test_bitmap.c for an examples of how the compile-time expressions
are tested. The best place for the test would be lib/test_bitops, I
think.
Thanks,
Yury