[PATCH v3] bitfield: fix *_encode_bits()

[PATCH v3] bitfield: fix *_encode_bits()

[Johannes Berg]

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.")
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
v2: replace stray tab by space
v3: u8 helpers, tests

If you don't mind, I'd like to take this through the networking
tree(s) as a fix since I have some pending code that depends on
it.
---
 include/linux/bitfield.h |   7 +-
 lib/Kconfig.debug        |   7 ++
 lib/Makefile             |   1 +
 lib/test_bitfield.c      | 163 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 175 insertions(+), 3 deletions(-)
 create mode 100644 lib/test_bitfield.c

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index cf2588d81148..65a6981eef7b 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,	\
@@ -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)
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..3b50067611d9
--- /dev/null
+++ b/lib/test_bitfield.c
@@ -0,0 +1,163 @@
+/*
+ * Test cases for bitfield helpers.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#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);
+/*
+ * This should fail compilation:
+ *	CHECK_ENC_GET(16, 16, 0x0f00, 0x1000);
+ */
+
+	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;
+	}
+
+	pr_info("tests passed\n");
+
+	return 0;
+}
+module_init(test_bitfields)
+
+MODULE_AUTHOR("Johannes Berg <johannes@sipsolutions.net>");
+MODULE_LICENSE("GPL");
-- 
2.14.4

Re: [PATCH v3] bitfield: fix *_encode_bits()

[Andy Shevchenko]

On Mon, Jun 18, 2018 at 10:56 PM, 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.
>

Thanks!
Few nitpicks / questions below, and I'm fine with the result!

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


> Fixes: 00b0c9b82663 ("Add primitives for manipulating bitfields both in host- and fixed-endian.")
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> v2: replace stray tab by space
> v3: u8 helpers, tests
>
> If you don't mind, I'd like to take this through the networking
> tree(s) as a fix since I have some pending code that depends on
> it.
> ---
>  include/linux/bitfield.h |   7 +-
>  lib/Kconfig.debug        |   7 ++
>  lib/Makefile             |   1 +
>  lib/test_bitfield.c      | 163 +++++++++++++++++++++++++++++++++++++++++++++++

I think would be better to add test cases first, followed by fix. (1
patch -> 2 patches)
In this case Fixes tag would be only for the fix part and backporting
(if needed) will be much easier.

>  4 files changed, 175 insertions(+), 3 deletions(-)
>  create mode 100644 lib/test_bitfield.c
>
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index cf2588d81148..65a6981eef7b 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,      \
> @@ -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,,)

Is this one you need, or it's just for sake of tests?

For me looks like for consistency we may add fake conversion macros
for this, such as

#define cpu_to_le8(x) x
#define le8_to_cpu(x) x
...
#undef le8_to_cpu
#undef cpu_to_le8

And do in the same way like below

__MAKE_OP(8)


This might be third patch w/o Fixes tag as well.

>  __MAKE_OP(16)
>  __MAKE_OP(32)
>  __MAKE_OP(64)
> 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..3b50067611d9
> --- /dev/null
> +++ b/lib/test_bitfield.c
> @@ -0,0 +1,163 @@

Perhaps
// SPDX... GPL-2.0+

> +/*
> + * Test cases for bitfield helpers.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +

> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>

Either module.h (if we can compile as a module) or just init.h otherwise.

> +#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);

> +/*
> + * This should fail compilation:
> + *     CHECK_ENC_GET(16, 16, 0x0f00, 0x1000);
> + */

Perhaps we need some ifdeffery around this. It would allow you to try
w/o altering the source code.

#ifdef TEST_BITFIELD_COMPILE
...
#endif


> +
> +       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;                         \

I guess you rather continue and print a statistics X passed out of Y.
Check how it's done, for example, in other test_* modules.
(test_printf.c comes first to my mind).

> +       } 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;
> +       }
> +
> +       pr_info("tests passed\n");
> +
> +       return 0;
> +}
> +module_init(test_bitfields)
> +
> +MODULE_AUTHOR("Johannes Berg <johannes@sipsolutions.net>");
> +MODULE_LICENSE("GPL");
> --
> 2.14.4
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] bitfield: fix *_encode_bits()

[Johannes Berg]

> I think would be better to add test cases first, followed by fix. (1
> patch -> 2 patches)
> In this case Fixes tag would be only for the fix part and backporting
> (if needed) will be much easier.

Can't, unless I introduce a compilation issue in the tests first? That
seems weird. But I guess I can do it the other way around.

> > @@ -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,,)
> 
> Is this one you need, or it's just for sake of tests?

All three ;-)

We'll probably need it eventually (we do have bytes to take bits out
of), for consistency I think we want it, and I wanted to add it to the
tests too.

> For me looks like for consistency we may add fake conversion macros
> for this, such as
> 
> #define cpu_to_le8(x) x
> #define le8_to_cpu(x) x
> ...
> #undef le8_to_cpu
> #undef cpu_to_le8
> 
> And do in the same way like below
> 
> __MAKE_OP(8)

I disagree with this. I don't see why we should have le8_encode_bits()
and be8_encode_bits() and friends, that makes no sense.

> Perhaps
> // SPDX... GPL-2.0+

Yeah, I guess I should have that.

> > +/*
> > + * Test cases for bitfield helpers.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> 
> Either module.h (if we can compile as a module) or just init.h otherwise.

It can be a module ... guess I cargo-culted that from another test.

> > +/*
> > + * This should fail compilation:
> > + *     CHECK_ENC_GET(16, 16, 0x0f00, 0x1000);
> > + */
> 
> Perhaps we need some ifdeffery around this. It would allow you to try
> w/o altering the source code.
> 
> #ifdef TEST_BITFIELD_COMPILE
> ...
> #endif

Yeah, I guess we could do that.

> I guess you rather continue and print a statistics X passed out of Y.
> Check how it's done, for example, in other test_* modules.
> (test_printf.c comes first to my mind).

I see it's done that way elsewhere, but I don't really see the point. It
makes the test code more complex, and if you fail here you'd better fix
it, and if you need a few iterations for that it's not really a problem?

johannes

Re: [PATCH v3] bitfield: fix *_encode_bits()

[Jakub Kicinski]

On Mon, 18 Jun 2018 22:28:03 +0200, Johannes Berg wrote:
> > For me looks like for consistency we may add fake conversion macros
> > for this, such as
> > 
> > #define cpu_to_le8(x) x
> > #define le8_to_cpu(x) x
> > ...
> > #undef le8_to_cpu
> > #undef cpu_to_le8
> > 
> > And do in the same way like below
> > 
> > __MAKE_OP(8)  
> 
> I disagree with this. I don't see why we should have le8_encode_bits()
> and be8_encode_bits() and friends, that makes no sense.

+1

Re: [PATCH v3] bitfield: fix *_encode_bits()

[Andy Shevchenko]

On Mon, Jun 18, 2018 at 11:28 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> I think would be better to add test cases first, followed by fix. (1
>> patch -> 2 patches)
>> In this case Fixes tag would be only for the fix part and backporting
>> (if needed) will be much easier.
>
> Can't, unless I introduce a compilation issue in the tests first? That
> seems weird. But I guess I can do it the other way around.

Works for me.

>
>> > @@ -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,,)
>>
>> Is this one you need, or it's just for sake of tests?
>
> All three ;-)
>
> We'll probably need it eventually (we do have bytes to take bits out
> of), for consistency I think we want it, and I wanted to add it to the
> tests too.

Okay, but I still think it makes sense to have this oneliner as a
separate patch.

> I disagree with this. I don't see why we should have le8_encode_bits()
> and be8_encode_bits() and friends, that makes no sense.

OK, it was just a proposal.

>> I guess you rather continue and print a statistics X passed out of Y.
>> Check how it's done, for example, in other test_* modules.
>> (test_printf.c comes first to my mind).
>
> I see it's done that way elsewhere, but I don't really see the point. It
> makes the test code more complex, and if you fail here you'd better fix
> it, and if you need a few iterations for that it's not really a problem?

The idea is to print what was the input, expected output and actual result.
Then you would see what exactly is broken.

I dunno how much we may take away from this certain test case, though
it would be better for my opinion.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] bitfield: fix *_encode_bits()

[Johannes Berg]

On Mon, 2018-06-18 at 23:40 +0300, Andy Shevchenko wrote:

> The idea is to print what was the input, expected output and actual result.
> Then you would see what exactly is broken.

Yeah, I guess we could. I did some of that work.

> I dunno how much we may take away from this certain test case, though
> it would be better for my opinion.

TBH though, I'm not sure I want to do this (right now at least). I don't
think we gain anything from it, it's so basic ... so to me it's just
pointless extra code.

johannes

Re: [PATCH v3] bitfield: fix *_encode_bits()

[Andy Shevchenko]

On Mon, Jun 18, 2018 at 11:42 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2018-06-18 at 23:40 +0300, Andy Shevchenko wrote:
>
>> The idea is to print what was the input, expected output and actual result.
>> Then you would see what exactly is broken.
>
> Yeah, I guess we could. I did some of that work.
>
>> I dunno how much we may take away from this certain test case, though
>> it would be better for my opinion.
>
> TBH though, I'm not sure I want to do this (right now at least). I don't
> think we gain anything from it, it's so basic ... so to me it's just
> pointless extra code.

I'm not insisting I'm trying to specify rationale behind it.
We may add this later at some point.

-- 
With Best Regards,
Andy Shevchenko