[PATCH] bitfield.h: add FIELD_MAX() and field_max()

[PATCH] bitfield.h: add FIELD_MAX() and field_max()

[Alex Elder]

Define FIELD_MAX(), which supplies the maximum value that can be
represented by a field value.  Define field_max() as well, to go
along with the lower-case forms of the field mask functions.

Signed-off-by: Alex Elder <elder@linaro.org>
---

 NOTE:	I'm not entirely sure who owns/maintains this file so
	I'm sending it to those who have committed things to it.
	I hope someone will just take it in after review; I use
	field_max() in some code I'm about to send out.

				-Alex

 include/linux/bitfield.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 4bbb5f1c8b5b..48ea093ff04c 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -55,6 +55,19 @@
 					      (1ULL << __bf_shf(_mask))); \
 	})
 
+/**
+ * FIELD_MAX() - produce the maximum value representable by a field
+ * @_mask: shifted mask defining the field's length and position
+ *
+ * FIELD_MAX() returns the maximum value that can be held in the field
+ * specified by @_mask.
+ */
+#define FIELD_MAX(_mask)						\
+	({								\
+		__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: ");	\
+		(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
@@ -110,6 +123,7 @@ static __always_inline u64 field_mask(u64 field)
 {
 	return field / field_multiplier(field);
 }
+#define field_max(field)	((typeof(field))field_mask(field))
 #define ____MAKE_OP(type,base,to,from)					\
 static __always_inline __##type type##_encode_bits(base v, base field)	\
 {									\
-- 
2.20.1

Re: [PATCH] bitfield.h: add FIELD_MAX() and field_max()

[Jakub Kicinski]

On Fri, 28 Feb 2020 10:53:43 -0600 Alex Elder wrote:
> Define FIELD_MAX(), which supplies the maximum value that can be
> represented by a field value.  Define field_max() as well, to go
> along with the lower-case forms of the field mask functions.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
> 
>  NOTE:	I'm not entirely sure who owns/maintains this file so
> 	I'm sending it to those who have committed things to it.
> 	I hope someone will just take it in after review; I use
> 	field_max() in some code I'm about to send out.

Could you give us an example use?

Is it that you find the current macros misnamed or there's something
you can't do? Or are you trying to set fields to max?

Re: [PATCH] bitfield.h: add FIELD_MAX() and field_max()

[Alex Elder]

On 2/28/20 11:56 AM, Jakub Kicinski wrote:
> On Fri, 28 Feb 2020 10:53:43 -0600 Alex Elder wrote:
>> Define FIELD_MAX(), which supplies the maximum value that can be
>> represented by a field value.  Define field_max() as well, to go
>> along with the lower-case forms of the field mask functions.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>
>>  NOTE:	I'm not entirely sure who owns/maintains this file so
>> 	I'm sending it to those who have committed things to it.
>> 	I hope someone will just take it in after review; I use
>> 	field_max() in some code I'm about to send out.
> 
> Could you give us an example use?
> 
> Is it that you find the current macros misnamed or there's something
> you can't do? Or are you trying to set fields to max?

I'm trying to validate variable values are in range before attempting
to use them in a bitfield.


I find field_max() to be a good name for what I'm looking for.

					-Alex

#define FOO_FMASK	0x000ff000

static u32 register = 0x12345678;

int foo(u32 value)
{
	if (value > field_max(FOO_FMASK))
		return -EINVAL;

	u32_replace_bits(&register, value, FOO_FMASK);
	
	return 0;
}

Re: [PATCH] bitfield.h: add FIELD_MAX() and field_max()

[Alex Elder]

On 2/28/20 12:04 PM, Alex Elder wrote:
> 
> 
> I find field_max() to be a good name for what I'm looking for.

Sorry I wanted to add this but clicked "send" too fast.

Yes it's the same as field_mask(), but that name only *implies*
it is the same as the maximum value.  I mean, they're the same,
but the name I'm suggesting conveys its purpose better.

					-Alex

Re: [PATCH] bitfield.h: add FIELD_MAX() and field_max()

[Jakub Kicinski]

On Fri, 28 Feb 2020 12:06:09 -0600 Alex Elder wrote:
> On 2/28/20 12:04 PM, Alex Elder wrote:
> > 
> > 
> > I find field_max() to be a good name for what I'm looking for.  
> 
> Sorry I wanted to add this but clicked "send" too fast.
> 
> Yes it's the same as field_mask(), but that name only *implies*
> it is the same as the maximum value.  I mean, they're the same,
> but the name I'm suggesting conveys its purpose better.

We got FIELD_FIT tho.. The comparison is part of the macro there, 
and it catches negative values if they manage to sneak in.

Re: [PATCH] bitfield.h: add FIELD_MAX() and field_max()

[Alex Elder]

On 2/28/20 12:33 PM, Jakub Kicinski wrote:
> On Fri, 28 Feb 2020 12:06:09 -0600 Alex Elder wrote:
>> On 2/28/20 12:04 PM, Alex Elder wrote:
>>>
>>>
>>> I find field_max() to be a good name for what I'm looking for.  
>>
>> Sorry I wanted to add this but clicked "send" too fast.
>>
>> Yes it's the same as field_mask(), but that name only *implies*
>> it is the same as the maximum value.  I mean, they're the same,
>> but the name I'm suggesting conveys its purpose better.
> 
> We got FIELD_FIT tho.. The comparison is part of the macro there, 
> and it catches negative values if they manage to sneak in.

Ahhh!  I was using the lower-case macros and it looks like there
isn't one (despite seeming to have all(?) of the others).

How would you feel about having field_fit() be a lower-case
equivalent of FIELD_FIT()?

					-Alex

Re: [PATCH] bitfield.h: add FIELD_MAX() and field_max()

[Jakub Kicinski]

On Fri, 28 Feb 2020 12:37:14 -0600 Alex Elder wrote:
> On 2/28/20 12:33 PM, Jakub Kicinski wrote:
> > On Fri, 28 Feb 2020 12:06:09 -0600 Alex Elder wrote:  
> >> On 2/28/20 12:04 PM, Alex Elder wrote:  
> >>>
> >>>
> >>> I find field_max() to be a good name for what I'm looking for.    
> >>
> >> Sorry I wanted to add this but clicked "send" too fast.
> >>
> >> Yes it's the same as field_mask(), but that name only *implies*
> >> it is the same as the maximum value.  I mean, they're the same,
> >> but the name I'm suggesting conveys its purpose better.  
> > 
> > We got FIELD_FIT tho.. The comparison is part of the macro there, 
> > and it catches negative values if they manage to sneak in.  
> 
> Ahhh!  I was using the lower-case macros and it looks like there
> isn't one (despite seeming to have all(?) of the others).

Ah, damn, I didn't check for the lower case version.

> How would you feel about having field_fit() be a lower-case
> equivalent of FIELD_FIT()?

That'd be great!

Re: [PATCH] bitfield.h: add FIELD_MAX() and field_max()

[Alex Elder]

On 2/28/20 12:04 PM, Alex Elder wrote:
> On 2/28/20 11:56 AM, Jakub Kicinski wrote:
>> On Fri, 28 Feb 2020 10:53:43 -0600 Alex Elder wrote:
>>> Define FIELD_MAX(), which supplies the maximum value that can be
>>> represented by a field value.  Define field_max() as well, to go
>>> along with the lower-case forms of the field mask functions.
>>>
>>> Signed-off-by: Alex Elder <elder@linaro.org>
>>> ---
>>>
>>>  NOTE:	I'm not entirely sure who owns/maintains this file so
>>> 	I'm sending it to those who have committed things to it.
>>> 	I hope someone will just take it in after review; I use
>>> 	field_max() in some code I'm about to send out.
>>
>> Could you give us an example use?
>>
>> Is it that you find the current macros misnamed or there's something
>> you can't do? Or are you trying to set fields to max?
> 
> I'm trying to validate variable values are in range before attempting
> to use them in a bitfield.

I should have actually checked my code before I sent this.  Yes
I am using the macro as I described, to see if something fits.
But I'm also using it this way:

	foo = u32_get_bits(register, FOO_COUNT_FMASK);
	if (foo == field_max(FOO_COUNT_MASK))
		;	/* This has special meaning */

And another way:

	size_limit = field_max(FOO_COUNT_MASK) * sizeof(struct foo);

So field_max() is really what I need here.  It does imply a
signed interpretation of the field value, but that's true
for all of the lower-case bitfield functions.

I understand the value of FIELD_FIT() but I think field_max()
(and FIELD_MAX()) serves a purpose.  In fact one could argue it
makes FIELD_FIT() unnecessary (compare against field_max(mask)
instead) but I won't propose removing it.

So after further consideration I believe the original patch
is fine.  What are your thoughts?

					-Alex

> I find field_max() to be a good name for what I'm looking for.
> 
> 					-Alex
> 
> #define FOO_FMASK	0x000ff000
> 
> static u32 register = 0x12345678;
> 
> int foo(u32 value)
> {
> 	if (value > field_max(FOO_FMASK))
> 		return -EINVAL;
> 
> 	u32_replace_bits(&register, value, FOO_FMASK);
> 	
> 	return 0;
> }
> 

Re: [PATCH] bitfield.h: add FIELD_MAX() and field_max()

[Jakub Kicinski]

On Sat, 29 Feb 2020 07:10:45 -0600 Alex Elder wrote:
> I should have actually checked my code before I sent this.  Yes
> I am using the macro as I described, to see if something fits.
> But I'm also using it this way:
> 
> 	foo = u32_get_bits(register, FOO_COUNT_FMASK);
> 	if (foo == field_max(FOO_COUNT_MASK))
> 		;	/* This has special meaning */
> 
> And another way:
> 
> 	size_limit = field_max(FOO_COUNT_MASK) * sizeof(struct foo);
> 
> So field_max() is really what I need here. 

Makes sense, in that case:

Acked-by: Jakub Kicinski <kuba@kernel.org>