Message-ID: <CADEbmW0BZbJQbaycqq+vfisEJyGd57uzEgFEckusGLJSLvmYDA@mail.gmail.com>
Date: Tue, 21 May 2024 16:44:33 +0200
From: Michal Schmidt <mschmidt@...hat.com>
To: Alex Elder <elder@...aro.org>
Cc: Yury Norov <yury.norov@...il.com>, Rasmus Villemoes <linux@...musvillemoes.dk>, 
	linux-kernel@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>, 
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] bitfield.h: add FIELD_MAX_CONST

On Tue, May 21, 2024 at 2:33 PM Alex Elder <elder@...aro.org> wrote:
> On 5/20/24 2:29 PM, Yury Norov wrote:
> > + Alex Elder <elder@...aro.org>, Jakub Kicinski <kuba@...nel.org> and
> > David S. Miller <davem@...emloft.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.

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 <mschmidt@...hat.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);

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
>

Powered by blists - more mailing lists