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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>>>>>> 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?
>