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

[Jakub Kicinski <jakub.kicinski@netronome.com>]

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?