| Message ID | 1471360704-10507-2-git-send-email-jakub.kicinski@netronome.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Jakub Kicinski <jakub.kicinski@netronome.com> writes: > Common approach to accessing register fields is to define > structures or sets of macros containing mask and shift pair. > Operations on the register are then performed as follows: > > field = (reg >> shift) & mask; > > reg &= ~(mask << shift); > reg |= (field & mask) << shift; > > Defining shift and mask separately is tedious. Ivo van Doorn > came up with an idea of computing them at compilation time > based on a single shifted mask (later refined by Felix) which > can be used like this: > > #define REG_FIELD 0x000ff000 > > field = FIELD_GET(REG_FIELD, reg); > > reg &= ~REG_FIELD; > reg |= FIELD_PUT(REG_FIELD, field); > > FIELD_{GET,PUT} macros take care of finding out what the > appropriate shift is based on compilation time ffs operation. > > GENMASK can be used to define registers (which is usually > less error-prone and easier to match with datasheets). > > This approach is the most convenient I've seen so to limit code > multiplication let's move the macros to a global header file. > Attempts to use static inlines instead of macros failed due > to false positive triggering of BUILD_BUG_ON()s, especially with > GCC < 6.0. > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > Reviewed-by: Dinan Gunawardena <dinan.gunawardena@netronome.com> Are people ok with this? I think they are useful and I can take these through my tree, but I would prefer to get an ack from other maintainers first. Dave? Andrew? Full patches here: https://patchwork.kernel.org/patch/9284153/ https://patchwork.kernel.org/patch/9284155/
On Wed, Aug 17, 2016 at 3:31 AM, Kalle Valo <kvalo@codeaurora.org> wrote: > > Are people ok with this? I think they are useful and I can take these > through my tree, but I would prefer to get an ack from other maintainers > first. Dave? Andrew? I'm not a huge fan, since the interface fundamentally seems to be oddly designed (why pass in the mask rather than "start bit + length"?). I also don't like how this very much would match the GENMASK() macros we have, but then it clashes with them in other ways - it's in a different header file - it has completely different naming (GENMASK_ULL vs FIELD_GET64}. I actually think the naming could/should be fixed by just automatically doing the right thing based on sizes. For example, GENMASK could just have something like #define GENMASK(end,start) __builtin_choose_expr((end)>31, __GENMASK_64(end,start), __GENMASK_32(end,start)) and doing similar things with the FIELD_GET/SET ops. So I'm not entirely happy about this all. But if people love the interface, and think the above kind of cleanups might be possible, I'd just want to make sure that there is also a BUILD_BUG_ON(!__builtin_constant_p(_mask)); because if the mask isn't a build-time constant, the code ends up being *complete* shit. Also preferably something like BUILD_BUG_ON((_mask) > (typeof(_val)~0ull); to make sure you can't try to mask bits that don't exist in the value. Or something like that to make mis-uses *really* obvious. The FIELD_PUT macro also seems misnamed. It doesn't "put" anything (unlike the GET macro). It just prepares the field for inserting later. As exemplified by how you actually have to put things: First, "GET" - yeah, that looks like a "get" operation: * Get: * a = FIELD_GET(REG_FIELD_A, reg); But then "PUT" isn't actually a PUT operation at all, but the comments kind of gloss over it by talking about "Modify" instead: * Modify: * reg &= ~REG_FIELD_C; * reg |= FIELD_PUT(REG_FIELD_C, c); so I'm not entirely sure about the naming. I can live with the FIELD_PUT naming, because I see how it comes about, even if I think it's a bit odd. I might have called it "FIELD_PREP" instead, but I'm not really sure that's all that much better. Am I being a bit anal? Yeah. But when we add whole new abstractions that we haven't used historically, I'd really like those to be obvious and easy to use (or rather, really _hard_ to get wrong by mistake). Hmm? Linus
On Wed, 17 Aug 2016 09:33:26 -0700, Linus Torvalds wrote: > On Wed, Aug 17, 2016 at 3:31 AM, Kalle Valo <kvalo@codeaurora.org> wrote: > > > > Are people ok with this? I think they are useful and I can take these > > through my tree, but I would prefer to get an ack from other maintainers > > first. Dave? Andrew? > > I'm not a huge fan, since the interface fundamentally seems to be > oddly designed (why pass in the mask rather than "start bit + > length"?). Would that not require start and length to have separate defines? I assume doing: #define REG_BLA_FIELD_FOO 3, 4 val = FIELD_GET(REG_BLA_FIELD_FOO, reg); is not acceptable. Attempts to define a single value brought us to the shifted mask. > I also don't like how this very much would match the GENMASK() macros > we have, but then it clashes with them in other ways > > - it's in a different header file I'll move to bitops.h. > - it has completely different naming (GENMASK_ULL vs FIELD_GET64}. > > I actually think the naming could/should be fixed by just > automatically doing the right thing based on sizes. For example, > GENMASK could just have something like > > #define GENMASK(end,start) __builtin_choose_expr((end)>31, > __GENMASK_64(end,start), __GENMASK_32(end,start)) > > and doing similar things with the FIELD_GET/SET ops. > > So I'm not entirely happy about this all. Seems feasible. > But if people love the interface, and think the above kind of cleanups > might be possible, I'd just want to make sure that there is also a > > BUILD_BUG_ON(!__builtin_constant_p(_mask)); > > because if the mask isn't a build-time constant, the code ends up > being *complete* shit. Also preferably something like > > BUILD_BUG_ON((_mask) > (typeof(_val)~0ull); > > to make sure you can't try to mask bits that don't exist in the value. > > Or something like that to make mis-uses *really* obvious. OK! > The FIELD_PUT macro also seems misnamed. It doesn't "put" anything > (unlike the GET macro). It just prepares the field for inserting > later. As exemplified by how you actually have to put things: > > First, "GET" - yeah, that looks like a "get" operation: > > * Get: > * a = FIELD_GET(REG_FIELD_A, reg); > > But then "PUT" isn't actually a PUT operation at all, but the comments > kind of gloss over it by talking about "Modify" instead: > > * Modify: > * reg &= ~REG_FIELD_C; > * reg |= FIELD_PUT(REG_FIELD_C, c); > > so I'm not entirely sure about the naming. > > I can live with the FIELD_PUT naming, because I see how it comes > about, even if I think it's a bit odd. I might have called it > "FIELD_PREP" instead, but I'm not really sure that's all that much > better. Yes, it used to be called FIELD_SET, which was even worse. I think PREP sounds good. Thanks!
On Wed, Aug 17, 2016 at 10:11 AM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > On Wed, 17 Aug 2016 09:33:26 -0700, Linus Torvalds wrote: >> >> I'm not a huge fan, since the interface fundamentally seems to be >> oddly designed (why pass in the mask rather than "start bit + >> length"?). > > Would that not require start and length to have separate defines? Yeah. > I assume doing: > > #define REG_BLA_FIELD_FOO 3, 4 > val = FIELD_GET(REG_BLA_FIELD_FOO, reg); > > is not acceptable. Attempts to define a single value brought us to the > shifted mask. Agreed. Maybe the mask with the complexity to then undo it (at compile time) is the better approach. Linus
On Thu, Aug 18, 2016 at 10:11 AM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > Hi! > > This is what I came up with. Changes: I can live with this, certainly. I'm not really sure how many drivers (or perhaps core code, for that matter) will actually start using it, but it at least _looks_ like a usable interface that seems to be quite resistant to people doing stupid things with it that would result in surprising results (either performance or semantics). So I'm ok with something like this coming through (for example) the wireless tree if the drivers there are the first ones to start using this. Let's see if anybody else objects. Linus
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Thu, Aug 18, 2016 at 10:11 AM, Jakub Kicinski > <jakub.kicinski@netronome.com> wrote: >> Hi! >> >> This is what I came up with. Changes: > > I can live with this, certainly. I'm not really sure how many drivers > (or perhaps core code, for that matter) will actually start using it, > but it at least _looks_ like a usable interface that seems to be quite > resistant to people doing stupid things with it that would result in > surprising results (either performance or semantics). I'm guessing that at least in ath9k and ath10k there would be use of these macros. > So I'm ok with something like this coming through (for example) the > wireless tree if the drivers there are the first ones to start using > this. > > Let's see if anybody else objects. Great, thanks for the help. Let's wait for other comments and Jakub can then resend this without RFC. I can then take it through my tree.
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h new file mode 100644 index 000000000000..ff9fd0af2ac7 --- /dev/null +++ b/include/linux/bitfield.h @@ -0,0 +1,109 @@ +/* + * Copyright (C) 2014 Felix Fietkau <nbd@nbd.name> + * Copyright (C) 2004 - 2009 Ivo van Doorn <IvDoorn@gmail.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _LINUX_BITFIELD_H +#define _LINUX_BITFIELD_H + +#include <asm/types.h> +#include <linux/bug.h> + +#define _bf_shf(x) (__builtin_ffsll(x) - 1) + +#define _BF_FIELD_CHECK(_mask, _val) \ + ({ \ + BUILD_BUG_ON(!(_mask)); \ + BUILD_BUG_ON(__builtin_constant_p(_val) ? \ + ~((_mask) >> _bf_shf(_mask)) & (_val) : \ + 0); \ + __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \ + (1ULL << _bf_shf(_mask))); \ + }) + +/* + * Bitfield access macros + * + * This file contains macros which take as input shifted mask + * from which they extract the base mask and shift amount at + * compilation time. There are two separate sets of the macros + * one for 32bit registers and one for 64bit ones. + * + * Fields can be defined using GENMASK (which is usually + * less error-prone and easier to match with datasheets). + * + * FIELD_{GET,PUT} macros are designed to be used with masks which + * are compilation time constants. + * + * Example: + * + * #define REG_FIELD_A GENMASK(6, 0) + * #define REG_FIELD_B BIT(7) + * #define REG_FIELD_C GENMASK(15, 8) + * #define REG_FIELD_D GENMASK(31, 16) + * + * Get: + * a = FIELD_GET(REG_FIELD_A, reg); + * b = FIELD_GET(REG_FIELD_B, reg); + * + * Set: + * reg = FIELD_PUT(REG_FIELD_A, 1) | + * FIELD_PUT(REG_FIELD_B, 0) | + * FIELD_PUT(REG_FIELD_C, c) | + * FIELD_PUT(REG_FIELD_D, 0x40); + * + * Modify: + * reg &= ~REG_FIELD_C; + * reg |= FIELD_PUT(REG_FIELD_C, c); + */ + +/** + * FIELD_PUT() - construct a bitfield element + * @_mask: shifted mask defining the field's length and position + * @_val: value to put in the field + * + * FIELD_PUT() masks and shifts up the value. The result should + * be combined with other fields of the bitfield using logical OR. + */ +#define FIELD_PUT(_mask, _val) \ + ({ \ + _BF_FIELD_CHECK(_mask, _val); \ + ((u32)(_val) << _bf_shf(_mask)) & (_mask); \ + }) + +/** + * FIELD_GET() - extract a bitfield element + * @_mask: shifted mask defining the field's length and position + * @_val: 32bit value of entire bitfield + * + * FIELD_GET() extracts the field specified by @_mask from the + * bitfield passed in as @_val. + */ +#define FIELD_GET(_mask, _val) \ + ({ \ + _BF_FIELD_CHECK(_mask, 0); \ + (u32)(((_val) & (_mask)) >> _bf_shf(_mask)); \ + }) + +#define FIELD_PUT64(_mask, _val) \ + ({ \ + _BF_FIELD_CHECK(_mask, _val); \ + ((u64)(_val) << _bf_shf(_mask)) & (_mask); \ + }) + +#define FIELD_GET64(_mask, _val) \ + ({ \ + _BF_FIELD_CHECK(_mask, 0); \ + (u64)(((_val) & (_mask)) >> _bf_shf(_mask)); \ + }) + +#endif diff --git a/include/linux/bug.h b/include/linux/bug.h index e51b0709e78d..bba5bdae1681 100644 --- a/include/linux/bug.h +++ b/include/linux/bug.h @@ -13,6 +13,7 @@ enum bug_trap_type { struct pt_regs; #ifdef __CHECKER__ +#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0) #define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0) #define BUILD_BUG_ON_ZERO(e) (0) #define BUILD_BUG_ON_NULL(e) ((void*)0) @@ -24,6 +25,8 @@ struct pt_regs; #else /* __CHECKER__ */ /* Force a compilation error if a constant expression is not a power of 2 */ +#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) \ + BUILD_BUG_ON(((n) & ((n) - 1)) != 0) #define BUILD_BUG_ON_NOT_POWER_OF_2(n) \ BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
Common approach to accessing register fields is to define structures or sets of macros containing mask and shift pair. Operations on the register are then performed as follows: field = (reg >> shift) & mask; reg &= ~(mask << shift); reg |= (field & mask) << shift; Defining shift and mask separately is tedious. Ivo van Doorn came up with an idea of computing them at compilation time based on a single shifted mask (later refined by Felix) which can be used like this: #define REG_FIELD 0x000ff000 field = FIELD_GET(REG_FIELD, reg); reg &= ~REG_FIELD; reg |= FIELD_PUT(REG_FIELD, field); FIELD_{GET,PUT} macros take care of finding out what the appropriate shift is based on compilation time ffs operation. GENMASK can be used to define registers (which is usually less error-prone and easier to match with datasheets). This approach is the most convenient I've seen so to limit code multiplication let's move the macros to a global header file. Attempts to use static inlines instead of macros failed due to false positive triggering of BUILD_BUG_ON()s, especially with GCC < 6.0. Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> Reviewed-by: Dinan Gunawardena <dinan.gunawardena@netronome.com> --- include/linux/bitfield.h | 109 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/bug.h | 3 ++ 2 files changed, 112 insertions(+) create mode 100644 include/linux/bitfield.h