* Re: [PATCH v4 2/3] bitfield: add u8 helpers
From: Andy Shevchenko @ 2018-06-18 20:42 UTC (permalink / raw)
To: Johannes Berg; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <20180618203750.28658-2-johannes@sipsolutions.net>
On Mon, Jun 18, 2018 at 11:37 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> 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
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3] bitfield: fix *_encode_bits()
From: Johannes Berg @ 2018-06-18 20:42 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <CAHp75VesXgrh-XR-3bFMOF6eE0K=NyRvyY1tw1yOX44rJS9fag@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH v4 3/3] bitfield: add tests
From: Andy Shevchenko @ 2018-06-18 20:44 UTC (permalink / raw)
To: Johannes Berg; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <20180618203750.28658-3-johannes@sipsolutions.net>
On Mon, Jun 18, 2018 at 11:37 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> 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.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Though license mismatch. Either GPL-2.0+ for SPDX, or "GPL v2" for _LICENSE().
>
> Suggested-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..c9bf2d542244
> --- /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
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3] bitfield: fix *_encode_bits()
From: Andy Shevchenko @ 2018-06-18 20:46 UTC (permalink / raw)
To: Johannes Berg; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <1529354549.3092.36.camel@sipsolutions.net>
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
^ permalink raw reply
* Re: [PATCH v4 3/3] bitfield: add tests
From: Johannes Berg @ 2018-06-18 20:47 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linux Kernel Mailing List, netdev, Al Viro
In-Reply-To: <CAHp75VfeMBr57Frzvm2vJ2irJQat-6a+_dN9mtftAnwfPT=Bww@mail.gmail.com>
On Mon, 2018-06-18 at 23:44 +0300, Andy Shevchenko wrote:
> On Mon, Jun 18, 2018 at 11:37 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > 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.
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> Though license mismatch. Either GPL-2.0+ for SPDX, or "GPL v2" for _LICENSE().
Sigh. Yeah, I guess I'll resend. That's what I get for copy/pasting.
johannes
^ permalink raw reply
* [PATCH v5 1/3] bitfield: fix *_encode_bits()
From: Johannes Berg @ 2018-06-18 20:48 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: 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)
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 | 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
* [PATCH v5 2/3] bitfield: add u8 helpers
From: Johannes Berg @ 2018-06-18 20:48 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: Al Viro, Andy Shevchenko
In-Reply-To: <20180618204816.30806-1-johannes@sipsolutions.net>
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
* [PATCH v5 3/3] bitfield: add tests
From: Johannes Berg @ 2018-06-18 20:48 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: Al Viro, Andy Shevchenko
In-Reply-To: <20180618204816.30806-1-johannes@sipsolutions.net>
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
* Re: [PATCH rdma-next v2 06/20] IB/uverbs: Add PTR_IN attributes that are allocated/copied automatically
From: Jason Gunthorpe @ 2018-06-18 20:48 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Joonas Lahtinen,
Yishai Hadas, Saeed Mahameed, linux-netdev
In-Reply-To: <20180617100006.30663-7-leon@kernel.org>
On Sun, Jun 17, 2018 at 12:59:52PM +0300, Leon Romanovsky wrote:
> From: Matan Barak <matanb@mellanox.com>
>
> Adding UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY flag to PTR_IN attributes.
> By using this flag, the parse automatically allocates and copies the
> user-space data. This data is accessible by using uverbs_attr_get_len
> and uverbs_attr_get_alloced_ptr inline accessor functions from the
> handler.
>
> Signed-off-by: Matan Barak <matanb@mellanox.com>
> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/core/uverbs_ioctl.c | 25 ++++++++++++++++++++++++-
> include/rdma/uverbs_ioctl.h | 25 +++++++++++++++++++++++++
> 2 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
> index 8cc3e8dad9b5..ee15c9ca788b 100644
> +++ b/drivers/infiniband/core/uverbs_ioctl.c
> @@ -114,7 +114,26 @@ static int uverbs_process_attr(struct ib_device *ibdev,
> uattr->attr_data.reserved)
> return -EINVAL;
>
> - e->ptr_attr.data = uattr->data;
> + if (val_spec->flags & UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY &&
> + uattr->len > sizeof(((struct ib_uverbs_attr *)0)->data)) {
Why open-code uverbs_attr_ptr_is_inline() ?
> diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
> index bd6bba3a6e04..0e6f782727bd 100644
> +++ b/include/rdma/uverbs_ioctl.h
> @@ -65,6 +65,8 @@ enum {
> UVERBS_ATTR_SPEC_F_MANDATORY = 1U << 0,
> /* Support extending attributes by length, validate all unknown size == zero */
> UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO = 1U << 1,
> + /* Valid only for PTR_IN. Allocate and copy the data inside the parser */
> + UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY = 1U << 2,
> };
>
> /* Specification of a single attribute inside the ioctl message */
> @@ -431,6 +433,17 @@ static inline struct ib_uobject *uverbs_attr_get_uobject(const struct uverbs_att
> return attr->obj_attr.uobject;
> }
>
> +static inline int uverbs_attr_get_len(const struct uverbs_attr_bundle *attrs_bundle,
> + u16 idx)
> +{
> + const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx);
> +
> + if (IS_ERR(attr))
> + return PTR_ERR(attr);
> +
> + return attr->ptr_attr.len;
> +}
> +
> static inline int uverbs_copy_to(const struct uverbs_attr_bundle *attrs_bundle,
> size_t idx, const void *from, size_t size)
> {
> @@ -457,6 +470,18 @@ static inline bool uverbs_attr_ptr_is_inline(const struct uverbs_attr *attr)
> return attr->ptr_attr.len <= sizeof(attr->ptr_attr.data);
> }
>
> +static inline void *uverbs_attr_get_alloced_ptr(const struct uverbs_attr_bundle *attrs_bundle,
> + u16 idx)
> +{
> + const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx);
> +
> + if (IS_ERR(attr))
> + return (void *)attr;
> +
> + return uverbs_attr_ptr_is_inline(attr) ? u64_to_ptr(void *, attr->ptr_attr.data) :
> + u64_to_ptr(void, attr->ptr_attr.data);
WTF is this:
u64_to_ptr(void *, attr->ptr_attr.data)
That returns attr->ptr_attr.data casted to a void **, then casts it to
a void * - which is identical to u64_to_ptr(void, attr->ptr_attr.data)
It should be &attr->ptr_attr.data. And the return should be const, the
caller shouldn't be mutating the copy.
All this use of u64_to_ptr is ugly needless obfuscation here, and
look, it causes bugs. Use a union.
Like this:
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index 8cc3e8dad9b506..82c5d33195dfc7 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -114,9 +114,27 @@ static int uverbs_process_attr(struct ib_device *ibdev,
uattr->attr_data.reserved)
return -EINVAL;
- e->ptr_attr.data = uattr->data;
e->ptr_attr.len = uattr->len;
e->ptr_attr.flags = uattr->flags;
+
+ if (val_spec->flags & UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY &&
+ !uverbs_attr_ptr_is_inline(e)) {
+ void *p;
+
+ p = kvmalloc(uattr->len, GFP_KERNEL);
+ if (!p)
+ return -ENOMEM;
+
+ e->ptr_attr.ptr = p;
+
+ if (copy_from_user(p, u64_to_user_ptr(uattr->data),
+ uattr->len)) {
+ kvfree(p);
+ return -EFAULT;
+ }
+ } else {
+ e->ptr_attr.data = uattr->data;
+ }
break;
case UVERBS_ATTR_TYPE_IDR:
@@ -201,6 +219,10 @@ static int uverbs_finalize_attrs(struct uverbs_attr_bundle *attrs_bundle,
commit);
if (!ret)
ret = current_ret;
+ } else if (spec->type == UVERBS_ATTR_TYPE_PTR_IN &&
+ spec->flags & UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY &&
+ !uverbs_attr_ptr_is_inline(attr)) {
+ kvfree(attr->ptr_attr.ptr);
}
}
}
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index bd6bba3a6e04a1..6e1c322ff5c015 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -65,6 +65,8 @@ enum {
UVERBS_ATTR_SPEC_F_MANDATORY = 1U << 0,
/* Support extending attributes by length, validate all unknown size == zero */
UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO = 1U << 1,
+ /* Valid only for PTR_IN. Allocate and copy the data inside the parser */
+ UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY = 1U << 2,
};
/* Specification of a single attribute inside the ioctl message */
@@ -323,7 +325,15 @@ struct uverbs_object_tree_def {
*/
struct uverbs_ptr_attr {
- u64 data;
+ /*
+ * If UVERBS_ATTR_SPEC_F_ALLOC_AND_COPY is set then the 'ptr' is
+ * used.
+ */
+ union
+ {
+ void *ptr;
+ u64 data;
+ };
u16 len;
/* Combination of bits from enum UVERBS_ATTR_F_XXXX */
u16 flags;
@@ -431,6 +441,17 @@ static inline struct ib_uobject *uverbs_attr_get_uobject(const struct uverbs_att
return attr->obj_attr.uobject;
}
+static inline int uverbs_attr_get_len(const struct uverbs_attr_bundle *attrs_bundle,
+ u16 idx)
+{
+ const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx);
+
+ if (IS_ERR(attr))
+ return PTR_ERR(attr);
+
+ return attr->ptr_attr.len;
+}
+
static inline int uverbs_copy_to(const struct uverbs_attr_bundle *attrs_bundle,
size_t idx, const void *from, size_t size)
{
@@ -457,6 +478,18 @@ static inline bool uverbs_attr_ptr_is_inline(const struct uverbs_attr *attr)
return attr->ptr_attr.len <= sizeof(attr->ptr_attr.data);
}
+static inline const void *uverbs_attr_get_alloced_ptr(
+ const struct uverbs_attr_bundle *attrs_bundle, u16 idx)
+{
+ const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx);
+
+ if (IS_ERR(attr))
+ return (void *)attr;
+
+ return uverbs_attr_ptr_is_inline(attr) ? &attr->ptr_attr.data :
+ attr->ptr_attr.ptr;
+}
+
static inline int _uverbs_copy_from(void *to,
const struct uverbs_attr_bundle *attrs_bundle,
size_t idx,
^ permalink raw reply related
* Re: [PATCH 1/4] net/ncsi: Silence debug messages
From: Joe Perches @ 2018-06-18 20:49 UTC (permalink / raw)
To: Joel Stanley, Samuel Mendoza-Jonas, David S . Miller; +Cc: netdev
In-Reply-To: <20180618071916.6765-2-joel@jms.id.au>
On Mon, 2018-06-18 at 16:49 +0930, Joel Stanley wrote:
> In normal operation we see this series of messages as the host drives
> the network device:
>
> ftgmac100 1e660000.ethernet eth0: NCSI: LSC AEN - channel 0 state down
> ftgmac100 1e660000.ethernet eth0: NCSI: suspending channel 0
[...]
> This makes all of these messages netdev_dbg. They are still useful to
> debug eg. misbehaving network device firmware, but we do not need them
> filling up the kernel logs in normal operation.
trivia:
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
[]
> @@ -1735,7 +1735,7 @@ static void ftgmac100_ncsi_handler(struct ncsi_dev *nd)
> if (unlikely(nd->state != ncsi_dev_state_functional))
> return;
>
> - netdev_info(nd->dev, "NCSI interface %s\n",
> + netdev_dbg(nd->dev, "NCSI interface %s\n",
> nd->link_up ? "up" : "down");
It's nicer to realign the multiple line statements
to the open parenthesis
> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
[]
> @@ -73,8 +73,8 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv *ndp,
> ncm->data[2] = data;
> ncm->data[4] = ntohl(lsc->oem_status);
>
> - netdev_info(ndp->ndev.dev, "NCSI: LSC AEN - channel %u state %s\n",
> - nc->id, data & 0x1 ? "up" : "down");
> + netdev_dbg(ndp->ndev.dev, "NCSI: LSC AEN - channel %u state %s\n",
> + nc->id, data & 0x1 ? "up" : "down");
as was done for the rest of these...
^ permalink raw reply
* Re: [PATCH 3/4] net/ncsi: Use netdev_dbg for debug messages
From: Joe Perches @ 2018-06-18 20:53 UTC (permalink / raw)
To: Joel Stanley, Samuel Mendoza-Jonas, David S . Miller; +Cc: netdev
In-Reply-To: <20180618071916.6765-4-joel@jms.id.au>
On Mon, 2018-06-18 at 16:49 +0930, Joel Stanley wrote:
> This moves all of the netdev_printk(KERN_DEBUG, ...) messages over to
> netdev_dbg. There is no change in behaviour.
Not quite, but I think the patch is fine anyway.
netdev_printk(KERN_DEBUG ... is always emitted as
long as the console level includes debug output.
netdev_dbg is not included in object code unless
DEBUG is defined or CONFIG_DYNAMIC_DEBUG is set.
And then, it is not emitted into the log unless
DEBUG is set or this specific netdev_dbg is enabled
via the dynamic debug control file.
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> net/ncsi/ncsi-aen.c | 6 +++---
> net/ncsi/ncsi-manage.c | 33 +++++++++++++++------------------
> 2 files changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> index f899ed61bb57..25e483e8278b 100644
> --- a/net/ncsi/ncsi-aen.c
> +++ b/net/ncsi/ncsi-aen.c
> @@ -148,9 +148,9 @@ static int ncsi_aen_handler_hncdsc(struct ncsi_dev_priv *ndp,
> hncdsc = (struct ncsi_aen_hncdsc_pkt *)h;
> ncm->data[3] = ntohl(hncdsc->status);
> spin_unlock_irqrestore(&nc->lock, flags);
> - netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> - "NCSI: host driver %srunning on channel %u\n",
> - ncm->data[3] & 0x1 ? "" : "not ", nc->id);
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: host driver %srunning on channel %u\n",
> + ncm->data[3] & 0x1 ? "" : "not ", nc->id);
>
> return 0;
> }
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 716493a61ba6..091284760d21 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -788,8 +788,8 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> }
> break;
> case ncsi_dev_state_config_done:
> - netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> - "NCSI: channel %u config done\n", nc->id);
> + netdev_dbg(ndp->ndev.dev, "NCSI: channel %u config done\n",
> + nc->id);
> spin_lock_irqsave(&nc->lock, flags);
> if (nc->reconfigure_needed) {
> /* This channel's configuration has been updated
> @@ -804,8 +804,7 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> - netdev_printk(KERN_DEBUG, dev,
> - "Dirty NCSI channel state reset\n");
> + netdev_dbg(dev, "Dirty NCSI channel state reset\n");
> ncsi_process_next_channel(ndp);
> break;
> }
> @@ -908,9 +907,9 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
> }
>
> ncm = &found->modes[NCSI_MODE_LINK];
> - netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> - "NCSI: Channel %u added to queue (link %s)\n",
> - found->id, ncm->data[2] & 0x1 ? "up" : "down");
> + netdev_dbg(ndp->ndev.dev,
> + "NCSI: Channel %u added to queue (link %s)\n",
> + found->id, ncm->data[2] & 0x1 ? "up" : "down");
>
> out:
> spin_lock_irqsave(&ndp->lock, flags);
> @@ -1316,9 +1315,9 @@ static int ncsi_kick_channels(struct ncsi_dev_priv *ndp)
> if ((ndp->ndev.state & 0xff00) ==
> ncsi_dev_state_config ||
> !list_empty(&nc->link)) {
> - netdev_printk(KERN_DEBUG, nd->dev,
> - "NCSI: channel %p marked dirty\n",
> - nc);
> + netdev_dbg(nd->dev,
> + "NCSI: channel %p marked dirty\n",
> + nc);
> nc->reconfigure_needed = true;
> }
> spin_unlock_irqrestore(&nc->lock, flags);
> @@ -1336,8 +1335,7 @@ static int ncsi_kick_channels(struct ncsi_dev_priv *ndp)
> list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> spin_unlock_irqrestore(&ndp->lock, flags);
>
> - netdev_printk(KERN_DEBUG, nd->dev,
> - "NCSI: kicked channel %p\n", nc);
> + netdev_dbg(nd->dev, "NCSI: kicked channel %p\n", nc);
> n++;
> }
> }
> @@ -1368,8 +1366,8 @@ int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
> list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list) {
> n_vids++;
> if (vlan->vid == vid) {
> - netdev_printk(KERN_DEBUG, dev,
> - "NCSI: vid %u already registered\n", vid);
> + netdev_dbg(dev, "NCSI: vid %u already registered\n",
> + vid);
> return 0;
> }
> }
> @@ -1388,7 +1386,7 @@ int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
> vlan->vid = vid;
> list_add_rcu(&vlan->list, &ndp->vlan_vids);
>
> - netdev_printk(KERN_DEBUG, dev, "NCSI: Added new vid %u\n", vid);
> + netdev_dbg(dev, "NCSI: Added new vid %u\n", vid);
>
> found = ncsi_kick_channels(ndp) != 0;
>
> @@ -1417,8 +1415,7 @@ int ncsi_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
> /* Remove the VLAN id from our internal list */
> list_for_each_entry_safe(vlan, tmp, &ndp->vlan_vids, list)
> if (vlan->vid == vid) {
> - netdev_printk(KERN_DEBUG, dev,
> - "NCSI: vid %u found, removing\n", vid);
> + netdev_dbg(dev, "NCSI: vid %u found, removing\n", vid);
> list_del_rcu(&vlan->list);
> found = true;
> kfree(vlan);
> @@ -1545,7 +1542,7 @@ void ncsi_stop_dev(struct ncsi_dev *nd)
> }
> }
>
> - netdev_printk(KERN_DEBUG, ndp->ndev.dev, "NCSI: Stopping device\n");
> + netdev_dbg(ndp->ndev.dev, "NCSI: Stopping device\n");
> ncsi_report_link(ndp, true);
> }
> EXPORT_SYMBOL_GPL(ncsi_stop_dev);
^ permalink raw reply
* Re: [PATCH bpf-net] bpf: Change bpf_fib_lookup to return lookup status
From: Martin KaFai Lau @ 2018-06-18 20:55 UTC (permalink / raw)
To: David Ahern; +Cc: dsahern, netdev, borkmann, ast, davem
In-Reply-To: <780331bd-947a-83fe-6e62-c0efc05cfc04@gmail.com>
On Mon, Jun 18, 2018 at 12:27:07PM -0600, David Ahern wrote:
> On 6/18/18 12:11 PM, Martin KaFai Lau wrote:
> > On Sun, Jun 17, 2018 at 08:18:19AM -0700, dsahern@kernel.org wrote:
> >> From: David Ahern <dsahern@gmail.com>
> >>
> >> For ACLs implemented using either FIB rules or FIB entries, the BPF
> >> program needs the FIB lookup status to be able to drop the packet.
> > Except BPF_FIB_LKUP_RET_SUCCESS and BPF_FIB_LKUP_RET_NO_NEIGH, can you
> > give an example on how the xdp_prog may decide XDP_PASS vs XDP_DROP based
> > on other BPF_FIB_LKUP_RET_*?
> >
>
> rc = bpf_fib_lookup(ctx, &fib_params, sizeof(fib_params), flags);
> if (rc == 0)
> packet is forwarded, do the redirect
>
> /* the program is misconfigured -- wrong parameters in struct or flags */
> if (rc < 0)
> ....
>
> /* rc > 0 case */
> switch(rc) {
> case BPF_FIB_LKUP_RET_BLACKHOLE:
> case BPF_FIB_LKUP_RET_UNREACHABLE:
> case BPF_FIB_LKUP_RET_PROHIBIT:
> return XDP_DROP;
> }
>
> For the others it becomes a question of do we share why the stack needs
> to be involved? Maybe the program wants to collect stats to show traffic
> patterns that can be improved (BPF_FIB_LKUP_RET_FRAG_NEEDED) or support
> in the kernel needs to be improved (BPF_FIB_LKUP_RET_UNSUPP_LWT) or an
> interface is misconfigured (BPF_FIB_LKUP_RET_FWD_DISABLED).
Thanks for the explanation.
Agree on the bpf able to collect stats will be useful.
I am wondering, if a new BPF_FIB_LKUP_RET_XYZ is added later,
how may the old xdp_prog work/not-work? As of now, the return value
is straight forward, FWD, PASS (to stack) or DROP (error).
With this change, the xdp_prog needs to match/switch() the
BPF_FIB_LKUP_RET_* to at least PASS and DROP.
>
> Arguably BPF_FIB_LKUP_RET_NO_NHDEV is not needed. See below.
>
> >> @@ -2612,6 +2613,19 @@ struct bpf_raw_tracepoint_args {
> >> #define BPF_FIB_LOOKUP_DIRECT BIT(0)
> >> #define BPF_FIB_LOOKUP_OUTPUT BIT(1)
> >>
> >> +enum {
> >> + BPF_FIB_LKUP_RET_SUCCESS, /* lookup successful */
> >> + BPF_FIB_LKUP_RET_BLACKHOLE, /* dest is blackholed */
> >> + BPF_FIB_LKUP_RET_UNREACHABLE, /* dest is unreachable */
> >> + BPF_FIB_LKUP_RET_PROHIBIT, /* dest not allowed */
> >> + BPF_FIB_LKUP_RET_NOT_FWDED, /* pkt is not forwardded */
> > BPF_FIB_LKUP_RET_NOT_FWDED is a catch all?
> >
>
> Destination is local. More precisely, the FIB lookup is not unicast so
> not forwarded. It could be RTN_LOCAL, RTN_BROADCAST, RTN_ANYCAST, or
> RTN_MULTICAST. The next ones -- blackhole, reachable, prohibit -- are
> called out.
I think it also includes the tbid not found case.
>
> >> @@ -4252,16 +4277,19 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
> >> if (check_mtu) {
> >> mtu = ipv6_stub->ip6_mtu_from_fib6(f6i, dst, src);
> >> if (params->tot_len > mtu)
> >> - return 0;
> >> + return BPF_FIB_LKUP_RET_FRAG_NEEDED;
> >> }
> >>
> >> if (f6i->fib6_nh.nh_lwtstate)
> >> - return 0;
> >> + return BPF_FIB_LKUP_RET_UNSUPP_LWT;
> >>
> >> if (f6i->fib6_flags & RTF_GATEWAY)
> >> *dst = f6i->fib6_nh.nh_gw;
> >>
> >> dev = f6i->fib6_nh.nh_dev;
> >> + if (unlikely(!dev))
> >> + return BPF_FIB_LKUP_RET_NO_NHDEV;
> > Is this a bug fix?
> >
>
> Difference between IPv4 and IPv6. Making them consistent.
>
> It is a major BUG in the kernel to reach this point in either protocol
> to have a unicast route not tied to a device. IPv4 has checks; v6 does
> not. I figured this being new code, why not make bpf_ipv{4,6}_fib_lookup
> as close to the same as possible.
Make sense. A comment in the commit log will be useful if there is a
re-spin.
^ permalink raw reply
* Re: [bpf PATCH v2 2/6] bpf: sockmap only allow ESTABLISHED sock state
From: Martin KaFai Lau @ 2018-06-18 21:17 UTC (permalink / raw)
To: John Fastabend; +Cc: ast, daniel, netdev
In-Reply-To: <783d9ac4-3291-04cf-98a7-a05f31a833e4@gmail.com>
On Mon, Jun 18, 2018 at 07:50:19AM -0700, John Fastabend wrote:
> On 06/14/2018 05:18 PM, Martin KaFai Lau wrote:
> > On Thu, Jun 14, 2018 at 09:44:52AM -0700, John Fastabend wrote:
> >> Per the note in the TLS ULP (which is actually a generic statement
> >> regarding ULPs)
> >>
> >> /* The TLS ulp is currently supported only for TCP sockets
> >> * in ESTABLISHED state.
> >> * Supporting sockets in LISTEN state will require us
> >> * to modify the accept implementation to clone rather then
> >> * share the ulp context.
> >> */
> > Can you explain how that apply to bpf_tcp ulp?
> >
> > My understanding is the "ulp context" referred in TLS ulp is
> > the tls_context stored in icsk_ulp_data but I don't see bpf_tcp's
> > ulp is using icsk_ulp_data.
> >
> > Others LGTM.
> >
>
> So I think you are right we could probably allow it
> here but I am thinking I'll leave the check for now
> anyways for a couple reasons. First, we will shortly
> add support to allow ULP types to coexist. At the moment
> the two ULP types can not coexist. When this happens it
> looks like we will need to restrict to only ESTABLISHED
> types or somehow make all ULPs work in all states.
>
> Second, I don't have any use cases (nor can I think of
> any) for the sock{map|hash} ULP to be running on a non
> ESTABLISHED socket. Its not clear to me that having the
> sendmsg/sendpage hooks for a LISTEN socket makes sense.
> I would rather restrict it now and if we add something
> later where it makes sense to run on non-ESTABLISHED
> socks we can remove the check.
Make sense if there is no use case. It will be helpful if the commit log
is updated accordingly. Thanks!
Acked-by: Martin KaFai Lau <kafai@fb.com>
^ permalink raw reply
* Debido al no el pago de sus impuestos se hara efectivo un embargo bancario
From: Dian @ 2018-06-18 20:02 UTC (permalink / raw)
[-- Attachment #1: Type: text/plain, Size: 436 bytes --]
Estimado contribuyente:
Es nuestro deber informarle que debido al no pago de sus impuestos se hara efectivo un embargo bancario en el dia de hoy.
Por la seguridad de sus datos hemos adjuntado un documento con su deuda a la fecha con una clave la cual es :
421e68dd993c4a8bb9e3d5e6c066946r
Este documento contiene su deuda a la fecha y todos los datos del proceso detalladamente.
esperamos pronta respuesta
[-- Attachment #2: estado de cuenta.rar --]
[-- Type: *, Size: 73692 bytes --]
^ permalink raw reply
* Re: [PATCH bpf-net] bpf: Change bpf_fib_lookup to return lookup status
From: David Ahern @ 2018-06-18 21:35 UTC (permalink / raw)
To: Martin KaFai Lau; +Cc: dsahern, netdev, borkmann, ast, davem
In-Reply-To: <20180618205537.2j645mfujdsqxf2b@kafai-mbp.dhcp.thefacebook.com>
On 6/18/18 2:55 PM, Martin KaFai Lau wrote:
>> /* rc > 0 case */
>> switch(rc) {
>> case BPF_FIB_LKUP_RET_BLACKHOLE:
>> case BPF_FIB_LKUP_RET_UNREACHABLE:
>> case BPF_FIB_LKUP_RET_PROHIBIT:
>> return XDP_DROP;
>> }
>>
>> For the others it becomes a question of do we share why the stack needs
>> to be involved? Maybe the program wants to collect stats to show traffic
>> patterns that can be improved (BPF_FIB_LKUP_RET_FRAG_NEEDED) or support
>> in the kernel needs to be improved (BPF_FIB_LKUP_RET_UNSUPP_LWT) or an
>> interface is misconfigured (BPF_FIB_LKUP_RET_FWD_DISABLED).
> Thanks for the explanation.
>
> Agree on the bpf able to collect stats will be useful.
>
> I am wondering, if a new BPF_FIB_LKUP_RET_XYZ is added later,
> how may the old xdp_prog work/not-work? As of now, the return value
> is straight forward, FWD, PASS (to stack) or DROP (error).
> With this change, the xdp_prog needs to match/switch() the
> BPF_FIB_LKUP_RET_* to at least PASS and DROP.
IMO, programs should only call XDP_DROP for known reasons - like the 3
above. Anything else punt to the stack.
If a new RET_XYZ comes along:
1. the new XYZ is a new ACL response where the packet is to be dropped.
If the program does not understand XYZ and punts to the stack
(recommendation), then a second lookup is done during normal packet
processing and the stack drops it.
2. the new XYZ is a new path in the kernel that is unsupported with
respect to XDP forwarding, nothing new for the program to do.
Either way I would expect stats on BPF_FIB_LKUP_RET_* to give a hint to
the program writer.
Worst case of punting packets to the stack for any rc != 0 means the
stack is doing 2 lookups - 1 in XDP based on its lookup parameters and 1
in normal stack processing - to handle the packet.
>
>>
>> Arguably BPF_FIB_LKUP_RET_NO_NHDEV is not needed. See below.
>>
>>>> @@ -2612,6 +2613,19 @@ struct bpf_raw_tracepoint_args {
>>>> #define BPF_FIB_LOOKUP_DIRECT BIT(0)
>>>> #define BPF_FIB_LOOKUP_OUTPUT BIT(1)
>>>>
>>>> +enum {
>>>> + BPF_FIB_LKUP_RET_SUCCESS, /* lookup successful */
>>>> + BPF_FIB_LKUP_RET_BLACKHOLE, /* dest is blackholed */
>>>> + BPF_FIB_LKUP_RET_UNREACHABLE, /* dest is unreachable */
>>>> + BPF_FIB_LKUP_RET_PROHIBIT, /* dest not allowed */
>>>> + BPF_FIB_LKUP_RET_NOT_FWDED, /* pkt is not forwardded */
>>> BPF_FIB_LKUP_RET_NOT_FWDED is a catch all?
>>>
>>
>> Destination is local. More precisely, the FIB lookup is not unicast so
>> not forwarded. It could be RTN_LOCAL, RTN_BROADCAST, RTN_ANYCAST, or
>> RTN_MULTICAST. The next ones -- blackhole, reachable, prohibit -- are
>> called out.
> I think it also includes the tbid not found case.
Another one of those "should never happen scenarios". The user does not
specify the table; it is retrieved based on device association. Table
defaults to the main table - which always exists - and any VRF
enslavement of a device happens after the VRF device creates the table.
>
>>
>>>> @@ -4252,16 +4277,19 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>>>> if (check_mtu) {
>>>> mtu = ipv6_stub->ip6_mtu_from_fib6(f6i, dst, src);
>>>> if (params->tot_len > mtu)
>>>> - return 0;
>>>> + return BPF_FIB_LKUP_RET_FRAG_NEEDED;
>>>> }
>>>>
>>>> if (f6i->fib6_nh.nh_lwtstate)
>>>> - return 0;
>>>> + return BPF_FIB_LKUP_RET_UNSUPP_LWT;
>>>>
>>>> if (f6i->fib6_flags & RTF_GATEWAY)
>>>> *dst = f6i->fib6_nh.nh_gw;
>>>>
>>>> dev = f6i->fib6_nh.nh_dev;
>>>> + if (unlikely(!dev))
>>>> + return BPF_FIB_LKUP_RET_NO_NHDEV;
>>> Is this a bug fix?
>>>
>>
>> Difference between IPv4 and IPv6. Making them consistent.
>>
>> It is a major BUG in the kernel to reach this point in either protocol
>> to have a unicast route not tied to a device. IPv4 has checks; v6 does
>> not. I figured this being new code, why not make bpf_ipv{4,6}_fib_lookup
>> as close to the same as possible.
> Make sense. A comment in the commit log will be useful if there is a
> re-spin.
>
ok.
^ permalink raw reply
* Re: [PATCH] tc, bpf: add option to dump bpf verifier as C program fragment
From: David Ahern @ 2018-06-18 21:44 UTC (permalink / raw)
To: Jakub Kicinski, Ophir Munk
Cc: netdev, Stephen Hemminger, Thomas Monjalon, Olga Shern
In-Reply-To: <20180618131842.7b4d259f@cakuba.netronome.com>
[-- Attachment #1: Type: text/plain, Size: 1670 bytes --]
On 6/18/18 2:18 PM, Jakub Kicinski wrote:
> On Sun, 17 Jun 2018 08:48:41 +0000, Ophir Munk wrote:
>> Similar to cbpf used within tcpdump utility with a "-d" option to dump
>> the compiled packet-matching code in a human readable form - tc has the
>> "verbose" option to dump ebpf verifier output.
>> Another useful option of cbpf using tcpdump "-dd" option is to dump
>> packet-matching code a C program fragment. Similar to this - this commit
>> adds a new tc ebpf option named "code" to dump ebpf verifier as C program
>> fragment.
>>
>> Existing "verbose" option sample output:
>>
>> Verifier analysis:
>> 0: (61) r2 = *(u32 *)(r1 +52)
>> 1: (18) r3 = 0xdeadbeef
>> 3: (63) *(u32 *)(r10 -4) = r3
>> .
>> .
>> 11: (63) *(u32 *)(r1 +52) = r2
>> 12: (18) r0 = 0xffffffff
>> 14: (95) exit
>>
>> New "code" option sample output:
>>
>> /* struct bpf_insn cls_q_code[] = { */
>> {0x61, 2, 1, 52, 0x00000000},
>> {0x18, 3, 0, 0, 0xdeadbeef},
>> {0x00, 0, 0, 0, 0x00000000},
>> .
>> .
>> {0x63, 1, 2, 52, 0x00000000},
>> {0x18, 0, 0, 0, 0xffffffff},
>> {0x00, 0, 0, 0, 0x00000000},
>> {0x95, 0, 0, 0, 0x00000000},
>>
>> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
>
> Hmm... printing C arrays looks like hacky integration with some C
> code... Would you not be better served by simply using libbpf in
> whatever is consuming this output?
>
I was thinking the same. bpftool would provide options too -- print the
above, print in macro encodings and verifier. I gave an example of this
side by side dump at netconf 2.1. Does not look like the slides made it
online; see attached.
[-- Attachment #2: netconf-2.1-bpf.pdf --]
[-- Type: application/pdf, Size: 38266 bytes --]
^ permalink raw reply
* Re: [PATCH rdma-next v2 00/20] Introduce mlx5 DEVX interface
From: Jason Gunthorpe @ 2018-06-18 22:05 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Joonas Lahtinen,
Matan Barak, Yishai Hadas, Saeed Mahameed, linux-netdev
In-Reply-To: <20180617100006.30663-1-leon@kernel.org>
On Sun, Jun 17, 2018 at 12:59:46PM +0300, Leon Romanovsky wrote:
> Leon Romanovsky (2):
> drm/i915: Move u64-to-ptr helpers to general header
> kernel.h: Reuse u64_to_ptr macro to cast __user pointers
I dropped these since they are not needed by this series when using a
union.
> Matan Barak (5):
> IB/uverbs: Export uverbs idr and fd types
> IB/uverbs: Add PTR_IN attributes that are allocated/copied
> automatically
Revised this one, as noted
> IB/uverbs: Add a macro to define a type with no kernel known size
> IB/uverbs: Allow an empty namespace in ioctl() framework
> IB/uverbs: Refactor uverbs_finalize_objects
I put the above in a branch and can apply them if you ack my revisions..
> net/mlx5_core: Prevent warns in dmesg upon firmware commands
> IB/core: Improve uverbs_cleanup_ucontext algorithm
I dropped these two (they are linked), need comments addressed and
resent.
> Yishai Hadas (13):
> net/mlx5: Expose DEVX ifc structures
> IB/mlx5: Introduce DEVX
> IB/core: Introduce DECLARE_UVERBS_GLOBAL_METHODS
> IB: Expose ib_ucontext from a given ib_uverbs_file
> IB/mlx5: Add support for DEVX general command
> IB/mlx5: Add obj create and destroy functionality
> IB/mlx5: Add DEVX support for modify and query commands
> IB/mlx5: Add support for DEVX query UAR
> IB/mlx5: Add DEVX support for memory registration
> IB/mlx5: Add DEVX query EQN support
> IB/mlx5: Expose DEVX tree
I put these in a branch also and can apply them, but I need the first
two patches in the mlx5 core branch first please, thanks.
Since this requires so many core patches I think I prefer to merge the
mlx core branch then apply rather merge a branch.
Jason
^ permalink raw reply
* Re: [PATCH v3] bpf: attach type BPF_LIRC_MODE2 should not depend on CONFIG_CGROUP_BPF
From: Sean Young @ 2018-06-18 22:56 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, Daniel Borkmann, Y Song, Matthias Reichl, linux-media,
LKML, Alexei Starovoitov, Mauro Carvalho Chehab, netdev,
Devin Heitmueller, Quentin Monnet
In-Reply-To: <201806190203.kfm0Xhda%fengguang.wu@intel.com>
On Tue, Jun 19, 2018 at 02:46:29AM +0800, kbuild test robot wrote:
> Hi Sean,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.18-rc1 next-20180618]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Sean-Young/bpf-attach-type-BPF_LIRC_MODE2-should-not-depend-on-CONFIG_CGROUP_BPF/20180619-023056
> config: i386-tinyconfig (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386
>
> All errors (new ones prefixed by >>):
>
> In file included from kernel///events/core.c:45:0:
> >> include/linux/bpf.h:710:1: error: expected identifier or '(' before '{' token
> {
> ^
Oh dear I never tested that with CONFIG_INET off (no ip? madness!). I'll
send out a v4 soon.
Sean
^ permalink raw reply
* [PATCH v4] bpf: attach type BPF_LIRC_MODE2 should not depend on CONFIG_CGROUP_BPF
From: Sean Young @ 2018-06-18 23:04 UTC (permalink / raw)
To: Daniel Borkmann, Y Song, Matthias Reichl, linux-media, LKML,
Alexei Starovoitov, Mauro Carvalho Chehab, netdev,
Devin Heitmueller, Quentin Monnet
In-Reply-To: <201806190203.kfm0Xhda%fengguang.wu@intel.com>
If the kernel is compiled with CONFIG_CGROUP_BPF not enabled, it is not
possible to attach, detach or query IR BPF programs to /dev/lircN devices,
making them impossible to use. For embedded devices, it should be possible
to use IR decoding without cgroups or CONFIG_CGROUP_BPF enabled.
This change requires some refactoring, since bpf_prog_{attach,detach,query}
functions are now always compiled, but their code paths for cgroups need
moving out. Rather than a #ifdef CONFIG_CGROUP_BPF in kernel/bpf/syscall.c,
moving them to kernel/bpf/cgroup.c and kernel/bpf/sockmap.c does not
require #ifdefs since that is already conditionally compiled.
Signed-off-by: Sean Young <sean@mess.org>
---
drivers/media/rc/bpf-lirc.c | 14 +-----
include/linux/bpf-cgroup.h | 26 ++++++++++
include/linux/bpf.h | 8 +++
include/linux/bpf_lirc.h | 5 +-
kernel/bpf/cgroup.c | 52 ++++++++++++++++++++
kernel/bpf/sockmap.c | 18 +++++++
kernel/bpf/syscall.c | 98 ++++++++-----------------------------
7 files changed, 130 insertions(+), 91 deletions(-)
diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
index 40826bba06b6..fcfab6635f9c 100644
--- a/drivers/media/rc/bpf-lirc.c
+++ b/drivers/media/rc/bpf-lirc.c
@@ -207,29 +207,19 @@ void lirc_bpf_free(struct rc_dev *rcdev)
bpf_prog_array_free(rcdev->raw->progs);
}
-int lirc_prog_attach(const union bpf_attr *attr)
+int lirc_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
{
- struct bpf_prog *prog;
struct rc_dev *rcdev;
int ret;
if (attr->attach_flags)
return -EINVAL;
- prog = bpf_prog_get_type(attr->attach_bpf_fd,
- BPF_PROG_TYPE_LIRC_MODE2);
- if (IS_ERR(prog))
- return PTR_ERR(prog);
-
rcdev = rc_dev_get_from_fd(attr->target_fd);
- if (IS_ERR(rcdev)) {
- bpf_prog_put(prog);
+ if (IS_ERR(rcdev))
return PTR_ERR(rcdev);
- }
ret = lirc_bpf_attach(rcdev, prog);
- if (ret)
- bpf_prog_put(prog);
put_device(&rcdev->dev);
diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index 975fb4cf1bb7..79795c5fa7c3 100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -188,12 +188,38 @@ int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor,
\
__ret; \
})
+int cgroup_bpf_prog_attach(const union bpf_attr *attr,
+ enum bpf_prog_type ptype, struct bpf_prog *prog);
+int cgroup_bpf_prog_detach(const union bpf_attr *attr,
+ enum bpf_prog_type ptype);
+int cgroup_bpf_prog_query(const union bpf_attr *attr,
+ union bpf_attr __user *uattr);
#else
+struct bpf_prog;
struct cgroup_bpf {};
static inline void cgroup_bpf_put(struct cgroup *cgrp) {}
static inline int cgroup_bpf_inherit(struct cgroup *cgrp) { return 0; }
+static inline int cgroup_bpf_prog_attach(const union bpf_attr *attr,
+ enum bpf_prog_type ptype,
+ struct bpf_prog *prog)
+{
+ return -EINVAL;
+}
+
+static inline int cgroup_bpf_prog_detach(const union bpf_attr *attr,
+ enum bpf_prog_type ptype)
+{
+ return -EINVAL;
+}
+
+static inline int cgroup_bpf_prog_query(const union bpf_attr *attr,
+ union bpf_attr __user *uattr)
+{
+ return -EINVAL;
+}
+
#define cgroup_bpf_enabled (0)
#define BPF_CGROUP_PRE_CONNECT_ENABLED(sk) (0)
#define BPF_CGROUP_RUN_PROG_INET_INGRESS(sk,skb) ({ 0; })
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 995c3b1e59bf..7c87ec22d465 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -684,6 +684,8 @@ static inline void bpf_map_offload_map_free(struct bpf_map *map)
struct sock *__sock_map_lookup_elem(struct bpf_map *map, u32 key);
struct sock *__sock_hash_lookup_elem(struct bpf_map *map, void *key);
int sock_map_prog(struct bpf_map *map, struct bpf_prog *prog, u32 type);
+int sockmap_get_from_fd(const union bpf_attr *attr, int type,
+ struct bpf_prog *prog);
#else
static inline struct sock *__sock_map_lookup_elem(struct bpf_map *map, u32 key)
{
@@ -702,6 +704,12 @@ static inline int sock_map_prog(struct bpf_map *map,
{
return -EOPNOTSUPP;
}
+
+static inline int sockmap_get_from_fd(const union bpf_attr *attr, int type,
+ struct bpf_prog *prog)
+{
+ return -EINVAL;
+}
#endif
#if defined(CONFIG_XDP_SOCKETS)
diff --git a/include/linux/bpf_lirc.h b/include/linux/bpf_lirc.h
index 5f8a4283092d..9d9ff755ec29 100644
--- a/include/linux/bpf_lirc.h
+++ b/include/linux/bpf_lirc.h
@@ -5,11 +5,12 @@
#include <uapi/linux/bpf.h>
#ifdef CONFIG_BPF_LIRC_MODE2
-int lirc_prog_attach(const union bpf_attr *attr);
+int lirc_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog);
int lirc_prog_detach(const union bpf_attr *attr);
int lirc_prog_query(const union bpf_attr *attr, union bpf_attr __user *uattr);
#else
-static inline int lirc_prog_attach(const union bpf_attr *attr)
+static inline int lirc_prog_attach(const union bpf_attr *attr,
+ struct bpf_prog *prog)
{
return -EINVAL;
}
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index f7c00bd6f8e4..f0ae8a3d01f9 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -428,6 +428,58 @@ int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
return ret;
}
+int cgroup_bpf_prog_attach(const union bpf_attr *attr,
+ enum bpf_prog_type ptype, struct bpf_prog *prog)
+{
+ struct cgroup *cgrp;
+ int ret;
+
+ cgrp = cgroup_get_from_fd(attr->target_fd);
+ if (IS_ERR(cgrp))
+ return PTR_ERR(cgrp);
+
+ ret = cgroup_bpf_attach(cgrp, prog, attr->attach_type,
+ attr->attach_flags);
+ cgroup_put(cgrp);
+
+ return ret;
+}
+
+int cgroup_bpf_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype)
+{
+ struct bpf_prog *prog;
+ struct cgroup *cgrp;
+ int ret;
+
+ cgrp = cgroup_get_from_fd(attr->target_fd);
+ if (IS_ERR(cgrp))
+ return PTR_ERR(cgrp);
+
+ prog = bpf_prog_get_type(attr->attach_bpf_fd, ptype);
+ if (IS_ERR(prog))
+ prog = NULL;
+
+ ret = cgroup_bpf_detach(cgrp, prog, attr->attach_type, 0);
+ if (prog)
+ bpf_prog_put(prog);
+ cgroup_put(cgrp);
+ return ret;
+}
+
+int cgroup_bpf_prog_query(const union bpf_attr *attr,
+ union bpf_attr __user *uattr)
+{
+ struct cgroup *cgrp;
+ int ret;
+
+ cgrp = cgroup_get_from_fd(attr->query.target_fd);
+ if (IS_ERR(cgrp))
+ return PTR_ERR(cgrp);
+ ret = cgroup_bpf_query(cgrp, attr, uattr);
+ cgroup_put(cgrp);
+ return ret;
+}
+
/**
* __cgroup_bpf_run_filter_skb() - Run a program for packet filtering
* @sk: The socket sending or receiving traffic
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 52a91d816c0e..81d0c55a77aa 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -1915,6 +1915,24 @@ int sock_map_prog(struct bpf_map *map, struct bpf_prog *prog, u32 type)
return 0;
}
+int sockmap_get_from_fd(const union bpf_attr *attr, int type,
+ struct bpf_prog *prog)
+{
+ int ufd = attr->target_fd;
+ struct bpf_map *map;
+ struct fd f;
+ int err;
+
+ f = fdget(ufd);
+ map = __bpf_map_get(f);
+ if (IS_ERR(map))
+ return PTR_ERR(map);
+
+ err = sock_map_prog(map, prog, attr->attach_type);
+ fdput(f);
+ return err;
+}
+
static void *sock_map_lookup(struct bpf_map *map, void *key)
{
return NULL;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 0fa20624707f..93993c03c9ac 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1489,8 +1489,6 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
return err;
}
-#ifdef CONFIG_CGROUP_BPF
-
static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
enum bpf_attach_type attach_type)
{
@@ -1505,40 +1503,6 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
#define BPF_PROG_ATTACH_LAST_FIELD attach_flags
-static int sockmap_get_from_fd(const union bpf_attr *attr,
- int type, bool attach)
-{
- struct bpf_prog *prog = NULL;
- int ufd = attr->target_fd;
- struct bpf_map *map;
- struct fd f;
- int err;
-
- f = fdget(ufd);
- map = __bpf_map_get(f);
- if (IS_ERR(map))
- return PTR_ERR(map);
-
- if (attach) {
- prog = bpf_prog_get_type(attr->attach_bpf_fd, type);
- if (IS_ERR(prog)) {
- fdput(f);
- return PTR_ERR(prog);
- }
- }
-
- err = sock_map_prog(map, prog, attr->attach_type);
- if (err) {
- fdput(f);
- if (prog)
- bpf_prog_put(prog);
- return err;
- }
-
- fdput(f);
- return 0;
-}
-
#define BPF_F_ATTACH_MASK \
(BPF_F_ALLOW_OVERRIDE | BPF_F_ALLOW_MULTI)
@@ -1546,7 +1510,6 @@ static int bpf_prog_attach(const union bpf_attr *attr)
{
enum bpf_prog_type ptype;
struct bpf_prog *prog;
- struct cgroup *cgrp;
int ret;
if (!capable(CAP_NET_ADMIN))
@@ -1583,12 +1546,15 @@ static int bpf_prog_attach(const union bpf_attr *attr)
ptype = BPF_PROG_TYPE_CGROUP_DEVICE;
break;
case BPF_SK_MSG_VERDICT:
- return sockmap_get_from_fd(attr, BPF_PROG_TYPE_SK_MSG, true);
+ ptype = BPF_PROG_TYPE_SK_MSG;
+ break;
case BPF_SK_SKB_STREAM_PARSER:
case BPF_SK_SKB_STREAM_VERDICT:
- return sockmap_get_from_fd(attr, BPF_PROG_TYPE_SK_SKB, true);
+ ptype = BPF_PROG_TYPE_SK_SKB;
+ break;
case BPF_LIRC_MODE2:
- return lirc_prog_attach(attr);
+ ptype = BPF_PROG_TYPE_LIRC_MODE2;
+ break;
default:
return -EINVAL;
}
@@ -1602,17 +1568,20 @@ static int bpf_prog_attach(const union bpf_attr *attr)
return -EINVAL;
}
- cgrp = cgroup_get_from_fd(attr->target_fd);
- if (IS_ERR(cgrp)) {
- bpf_prog_put(prog);
- return PTR_ERR(cgrp);
+ switch (ptype) {
+ case BPF_PROG_TYPE_SK_SKB:
+ case BPF_PROG_TYPE_SK_MSG:
+ ret = sockmap_get_from_fd(attr, ptype, prog);
+ break;
+ case BPF_PROG_TYPE_LIRC_MODE2:
+ ret = lirc_prog_attach(attr, prog);
+ break;
+ default:
+ ret = cgroup_bpf_prog_attach(attr, ptype, prog);
}
- ret = cgroup_bpf_attach(cgrp, prog, attr->attach_type,
- attr->attach_flags);
if (ret)
bpf_prog_put(prog);
- cgroup_put(cgrp);
return ret;
}
@@ -1622,9 +1591,6 @@ static int bpf_prog_attach(const union bpf_attr *attr)
static int bpf_prog_detach(const union bpf_attr *attr)
{
enum bpf_prog_type ptype;
- struct bpf_prog *prog;
- struct cgroup *cgrp;
- int ret;
if (!capable(CAP_NET_ADMIN))
return -EPERM;
@@ -1657,29 +1623,17 @@ static int bpf_prog_detach(const union bpf_attr *attr)
ptype = BPF_PROG_TYPE_CGROUP_DEVICE;
break;
case BPF_SK_MSG_VERDICT:
- return sockmap_get_from_fd(attr, BPF_PROG_TYPE_SK_MSG, false);
+ return sockmap_get_from_fd(attr, BPF_PROG_TYPE_SK_MSG, NULL);
case BPF_SK_SKB_STREAM_PARSER:
case BPF_SK_SKB_STREAM_VERDICT:
- return sockmap_get_from_fd(attr, BPF_PROG_TYPE_SK_SKB, false);
+ return sockmap_get_from_fd(attr, BPF_PROG_TYPE_SK_SKB, NULL);
case BPF_LIRC_MODE2:
return lirc_prog_detach(attr);
default:
return -EINVAL;
}
- cgrp = cgroup_get_from_fd(attr->target_fd);
- if (IS_ERR(cgrp))
- return PTR_ERR(cgrp);
-
- prog = bpf_prog_get_type(attr->attach_bpf_fd, ptype);
- if (IS_ERR(prog))
- prog = NULL;
-
- ret = cgroup_bpf_detach(cgrp, prog, attr->attach_type, 0);
- if (prog)
- bpf_prog_put(prog);
- cgroup_put(cgrp);
- return ret;
+ return cgroup_bpf_prog_detach(attr, ptype);
}
#define BPF_PROG_QUERY_LAST_FIELD query.prog_cnt
@@ -1687,9 +1641,6 @@ static int bpf_prog_detach(const union bpf_attr *attr)
static int bpf_prog_query(const union bpf_attr *attr,
union bpf_attr __user *uattr)
{
- struct cgroup *cgrp;
- int ret;
-
if (!capable(CAP_NET_ADMIN))
return -EPERM;
if (CHECK_ATTR(BPF_PROG_QUERY))
@@ -1717,14 +1668,9 @@ static int bpf_prog_query(const union bpf_attr *attr,
default:
return -EINVAL;
}
- cgrp = cgroup_get_from_fd(attr->query.target_fd);
- if (IS_ERR(cgrp))
- return PTR_ERR(cgrp);
- ret = cgroup_bpf_query(cgrp, attr, uattr);
- cgroup_put(cgrp);
- return ret;
+
+ return cgroup_bpf_prog_query(attr, uattr);
}
-#endif /* CONFIG_CGROUP_BPF */
#define BPF_PROG_TEST_RUN_LAST_FIELD test.duration
@@ -2371,7 +2317,6 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
case BPF_OBJ_GET:
err = bpf_obj_get(&attr);
break;
-#ifdef CONFIG_CGROUP_BPF
case BPF_PROG_ATTACH:
err = bpf_prog_attach(&attr);
break;
@@ -2381,7 +2326,6 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
case BPF_PROG_QUERY:
err = bpf_prog_query(&attr, uattr);
break;
-#endif
case BPF_PROG_TEST_RUN:
err = bpf_prog_test_run(&attr, uattr);
break;
--
2.17.1
^ permalink raw reply related
* Re: [RFC v2, net-next, PATCH 4/4] net/cpsw_switchdev: add switchdev mode of operation on cpsw driver
From: Grygorii Strashko @ 2018-06-18 23:19 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: netdev, ivan.khoronzhuk, nsekhar, ivecera, andrew, f.fainelli,
francois.ozog, yogeshs, spatton, Jose.Abreu
In-Reply-To: <20180614114300.GA416@apalos>
On 06/14/2018 06:43 AM, Ilias Apalodimas wrote:
> On Thu, Jun 14, 2018 at 01:39:58PM +0200, Jiri Pirko wrote:
>> Thu, Jun 14, 2018 at 01:34:04PM CEST, ilias.apalodimas@linaro.org wrote:
>>> On Thu, Jun 14, 2018 at 01:30:28PM +0200, Jiri Pirko wrote:
>>>> Thu, Jun 14, 2018 at 01:11:30PM CEST, ilias.apalodimas@linaro.org wrote:
>>>>
>>>> [...]
>>>>
>>>>> @@ -2711,6 +2789,10 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
>>>>> if (of_property_read_bool(node, "dual_emac"))
>>>>> data->switch_mode = CPSW_DUAL_EMAC;
>>>>>
>>>>> + /* switchdev overrides DTS */
>>>>> + if (IS_ENABLED(CONFIG_TI_CPSW_SWITCHDEV))
>>>>> + data->switch_mode = CPSW_SWITCHDEV;
>>>>
>>>> So you force CPSW_SWITCHDEV mode if the CONFIG_TI_CPSW_SWITCHDEV is
>>>> enabled. That does not sound right. I think that user should tell what
>>>> mode does he want regardless what the kernel config is.
>>> We discussed this during the V1 of the RFC. Yes it doesn't seem good, but the
>>> device currently configures the modes using DTS (which is not correct). I choose
>>> the .config due to that. I can't think of anything better, but i am open to
>>> suggestions
>>
>> Agreed that DTS does fit as well. I think that this might be a job for
>> devlink parameters (patchset is going to be sent upstream next week).
>> You do have 1 bus address for the whole device (both ports), right?
>>
> Yes devlink sounds reasonable. I thyink there's only one bus for it, but then
> again i am far from an expert on the hardware interrnals. Grygorii can correct
> me if i am wrong.
Devlink and NFS boot are not compatible as per my understanding, so ..
Again, current driver, as is, supports NFS boot in all modes
(how good is the cur driver is question which out of scope of this discussion).
And we discussed this in RFC v1 - driver mode selection will be changed
if we will proceed and it will be new driver for proper switch support.
Not sure about about Devlink (need to study it and we never got any requests from end
users for this as I know), and I'd like to note (again) that this is embedded
(industrial/automotive etc), so everything preferred to be simple, fast and
preferably configured statically (in most of the cases end users what boot time
configuration).
--
regards,
-grygorii
^ permalink raw reply
* Re: [RFC v2, net-next, PATCH 4/4] net/cpsw_switchdev: add switchdev mode of operation on cpsw driver
From: Grygorii Strashko @ 2018-06-18 23:20 UTC (permalink / raw)
To: Ilias Apalodimas, Andrew Lunn
Cc: netdev, ivan.khoronzhuk, nsekhar, jiri, ivecera, f.fainelli,
francois.ozog, yogeshs, spatton, Jose.Abreu
In-Reply-To: <20180618201940.GA5890@apalos>
On 06/18/2018 03:19 PM, Ilias Apalodimas wrote:
> On Mon, Jun 18, 2018 at 06:16:27PM +0200, Andrew Lunn wrote:
>>> @@ -2711,6 +2789,10 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
>>> if (of_property_read_bool(node, "dual_emac"))
>>> data->switch_mode = CPSW_DUAL_EMAC;
>>>
>>> + /* switchdev overrides DTS */
>>> + if (IS_ENABLED(CONFIG_TI_CPSW_SWITCHDEV))
>>> + data->switch_mode = CPSW_SWITCHDEV;
>>> +
>>
>> I know you discussed this a bit with Jiri, but i still think if
>> 'dual_mac" is found, you should do dual mac. The DT clearly requests
>> dual mac, and doing anything else is going to cause confusion.
>>
>> The device tree binding is ambiguous what should happen when dual-mac
>> is missing. So i would only enable swithdev mode in that case.
> At the moment if no 'dual_emac;' is found on DTS the driver operates in "switch
> mode". It only registers 1 ethernet interface with no ability (unless you patch
> the kernel) to configure the switch. If we use DTS instead of a .config option
> we should add parsing for something like 'switchdev;' in the DTS.
> Jiri proposed using devlink, which makes sense, but i am not sure it's
> applicable on this patchset. This will change the driver completely and will
> totally break backwards compatibility.
>
> Ideally i'd prefer something like:
> 1. Add a DTS option and continue the current behavior. I agree with you that
> this will cause less confusion (in fact i prefer it for the current state of the
> driver compared to the .config).
> or
> 2. Keep the .config option which is better suited over DTS but might cause some
> confusion.
>>
>> But ideally, it should be a new driver with a new binding.
> TI is better suited to comment on this. The work proposed here is mostly to
> accomodate future TSN related configuration for switches. This patchset has
> been tested against Ivan's patchset for CBS and is working as expected
> configuration wise.
> (http://lkml.iu.edu/hypermail/linux/kernel/1806.1/05302.html)
We'd try the new driver in the future
--
regards,
-grygorii
^ permalink raw reply
* Re: [PATCH] Revert "net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"
From: Eric Dumazet @ 2018-06-18 23:29 UTC (permalink / raw)
To: Mathieu Malaterre
Cc: schwab, David S. Miller, Eric Dumazet, LKML, Christophe LEROY,
Meelis Roos, netdev, linuxppc-dev
In-Reply-To: <CA+7wUszG3Tmq5qT7o7w7ttAU+bzyJfRORL_Qzk4wUG6y2gZUuw@mail.gmail.com>
On 06/18/2018 11:45 AM, Mathieu Malaterre wrote:
>
> Here is what I get on my side
>
> [ 53.628847] sungem: sungem wrong csum : 4e04/f97, len 64 bytes
> [ 53.667063] sungem: sungem wrong csum : eea8/6eec, len 149 bytes
> [ 58.648952] sungem: sungem wrong csum : 2095/3d06, len 64 bytes
> [ 58.669414] sungem: sungem wrong csum : 5245/b50, len 149 bytes
> [ 63.674451] sungem: sungem wrong csum : 2d8/5abd, len 149 bytes
> [ 68.678233] sungem: sungem wrong csum : b8fc/a498, len 149 bytes
> [ 73.685771] sungem: sungem wrong csum : 374/5a21, len 149 bytes
> [ 78.689089] sungem: sungem wrong csum : d81/5014, len 149 bytes
> [ 83.683261] sungem: sungem wrong csum : 4e04/f97, len 64 bytes
> [ 83.690193] sungem: sungem wrong csum : c2f7/9a9d, len 149 bytes
> [ 88.692511] sungem: sungem wrong csum : f4d8/68bc, len 149 bytes
> [ 93.699915] sungem: sungem wrong csum : 1370/4a25, len 149 bytes
> [ 98.703136] sungem: sungem wrong csum : e1b5/7bdf, len 149 bytes
> [ 103.704230] sungem: sungem wrong csum : 5321/a74, len 149 bytes
> [ 108.688912] sungem: sungem wrong csum : 2095/3d06, len 64 bytes
> [ 108.706559] sungem: sungem wrong csum : ddbc/7fd8, len 149 bytes
> [ 113.713189] sungem: sungem wrong csum : 5a65/330, len 149 bytes
> [ 113.891697] sungem: sungem wrong csum : 4e04/f97, len 64 bytes
> [ 118.717151] sungem: sungem wrong csum : f7c8/65cc, len 149 bytes
> [ 123.722680] sungem: sungem wrong csum : 3d7a/201b, len 149 bytes
> [ 128.726524] sungem: sungem wrong csum : c8fd/9497, len 149 bytes
> [ 133.732045] sungem: sungem wrong csum : de0d/7f87, len 149 bytes
> [ 135.529163] sungem: sungem wrong csum : 3089/b6dd, len 96 bytes
> [ 135.529208] eth0: hw csum failure
> [ 135.529220] CPU: 0 PID: 0 Comm: swapper Not tainted 4.17.0+ #7
> [ 135.529226] Call Trace:
> [ 135.529243] [dffedbe0] [c069ddac]
> __skb_checksum_complete+0xf0/0x108 (unreliable)
Thanks, then I guess next step would be to dump the content of the frames
having a wrong checksum, hoping we find an easy way to discard the CHECKSUM_COMPLETE
in a selective way.
Otherwise, we will need to remove CHECKSUM_COMPLETE setting in this driver.
diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
index 7a16d40a72d13cf1d522e8a3a396c826fe76f9b9..77a761f95be788bb86c8d917f613c9084818f826 100644
--- a/drivers/net/ethernet/sun/sungem.c
+++ b/drivers/net/ethernet/sun/sungem.c
@@ -857,6 +857,14 @@ static int gem_rx(struct gem *gp, int work_to_do)
csum = (__force __sum16)htons((status & RXDCTRL_TCPCSUM) ^ 0xffff);
skb->csum = csum_unfold(csum);
+ {
+ __wsum rsum = csum_partial(skb->data + ETH_HLEN, len - ETH_HLEN, 0);
+ if (csum != csum_fold(rsum) && net_ratelimit())
+ pr_err("sungem wrong csum : %04x/%04x, len %u bytes\n",
+ csum, csum_fold(rsum), len);
+ print_hex_dump(KERN_ERR, "raw data: ", DUMP_PREFIX_OFFSET,
+ 16, 1, skb->data, len, true);
+ }
skb->ip_summed = CHECKSUM_COMPLETE;
skb->protocol = eth_type_trans(skb, gp->dev);
^ permalink raw reply related
* Re: [PATCH] Revert "net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"
From: Eric Dumazet @ 2018-06-18 23:36 UTC (permalink / raw)
To: Eric Dumazet, Mathieu Malaterre
Cc: schwab, David S. Miller, Eric Dumazet, LKML, Christophe LEROY,
Meelis Roos, netdev, linuxppc-dev
In-Reply-To: <21523399-92ee-f8da-1a3e-0561f62850b7@gmail.com>
On 06/18/2018 04:29 PM, Eric Dumazet wrote:
>
>
> On 06/18/2018 11:45 AM, Mathieu Malaterre wrote:
>
>>
>> Here is what I get on my side
>>
>> [ 53.628847] sungem: sungem wrong csum : 4e04/f97, len 64 bytes
>> [ 53.667063] sungem: sungem wrong csum : eea8/6eec, len 149 bytes
>> [ 58.648952] sungem: sungem wrong csum : 2095/3d06, len 64 bytes
>> [ 58.669414] sungem: sungem wrong csum : 5245/b50, len 149 bytes
>> [ 63.674451] sungem: sungem wrong csum : 2d8/5abd, len 149 bytes
>> [ 68.678233] sungem: sungem wrong csum : b8fc/a498, len 149 bytes
>> [ 73.685771] sungem: sungem wrong csum : 374/5a21, len 149 bytes
>> [ 78.689089] sungem: sungem wrong csum : d81/5014, len 149 bytes
>> [ 83.683261] sungem: sungem wrong csum : 4e04/f97, len 64 bytes
>> [ 83.690193] sungem: sungem wrong csum : c2f7/9a9d, len 149 bytes
>> [ 88.692511] sungem: sungem wrong csum : f4d8/68bc, len 149 bytes
>> [ 93.699915] sungem: sungem wrong csum : 1370/4a25, len 149 bytes
>> [ 98.703136] sungem: sungem wrong csum : e1b5/7bdf, len 149 bytes
>> [ 103.704230] sungem: sungem wrong csum : 5321/a74, len 149 bytes
>> [ 108.688912] sungem: sungem wrong csum : 2095/3d06, len 64 bytes
>> [ 108.706559] sungem: sungem wrong csum : ddbc/7fd8, len 149 bytes
>> [ 113.713189] sungem: sungem wrong csum : 5a65/330, len 149 bytes
>> [ 113.891697] sungem: sungem wrong csum : 4e04/f97, len 64 bytes
>> [ 118.717151] sungem: sungem wrong csum : f7c8/65cc, len 149 bytes
>> [ 123.722680] sungem: sungem wrong csum : 3d7a/201b, len 149 bytes
>> [ 128.726524] sungem: sungem wrong csum : c8fd/9497, len 149 bytes
>> [ 133.732045] sungem: sungem wrong csum : de0d/7f87, len 149 bytes
>> [ 135.529163] sungem: sungem wrong csum : 3089/b6dd, len 96 bytes
>> [ 135.529208] eth0: hw csum failure
>> [ 135.529220] CPU: 0 PID: 0 Comm: swapper Not tainted 4.17.0+ #7
>> [ 135.529226] Call Trace:
>> [ 135.529243] [dffedbe0] [c069ddac]
>> __skb_checksum_complete+0xf0/0x108 (unreliable)
>
> Thanks, then I guess next step would be to dump the content of the frames
> having a wrong checksum, hoping we find an easy way to discard the CHECKSUM_COMPLETE
> in a selective way.
>
> Otherwise, we will need to remove CHECKSUM_COMPLETE setting in this driver.
>
> diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
> index 7a16d40a72d13cf1d522e8a3a396c826fe76f9b9..77a761f95be788bb86c8d917f613c9084818f826 100644
> --- a/drivers/net/ethernet/sun/sungem.c
> +++ b/drivers/net/ethernet/sun/sungem.c
> @@ -857,6 +857,14 @@ static int gem_rx(struct gem *gp, int work_to_do)
>
> csum = (__force __sum16)htons((status & RXDCTRL_TCPCSUM) ^ 0xffff);
> skb->csum = csum_unfold(csum);
> + {
> + __wsum rsum = csum_partial(skb->data + ETH_HLEN, len - ETH_HLEN, 0);
> + if (csum != csum_fold(rsum) && net_ratelimit())
> + pr_err("sungem wrong csum : %04x/%04x, len %u bytes\n",
> + csum, csum_fold(rsum), len);
> + print_hex_dump(KERN_ERR, "raw data: ", DUMP_PREFIX_OFFSET,
DUMP_PREFIX_ADDRESS might give us more information (say alignment problem, or crossing page boundaries)
> + 16, 1, skb->data, len, true);
> + }
> skb->ip_summed = CHECKSUM_COMPLETE;
> skb->protocol = eth_type_trans(skb, gp->dev);
>
>
^ permalink raw reply
* [PATCH net] enic: do not overwrite error code
From: Govindarajulu Varadarajan @ 2018-06-18 17:01 UTC (permalink / raw)
To: davem, netdev, ben.hutchings, benve, gregkh, alexander.levin
Cc: Govindarajulu Varadarajan
In failure path, we overwrite err to what vnic_rq_disable() returns. In
case it returns 0, enic_open() returns success in case of error.
Reported-by: Ben Hutchings <ben.hutchings@codethink.co.uk>
Fixes: e8588e268509 ("enic: enable rq before updating rq descriptors")
Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com>
---
drivers/net/ethernet/cisco/enic/enic_main.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 30d2eaa18c04..d3962fb0d32d 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -1920,7 +1920,7 @@ static int enic_open(struct net_device *netdev)
{
struct enic *enic = netdev_priv(netdev);
unsigned int i;
- int err;
+ int err, ret;
err = enic_request_intr(enic);
if (err) {
@@ -1977,10 +1977,9 @@ static int enic_open(struct net_device *netdev)
err_out_free_rq:
for (i = 0; i < enic->rq_count; i++) {
- err = vnic_rq_disable(&enic->rq[i]);
- if (err)
- return err;
- vnic_rq_clean(&enic->rq[i], enic_free_rq_buf);
+ ret = vnic_rq_disable(&enic->rq[i]);
+ if (!ret)
+ vnic_rq_clean(&enic->rq[i], enic_free_rq_buf);
}
enic_dev_notify_unset(enic);
err_out_free_intr:
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 3/4] net/ncsi: Use netdev_dbg for debug messages
From: Samuel Mendoza-Jonas @ 2018-06-19 0:47 UTC (permalink / raw)
To: Joe Perches, Joel Stanley, David S . Miller; +Cc: netdev
In-Reply-To: <15f1e73b4ff4bccf818b30d74d2037837ab51b35.camel@perches.com>
On Mon, 2018-06-18 at 13:53 -0700, Joe Perches wrote:
> On Mon, 2018-06-18 at 16:49 +0930, Joel Stanley wrote:
> > This moves all of the netdev_printk(KERN_DEBUG, ...) messages over to
> > netdev_dbg. There is no change in behaviour.
>
> Not quite, but I think the patch is fine anyway.
>
> netdev_printk(KERN_DEBUG ... is always emitted as
> long as the console level includes debug output.
>
> netdev_dbg is not included in object code unless
> DEBUG is defined or CONFIG_DYNAMIC_DEBUG is set.
> And then, it is not emitted into the log unless
> DEBUG is set or this specific netdev_dbg is enabled
> via the dynamic debug control file.
Right this is fine for these sort of messages; very noisy and not particularly
critical.
For this and the other logging updates:
Acked-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
>
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > ---
> > net/ncsi/ncsi-aen.c | 6 +++---
> > net/ncsi/ncsi-manage.c | 33 +++++++++++++++------------------
> > 2 files changed, 18 insertions(+), 21 deletions(-)
> >
> > diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> > index f899ed61bb57..25e483e8278b 100644
> > --- a/net/ncsi/ncsi-aen.c
> > +++ b/net/ncsi/ncsi-aen.c
> > @@ -148,9 +148,9 @@ static int ncsi_aen_handler_hncdsc(struct ncsi_dev_priv *ndp,
> > hncdsc = (struct ncsi_aen_hncdsc_pkt *)h;
> > ncm->data[3] = ntohl(hncdsc->status);
> > spin_unlock_irqrestore(&nc->lock, flags);
> > - netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> > - "NCSI: host driver %srunning on channel %u\n",
> > - ncm->data[3] & 0x1 ? "" : "not ", nc->id);
> > + netdev_dbg(ndp->ndev.dev,
> > + "NCSI: host driver %srunning on channel %u\n",
> > + ncm->data[3] & 0x1 ? "" : "not ", nc->id);
> >
> > return 0;
> > }
> > diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> > index 716493a61ba6..091284760d21 100644
> > --- a/net/ncsi/ncsi-manage.c
> > +++ b/net/ncsi/ncsi-manage.c
> > @@ -788,8 +788,8 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> > }
> > break;
> > case ncsi_dev_state_config_done:
> > - netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> > - "NCSI: channel %u config done\n", nc->id);
> > + netdev_dbg(ndp->ndev.dev, "NCSI: channel %u config done\n",
> > + nc->id);
> > spin_lock_irqsave(&nc->lock, flags);
> > if (nc->reconfigure_needed) {
> > /* This channel's configuration has been updated
> > @@ -804,8 +804,7 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> > list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> > spin_unlock_irqrestore(&ndp->lock, flags);
> >
> > - netdev_printk(KERN_DEBUG, dev,
> > - "Dirty NCSI channel state reset\n");
> > + netdev_dbg(dev, "Dirty NCSI channel state reset\n");
> > ncsi_process_next_channel(ndp);
> > break;
> > }
> > @@ -908,9 +907,9 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
> > }
> >
> > ncm = &found->modes[NCSI_MODE_LINK];
> > - netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> > - "NCSI: Channel %u added to queue (link %s)\n",
> > - found->id, ncm->data[2] & 0x1 ? "up" : "down");
> > + netdev_dbg(ndp->ndev.dev,
> > + "NCSI: Channel %u added to queue (link %s)\n",
> > + found->id, ncm->data[2] & 0x1 ? "up" : "down");
> >
> > out:
> > spin_lock_irqsave(&ndp->lock, flags);
> > @@ -1316,9 +1315,9 @@ static int ncsi_kick_channels(struct ncsi_dev_priv *ndp)
> > if ((ndp->ndev.state & 0xff00) ==
> > ncsi_dev_state_config ||
> > !list_empty(&nc->link)) {
> > - netdev_printk(KERN_DEBUG, nd->dev,
> > - "NCSI: channel %p marked dirty\n",
> > - nc);
> > + netdev_dbg(nd->dev,
> > + "NCSI: channel %p marked dirty\n",
> > + nc);
> > nc->reconfigure_needed = true;
> > }
> > spin_unlock_irqrestore(&nc->lock, flags);
> > @@ -1336,8 +1335,7 @@ static int ncsi_kick_channels(struct ncsi_dev_priv *ndp)
> > list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> > spin_unlock_irqrestore(&ndp->lock, flags);
> >
> > - netdev_printk(KERN_DEBUG, nd->dev,
> > - "NCSI: kicked channel %p\n", nc);
> > + netdev_dbg(nd->dev, "NCSI: kicked channel %p\n", nc);
> > n++;
> > }
> > }
> > @@ -1368,8 +1366,8 @@ int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
> > list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list) {
> > n_vids++;
> > if (vlan->vid == vid) {
> > - netdev_printk(KERN_DEBUG, dev,
> > - "NCSI: vid %u already registered\n", vid);
> > + netdev_dbg(dev, "NCSI: vid %u already registered\n",
> > + vid);
> > return 0;
> > }
> > }
> > @@ -1388,7 +1386,7 @@ int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
> > vlan->vid = vid;
> > list_add_rcu(&vlan->list, &ndp->vlan_vids);
> >
> > - netdev_printk(KERN_DEBUG, dev, "NCSI: Added new vid %u\n", vid);
> > + netdev_dbg(dev, "NCSI: Added new vid %u\n", vid);
> >
> > found = ncsi_kick_channels(ndp) != 0;
> >
> > @@ -1417,8 +1415,7 @@ int ncsi_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
> > /* Remove the VLAN id from our internal list */
> > list_for_each_entry_safe(vlan, tmp, &ndp->vlan_vids, list)
> > if (vlan->vid == vid) {
> > - netdev_printk(KERN_DEBUG, dev,
> > - "NCSI: vid %u found, removing\n", vid);
> > + netdev_dbg(dev, "NCSI: vid %u found, removing\n", vid);
> > list_del_rcu(&vlan->list);
> > found = true;
> > kfree(vlan);
> > @@ -1545,7 +1542,7 @@ void ncsi_stop_dev(struct ncsi_dev *nd)
> > }
> > }
> >
> > - netdev_printk(KERN_DEBUG, ndp->ndev.dev, "NCSI: Stopping device\n");
> > + netdev_dbg(ndp->ndev.dev, "NCSI: Stopping device\n");
> > ncsi_report_link(ndp, true);
> > }
> > EXPORT_SYMBOL_GPL(ncsi_stop_dev);
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox