Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752315AbcLGMib (ORCPT ); Wed, 7 Dec 2016 07:38:31 -0500 Received: from mail-wm0-f54.google.com ([74.125.82.54]:36570 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751742AbcLGMi3 (ORCPT ); Wed, 7 Dec 2016 07:38:29 -0500 Date: Wed, 7 Dec 2016 12:38:00 +0000 From: Jakub Kicinski To: Sebastian Frias Cc: Kalle Valo , zijun_hu , Andrew Morton , linux-kernel@vger.kernel.org, Linus Torvalds , Mason , Harvey Harrison , Borislav Petkov Subject: Re: [PATCH v3] add equivalent of BIT(x) for bitfields Message-ID: <20161207123800.6dd89eff@jkicinski-Precision-T1700> In-Reply-To: References: <87bmwom8on.fsf@purkki.adurom.net> <489b2ea4-0678-97ac-f46e-4fc8a7853358@laposte.net> <20161207110517.45f54a9a@jkicinski-Precision-T1700> Organization: Netronome Systems, Ltd. MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2977 Lines: 89 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 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 > >>>> 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 > - 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?