[PATCH v3] add equivalent of BIT(x) for bitfields

[PATCH v3] add equivalent of BIT(x) for bitfields

[Sebastian Frias]

Introduce GENVALUE(msb, lsb, value) macro to ease dealing with
continuous bitfields, just as BIT(x) does for single bits.

GENVALUE_ULL(msb, lsb, value) macro is also added.

This is useful mostly for creating values to be packed together
via OR operations, ex:

   u32 val = 0x11110000;
   val |= GENVALUE(19, 12, 0x5a);

now 'val = 0x1115a000'


Signed-off-by: Sebastian Frias <sf84@laposte.net>
Link: https://marc.info/?l=linux-kernel&m=148094498711000&w=2
---

Change in v2:
- rename the macro to GENVALUE as proposed by Linus
- longer comment attempts to show use case for the macro as
proposed by Borislav

Change in v3:
- use BUILD_BUG_ON_ZERO() to break if some input parameters
(essentially 'lsb' but also 'msb') are not constants as
proposed by Linus.
Indeed, 'lsb' is used twice so it cannot have side-effects;
'msb' is subjected to same constraints for consistency.

---
 include/linux/bitops.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index a83c822..419529a0 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -24,6 +24,31 @@
 #define GENMASK_ULL(h, l) \
 	(((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
 
+#ifdef	__KERNEL__
+/*
+ * Equivalent of BIT(x) but for contiguous bitfields
+ * GENVALUE(1, 0,0xff) = 0x00000003
+ * GENVALUE(3, 0,0xff) = 0x0000000f
+ * GENVALUE(15,8,0xff) = 0x0000ff00
+ * GENVALUE(6, 6,   1) = 0x00000040 == BIT(6)
+ */
+#define GENVALUE(msb, lsb, val)					\
+	(								\
+		/* BUILD_BUG_ON_ZERO() evaluates to 0 */		\
+		BUILD_BUG_ON_ZERO(!__builtin_constant_p(msb)) |		\
+		BUILD_BUG_ON_ZERO(!__builtin_constant_p(lsb)) |		\
+		(((val) << (lsb)) & (GENMASK((msb), (lsb))))		\
+	)
+
+#define GENVALUE_ULL(msb, lsb, val)					\
+	(								\
+		/* BUILD_BUG_ON_ZERO() evaluates to 0 */		\
+		BUILD_BUG_ON_ZERO(!__builtin_constant_p(msb)) |		\
+		BUILD_BUG_ON_ZERO(!__builtin_constant_p(lsb)) |		\
+		(((val) << (lsb)) & (GENMASK_ULL((msb), (lsb))))	\
+	)
+#endif
+
 extern unsigned int __sw_hweight8(unsigned int w);
 extern unsigned int __sw_hweight16(unsigned int w);
 extern unsigned int __sw_hweight32(unsigned int w);
-- 
1.8.3.1

Re: [PATCH v3] add equivalent of BIT(x) for bitfields

[Kalle Valo]

Sebastian Frias <sf84@laposte.net> writes:

> Introduce GENVALUE(msb, lsb, value) macro to ease dealing with
> continuous bitfields, just as BIT(x) does for single bits.
>
> GENVALUE_ULL(msb, lsb, value) macro is also added.
>
> This is useful mostly for creating values to be packed together
> via OR operations, ex:
>
>    u32 val = 0x11110000;
>    val |= GENVALUE(19, 12, 0x5a);
>
> now 'val = 0x1115a000'
>
>
> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> Link: https://marc.info/?l=linux-kernel&m=148094498711000&w=2
> ---
>
> Change in v2:
> - rename the macro to GENVALUE as proposed by Linus
> - longer comment attempts to show use case for the macro as
> proposed by Borislav
>
> Change in v3:
> - use BUILD_BUG_ON_ZERO() to break if some input parameters
> (essentially 'lsb' but also 'msb') are not constants as
> proposed by Linus.
> Indeed, 'lsb' is used twice so it cannot have side-effects;
> 'msb' is subjected to same constraints for consistency.

(I missed there was v3 already, but I'll repeat what I said in v1.)

Please check FIELD_PREP() from include/linux/bitfield.h, doesn't it
already do the same?

Adding Jakub to comment more.

-- 
Kalle Valo

Re: [PATCH v3] add equivalent of BIT(x) for bitfields

[Sebastian Frias]

On 07/12/16 09:42, Kalle Valo wrote:
> Sebastian Frias <sf84@laposte.net> writes:
> 
>> Introduce GENVALUE(msb, lsb, value) macro to ease dealing with
>> continuous bitfields, just as BIT(x) does for single bits.
>>
>> GENVALUE_ULL(msb, lsb, value) macro is also added.
>>
>> This is useful mostly for creating values to be packed together
>> via OR operations, ex:
>>
>>    u32 val = 0x11110000;
>>    val |= GENVALUE(19, 12, 0x5a);
>>
>> now 'val = 0x1115a000'
>>
>>
>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>> Link: https://marc.info/?l=linux-kernel&m=148094498711000&w=2
>> ---
>>
>> Change in v2:
>> - rename the macro to GENVALUE as proposed by Linus
>> - longer comment attempts to show use case for the macro as
>> proposed by Borislav
>>
>> Change in v3:
>> - use BUILD_BUG_ON_ZERO() to break if some input parameters
>> (essentially 'lsb' but also 'msb') are not constants as
>> proposed by Linus.
>> Indeed, 'lsb' is used twice so it cannot have side-effects;
>> 'msb' is subjected to same constraints for consistency.
> 
> (I missed there was v3 already, but I'll repeat what I said in v1.)
> 
> Please check FIELD_PREP() from include/linux/bitfield.h, doesn't it
> already do the same?

Indeed, it appears to do the same :-)
Any reason why "include/linux/bitfield.h" is not included by default in
bitops.h?

Re: [PATCH v3] add equivalent of BIT(x) for bitfields

[Jakub Kicinski]

On Wed, 7 Dec 2016 11:00:57 +0100, Sebastian Frias wrote:
> On 07/12/16 09:42, Kalle Valo wrote:
> > Sebastian Frias <sf84@laposte.net> writes:
> >   
> >> Introduce GENVALUE(msb, lsb, value) macro to ease dealing with
> >> continuous bitfields, just as BIT(x) does for single bits.
> >>
> >> GENVALUE_ULL(msb, lsb, value) macro is also added.
> >>
> >> This is useful mostly for creating values to be packed together
> >> via OR operations, ex:
> >>
> >>    u32 val = 0x11110000;
> >>    val |= GENVALUE(19, 12, 0x5a);
> >>
> >> now 'val = 0x1115a000'
> >>
> >>
> >> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> >> Link: https://marc.info/?l=linux-kernel&m=148094498711000&w=2
> >> ---
> >>
> >> Change in v2:
> >> - rename the macro to GENVALUE as proposed by Linus
> >> - longer comment attempts to show use case for the macro as
> >> proposed by Borislav
> >>
> >> Change in v3:
> >> - use BUILD_BUG_ON_ZERO() to break if some input parameters
> >> (essentially 'lsb' but also 'msb') are not constants as
> >> proposed by Linus.
> >> Indeed, 'lsb' is used twice so it cannot have side-effects;
> >> 'msb' is subjected to same constraints for consistency.  
> > 
> > (I missed there was v3 already, but I'll repeat what I said in v1.)
> > 
> > Please check FIELD_PREP() from include/linux/bitfield.h, doesn't it
> > already do the same?  
> 
> Indeed, it appears to do the same :-)
> Any reason why "include/linux/bitfield.h" is not included by default in
> bitops.h?

Hi!

The code is in a separate header because of circular dependencies
(coming from bug.h including bitops.h, IIRC).  You could possibly add an
include of bitfield.h in bitops.h if you're very careful, I haven't
tried TBH :)

Re: [PATCH v3] add equivalent of BIT(x) for bitfields

[Sebastian Frias]

On 07/12/16 12:05, Jakub Kicinski wrote:
> On Wed, 7 Dec 2016 11:00:57 +0100, Sebastian Frias wrote:
>> On 07/12/16 09:42, Kalle Valo wrote:
>>> Sebastian Frias <sf84@laposte.net> writes:
>>>   
>>>> Introduce GENVALUE(msb, lsb, value) macro to ease dealing with
>>>> continuous bitfields, just as BIT(x) does for single bits.
>>>>
>>>> GENVALUE_ULL(msb, lsb, value) macro is also added.
>>>>
>>>> This is useful mostly for creating values to be packed together
>>>> via OR operations, ex:
>>>>
>>>>    u32 val = 0x11110000;
>>>>    val |= GENVALUE(19, 12, 0x5a);
>>>>
>>>> now 'val = 0x1115a000'
>>>>
>>>>
>>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>>>> Link: https://marc.info/?l=linux-kernel&m=148094498711000&w=2
>>>> ---
>>>>
>>>> Change in v2:
>>>> - rename the macro to GENVALUE as proposed by Linus
>>>> - longer comment attempts to show use case for the macro as
>>>> proposed by Borislav
>>>>
>>>> Change in v3:
>>>> - use BUILD_BUG_ON_ZERO() to break if some input parameters
>>>> (essentially 'lsb' but also 'msb') are not constants as
>>>> proposed by Linus.
>>>> Indeed, 'lsb' is used twice so it cannot have side-effects;
>>>> 'msb' is subjected to same constraints for consistency.  
>>>
>>> (I missed there was v3 already, but I'll repeat what I said in v1.)
>>>
>>> Please check FIELD_PREP() from include/linux/bitfield.h, doesn't it
>>> already do the same?  
>>
>> Indeed, it appears to do the same :-)
>> Any reason why "include/linux/bitfield.h" is not included by default in
>> bitops.h?
> 
> Hi!
> 
> The code is in a separate header because of circular dependencies
> (coming from bug.h including bitops.h, IIRC).  You could possibly add an
> include of bitfield.h in bitops.h if you're very careful, I haven't
> tried TBH :)
> 

Well, the following seems to work just fine:

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index f6505d8..24c7480 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -15,8 +15,6 @@
 #ifndef _LINUX_BITFIELD_H
 #define _LINUX_BITFIELD_H
 
-#include <linux/bug.h>
-
 /*
  * Bitfield access macros
  *
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index a83c822..7e5fab8 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -24,6 +24,8 @@
 #define GENMASK_ULL(h, l) \
        (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
 
+#include "bitfield.h"
+
 extern unsigned int __sw_hweight8(unsigned int w);
 extern unsigned int __sw_hweight16(unsigned int w);
 extern unsigned int __sw_hweight32(unsigned int w);


Is there a way to be sure it works in all cases? Otherwise
I could just submit that, right?

Re: [PATCH v3] add equivalent of BIT(x) for bitfields

[Jakub Kicinski]

On Wed, 7 Dec 2016 12:23:56 +0100, Sebastian Frias wrote:
> On 07/12/16 12:05, Jakub Kicinski wrote:
> > On Wed, 7 Dec 2016 11:00:57 +0100, Sebastian Frias wrote:  
> >> On 07/12/16 09:42, Kalle Valo wrote:  
> >>> Sebastian Frias <sf84@laposte.net> writes:
> >>>     
> >>>> Introduce GENVALUE(msb, lsb, value) macro to ease dealing with
> >>>> continuous bitfields, just as BIT(x) does for single bits.
> >>>>
> >>>> GENVALUE_ULL(msb, lsb, value) macro is also added.
> >>>>
> >>>> This is useful mostly for creating values to be packed together
> >>>> via OR operations, ex:
> >>>>
> >>>>    u32 val = 0x11110000;
> >>>>    val |= GENVALUE(19, 12, 0x5a);
> >>>>
> >>>> now 'val = 0x1115a000'
> >>>>
> >>>>
> >>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> >>>> Link: https://marc.info/?l=linux-kernel&m=148094498711000&w=2
> >>>> ---
> >>>>
> >>>> Change in v2:
> >>>> - rename the macro to GENVALUE as proposed by Linus
> >>>> - longer comment attempts to show use case for the macro as
> >>>> proposed by Borislav
> >>>>
> >>>> Change in v3:
> >>>> - use BUILD_BUG_ON_ZERO() to break if some input parameters
> >>>> (essentially 'lsb' but also 'msb') are not constants as
> >>>> proposed by Linus.
> >>>> Indeed, 'lsb' is used twice so it cannot have side-effects;
> >>>> 'msb' is subjected to same constraints for consistency.    
> >>>
> >>> (I missed there was v3 already, but I'll repeat what I said in v1.)
> >>>
> >>> Please check FIELD_PREP() from include/linux/bitfield.h, doesn't it
> >>> already do the same?    
> >>
> >> Indeed, it appears to do the same :-)
> >> Any reason why "include/linux/bitfield.h" is not included by default in
> >> bitops.h?  
> > 
> > Hi!
> > 
> > The code is in a separate header because of circular dependencies
> > (coming from bug.h including bitops.h, IIRC).  You could possibly add an
> > include of bitfield.h in bitops.h if you're very careful, I haven't
> > tried TBH :)
> >   
> 
> Well, the following seems to work just fine:
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index f6505d8..24c7480 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -15,8 +15,6 @@
>  #ifndef _LINUX_BITFIELD_H
>  #define _LINUX_BITFIELD_H
>  
> -#include <linux/bug.h>
> -

That would break users who include bitfield.h directly and don't
include bug.h, no?

>  /*
>   * Bitfield access macros
>   *
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index a83c822..7e5fab8 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -24,6 +24,8 @@
>  #define GENMASK_ULL(h, l) \
>         (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>  
> +#include "bitfield.h"
> +
>  extern unsigned int __sw_hweight8(unsigned int w);
>  extern unsigned int __sw_hweight16(unsigned int w);
>  extern unsigned int __sw_hweight32(unsigned int w);
> 
> 
> Is there a way to be sure it works in all cases? Otherwise
> I could just submit that, right?

Re: [PATCH v3] add equivalent of BIT(x) for bitfields

[Sebastian Frias]

On 07/12/16 13:38, Jakub Kicinski wrote:
> On Wed, 7 Dec 2016 12:23:56 +0100, Sebastian Frias wrote:
>> On 07/12/16 12:05, Jakub Kicinski wrote:
>>> On Wed, 7 Dec 2016 11:00:57 +0100, Sebastian Frias wrote:  
>>>> On 07/12/16 09:42, Kalle Valo wrote:  
>>>>> Sebastian Frias <sf84@laposte.net> writes:
>>>>>     
>>>>>> Introduce GENVALUE(msb, lsb, value) macro to ease dealing with
>>>>>> continuous bitfields, just as BIT(x) does for single bits.
>>>>>>
>>>>>> GENVALUE_ULL(msb, lsb, value) macro is also added.
>>>>>>
>>>>>> This is useful mostly for creating values to be packed together
>>>>>> via OR operations, ex:
>>>>>>
>>>>>>    u32 val = 0x11110000;
>>>>>>    val |= GENVALUE(19, 12, 0x5a);
>>>>>>
>>>>>> now 'val = 0x1115a000'
>>>>>>
>>>>>>
>>>>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>>>>>> Link: https://marc.info/?l=linux-kernel&m=148094498711000&w=2
>>>>>> ---
>>>>>>
>>>>>> Change in v2:
>>>>>> - rename the macro to GENVALUE as proposed by Linus
>>>>>> - longer comment attempts to show use case for the macro as
>>>>>> proposed by Borislav
>>>>>>
>>>>>> Change in v3:
>>>>>> - use BUILD_BUG_ON_ZERO() to break if some input parameters
>>>>>> (essentially 'lsb' but also 'msb') are not constants as
>>>>>> proposed by Linus.
>>>>>> Indeed, 'lsb' is used twice so it cannot have side-effects;
>>>>>> 'msb' is subjected to same constraints for consistency.    
>>>>>
>>>>> (I missed there was v3 already, but I'll repeat what I said in v1.)
>>>>>
>>>>> Please check FIELD_PREP() from include/linux/bitfield.h, doesn't it
>>>>> already do the same?    
>>>>
>>>> Indeed, it appears to do the same :-)
>>>> Any reason why "include/linux/bitfield.h" is not included by default in
>>>> bitops.h?  
>>>
>>> Hi!
>>>
>>> The code is in a separate header because of circular dependencies
>>> (coming from bug.h including bitops.h, IIRC).  You could possibly add an
>>> include of bitfield.h in bitops.h if you're very careful, I haven't
>>> tried TBH :)
>>>   
>>
>> Well, the following seems to work just fine:
>>
>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
>> index f6505d8..24c7480 100644
>> --- a/include/linux/bitfield.h
>> +++ b/include/linux/bitfield.h
>> @@ -15,8 +15,6 @@
>>  #ifndef _LINUX_BITFIELD_H
>>  #define _LINUX_BITFIELD_H
>>  
>> -#include <linux/bug.h>
>> -
> 
> That would break users who include bitfield.h directly and don't
> include bug.h, no?

That's why I was asking if there's a way to verify that I did not break
anything, as it may be simpler to just modify the users.

Right now bitops.h does not include bug.h yet my patch on this thread
used BUILD_BUG_ON_ZERO() inside bitops.h without issues.

So it seems like the "proper" include order is already in place.
(even though I agree with you that ideally each header file should include
headers required for it to work properly, regardless of include order)

Would the following files be the only two users we would need to worry
about?

$ git grep "linux/bitfield\.h"
drivers/net/ethernet/netronome/nfp/nfp_bpf.h:#include <linux/bitfield.h>
drivers/net/wireless/mediatek/mt7601u/mt7601u.h:#include <linux/bitfield.h>

I can take a look at the underlying include order issue later.

> 
>>  /*
>>   * Bitfield access macros
>>   *
>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
>> index a83c822..7e5fab8 100644
>> --- a/include/linux/bitops.h
>> +++ b/include/linux/bitops.h
>> @@ -24,6 +24,8 @@
>>  #define GENMASK_ULL(h, l) \
>>         (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
>>  
>> +#include "bitfield.h"
>> +
>>  extern unsigned int __sw_hweight8(unsigned int w);
>>  extern unsigned int __sw_hweight16(unsigned int w);
>>  extern unsigned int __sw_hweight32(unsigned int w);
>>
>>
>> Is there a way to be sure it works in all cases? Otherwise
>> I could just submit that, right?
> 

Re: [PATCH v3] add equivalent of BIT(x) for bitfields

[Jakub Kicinski]

On Wed, 7 Dec 2016 14:51:36 +0100, Sebastian Frias wrote:
> On 07/12/16 13:38, Jakub Kicinski wrote:
> > On Wed, 7 Dec 2016 12:23:56 +0100, Sebastian Frias wrote:  
> >> On 07/12/16 12:05, Jakub Kicinski wrote:  
> >>> On Wed, 7 Dec 2016 11:00:57 +0100, Sebastian Frias wrote:    
> >>>> On 07/12/16 09:42, Kalle Valo wrote:    
> >>>>> Sebastian Frias <sf84@laposte.net> writes:
> >>>>>       
> >>>>>> Introduce GENVALUE(msb, lsb, value) macro to ease dealing with
> >>>>>> continuous bitfields, just as BIT(x) does for single bits.
> >>>>>>
> >>>>>> GENVALUE_ULL(msb, lsb, value) macro is also added.
> >>>>>>
> >>>>>> This is useful mostly for creating values to be packed together
> >>>>>> via OR operations, ex:
> >>>>>>
> >>>>>>    u32 val = 0x11110000;
> >>>>>>    val |= GENVALUE(19, 12, 0x5a);
> >>>>>>
> >>>>>> now 'val = 0x1115a000'
> >>>>>>
> >>>>>>
> >>>>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> >>>>>> Link: https://marc.info/?l=linux-kernel&m=148094498711000&w=2
> >>>>>> ---
> >>>>>>
> >>>>>> Change in v2:
> >>>>>> - rename the macro to GENVALUE as proposed by Linus
> >>>>>> - longer comment attempts to show use case for the macro as
> >>>>>> proposed by Borislav
> >>>>>>
> >>>>>> Change in v3:
> >>>>>> - use BUILD_BUG_ON_ZERO() to break if some input parameters
> >>>>>> (essentially 'lsb' but also 'msb') are not constants as
> >>>>>> proposed by Linus.
> >>>>>> Indeed, 'lsb' is used twice so it cannot have side-effects;
> >>>>>> 'msb' is subjected to same constraints for consistency.      
> >>>>>
> >>>>> (I missed there was v3 already, but I'll repeat what I said in v1.)
> >>>>>
> >>>>> Please check FIELD_PREP() from include/linux/bitfield.h, doesn't it
> >>>>> already do the same?      
> >>>>
> >>>> Indeed, it appears to do the same :-)
> >>>> Any reason why "include/linux/bitfield.h" is not included by default in
> >>>> bitops.h?    
> >>>
> >>> Hi!
> >>>
> >>> The code is in a separate header because of circular dependencies
> >>> (coming from bug.h including bitops.h, IIRC).  You could possibly add an
> >>> include of bitfield.h in bitops.h if you're very careful, I haven't
> >>> tried TBH :)
> >>>     
> >>
> >> Well, the following seems to work just fine:
> >>
> >> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> >> index f6505d8..24c7480 100644
> >> --- a/include/linux/bitfield.h
> >> +++ b/include/linux/bitfield.h
> >> @@ -15,8 +15,6 @@
> >>  #ifndef _LINUX_BITFIELD_H
> >>  #define _LINUX_BITFIELD_H
> >>  
> >> -#include <linux/bug.h>
> >> -  
> > 
> > That would break users who include bitfield.h directly and don't
> > include bug.h, no?  
> 
> That's why I was asking if there's a way to verify that I did not break
> anything, as it may be simpler to just modify the users.
> 
> Right now bitops.h does not include bug.h yet my patch on this thread
> used BUILD_BUG_ON_ZERO() inside bitops.h without issues.
> 
> So it seems like the "proper" include order is already in place.
> (even though I agree with you that ideally each header file should include
> headers required for it to work properly, regardless of include order)
> 
> Would the following files be the only two users we would need to worry
> about?
> 
> $ git grep "linux/bitfield\.h"
> drivers/net/ethernet/netronome/nfp/nfp_bpf.h:#include <linux/bitfield.h>
> drivers/net/wireless/mediatek/mt7601u/mt7601u.h:#include <linux/bitfield.h>

If you're proposing to require users to have to remember to include
bug.h before bitfield.h then I don't think that's acceptable.  

There are also out-of-tree users like LEDE/OpenWRT who such changes may
disturb.

BTW I dug out the old conversation: https://lkml.org/lkml/2016/8/19/96

> I can take a look at the underlying include order issue later.

I think that would be the only way forward, but is not easy.

Is your concern that bitfield.h is hard to discover?  Or that users need
an extra #include beyond just bitops.h?

Re: [PATCH v3] add equivalent of BIT(x) for bitfields

[Sebastian Frias]

On 07/12/16 15:02, Jakub Kicinski wrote:
> On Wed, 7 Dec 2016 14:51:36 +0100, Sebastian Frias wrote:
>> On 07/12/16 13:38, Jakub Kicinski wrote:
>>> On Wed, 7 Dec 2016 12:23:56 +0100, Sebastian Frias wrote:  
>>>> On 07/12/16 12:05, Jakub Kicinski wrote:  
>>>>> On Wed, 7 Dec 2016 11:00:57 +0100, Sebastian Frias wrote:    
>>>>>> On 07/12/16 09:42, Kalle Valo wrote:    
>>>>>>> Sebastian Frias <sf84@laposte.net> writes:
>>>>>>>       
>>>>>>>> Introduce GENVALUE(msb, lsb, value) macro to ease dealing with
>>>>>>>> continuous bitfields, just as BIT(x) does for single bits.
>>>>>>>>
>>>>>>>> GENVALUE_ULL(msb, lsb, value) macro is also added.
>>>>>>>>
>>>>>>>> This is useful mostly for creating values to be packed together
>>>>>>>> via OR operations, ex:
>>>>>>>>
>>>>>>>>    u32 val = 0x11110000;
>>>>>>>>    val |= GENVALUE(19, 12, 0x5a);
>>>>>>>>
>>>>>>>> now 'val = 0x1115a000'
>>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Sebastian Frias <sf84@laposte.net>
>>>>>>>> Link: https://marc.info/?l=linux-kernel&m=148094498711000&w=2
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Change in v2:
>>>>>>>> - rename the macro to GENVALUE as proposed by Linus
>>>>>>>> - longer comment attempts to show use case for the macro as
>>>>>>>> proposed by Borislav
>>>>>>>>
>>>>>>>> Change in v3:
>>>>>>>> - use BUILD_BUG_ON_ZERO() to break if some input parameters
>>>>>>>> (essentially 'lsb' but also 'msb') are not constants as
>>>>>>>> proposed by Linus.
>>>>>>>> Indeed, 'lsb' is used twice so it cannot have side-effects;
>>>>>>>> 'msb' is subjected to same constraints for consistency.      
>>>>>>>
>>>>>>> (I missed there was v3 already, but I'll repeat what I said in v1.)
>>>>>>>
>>>>>>> Please check FIELD_PREP() from include/linux/bitfield.h, doesn't it
>>>>>>> already do the same?      
>>>>>>
>>>>>> Indeed, it appears to do the same :-)
>>>>>> Any reason why "include/linux/bitfield.h" is not included by default in
>>>>>> bitops.h?    
>>>>>
>>>>> Hi!
>>>>>
>>>>> The code is in a separate header because of circular dependencies
>>>>> (coming from bug.h including bitops.h, IIRC).  You could possibly add an
>>>>> include of bitfield.h in bitops.h if you're very careful, I haven't
>>>>> tried TBH :)
>>>>>     
>>>>
>>>> Well, the following seems to work just fine:
>>>>
>>>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
>>>> index f6505d8..24c7480 100644
>>>> --- a/include/linux/bitfield.h
>>>> +++ b/include/linux/bitfield.h
>>>> @@ -15,8 +15,6 @@
>>>>  #ifndef _LINUX_BITFIELD_H
>>>>  #define _LINUX_BITFIELD_H
>>>>  
>>>> -#include <linux/bug.h>
>>>> -  
>>>
>>> That would break users who include bitfield.h directly and don't
>>> include bug.h, no?  
>>
>> That's why I was asking if there's a way to verify that I did not break
>> anything, as it may be simpler to just modify the users.
>>
>> Right now bitops.h does not include bug.h yet my patch on this thread
>> used BUILD_BUG_ON_ZERO() inside bitops.h without issues.
>>
>> So it seems like the "proper" include order is already in place.
>> (even though I agree with you that ideally each header file should include
>> headers required for it to work properly, regardless of include order)
>>
>> Would the following files be the only two users we would need to worry
>> about?
>>
>> $ git grep "linux/bitfield\.h"
>> drivers/net/ethernet/netronome/nfp/nfp_bpf.h:#include <linux/bitfield.h>
>> drivers/net/wireless/mediatek/mt7601u/mt7601u.h:#include <linux/bitfield.h>
> 
> If you're proposing to require users to have to remember to include
> bug.h before bitfield.h then I don't think that's acceptable.  
> 

I was proposing a quick fix, and noted that "I agree with you that ideally 
each header file should include headers required for it to work properly,
regardless of include order"

The reason is probably the same that you had when you submitted bitfield.h
without including it in bitops.h: it seems that the "circular dependency"
issue is very deep.

However, we have to keep in mind that after fixing the current users of
bitfield.h, the rest won't have to do anything, because bitfield.h would
come with bitops.h and work properly.

> There are also out-of-tree users like LEDE/OpenWRT who such changes may
> disturb.
> 

I thought, maybe incorrectly, that out-of-tree users are not to be worried
about.

> BTW I dug out the old conversation: https://lkml.org/lkml/2016/8/19/96
> 
>> I can take a look at the underlying include order issue later.
> 
> I think that would be the only way forward, but is not easy.
> 

Right :-)

> Is your concern that bitfield.h is hard to discover?  Or that users need
> an extra #include beyond just bitops.h?
> 

The thing is that there are so many files that it is indeed not easy to know
that some facilities are already there.

I wrote the original macros in 4.7 which does not has bitfield.h, so it is
not really the case here, but I think it would be a good idea to include
these macros by default.