Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Vincent Mailhol]

On 03/02/2025 at 22:59, Geert Uytterhoeven wrote:
> Hi Vincent,
> 
> On Mon, 3 Feb 2025 at 14:37, Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> wrote:
>> On 03/02/2025 at 16:44, Johannes Berg wrote:
>>> On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote:
>>>>> Instead of creating another variant for
>>>>> non-constant bitfields, wouldn't it be better to make the existing macro
>>>>> accept both?
>>>>
>>>> Yes, it would definitely be better IMO.
>>>
>>> On the flip side, there have been discussions in the past (though I
>>> think not all, if any, on the list(s)) about the argument order. Since
>>> the value is typically not a constant, requiring the mask to be a
>>> constant has ensured that the argument order isn't as easily mixed up as
>>> otherwise.
>>
>> If this is a concern, then it can be checked with:
>>
>>   BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) &&
>>                    __builtin_constant_p(_val),
>>                    _pfx "mask is not constant");
>>
>> It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow
>> any other combination.
> 
> Even that case looks valid to me. Actually there is already such a user
> in drivers/iio/temperature/mlx90614.c:
> 
>     ret |= field_prep(chip_info->fir_config_mask, MLX90614_CONST_FIR);
> 
> So if you want enhanced safety, having both the safer/const upper-case
> variants and the less-safe/non-const lower-case variants makes sense.

So, we are scared of people calling FIELD_PREP() with the arguments in
the wrong order:

  FIELD_PREP(val, mask)

thus adding the check that mask must be a compile time constant.

But if we introduce a second function, don't we introduce the risk of
having people use the lower case variant instead of the upper case variant?

  field_prep(incorrect_const_mask, val)

I am not sure to follow the logic of why having two functions is the
safer choice. Whatever the solution you propose, there will be a way to
misuse it. Let me ask, what is the most likely to happen:

  1. wrong parameter order
  2. wrong function name

?

If you have the conviction that people more often do mistake 1. then I
am fine with your solution. Otherwise, if 1. and 2. have an equally
likelihood, then I would argue to go with the simplicity of the single
function.


Yours sincerely,
Vincent Mailhol