* [PATCH v6 (resend) 0/3] bitfield fixes and tests
@ 2018-06-20 6:58 Johannes Berg
2018-06-20 6:58 ` [PATCH v6 (resend) 1/3] bitfield: fix *_encode_bits() Johannes Berg
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Johannes Berg @ 2018-06-20 6:58 UTC (permalink / raw)
To: linux-wireless; +Cc: linux-kernel, Al Viro, Andy Shevchenko
This is just a resend of v5, to Cc linux-wireless - we decided that
since we have the user coming in through the wireless-drivers tree,
it'll avoid all the usual problems if we'll take these also through
that tree.
johannes
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v6 (resend) 1/3] bitfield: fix *_encode_bits() 2018-06-20 6:58 [PATCH v6 (resend) 0/3] bitfield fixes and tests Johannes Berg @ 2018-06-20 6:58 ` Johannes Berg 2018-06-27 16:00 ` [v6,(resend),1/3] " Kalle Valo 2018-06-20 6:58 ` [PATCH v6 (resend) 2/3] bitfield: add u8 helpers Johannes Berg 2018-06-20 6:58 ` [PATCH v6 (resend) 3/3] bitfield: add tests Johannes Berg 2 siblings, 1 reply; 5+ messages in thread From: Johannes Berg @ 2018-06-20 6:58 UTC (permalink / raw) To: linux-wireless; +Cc: linux-kernel, Al Viro, Andy Shevchenko There's a bug in *_encode_bits() in using ~field_multiplier() for the check whether or not the constant value fits into the field, this is wrong and clearly ~field_mask() was intended. This was triggering for me for both constant and non-constant values. Additionally, make this case actually into an compile error. Declaring the extern function that will never exist with just a warning is pointless as then later we'll just get a link error. While at it, also fix the indentation in those lines I'm touching. Finally, as suggested by Andy Shevchenko, add some tests and for that introduce also u8 helpers. The tests don't compile without the fix, showing that it's necessary. Fixes: 00b0c9b82663 ("Add primitives for manipulating bitfields both in host- and fixed-endian.") Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Johannes Berg <johannes@sipsolutions.net> --- v2: replace stray tab by space v3: u8 helpers, tests v4: minor cleanup (Andy) split into two patches (Andy) --- include/linux/bitfield.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h index cf2588d81148..147a7bb341dd 100644 --- a/include/linux/bitfield.h +++ b/include/linux/bitfield.h @@ -104,7 +104,7 @@ (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \ }) -extern void __compiletime_warning("value doesn't fit into mask") +extern void __compiletime_error("value doesn't fit into mask") __field_overflow(void); extern void __compiletime_error("bad bitfield mask") __bad_mask(void); @@ -121,8 +121,8 @@ static __always_inline u64 field_mask(u64 field) #define ____MAKE_OP(type,base,to,from) \ static __always_inline __##type type##_encode_bits(base v, base field) \ { \ - if (__builtin_constant_p(v) && (v & ~field_multiplier(field))) \ - __field_overflow(); \ + if (__builtin_constant_p(v) && (v & ~field_mask(field))) \ + __field_overflow(); \ return to((v & field_mask(field)) * field_multiplier(field)); \ } \ static __always_inline __##type type##_replace_bits(__##type old, \ -- 2.14.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [v6,(resend),1/3] bitfield: fix *_encode_bits() 2018-06-20 6:58 ` [PATCH v6 (resend) 1/3] bitfield: fix *_encode_bits() Johannes Berg @ 2018-06-27 16:00 ` Kalle Valo 0 siblings, 0 replies; 5+ messages in thread From: Kalle Valo @ 2018-06-27 16:00 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless, linux-kernel, Al Viro, Andy Shevchenko Johannes Berg <johannes@sipsolutions.net> wrote: > There's a bug in *_encode_bits() in using ~field_multiplier() for > the check whether or not the constant value fits into the field, > this is wrong and clearly ~field_mask() was intended. This was > triggering for me for both constant and non-constant values. > > Additionally, make this case actually into an compile error. > Declaring the extern function that will never exist with just a > warning is pointless as then later we'll just get a link error. > > While at it, also fix the indentation in those lines I'm touching. > > Finally, as suggested by Andy Shevchenko, add some tests and for > that introduce also u8 helpers. The tests don't compile without > the fix, showing that it's necessary. > > Fixes: 00b0c9b82663 ("Add primitives for manipulating bitfields both in host- and fixed-endian.") > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net> 3 patches applied to wireless-drivers-next.git, thanks. e7d4a95da86e bitfield: fix *_encode_bits() 37a3862e1238 bitfield: add u8 helpers 0e2dc70e3d0d bitfield: add tests -- https://patchwork.kernel.org/patch/10476545/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v6 (resend) 2/3] bitfield: add u8 helpers 2018-06-20 6:58 [PATCH v6 (resend) 0/3] bitfield fixes and tests Johannes Berg 2018-06-20 6:58 ` [PATCH v6 (resend) 1/3] bitfield: fix *_encode_bits() Johannes Berg @ 2018-06-20 6:58 ` Johannes Berg 2018-06-20 6:58 ` [PATCH v6 (resend) 3/3] bitfield: add tests Johannes Berg 2 siblings, 0 replies; 5+ messages in thread From: Johannes Berg @ 2018-06-20 6:58 UTC (permalink / raw) To: linux-wireless; +Cc: linux-kernel, Al Viro, Andy Shevchenko There's no reason why we shouldn't pack/unpack bits into/from u8 values/registers/etc., so add u8 helpers. Use the ____MAKE_OP() macro directly to avoid having nonsense le8_encode_bits() and similar functions. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Johannes Berg <johannes@sipsolutions.net> --- include/linux/bitfield.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h index 147a7bb341dd..65a6981eef7b 100644 --- a/include/linux/bitfield.h +++ b/include/linux/bitfield.h @@ -143,6 +143,7 @@ static __always_inline base type##_get_bits(__##type v, base field) \ ____MAKE_OP(le##size,u##size,cpu_to_le##size,le##size##_to_cpu) \ ____MAKE_OP(be##size,u##size,cpu_to_be##size,be##size##_to_cpu) \ ____MAKE_OP(u##size,u##size,,) +____MAKE_OP(u8,u8,,) __MAKE_OP(16) __MAKE_OP(32) __MAKE_OP(64) -- 2.14.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v6 (resend) 3/3] bitfield: add tests 2018-06-20 6:58 [PATCH v6 (resend) 0/3] bitfield fixes and tests Johannes Berg 2018-06-20 6:58 ` [PATCH v6 (resend) 1/3] bitfield: fix *_encode_bits() Johannes Berg 2018-06-20 6:58 ` [PATCH v6 (resend) 2/3] bitfield: add u8 helpers Johannes Berg @ 2018-06-20 6:58 ` Johannes Berg 2 siblings, 0 replies; 5+ messages in thread From: Johannes Berg @ 2018-06-20 6:58 UTC (permalink / raw) To: linux-wireless; +Cc: linux-kernel, Al Viro, Andy Shevchenko Add tests for the bitfield helpers. The constant ones will all be folded to nothing by the compiler (if everything is correct in the header file), and the variable ones do some tests against open-coding the necessary shifts. A few test cases that should fail/warn compilation are provided under ifdef. Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Johannes Berg <johannes@sipsolutions.net> --- lib/Kconfig.debug | 7 +++ lib/Makefile | 1 + lib/test_bitfield.c | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 176 insertions(+) create mode 100644 lib/test_bitfield.c diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index eb885942eb0f..b0870377b4d1 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1799,6 +1799,13 @@ config TEST_BITMAP If unsure, say N. +config TEST_BITFIELD + tristate "Test bitfield functions at runtime" + help + Enable this option to test the bitfield functions at boot. + + If unsure, say N. + config TEST_UUID tristate "Test functions located in the uuid module at runtime" diff --git a/lib/Makefile b/lib/Makefile index 84c6dcb31fbb..02ab4c1a64d1 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -68,6 +68,7 @@ obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o obj-$(CONFIG_TEST_PRINTF) += test_printf.o obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o +obj-$(CONFIG_TEST_BITFIELD) += test_bitfield.o obj-$(CONFIG_TEST_UUID) += test_uuid.o obj-$(CONFIG_TEST_PARMAN) += test_parman.o obj-$(CONFIG_TEST_KMOD) += test_kmod.o diff --git a/lib/test_bitfield.c b/lib/test_bitfield.c new file mode 100644 index 000000000000..5b8f4108662d --- /dev/null +++ b/lib/test_bitfield.c @@ -0,0 +1,168 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Test cases for bitfield helpers. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/bitfield.h> + +#define CHECK_ENC_GET_U(tp, v, field, res) do { \ + { \ + u##tp _res; \ + \ + _res = u##tp##_encode_bits(v, field); \ + if (_res != res) { \ + pr_warn("u" #tp "_encode_bits(" #v ", " #field ") is 0x%llx != " #res "\n",\ + (u64)_res); \ + return -EINVAL; \ + } \ + if (u##tp##_get_bits(_res, field) != v) \ + return -EINVAL; \ + } \ + } while (0) + +#define CHECK_ENC_GET_LE(tp, v, field, res) do { \ + { \ + __le##tp _res; \ + \ + _res = le##tp##_encode_bits(v, field); \ + if (_res != cpu_to_le##tp(res)) { \ + pr_warn("le" #tp "_encode_bits(" #v ", " #field ") is 0x%llx != 0x%llx\n",\ + (u64)le##tp##_to_cpu(_res), \ + (u64)(res)); \ + return -EINVAL; \ + } \ + if (le##tp##_get_bits(_res, field) != v) \ + return -EINVAL; \ + } \ + } while (0) + +#define CHECK_ENC_GET_BE(tp, v, field, res) do { \ + { \ + __be##tp _res; \ + \ + _res = be##tp##_encode_bits(v, field); \ + if (_res != cpu_to_be##tp(res)) { \ + pr_warn("be" #tp "_encode_bits(" #v ", " #field ") is 0x%llx != 0x%llx\n",\ + (u64)be##tp##_to_cpu(_res), \ + (u64)(res)); \ + return -EINVAL; \ + } \ + if (be##tp##_get_bits(_res, field) != v) \ + return -EINVAL; \ + } \ + } while (0) + +#define CHECK_ENC_GET(tp, v, field, res) do { \ + CHECK_ENC_GET_U(tp, v, field, res); \ + CHECK_ENC_GET_LE(tp, v, field, res); \ + CHECK_ENC_GET_BE(tp, v, field, res); \ + } while (0) + +static int test_constants(void) +{ + /* + * NOTE + * This whole function compiles (or at least should, if everything + * is going according to plan) to nothing after optimisation. + */ + + CHECK_ENC_GET(16, 1, 0x000f, 0x0001); + CHECK_ENC_GET(16, 3, 0x00f0, 0x0030); + CHECK_ENC_GET(16, 5, 0x0f00, 0x0500); + CHECK_ENC_GET(16, 7, 0xf000, 0x7000); + CHECK_ENC_GET(16, 14, 0x000f, 0x000e); + CHECK_ENC_GET(16, 15, 0x00f0, 0x00f0); + + CHECK_ENC_GET_U(8, 1, 0x0f, 0x01); + CHECK_ENC_GET_U(8, 3, 0xf0, 0x30); + CHECK_ENC_GET_U(8, 14, 0x0f, 0x0e); + CHECK_ENC_GET_U(8, 15, 0xf0, 0xf0); + + CHECK_ENC_GET(32, 1, 0x00000f00, 0x00000100); + CHECK_ENC_GET(32, 3, 0x0000f000, 0x00003000); + CHECK_ENC_GET(32, 5, 0x000f0000, 0x00050000); + CHECK_ENC_GET(32, 7, 0x00f00000, 0x00700000); + CHECK_ENC_GET(32, 14, 0x0f000000, 0x0e000000); + CHECK_ENC_GET(32, 15, 0xf0000000, 0xf0000000); + + CHECK_ENC_GET(64, 1, 0x00000f0000000000ull, 0x0000010000000000ull); + CHECK_ENC_GET(64, 3, 0x0000f00000000000ull, 0x0000300000000000ull); + CHECK_ENC_GET(64, 5, 0x000f000000000000ull, 0x0005000000000000ull); + CHECK_ENC_GET(64, 7, 0x00f0000000000000ull, 0x0070000000000000ull); + CHECK_ENC_GET(64, 14, 0x0f00000000000000ull, 0x0e00000000000000ull); + CHECK_ENC_GET(64, 15, 0xf000000000000000ull, 0xf000000000000000ull); + + return 0; +} + +#define CHECK(tp, mask) do { \ + u64 v; \ + \ + for (v = 0; v < 1 << hweight32(mask); v++) \ + if (tp##_encode_bits(v, mask) != v << __ffs64(mask)) \ + return -EINVAL; \ + } while (0) + +static int test_variables(void) +{ + CHECK(u8, 0x0f); + CHECK(u8, 0xf0); + CHECK(u8, 0x38); + + CHECK(u16, 0x0038); + CHECK(u16, 0x0380); + CHECK(u16, 0x3800); + CHECK(u16, 0x8000); + + CHECK(u32, 0x80000000); + CHECK(u32, 0x7f000000); + CHECK(u32, 0x07e00000); + CHECK(u32, 0x00018000); + + CHECK(u64, 0x8000000000000000ull); + CHECK(u64, 0x7f00000000000000ull); + CHECK(u64, 0x0001800000000000ull); + CHECK(u64, 0x0000000080000000ull); + CHECK(u64, 0x000000007f000000ull); + CHECK(u64, 0x0000000018000000ull); + CHECK(u64, 0x0000001f8000000ull); + + return 0; +} + +static int __init test_bitfields(void) +{ + int ret = test_constants(); + + if (ret) { + pr_warn("constant tests failed!\n"); + return ret; + } + + ret = test_variables(); + if (ret) { + pr_warn("variable tests failed!\n"); + return ret; + } + +#ifdef TEST_BITFIELD_COMPILE + /* these should fail compilation */ + CHECK_ENC_GET(16, 16, 0x0f00, 0x1000); + u32_encode_bits(7, 0x06000000); + + /* this should at least give a warning */ + u16_encode_bits(0, 0x60000); +#endif + + pr_info("tests passed\n"); + + return 0; +} +module_init(test_bitfields) + +MODULE_AUTHOR("Johannes Berg <johannes@sipsolutions.net>"); +MODULE_LICENSE("GPL"); -- 2.14.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-06-27 16:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-20 6:58 [PATCH v6 (resend) 0/3] bitfield fixes and tests Johannes Berg 2018-06-20 6:58 ` [PATCH v6 (resend) 1/3] bitfield: fix *_encode_bits() Johannes Berg 2018-06-27 16:00 ` [v6,(resend),1/3] " Kalle Valo 2018-06-20 6:58 ` [PATCH v6 (resend) 2/3] bitfield: add u8 helpers Johannes Berg 2018-06-20 6:58 ` [PATCH v6 (resend) 3/3] bitfield: add tests Johannes Berg
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.