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@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>
> 
> 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.