Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752785AbcLGOCq (ORCPT ); Wed, 7 Dec 2016 09:02:46 -0500 Received: from mail-wm0-f47.google.com ([74.125.82.47]:35841 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752462AbcLGOCo (ORCPT ); Wed, 7 Dec 2016 09:02:44 -0500 Date: Wed, 7 Dec 2016 14:02:39 +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: <20161207140239.22d27df4@jkicinski-Precision-T1700> In-Reply-To: <99472464-f6a0-3d92-29c0-6728d9b50b00@laposte.net> References: <87bmwom8on.fsf@purkki.adurom.net> <489b2ea4-0678-97ac-f46e-4fc8a7853358@laposte.net> <20161207110517.45f54a9a@jkicinski-Precision-T1700> <20161207123800.6dd89eff@jkicinski-Precision-T1700> <99472464-f6a0-3d92-29c0-6728d9b50b00@laposte.net> 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: 3925 Lines: 102 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 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? > > 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 > drivers/net/wireless/mediatek/mt7601u/mt7601u.h:#include 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?