* [PATCH v4 2/7] lib/hexdump.c: Relax rowsize checks in hex_dump_to_buffer
From: Alastair D'Silva @ 2019-06-25 3:17 UTC (permalink / raw)
To: alastair
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Daniel Vetter, Dan Carpenter, Karsten Keil, Jassi Brar,
Tom Lendacky, David S. Miller, Jose Abreu, Kalle Valo,
Stanislaw Gruszka, Benson Leung, Enric Balletbo i Serra,
James E.J. Bottomley, Martin K. Petersen, Greg Kroah-Hartman,
Alexander Viro, Petr Mladek, Sergey Senozhatsky, Steven Rostedt,
David Laight, Andrew Morton, intel-gfx, dri-devel, linux-kernel,
netdev, ath10k, linux-wireless, linux-scsi, linux-fbdev, devel,
linux-fsdevel
In-Reply-To: <20190625031726.12173-1-alastair@au1.ibm.com>
From: Alastair D'Silva <alastair@d-silva.org>
This patch removes the hardcoded row limits and allows for
other lengths. These lengths must still be a multiple of
groupsize.
This allows structs that are not 16/32 bytes to display on
a single line.
This patch also expands the self-tests to test row sizes
up to 64 bytes (though they can now be arbitrarily long).
Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
lib/hexdump.c | 49 +++++++++++++++++++++++++++++--------------
lib/test_hexdump.c | 52 ++++++++++++++++++++++++++++++++++++++--------
2 files changed, 76 insertions(+), 25 deletions(-)
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 81b70ed37209..870c8cbff1e1 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -12,6 +12,7 @@
#include <linux/errno.h>
#include <linux/kernel.h>
#include <linux/export.h>
+#include <linux/slab.h>
#include <asm/unaligned.h>
const char hex_asc[] = "0123456789abcdef";
@@ -80,14 +81,15 @@ EXPORT_SYMBOL(bin2hex);
* hex_dump_to_buffer - convert a blob of data to "hex ASCII" in memory
* @buf: data blob to dump
* @len: number of bytes in the @buf
- * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @rowsize: number of bytes to print per line; must be a multiple of groupsize
* @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
* @linebuf: where to put the converted data
* @linebuflen: total size of @linebuf, including space for terminating NUL
* @ascii: include ASCII after the hex output
*
- * hex_dump_to_buffer() works on one "line" of output at a time, i.e.,
- * 16 or 32 bytes of input data converted to hex + ASCII output.
+ * hex_dump_to_buffer() works on one "line" of output at a time, converting
+ * <groupsize> bytes of input to hexadecimal (and optionally printable ASCII)
+ * until <rowsize> bytes have been emitted.
*
* Given a buffer of u8 data, hex_dump_to_buffer() converts the input data
* to a hex + ASCII dump at the supplied memory location.
@@ -116,16 +118,17 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
int ascii_column;
int ret;
- if (rowsize != 16 && rowsize != 32)
- rowsize = 16;
-
- if (len > rowsize) /* limit to one line at a time */
- len = rowsize;
if (!is_power_of_2(groupsize) || groupsize > 8)
groupsize = 1;
if ((len % groupsize) != 0) /* no mixed size output */
groupsize = 1;
+ if (rowsize % groupsize)
+ rowsize -= rowsize % groupsize;
+
+ if (len > rowsize) /* limit to one line at a time */
+ len = rowsize;
+
ngroups = len / groupsize;
ascii_column = rowsize * 2 + rowsize / groupsize + 1;
@@ -216,7 +219,7 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
* caller supplies trailing spaces for alignment if desired
* @prefix_type: controls whether prefix of an offset, address, or none
* is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
- * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @rowsize: number of bytes to print per line; must be a multiple of groupsize
* @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
* @buf: data blob to dump
* @len: number of bytes in the @buf
@@ -226,10 +229,9 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
* to the kernel log at the specified kernel log level, with an optional
* leading prefix.
*
- * print_hex_dump() works on one "line" of output at a time, i.e.,
- * 16 or 32 bytes of input data converted to hex + ASCII output.
* print_hex_dump() iterates over the entire input @buf, breaking it into
- * "line size" chunks to format and print.
+ * lines of rowsize/groupsize groups of input data converted to hex +
+ * (optionally) ASCII output.
*
* E.g.:
* print_hex_dump(KERN_DEBUG, "raw data: ", DUMP_PREFIX_ADDRESS,
@@ -246,17 +248,30 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
{
const u8 *ptr = buf;
int i, linelen, remaining = len;
- unsigned char linebuf[32 * 3 + 2 + 32 + 1];
+ unsigned char *linebuf;
+ unsigned int linebuf_len;
- if (rowsize != 16 && rowsize != 32)
- rowsize = 16;
+ if (rowsize % groupsize)
+ rowsize -= rowsize % groupsize;
+
+ /*
+ * Worst case line length:
+ * 2 hex chars + space per byte in, 2 spaces, 1 char per byte in, NULL
+ */
+ linebuf_len = rowsize * 3 + 2 + rowsize + 1;
+ linebuf = kzalloc(linebuf_len, GFP_KERNEL);
+ if (!linebuf) {
+ printk("%s%shexdump: Could not alloc %u bytes for buffer\n",
+ level, prefix_str, linebuf_len);
+ return;
+ }
for (i = 0; i < len; i += rowsize) {
linelen = min(remaining, rowsize);
remaining -= rowsize;
hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
- linebuf, sizeof(linebuf), ascii);
+ linebuf, linebuf_len, ascii);
switch (prefix_type) {
case DUMP_PREFIX_ADDRESS:
@@ -271,6 +286,8 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
break;
}
}
+
+ kfree(linebuf);
}
EXPORT_SYMBOL(print_hex_dump);
diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index bef97a964582..5e54525a937c 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -14,15 +14,25 @@ static const unsigned char data_b[] = {
'\x70', '\xba', '\xc4', '\x24', '\x7d', '\x83', '\x34', '\x9b', /* 08 - 0f */
'\xa6', '\x9c', '\x31', '\xad', '\x9c', '\x0f', '\xac', '\xe9', /* 10 - 17 */
'\x4c', '\xd1', '\x19', '\x99', '\x43', '\xb1', '\xaf', '\x0c', /* 18 - 1f */
+ '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x06', '\x07', /* 20 - 27 */
+ '\x0f', '\x0e', '\x0d', '\x0c', '\x0b', '\x0a', '\x09', '\x08', /* 28 - 2f */
+ '\x10', '\x11', '\x12', '\x13', '\x14', '\x15', '\x16', '\x17', /* 30 - 37 */
+ '\x1f', '\x1e', '\x1d', '\x1c', '\x1b', '\x1a', '\x19', '\x18', /* 38 - 3f */
};
-static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C...";
+static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C..."
+ "................................";
static const char * const test_data_1[] __initconst = {
"be", "32", "db", "7b", "0a", "18", "93", "b2",
"70", "ba", "c4", "24", "7d", "83", "34", "9b",
"a6", "9c", "31", "ad", "9c", "0f", "ac", "e9",
"4c", "d1", "19", "99", "43", "b1", "af", "0c",
+ "00", "01", "02", "03", "04", "05", "06", "07",
+ "0f", "0e", "0d", "0c", "0b", "0a", "09", "08",
+ "10", "11", "12", "13", "14", "15", "16", "17",
+ "1f", "1e", "1d", "1c", "1b", "1a", "19", "18",
+ NULL
};
static const char * const test_data_2_le[] __initconst = {
@@ -30,6 +40,11 @@ static const char * const test_data_2_le[] __initconst = {
"ba70", "24c4", "837d", "9b34",
"9ca6", "ad31", "0f9c", "e9ac",
"d14c", "9919", "b143", "0caf",
+ "0100", "0302", "0504", "0706",
+ "0e0f", "0c0d", "0a0b", "0809",
+ "1110", "1312", "1514", "1716",
+ "1e1f", "1c1d", "1a1b", "1819",
+ NULL
};
static const char * const test_data_2_be[] __initconst = {
@@ -37,26 +52,43 @@ static const char * const test_data_2_be[] __initconst = {
"70ba", "c424", "7d83", "349b",
"a69c", "31ad", "9c0f", "ace9",
"4cd1", "1999", "43b1", "af0c",
+ "0001", "0203", "0405", "0607",
+ "0f0e", "0d0c", "0b0a", "0908",
+ "1011", "1213", "1415", "1617",
+ "1f1e", "1d1c", "1b1a", "1918",
+ NULL
};
static const char * const test_data_4_le[] __initconst = {
"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
+ "03020100", "07060504", "0c0d0e0f", "08090a0b",
+ "13121110", "17161514", "1c1d1e1f", "18191a1b",
+ NULL
};
static const char * const test_data_4_be[] __initconst = {
"be32db7b", "0a1893b2", "70bac424", "7d83349b",
"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
+ "00010203", "04050607", "0f0e0d0c", "0b0a0908",
+ "10111213", "14151617", "1f1e1d1c", "1b1a1918",
+ NULL
};
static const char * const test_data_8_le[] __initconst = {
"b293180a7bdb32be", "9b34837d24c4ba70",
"e9ac0f9cad319ca6", "0cafb1439919d14c",
+ "0706050403020100", "08090a0b0c0d0e0f",
+ "1716151413121110", "18191a1b1c1d1e1f",
+ NULL
};
static const char * const test_data_8_be[] __initconst = {
"be32db7b0a1893b2", "70bac4247d83349b",
"a69c31ad9c0face9", "4cd1199943b1af0c",
+ "0001020304050607", "0f0e0d0c0b0a0908",
+ "1011121314151617", "1f1e1d1c1b1a1918",
+ NULL
};
#define FILL_CHAR '#'
@@ -75,9 +107,6 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
unsigned int i;
const bool is_be = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
- if (rs != 16 && rs != 32)
- rs = 16;
-
if (l > rs)
l = rs;
@@ -97,7 +126,12 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
p = test;
for (i = 0; i < l / gs; i++) {
const char *q = *result++;
- size_t amount = strlen(q);
+ size_t amount;
+
+ if (!q)
+ break;
+
+ amount = strlen(q);
memcpy(p, q, amount);
p += amount;
@@ -120,7 +154,7 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
*p = '\0';
}
-#define TEST_HEXDUMP_BUF_SIZE (32 * 3 + 2 + 32 + 1)
+#define TEST_HEXDUMP_BUF_SIZE (64 * 3 + 2 + 64 + 1)
static void __init test_hexdump(size_t len, int rowsize, int groupsize,
bool ascii)
@@ -216,7 +250,7 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
static void __init test_hexdump_overflow_set(size_t buflen, bool ascii)
{
unsigned int i = 0;
- int rs = (get_random_int() % 2 + 1) * 16;
+ int rs = (get_random_int() % 4 + 1) * 16;
do {
int gs = 1 << i;
@@ -231,11 +265,11 @@ static int __init test_hexdump_init(void)
unsigned int i;
int rowsize;
- rowsize = (get_random_int() % 2 + 1) * 16;
+ rowsize = (get_random_int() % 4 + 1) * 16;
for (i = 0; i < 16; i++)
test_hexdump_set(rowsize, false);
- rowsize = (get_random_int() % 2 + 1) * 16;
+ rowsize = (get_random_int() % 4 + 1) * 16;
for (i = 0; i < 16; i++)
test_hexdump_set(rowsize, true);
--
2.21.0
^ permalink raw reply related
* [PATCH v4 0/7] Hexdump Enhancements
From: Alastair D'Silva @ 2019-06-25 3:17 UTC (permalink / raw)
To: alastair
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Daniel Vetter, Dan Carpenter, Karsten Keil, Jassi Brar,
Tom Lendacky, David S. Miller, Jose Abreu, Kalle Valo,
Stanislaw Gruszka, Benson Leung, Enric Balletbo i Serra,
James E.J. Bottomley, Martin K. Petersen, Greg Kroah-Hartman,
Alexander Viro, Petr Mladek, Sergey Senozhatsky, Steven Rostedt,
David Laight, Andrew Morton, intel-gfx, dri-devel, linux-kernel,
netdev, ath10k, linux-wireless, linux-scsi, linux-fbdev, devel,
linux-fsdevel
From: Alastair D'Silva <alastair@d-silva.org>
Apologies for the large CC list, it's a heads up for those responsible
for subsystems where a prototype change in generic code causes a change
in those subsystems.
This series enhances hexdump.
These improve the readability of the dumped data in certain situations
(eg. wide terminals are available, many lines of empty bytes exist, etc).
The default behaviour of hexdump is unchanged, however, the prototype
for hex_dump_to_buffer() has changed, and print_hex_dump() has been
renamed to print_hex_dump_ext(), with a wrapper replacing it for
compatibility with existing code, which would have been too invasive to
change.
Hexdump selftests have be run & confirmed passed.
Changelog:
V4:
- Add missing header (linux/bits.h)
- Fix comment formatting
- Create hex_dump_to_buffer_ext & make hex_dump_to_buffer a wrapper
V3:
- Fix inline documention
- use BIT macros
- use u32 rather than u64 for flags
V2:
- Fix failing selftests
- Fix precedence bug in 'Replace ascii bool in hex_dump_to_buffer...'
- Remove hardcoded new lengths & instead relax the checks in
hex_dump_to_buffer, allocating the buffer from the heap instead of the
stack.
- Replace the skipping of lines of 0x00/0xff with skipping lines of
repeated characters, announcing what has been skipped.
- Add spaces as an optional N-group separator
- Allow byte ordering to be maintained when HEXDUMP_RETAIN_BYTE_ORDERING
is set.
- Updated selftests to cover 'Relax rowsize checks' &
'Optionally retain byte ordering'
Alastair D'Silva (7):
lib/hexdump.c: Fix selftests
lib/hexdump.c: Relax rowsize checks in hex_dump_to_buffer
lib/hexdump.c: Optionally suppress lines of repeated bytes
lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags
lib/hexdump.c: Allow multiple groups to be separated by lines '|'
lib/hexdump.c: Allow multiple groups to be separated by spaces
lib/hexdump.c: Optionally retain byte ordering
drivers/gpu/drm/i915/intel_engine_cs.c | 5 +-
drivers/isdn/hardware/mISDN/mISDNisar.c | 10 +-
drivers/mailbox/mailbox-test.c | 8 +-
drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 2 +-
.../net/ethernet/synopsys/dwc-xlgmac-common.c | 2 +-
drivers/net/wireless/ath/ath10k/debug.c | 7 +-
.../net/wireless/intel/iwlegacy/3945-mac.c | 4 +-
drivers/platform/chrome/wilco_ec/debugfs.c | 10 +-
drivers/scsi/scsi_logging.c | 8 +-
drivers/staging/fbtft/fbtft-core.c | 2 +-
fs/seq_file.c | 6 +-
include/linux/printk.h | 75 ++++-
lib/hexdump.c | 267 +++++++++++++++---
lib/test_hexdump.c | 154 +++++++---
14 files changed, 438 insertions(+), 122 deletions(-)
--
2.21.0
^ permalink raw reply
* [PATCH v4 7/7] lib/hexdump.c: Optionally retain byte ordering
From: Alastair D'Silva @ 2019-06-25 3:17 UTC (permalink / raw)
To: alastair
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Daniel Vetter, Dan Carpenter, Karsten Keil, Jassi Brar,
Tom Lendacky, David S. Miller, Jose Abreu, Kalle Valo,
Stanislaw Gruszka, Benson Leung, Enric Balletbo i Serra,
James E.J. Bottomley, Martin K. Petersen, Greg Kroah-Hartman,
Alexander Viro, Petr Mladek, Sergey Senozhatsky, Steven Rostedt,
David Laight, Andrew Morton, intel-gfx, dri-devel, linux-kernel,
netdev, ath10k, linux-wireless, linux-scsi, linux-fbdev, devel,
linux-fsdevel
In-Reply-To: <20190625031726.12173-1-alastair@au1.ibm.com>
From: Alastair D'Silva <alastair@d-silva.org>
The behaviour of hexdump groups is to print the data out as if
it was a native-endian number.
This patch tweaks the documentation to make this clear, and also
adds the HEXDUMP_RETAIN_BYTE_ORDER flag to allow groups of
multiple bytes to be printed without affecting the ordering
of the printed bytes.
Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
include/linux/printk.h | 1 +
lib/hexdump.c | 30 ++++++++++++++++----
lib/test_hexdump.c | 62 ++++++++++++++++++++++++++++--------------
3 files changed, 68 insertions(+), 25 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 1d082291facf..ed1a79aa9695 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -491,6 +491,7 @@ enum {
#define HEXDUMP_2_GRP_SPACES BIT(5)
#define HEXDUMP_4_GRP_SPACES BIT(6)
#define HEXDUMP_8_GRP_SPACES BIT(7)
+#define HEXDUMP_RETAIN_BYTE_ORDER BIT(8)
extern int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
int groupsize, char *linebuf, size_t linebuflen,
diff --git a/lib/hexdump.c b/lib/hexdump.c
index e09e3cf8e595..29024eccf5da 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -127,7 +127,8 @@ static void separator_parameters(u64 flags, int groupsize, int *sep_chars,
* @buf: data blob to dump
* @len: number of bytes in the @buf
* @rowsize: number of bytes to print per line; must be a multiple of groupsize
- * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @groupsize: number of bytes to convert to a native endian number and print:
+ * 1, 2, 4, 8; default = 1
* @linebuf: where to put the converted data
* @linebuflen: total size of @linebuf, including space for terminating NUL
* @flags: A bitwise OR of the following flags:
@@ -138,6 +139,9 @@ static void separator_parameters(u64 flags, int groupsize, int *sep_chars,
* HEXDUMP_2_GRP_SPACES: insert a ' ' after every 2 groups
* HEXDUMP_4_GRP_SPACES: insert a ' ' after every 4 groups
* HEXDUMP_8_GRP_SPACES: insert a ' ' after every 8 groups
+ * HEXDUMP_RETAIN_BYTE_ORDER: Retain the byte ordering of groups
+ * instead of treating each group as a
+ * native-endian number
*
* hex_dump_to_buffer() works on one "line" of output at a time, converting
* <groupsize> bytes of input to hexadecimal (and optionally printable ASCII)
@@ -172,6 +176,7 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
int ret;
int sep_chars = 0;
char sep = 0;
+ bool big_endian = (flags & HEXDUMP_RETAIN_BYTE_ORDER) ? 1 : 0;
if (!is_power_of_2(groupsize) || groupsize > 8)
groupsize = 1;
@@ -203,10 +208,13 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
const u64 *ptr8 = buf;
for (j = 0; j < ngroups; j++) {
+ u64 val = big_endian ?
+ be64_to_cpu(get_unaligned(ptr8 + j)) :
+ get_unaligned(ptr8 + j);
ret = snprintf(linebuf + lx, linebuflen - lx,
"%s%16.16llx",
j ? group_separator(j, flags) : "",
- get_unaligned(ptr8 + j));
+ val);
if (ret >= linebuflen - lx)
goto overflow1;
lx += ret;
@@ -215,10 +223,14 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
const u32 *ptr4 = buf;
for (j = 0; j < ngroups; j++) {
+ u32 val = big_endian ?
+ be32_to_cpu(get_unaligned(ptr4 + j)) :
+ get_unaligned(ptr4 + j);
+
ret = snprintf(linebuf + lx, linebuflen - lx,
"%s%8.8x",
j ? group_separator(j, flags) : "",
- get_unaligned(ptr4 + j));
+ val);
if (ret >= linebuflen - lx)
goto overflow1;
lx += ret;
@@ -227,10 +239,14 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
const u16 *ptr2 = buf;
for (j = 0; j < ngroups; j++) {
+ u16 val = big_endian ?
+ be16_to_cpu(get_unaligned(ptr2 + j)) :
+ get_unaligned(ptr2 + j);
+
ret = snprintf(linebuf + lx, linebuflen - lx,
"%s%4.4x",
j ? group_separator(j, flags) : "",
- get_unaligned(ptr2 + j));
+ val);
if (ret >= linebuflen - lx)
goto overflow1;
lx += ret;
@@ -332,7 +348,8 @@ static void announce_skipped(const char *level, const char *prefix_str,
* @prefix_type: controls whether prefix of an offset, address, or none
* is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
* @rowsize: number of bytes to print per line; must be a multiple of groupsize
- * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @groupsize: number of bytes to convert to a native endian number and print:
+ * 1, 2, 4, 8; default = 1
* @buf: data blob to dump
* @len: number of bytes in the @buf
* @ascii: include ASCII after the hex output
@@ -343,6 +360,9 @@ static void announce_skipped(const char *level, const char *prefix_str,
* HEXDUMP_2_GRP_LINES: insert a '|' after every 2 groups
* HEXDUMP_4_GRP_LINES: insert a '|' after every 4 groups
* HEXDUMP_8_GRP_LINES: insert a '|' after every 8 groups
+ * HEXDUMP_RETAIN_BYTE_ORDER: Retain the byte ordering of groups
+ * instead of treating each group as a
+ * native-endian number
*
* Given a buffer of u8 data, print_hex_dump() prints a hex + ASCII dump
* to the kernel log at the specified kernel log level, with an optional
diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index ad43218437f1..d2cfcb3e2d2b 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -98,14 +98,15 @@ static unsigned failed_tests __initdata;
static void __init test_hexdump_prepare_test(size_t len, int rowsize,
int groupsize, char *test,
- size_t testlen, bool ascii)
+ size_t testlen, u64 flags)
{
char *p;
const char * const *result;
size_t l = len;
int gs = groupsize, rs = rowsize;
unsigned int i;
- const bool is_be = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
+ const bool is_be = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) ||
+ (flags & HEXDUMP_RETAIN_BYTE_ORDER);
if (l > rs)
l = rs;
@@ -142,7 +143,7 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
p--;
/* ASCII part */
- if (ascii) {
+ if (flags & HEXDUMP_ASCII) {
do {
*p++ = ' ';
} while (p < test + rs * 2 + rs / gs + 1);
@@ -157,7 +158,7 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
#define TEST_HEXDUMP_BUF_SIZE (64 * 3 + 2 + 64 + 1)
static void __init test_hexdump(size_t len, int rowsize, int groupsize,
- bool ascii)
+ u64 flags)
{
char test[TEST_HEXDUMP_BUF_SIZE];
char real[TEST_HEXDUMP_BUF_SIZE];
@@ -166,12 +167,11 @@ static void __init test_hexdump(size_t len, int rowsize, int groupsize,
memset(real, FILL_CHAR, sizeof(real));
hex_dump_to_buffer_ext(data_b, len, rowsize, groupsize,
- real, sizeof(real),
- ascii ? HEXDUMP_ASCII : 0);
+ real, sizeof(real), flags);
memset(test, FILL_CHAR, sizeof(test));
test_hexdump_prepare_test(len, rowsize, groupsize, test, sizeof(test),
- ascii);
+ flags);
if (memcmp(test, real, TEST_HEXDUMP_BUF_SIZE)) {
pr_err("Len: %zu row: %d group: %d\n", len, rowsize, groupsize);
@@ -194,7 +194,7 @@ static void __init test_hexdump_set(int rowsize, bool ascii)
static void __init test_hexdump_overflow(size_t buflen, size_t len,
int rowsize, int groupsize,
- bool ascii)
+ u64 flags)
{
char test[TEST_HEXDUMP_BUF_SIZE];
char buf[TEST_HEXDUMP_BUF_SIZE];
@@ -206,8 +206,7 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
memset(buf, FILL_CHAR, sizeof(buf));
rc = hex_dump_to_buffer_ext(data_b, len, rowsize, groupsize,
- buf, buflen,
- ascii ? HEXDUMP_ASCII : 0);
+ buf, buflen, flags);
/*
* Caller must provide the data length multiple of groupsize. The
@@ -224,12 +223,12 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
- 1 /* no trailing space */;
}
- expected_len = (ascii) ? ascii_len : hex_len;
+ expected_len = (flags & HEXDUMP_ASCII) ? ascii_len : hex_len;
fill_point = min_t(int, expected_len + 1, buflen);
if (buflen) {
test_hexdump_prepare_test(len, rowsize, groupsize, test,
- sizeof(test), ascii);
+ sizeof(test), flags);
test[fill_point - 1] = '\0';
}
memset(test + fill_point, FILL_CHAR, sizeof(test) - fill_point);
@@ -239,8 +238,8 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
buf[sizeof(buf) - 1] = '\0';
if (!match) {
- pr_err("rowsize: %u groupsize: %u ascii: %d Len: %zu buflen: %zu strlen: %zu\n",
- rowsize, groupsize, ascii, len, buflen,
+ pr_err("rowsize: %u groupsize: %u flags: %llx Len: %zu buflen: %zu strlen: %zu\n",
+ rowsize, groupsize, flags, len, buflen,
strnlen(buf, sizeof(buf)));
pr_err("Result: %d '%-.*s'\n", rc, (int)buflen, buf);
pr_err("Expect: %d '%-.*s'\n", expected_len, (int)buflen, test);
@@ -249,7 +248,7 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
}
}
-static void __init test_hexdump_overflow_set(size_t buflen, bool ascii)
+static void __init test_hexdump_overflow_set(size_t buflen, u64 flags)
{
unsigned int i = 0;
int rs = (get_random_int() % 4 + 1) * 16;
@@ -258,7 +257,7 @@ static void __init test_hexdump_overflow_set(size_t buflen, bool ascii)
int gs = 1 << i;
size_t len = get_random_int() % rs + gs;
- test_hexdump_overflow(buflen, rounddown(len, gs), rs, gs, ascii);
+ test_hexdump_overflow(buflen, rounddown(len, gs), rs, gs, flags);
} while (i++ < 3);
}
@@ -266,20 +265,43 @@ static int __init test_hexdump_init(void)
{
unsigned int i;
int rowsize;
+ u64 flags;
+ flags = 0;
rowsize = (get_random_int() % 4 + 1) * 16;
for (i = 0; i < 16; i++)
- test_hexdump_set(rowsize, false);
+ test_hexdump_set(rowsize, flags);
+ flags = HEXDUMP_ASCII;
rowsize = (get_random_int() % 4 + 1) * 16;
for (i = 0; i < 16; i++)
- test_hexdump_set(rowsize, true);
+ test_hexdump_set(rowsize, flags);
+ flags = HEXDUMP_RETAIN_BYTE_ORDER;
+ rowsize = (get_random_int() % 2 + 1) * 16;
+ for (i = 0; i < 16; i++)
+ test_hexdump_set(rowsize, flags);
+
+ flags = HEXDUMP_ASCII | HEXDUMP_RETAIN_BYTE_ORDER;
+ rowsize = (get_random_int() % 2 + 1) * 16;
+ for (i = 0; i < 16; i++)
+ test_hexdump_set(rowsize, flags);
+
+ flags = 0;
+ for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
+ test_hexdump_overflow_set(i, flags);
+
+ flags = HEXDUMP_ASCII;
+ for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
+ test_hexdump_overflow_set(i, flags);
+
+ flags = HEXDUMP_RETAIN_BYTE_ORDER;
for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
- test_hexdump_overflow_set(i, false);
+ test_hexdump_overflow_set(i, flags);
+ flags = HEXDUMP_ASCII | HEXDUMP_RETAIN_BYTE_ORDER;
for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
- test_hexdump_overflow_set(i, true);
+ test_hexdump_overflow_set(i, flags);
if (failed_tests == 0)
pr_info("all %u tests passed\n", total_tests);
--
2.21.0
^ permalink raw reply related
* [PATCH v4 6/7] lib/hexdump.c: Allow multiple groups to be separated by spaces
From: Alastair D'Silva @ 2019-06-25 3:17 UTC (permalink / raw)
To: alastair
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Daniel Vetter, Dan Carpenter, Karsten Keil, Jassi Brar,
Tom Lendacky, David S. Miller, Jose Abreu, Kalle Valo,
Stanislaw Gruszka, Benson Leung, Enric Balletbo i Serra,
James E.J. Bottomley, Martin K. Petersen, Greg Kroah-Hartman,
Alexander Viro, Petr Mladek, Sergey Senozhatsky, Steven Rostedt,
David Laight, Andrew Morton, intel-gfx, dri-devel, linux-kernel,
netdev, ath10k, linux-wireless, linux-scsi, linux-fbdev, devel,
linux-fsdevel
In-Reply-To: <20190625031726.12173-1-alastair@au1.ibm.com>
From: Alastair D'Silva <alastair@d-silva.org>
Similar to the previous patch, this patch separates groups by 2 spaces for
the hex fields, and 1 space for the ASCII field.
eg.
buf:00000000: 454d414e 43415053 4e495f45 00584544 NAMESPAC E_INDEX.
buf:00000010: 00000000 00000002 00000000 00000000 ........ ........
Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
include/linux/printk.h | 3 ++
lib/hexdump.c | 65 +++++++++++++++++++++++++++++++-----------
2 files changed, 52 insertions(+), 16 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index ae80d7af82ab..1d082291facf 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -488,6 +488,9 @@ enum {
#define HEXDUMP_2_GRP_LINES BIT(2)
#define HEXDUMP_4_GRP_LINES BIT(3)
#define HEXDUMP_8_GRP_LINES BIT(4)
+#define HEXDUMP_2_GRP_SPACES BIT(5)
+#define HEXDUMP_4_GRP_SPACES BIT(6)
+#define HEXDUMP_8_GRP_SPACES BIT(7)
extern int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
int groupsize, char *linebuf, size_t linebuflen,
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 4dcf759fe048..e09e3cf8e595 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -91,9 +91,37 @@ static const char *group_separator(int group, u64 flags)
if ((flags & HEXDUMP_2_GRP_LINES) && !((group) % 2))
return "|";
+ if ((flags & HEXDUMP_8_GRP_SPACES) && !((group) % 8))
+ return " ";
+
+ if ((flags & HEXDUMP_4_GRP_SPACES) && !((group) % 4))
+ return " ";
+
+ if ((flags & HEXDUMP_2_GRP_SPACES) && !((group) % 2))
+ return " ";
+
return " ";
}
+static void separator_parameters(u64 flags, int groupsize, int *sep_chars,
+ char *sep)
+{
+ if (flags & (HEXDUMP_2_GRP_LINES | HEXDUMP_2_GRP_SPACES))
+ *sep_chars = groupsize * 2;
+ if (flags & (HEXDUMP_4_GRP_LINES | HEXDUMP_4_GRP_SPACES))
+ *sep_chars = groupsize * 4;
+ if (flags & (HEXDUMP_8_GRP_LINES | HEXDUMP_8_GRP_SPACES))
+ *sep_chars = groupsize * 8;
+
+ if (flags & (HEXDUMP_2_GRP_LINES | HEXDUMP_4_GRP_LINES |
+ HEXDUMP_8_GRP_LINES))
+ *sep = '|';
+
+ if (flags & (HEXDUMP_2_GRP_SPACES | HEXDUMP_4_GRP_SPACES |
+ HEXDUMP_8_GRP_SPACES))
+ *sep = ' ';
+}
+
/**
* hex_dump_to_buffer - convert a blob of data to "hex ASCII" in memory
* @buf: data blob to dump
@@ -107,6 +135,9 @@ static const char *group_separator(int group, u64 flags)
* HEXDUMP_2_GRP_LINES: insert a '|' after every 2 groups
* HEXDUMP_4_GRP_LINES: insert a '|' after every 4 groups
* HEXDUMP_8_GRP_LINES: insert a '|' after every 8 groups
+ * HEXDUMP_2_GRP_SPACES: insert a ' ' after every 2 groups
+ * HEXDUMP_4_GRP_SPACES: insert a ' ' after every 4 groups
+ * HEXDUMP_8_GRP_SPACES: insert a ' ' after every 8 groups
*
* hex_dump_to_buffer() works on one "line" of output at a time, converting
* <groupsize> bytes of input to hexadecimal (and optionally printable ASCII)
@@ -139,7 +170,8 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
int j, lx = 0;
int ascii_column;
int ret;
- int line_chars = 0;
+ int sep_chars = 0;
+ char sep = 0;
if (!is_power_of_2(groupsize) || groupsize > 8)
groupsize = 1;
@@ -153,8 +185,14 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
len = rowsize;
ngroups = len / groupsize;
+
ascii_column = rowsize * 2 + rowsize / groupsize + 1;
+ // space separators use 2 spaces in the hex output
+ separator_parameters(flags, groupsize, &sep_chars, &sep);
+ if (sep == ' ')
+ ascii_column += rowsize / sep_chars;
+
if (!linebuflen)
goto overflow1;
@@ -222,24 +260,17 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
linebuf[lx++] = ' ';
}
- if (flags & HEXDUMP_2_GRP_LINES)
- line_chars = groupsize * 2;
- if (flags & HEXDUMP_4_GRP_LINES)
- line_chars = groupsize * 4;
- if (flags & HEXDUMP_8_GRP_LINES)
- line_chars = groupsize * 8;
-
for (j = 0; j < len; j++) {
if (linebuflen < lx + 2)
goto overflow2;
ch = ptr[j];
linebuf[lx++] = (isascii(ch) && isprint(ch)) ? ch : '.';
- if (line_chars && ((j + 1) < len) &&
- ((j + 1) % line_chars == 0)) {
+ if (sep_chars && ((j + 1) < len) &&
+ ((j + 1) % sep_chars == 0)) {
if (linebuflen < lx + 2)
goto overflow2;
- linebuf[lx++] = '|';
+ linebuf[lx++] = sep;
}
}
nil:
@@ -248,9 +279,11 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
overflow2:
linebuf[lx++] = '\0';
overflow1:
- return (flags & HEXDUMP_ASCII) ? ascii_column + len +
- (len - 1) / line_chars :
- (groupsize * 2 + 1) * ngroups - 1;
+ if (flags & HEXDUMP_ASCII)
+ return ascii_column + len + (len - 1) / sep_chars;
+
+ return groupsize * 2 * ngroups +
+ ((sep == ' ') ? 2 : 1) * (ngroups - 1);
}
EXPORT_SYMBOL(hex_dump_to_buffer_ext);
@@ -345,9 +378,9 @@ void print_hex_dump_ext(const char *level, const char *prefix_str,
/*
* Worst case line length:
* 2 hex chars + space per byte in, 2 spaces, 1 char per byte in,
- * 1 char per N groups, NULL
+ * 2 char per N groups, NULL
*/
- linebuf_len = rowsize * 3 + 2 + rowsize + rowsize / groupsize + 1;
+ linebuf_len = rowsize * 3 + 2 + rowsize + 2 * rowsize / groupsize + 1;
linebuf = kzalloc(linebuf_len, GFP_KERNEL);
if (!linebuf) {
printk("%s%shexdump: Could not alloc %u bytes for buffer\n",
--
2.21.0
^ permalink raw reply related
* [PATCH v4 4/7] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags
From: Alastair D'Silva @ 2019-06-25 3:17 UTC (permalink / raw)
To: alastair
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Daniel Vetter, Dan Carpenter, Karsten Keil, Jassi Brar,
Tom Lendacky, David S. Miller, Jose Abreu, Kalle Valo,
Stanislaw Gruszka, Benson Leung, Enric Balletbo i Serra,
James E.J. Bottomley, Martin K. Petersen, Greg Kroah-Hartman,
Alexander Viro, Petr Mladek, Sergey Senozhatsky, Steven Rostedt,
David Laight, Andrew Morton, intel-gfx, dri-devel, linux-kernel,
netdev, ath10k, linux-wireless, linux-scsi, linux-fbdev, devel,
linux-fsdevel
In-Reply-To: <20190625031726.12173-1-alastair@au1.ibm.com>
From: Alastair D'Silva <alastair@d-silva.org>
In order to support additional features, rename hex_dump_to_buffer to
hex_dump_to_buffer_ext, and replace the ascii bool parameter with flags.
A wrapper is provided for callers that do not need anything but a basic
dump.
Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
drivers/gpu/drm/i915/intel_engine_cs.c | 5 +-
drivers/isdn/hardware/mISDN/mISDNisar.c | 10 ++--
drivers/mailbox/mailbox-test.c | 8 ++--
drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 2 +-
.../net/ethernet/synopsys/dwc-xlgmac-common.c | 2 +-
drivers/net/wireless/ath/ath10k/debug.c | 7 +--
.../net/wireless/intel/iwlegacy/3945-mac.c | 4 +-
drivers/platform/chrome/wilco_ec/debugfs.c | 10 ++--
drivers/scsi/scsi_logging.c | 8 ++--
drivers/staging/fbtft/fbtft-core.c | 2 +-
fs/seq_file.c | 6 ++-
include/linux/printk.h | 46 +++++++++++++++++--
lib/hexdump.c | 24 +++++-----
lib/test_hexdump.c | 10 ++--
14 files changed, 94 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index eea9bec04f1b..64189a0e5ec9 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1338,9 +1338,8 @@ static void hexdump(struct drm_printer *m, const void *buf, size_t len)
}
WARN_ON_ONCE(hex_dump_to_buffer(buf + pos, len - pos,
- rowsize, sizeof(u32),
- line, sizeof(line),
- false) >= sizeof(line));
+ rowsize, sizeof(u32), line,
+ sizeof(line)) >= sizeof(line));
drm_printf(m, "[%04zx] %s\n", pos, line);
prev = buf + pos;
diff --git a/drivers/isdn/hardware/mISDN/mISDNisar.c b/drivers/isdn/hardware/mISDN/mISDNisar.c
index fd5c52f37802..84455b521246 100644
--- a/drivers/isdn/hardware/mISDN/mISDNisar.c
+++ b/drivers/isdn/hardware/mISDN/mISDNisar.c
@@ -70,8 +70,9 @@ send_mbox(struct isar_hw *isar, u8 his, u8 creg, u8 len, u8 *msg)
int l = 0;
while (l < (int)len) {
- hex_dump_to_buffer(msg + l, len - l, 32, 1,
- isar->log, 256, 1);
+ hex_dump_to_buffer_ext(msg + l, len - l, 32, 1,
+ isar->log, 256,
+ HEXDUMP_ASCII);
pr_debug("%s: %s %02x: %s\n", isar->name,
__func__, l, isar->log);
l += 32;
@@ -99,8 +100,9 @@ rcv_mbox(struct isar_hw *isar, u8 *msg)
int l = 0;
while (l < (int)isar->clsb) {
- hex_dump_to_buffer(msg + l, isar->clsb - l, 32,
- 1, isar->log, 256, 1);
+ hex_dump_to_buffer_ext(msg + l, isar->clsb - l,
+ 32, 1, isar->log, 256,
+ HEXDUMP_ASCII);
pr_debug("%s: %s %02x: %s\n", isar->name,
__func__, l, isar->log);
l += 32;
diff --git a/drivers/mailbox/mailbox-test.c b/drivers/mailbox/mailbox-test.c
index 4555d678fadd..ce334f88a3ee 100644
--- a/drivers/mailbox/mailbox-test.c
+++ b/drivers/mailbox/mailbox-test.c
@@ -206,10 +206,10 @@ static ssize_t mbox_test_message_read(struct file *filp, char __user *userbuf,
ptr = tdev->rx_buffer;
while (l < MBOX_HEXDUMP_MAX_LEN) {
- hex_dump_to_buffer(ptr,
- MBOX_BYTES_PER_LINE,
- MBOX_BYTES_PER_LINE, 1, touser + l,
- MBOX_HEXDUMP_LINE_LEN, true);
+ hex_dump_to_buffer_ext(ptr,
+ MBOX_BYTES_PER_LINE,
+ MBOX_BYTES_PER_LINE, 1, touser + l,
+ MBOX_HEXDUMP_LINE_LEN, HEXDUMP_ASCII);
ptr += MBOX_BYTES_PER_LINE;
l += MBOX_HEXDUMP_LINE_LEN;
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
index 3dd0cecddba8..f0118fe35c41 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
@@ -2992,7 +2992,7 @@ void xgbe_print_pkt(struct net_device *netdev, struct sk_buff *skb, bool tx_rx)
unsigned int len = min(skb->len - i, 32U);
hex_dump_to_buffer(&skb->data[i], len, 32, 1,
- buffer, sizeof(buffer), false);
+ buffer, sizeof(buffer));
netdev_dbg(netdev, " %#06x: %s\n", i, buffer);
}
diff --git a/drivers/net/ethernet/synopsys/dwc-xlgmac-common.c b/drivers/net/ethernet/synopsys/dwc-xlgmac-common.c
index eb1c6b03c329..53991c123c4d 100644
--- a/drivers/net/ethernet/synopsys/dwc-xlgmac-common.c
+++ b/drivers/net/ethernet/synopsys/dwc-xlgmac-common.c
@@ -349,7 +349,7 @@ void xlgmac_print_pkt(struct net_device *netdev,
unsigned int len = min(skb->len - i, 32U);
hex_dump_to_buffer(&skb->data[i], len, 32, 1,
- buffer, sizeof(buffer), false);
+ buffer, sizeof(buffer));
netdev_dbg(netdev, " %#06x: %s\n", i, buffer);
}
diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 32d967a31c65..8d79f8fc694d 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -2660,9 +2660,10 @@ void ath10k_dbg_dump(struct ath10k *ar,
"%s%08x: ",
(prefix ? prefix : ""),
(unsigned int)(ptr - buf));
- hex_dump_to_buffer(ptr, len - (ptr - buf), 16, 1,
- linebuf + linebuflen,
- sizeof(linebuf) - linebuflen, true);
+ hex_dump_to_buffer_ext(ptr, len - (ptr - buf), 16, 1,
+ linebuf + linebuflen,
+ sizeof(linebuf) - linebuflen,
+ HEXDUMP_ASCII);
dev_printk(KERN_DEBUG, ar->dev, "%s\n", linebuf);
}
}
diff --git a/drivers/net/wireless/intel/iwlegacy/3945-mac.c b/drivers/net/wireless/intel/iwlegacy/3945-mac.c
index b82da75a9ae3..92d030109395 100644
--- a/drivers/net/wireless/intel/iwlegacy/3945-mac.c
+++ b/drivers/net/wireless/intel/iwlegacy/3945-mac.c
@@ -3231,8 +3231,8 @@ il3945_show_measurement(struct device *d, struct device_attribute *attr,
spin_unlock_irqrestore(&il->lock, flags);
while (size && PAGE_SIZE - len) {
- hex_dump_to_buffer(data + ofs, size, 16, 1, buf + len,
- PAGE_SIZE - len, true);
+ hex_dump_to_buffer_ext(data + ofs, size, 16, 1, buf + len,
+ PAGE_SIZE - len, HEXDUMP_ASCII);
len = strlen(buf);
if (PAGE_SIZE - len)
buf[len++] = '\n';
diff --git a/drivers/platform/chrome/wilco_ec/debugfs.c b/drivers/platform/chrome/wilco_ec/debugfs.c
index f163476d080d..6c0a443bdb1b 100644
--- a/drivers/platform/chrome/wilco_ec/debugfs.c
+++ b/drivers/platform/chrome/wilco_ec/debugfs.c
@@ -144,11 +144,11 @@ static ssize_t raw_read(struct file *file, char __user *user_buf, size_t count,
int fmt_len = 0;
if (debug_info->response_size) {
- fmt_len = hex_dump_to_buffer(debug_info->raw_data,
- debug_info->response_size,
- 16, 1, debug_info->formatted_data,
- sizeof(debug_info->formatted_data),
- true);
+ fmt_len = hex_dump_to_buffer_ext(debug_info->raw_data,
+ debug_info->response_size,
+ 16, 1, debug_info->formatted_data,
+ sizeof(debug_info->formatted_data),
+ HEXDUMP_ASCII);
/* Only return response the first time it is read */
debug_info->response_size = 0;
}
diff --git a/drivers/scsi/scsi_logging.c b/drivers/scsi/scsi_logging.c
index 39b8cc4574b4..aead28e5c4cd 100644
--- a/drivers/scsi/scsi_logging.c
+++ b/drivers/scsi/scsi_logging.c
@@ -262,7 +262,7 @@ void scsi_print_command(struct scsi_cmnd *cmd)
"CDB[%02x]: ", k);
hex_dump_to_buffer(&cmd->cmnd[k], linelen,
16, 1, logbuf + off,
- logbuf_len - off, false);
+ logbuf_len - off);
}
dev_printk(KERN_INFO, &cmd->device->sdev_gendev, "%s",
logbuf);
@@ -273,8 +273,7 @@ void scsi_print_command(struct scsi_cmnd *cmd)
if (!WARN_ON(off > logbuf_len - 49)) {
off += scnprintf(logbuf + off, logbuf_len - off, " ");
hex_dump_to_buffer(cmd->cmnd, cmd->cmd_len, 16, 1,
- logbuf + off, logbuf_len - off,
- false);
+ logbuf + off, logbuf_len - off);
}
out_printk:
dev_printk(KERN_INFO, &cmd->device->sdev_gendev, "%s", logbuf);
@@ -353,8 +352,7 @@ scsi_log_dump_sense(const struct scsi_device *sdev, const char *name, int tag,
off = sdev_format_header(logbuf, logbuf_len,
name, tag);
hex_dump_to_buffer(&sense_buffer[i], len, 16, 1,
- logbuf + off, logbuf_len - off,
- false);
+ logbuf + off, logbuf_len - off);
dev_printk(KERN_INFO, &sdev->sdev_gendev, "%s", logbuf);
}
scsi_log_release_buffer(logbuf);
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 9b07badf4c6c..9765a7962f0c 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -61,7 +61,7 @@ void fbtft_dbg_hex(const struct device *dev, int groupsize,
va_end(args);
hex_dump_to_buffer(buf, len, 32, groupsize, text + text_len,
- 512 - text_len, false);
+ 512 - text_len);
if (len > 32)
dev_info(dev, "%s ...\n", text);
diff --git a/fs/seq_file.c b/fs/seq_file.c
index abe27ec43176..5f3d656c5ed6 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -873,8 +873,10 @@ void seq_hex_dump(struct seq_file *m, const char *prefix_str, int prefix_type,
}
size = seq_get_buf(m, &buffer);
- ret = hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
- buffer, size, ascii);
+ ret = hex_dump_to_buffer_ext(ptr + i, linelen,
+ rowsize, groupsize,
+ buffer, size,
+ ascii ? HEXDUMP_ASCII : 0);
seq_commit(m, ret < size ? ret : -1);
seq_putc(m, '\n');
diff --git a/include/linux/printk.h b/include/linux/printk.h
index c0416d0eb9e2..f0761b3a2d5d 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -483,13 +483,51 @@ enum {
DUMP_PREFIX_OFFSET
};
-extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
- int groupsize, char *linebuf, size_t linebuflen,
- bool ascii);
-
#define HEXDUMP_ASCII BIT(0)
#define HEXDUMP_SUPPRESS_REPEATED BIT(1)
+extern int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
+ int groupsize, char *linebuf, size_t linebuflen,
+ u32 flags);
+
+/**
+ * hex_dump_to_buffer - convert a blob of data to "hex ASCII" in memory
+ * @buf: data blob to dump
+ * @len: number of bytes in the @buf
+ * @rowsize: number of bytes to print per line; must be a multiple of groupsize
+ * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @linebuf: where to put the converted data
+ * @linebuflen: total size of @linebuf, including space for terminating NUL
+ *
+ * hex_dump_to_buffer() works on one "line" of output at a time, converting
+ * <groupsize> bytes of input to hexadecimal (and optionally printable ASCII)
+ * until <rowsize> bytes have been emitted.
+ *
+ * Given a buffer of u8 data, hex_dump_to_buffer() converts the input data
+ * to a hex dump at the supplied memory location.
+ * The converted output is always NUL-terminated.
+ *
+ * E.g.:
+ * hex_dump_to_buffer(frame->data, frame->len, 16, 1,
+ * linebuf, sizeof(linebuf));
+ *
+ * example output buffer:
+ * 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f
+ *
+ * Return:
+ * The amount of bytes placed in the buffer without terminating NUL. If the
+ * output was truncated, then the return value is the number of bytes
+ * (excluding the terminating NUL) which would have been written to the final
+ * string if enough space had been available.
+ */
+static inline int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
+ int groupsize, char *linebuf, size_t linebuflen)
+{
+ return hex_dump_to_buffer_ext(buf, len, rowsize, groupsize,
+ linebuf, linebuflen, 0);
+}
+
+
#ifdef CONFIG_PRINTK
extern void print_hex_dump_ext(const char *level, const char *prefix_str,
int prefix_type, int rowsize, int groupsize,
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 61dc625c32f5..1bf838c1a568 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -85,7 +85,8 @@ EXPORT_SYMBOL(bin2hex);
* @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
* @linebuf: where to put the converted data
* @linebuflen: total size of @linebuf, including space for terminating NUL
- * @ascii: include ASCII after the hex output
+ * @flags: A bitwise OR of the following flags:
+ * HEXDUMP_ASCII: include ASCII after the hex output
*
* hex_dump_to_buffer() works on one "line" of output at a time, converting
* <groupsize> bytes of input to hexadecimal (and optionally printable ASCII)
@@ -96,8 +97,8 @@ EXPORT_SYMBOL(bin2hex);
* The converted output is always NUL-terminated.
*
* E.g.:
- * hex_dump_to_buffer(frame->data, frame->len, 16, 1,
- * linebuf, sizeof(linebuf), true);
+ * hex_dump_to_buffer_ext(frame->data, frame->len, 16, 1,
+ * linebuf, sizeof(linebuf), HEXDUMP_ASCII);
*
* example output buffer:
* 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f @ABCDEFGHIJKLMNO
@@ -108,8 +109,9 @@ EXPORT_SYMBOL(bin2hex);
* (excluding the terminating NUL) which would have been written to the final
* string if enough space had been available.
*/
-int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
- char *linebuf, size_t linebuflen, bool ascii)
+int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
+ int groupsize, char *linebuf, size_t linebuflen,
+ u32 flags)
{
const u8 *ptr = buf;
int ngroups;
@@ -187,7 +189,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
if (j)
lx--;
}
- if (!ascii)
+ if (!(flags & HEXDUMP_ASCII))
goto nil;
while (lx < ascii_column) {
@@ -207,9 +209,10 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
overflow2:
linebuf[lx++] = '\0';
overflow1:
- return ascii ? ascii_column + len : (groupsize * 2 + 1) * ngroups - 1;
+ return (flags & HEXDUMP_ASCII) ? ascii_column + len :
+ (groupsize * 2 + 1) * ngroups - 1;
}
-EXPORT_SYMBOL(hex_dump_to_buffer);
+EXPORT_SYMBOL(hex_dump_to_buffer_ext);
#ifdef CONFIG_PRINTK
@@ -336,9 +339,8 @@ void print_hex_dump_ext(const char *level, const char *prefix_str,
skipped = 0;
}
- hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
- linebuf, linebuf_len,
- flags & HEXDUMP_ASCII);
+ hex_dump_to_buffer_ext(ptr + i, linelen, rowsize, groupsize,
+ linebuf, linebuf_len, flags);
switch (prefix_type) {
case DUMP_PREFIX_ADDRESS:
diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 5e54525a937c..ad43218437f1 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -165,8 +165,9 @@ static void __init test_hexdump(size_t len, int rowsize, int groupsize,
total_tests++;
memset(real, FILL_CHAR, sizeof(real));
- hex_dump_to_buffer(data_b, len, rowsize, groupsize, real, sizeof(real),
- ascii);
+ hex_dump_to_buffer_ext(data_b, len, rowsize, groupsize,
+ real, sizeof(real),
+ ascii ? HEXDUMP_ASCII : 0);
memset(test, FILL_CHAR, sizeof(test));
test_hexdump_prepare_test(len, rowsize, groupsize, test, sizeof(test),
@@ -204,8 +205,9 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
memset(buf, FILL_CHAR, sizeof(buf));
- rc = hex_dump_to_buffer(data_b, len, rowsize, groupsize, buf, buflen,
- ascii);
+ rc = hex_dump_to_buffer_ext(data_b, len, rowsize, groupsize,
+ buf, buflen,
+ ascii ? HEXDUMP_ASCII : 0);
/*
* Caller must provide the data length multiple of groupsize. The
--
2.21.0
^ permalink raw reply related
* [PATCH v4 5/7] lib/hexdump.c: Allow multiple groups to be separated by lines '|'
From: Alastair D'Silva @ 2019-06-25 3:17 UTC (permalink / raw)
To: alastair
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
Daniel Vetter, Dan Carpenter, Karsten Keil, Jassi Brar,
Tom Lendacky, David S. Miller, Jose Abreu, Kalle Valo,
Stanislaw Gruszka, Benson Leung, Enric Balletbo i Serra,
James E.J. Bottomley, Martin K. Petersen, Greg Kroah-Hartman,
Alexander Viro, Petr Mladek, Sergey Senozhatsky, Steven Rostedt,
David Laight, Andrew Morton, intel-gfx, dri-devel, linux-kernel,
netdev, ath10k, linux-wireless, linux-scsi, linux-fbdev, devel,
linux-fsdevel
In-Reply-To: <20190625031726.12173-1-alastair@au1.ibm.com>
From: Alastair D'Silva <alastair@d-silva.org>
With the wider display format, it can become hard to identify how many
bytes into the line you are looking at.
The patch adds new flags to hex_dump_to_buffer() and print_hex_dump() to
print vertical lines to separate every N groups of bytes.
eg.
buf:00000000: 454d414e 43415053|4e495f45 00584544 NAMESPAC|E_INDEX.
buf:00000010: 00000000 00000002|00000000 00000000 ........|........
Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
include/linux/printk.h | 3 +++
lib/hexdump.c | 59 ++++++++++++++++++++++++++++++++++++------
2 files changed, 54 insertions(+), 8 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index f0761b3a2d5d..ae80d7af82ab 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -485,6 +485,9 @@ enum {
#define HEXDUMP_ASCII BIT(0)
#define HEXDUMP_SUPPRESS_REPEATED BIT(1)
+#define HEXDUMP_2_GRP_LINES BIT(2)
+#define HEXDUMP_4_GRP_LINES BIT(3)
+#define HEXDUMP_8_GRP_LINES BIT(4)
extern int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
int groupsize, char *linebuf, size_t linebuflen,
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 1bf838c1a568..4dcf759fe048 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -77,6 +77,23 @@ char *bin2hex(char *dst, const void *src, size_t count)
}
EXPORT_SYMBOL(bin2hex);
+static const char *group_separator(int group, u64 flags)
+{
+ if (group == 0)
+ return " ";
+
+ if ((flags & HEXDUMP_8_GRP_LINES) && !((group) % 8))
+ return "|";
+
+ if ((flags & HEXDUMP_4_GRP_LINES) && !((group) % 4))
+ return "|";
+
+ if ((flags & HEXDUMP_2_GRP_LINES) && !((group) % 2))
+ return "|";
+
+ return " ";
+}
+
/**
* hex_dump_to_buffer - convert a blob of data to "hex ASCII" in memory
* @buf: data blob to dump
@@ -87,6 +104,9 @@ EXPORT_SYMBOL(bin2hex);
* @linebuflen: total size of @linebuf, including space for terminating NUL
* @flags: A bitwise OR of the following flags:
* HEXDUMP_ASCII: include ASCII after the hex output
+ * HEXDUMP_2_GRP_LINES: insert a '|' after every 2 groups
+ * HEXDUMP_4_GRP_LINES: insert a '|' after every 4 groups
+ * HEXDUMP_8_GRP_LINES: insert a '|' after every 8 groups
*
* hex_dump_to_buffer() works on one "line" of output at a time, converting
* <groupsize> bytes of input to hexadecimal (and optionally printable ASCII)
@@ -119,6 +139,7 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
int j, lx = 0;
int ascii_column;
int ret;
+ int line_chars = 0;
if (!is_power_of_2(groupsize) || groupsize > 8)
groupsize = 1;
@@ -145,7 +166,8 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
for (j = 0; j < ngroups; j++) {
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%16.16llx", j ? " " : "",
+ "%s%16.16llx",
+ j ? group_separator(j, flags) : "",
get_unaligned(ptr8 + j));
if (ret >= linebuflen - lx)
goto overflow1;
@@ -156,7 +178,8 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
for (j = 0; j < ngroups; j++) {
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%8.8x", j ? " " : "",
+ "%s%8.8x",
+ j ? group_separator(j, flags) : "",
get_unaligned(ptr4 + j));
if (ret >= linebuflen - lx)
goto overflow1;
@@ -167,7 +190,8 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
for (j = 0; j < ngroups; j++) {
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%4.4x", j ? " " : "",
+ "%s%4.4x",
+ j ? group_separator(j, flags) : "",
get_unaligned(ptr2 + j));
if (ret >= linebuflen - lx)
goto overflow1;
@@ -197,11 +221,26 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
goto overflow2;
linebuf[lx++] = ' ';
}
+
+ if (flags & HEXDUMP_2_GRP_LINES)
+ line_chars = groupsize * 2;
+ if (flags & HEXDUMP_4_GRP_LINES)
+ line_chars = groupsize * 4;
+ if (flags & HEXDUMP_8_GRP_LINES)
+ line_chars = groupsize * 8;
+
for (j = 0; j < len; j++) {
if (linebuflen < lx + 2)
goto overflow2;
ch = ptr[j];
linebuf[lx++] = (isascii(ch) && isprint(ch)) ? ch : '.';
+
+ if (line_chars && ((j + 1) < len) &&
+ ((j + 1) % line_chars == 0)) {
+ if (linebuflen < lx + 2)
+ goto overflow2;
+ linebuf[lx++] = '|';
+ }
}
nil:
linebuf[lx] = '\0';
@@ -209,7 +248,8 @@ int hex_dump_to_buffer_ext(const void *buf, size_t len, int rowsize,
overflow2:
linebuf[lx++] = '\0';
overflow1:
- return (flags & HEXDUMP_ASCII) ? ascii_column + len :
+ return (flags & HEXDUMP_ASCII) ? ascii_column + len +
+ (len - 1) / line_chars :
(groupsize * 2 + 1) * ngroups - 1;
}
EXPORT_SYMBOL(hex_dump_to_buffer_ext);
@@ -247,7 +287,7 @@ static void announce_skipped(const char *level, const char *prefix_str,
if (count == 0)
return;
- printk("%s%s ** Skipped %lu bytes of value 0x%x **\n",
+ printk("%s%s ** Skipped %lu bytes of value 0x%02x **\n",
level, prefix_str, count, val);
}
@@ -267,6 +307,9 @@ static void announce_skipped(const char *level, const char *prefix_str,
* HEXDUMP_ASCII: include ASCII after the hex output
* HEXDUMP_SUPPRESS_REPEATED: suppress repeated lines of identical
* bytes
+ * HEXDUMP_2_GRP_LINES: insert a '|' after every 2 groups
+ * HEXDUMP_4_GRP_LINES: insert a '|' after every 4 groups
+ * HEXDUMP_8_GRP_LINES: insert a '|' after every 8 groups
*
* Given a buffer of u8 data, print_hex_dump() prints a hex + ASCII dump
* to the kernel log at the specified kernel log level, with an optional
@@ -296,15 +339,15 @@ void print_hex_dump_ext(const char *level, const char *prefix_str,
u8 skipped_val = 0;
size_t skipped = 0;
-
if (rowsize % groupsize)
rowsize -= rowsize % groupsize;
/*
* Worst case line length:
- * 2 hex chars + space per byte in, 2 spaces, 1 char per byte in, NULL
+ * 2 hex chars + space per byte in, 2 spaces, 1 char per byte in,
+ * 1 char per N groups, NULL
*/
- linebuf_len = rowsize * 3 + 2 + rowsize + 1;
+ linebuf_len = rowsize * 3 + 2 + rowsize + rowsize / groupsize + 1;
linebuf = kzalloc(linebuf_len, GFP_KERNEL);
if (!linebuf) {
printk("%s%shexdump: Could not alloc %u bytes for buffer\n",
--
2.21.0
^ permalink raw reply related
* Re: [RFC V1 net-next 1/1] net: ena: implement XDP drop support
From: Machulsky, Zorik @ 2019-06-25 3:19 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Jubran, Samih
Cc: davem@davemloft.net, netdev@vger.kernel.org, Woodhouse, David,
Matushevsky, Alexander, Bshara, Saeed, Wilson, Matt,
Liguori, Anthony, Bshara, Nafea, Tzalik, Guy, Belgazal, Netanel,
Saidi, Ali, Herrenschmidt, Benjamin, Kiyanovski, Arthur,
Daniel Borkmann
In-Reply-To: <20190623162133.6b7f24e1@carbon>
On 6/23/19, 7:21 AM, "Jesper Dangaard Brouer" <brouer@redhat.com> wrote:
On Sun, 23 Jun 2019 10:06:49 +0300 <sameehj@amazon.com> wrote:
> This commit implements the basic functionality of drop/pass logic in the
> ena driver.
Usually we require a driver to implement all the XDP return codes,
before we accept it. But as Daniel and I discussed with Zorik during
NetConf[1], we are going to make an exception and accept the driver
if you also implement XDP_TX.
As we trust that Zorik/Amazon will follow and implement XDP_REDIRECT
later, given he/you wants AF_XDP support which requires XDP_REDIRECT.
Jesper, thanks for your comments and very helpful discussion during NetConf!
That's the plan, as we agreed. From our side I would like to reiterate again the
importance of multi-buffer support by xdp frame. We would really prefer not
to see our MTU shrinking because of xdp support.
[1] http://vger.kernel.org/netconf2019.html
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: Removing skb_orphan() from ip_rcv_core()
From: Joe Stringer @ 2019-06-25 3:17 UTC (permalink / raw)
To: Florian Westphal
Cc: Joe Stringer, Eric Dumazet, netdev, john fastabend,
Daniel Borkmann, Lorenz Bauer, Jakub Sitnicki, Paolo Abeni
In-Reply-To: <20190621205935.og7ajx57j7usgycq@breakpoint.cc>
On Fri, Jun 21, 2019 at 1:59 PM Florian Westphal <fw@strlen.de> wrote:
>
> Joe Stringer <joe@wand.net.nz> wrote:
> > As discussed during LSFMM, I've been looking at adding something like
> > an `skb_sk_assign()` helper to BPF so that logic similar to TPROXY can
> > be implemented with integration into other BPF logic, however
> > currently any attempts to do so are blocked by the skb_orphan() call
> > in ip_rcv_core() (which will effectively ignore any socket assign
> > decision made by the TC BPF program).
> >
> > Recently I was attempting to remove the skb_orphan() call, and I've
> > been trying different things but there seems to be some context I'm
> > missing. Here's the core of the patch:
> >
> > diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
> > index ed97724c5e33..16aea980318a 100644
> > --- a/net/ipv4/ip_input.c
> > +++ b/net/ipv4/ip_input.c
> > @@ -500,8 +500,6 @@ static struct sk_buff *ip_rcv_core(struct sk_buff
> > *skb, struct net *net)
> > memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
> > IPCB(skb)->iif = skb->skb_iif;
> >
> > - /* Must drop socket now because of tproxy. */
> > - skb_orphan(skb);
> >
> > return skb;
> >
> > The statement that the socket must be dropped because of tproxy
> > doesn't make sense to me, because the PRE_ROUTING hook is hit after
> > this, which will call into the tproxy logic and eventually
> > nf_tproxy_assign_sock() which already does the skb_orphan() itself.
>
> in comment: s/tproxy/skb_steal_sock/
For reference, I was following the path like this:
ip_rcv()
( -> ip_rcv_core() for skb_orphan)
-> NF_INET_PRE_ROUTING hook
(... invoke iptables hooks)
-> iptable_mangle_hook()
-> ipt_do_table()
... -> tproxy_tg4()
... -> nf_tproxy_assign_sock()
-> skb_orphan()
(... finish iptables processing)
( -> ip_rcv_finish())
( ... -> ip_rcv_finish_core() for early demux / route lookup )
(... -> dst_input())
(... -> tcp_v4_rcv())
( -> __inet_lookup_skb())
( -> skb_steal_sock() )
> at least thats what I concluded a few years ago when I looked into
> the skb_oprhan() need.
>
> IIRC some device drivers use skb->sk for backpressure, so without this
> non-tcp socket would be stolen by skb_steal_sock.
Do you happen to recall which device drivers? Or have some idea of a
list I could try to go through? Are you referring to virtual drivers
like veth or something else?
> We also recently removed skb orphan when crossing netns:
>
> commit 9c4c325252c54b34d53b3d0ffd535182b744e03d
> Author: Flavio Leitner <fbl@redhat.com>
> skbuff: preserve sock reference when scrubbing the skb.
>
> So thats another case where this orphan is needed.
Presumably the orphan is only needed in this case if the packet
crosses a namespace and then is subsequently passed back into the
stack?
> What could be done is adding some way to delay/defer the orphaning
> further, but we would need at the very least some annotation for
> skb_steal_sock to know when the skb->sk is really from TPROXY or
> if it has to orphan.
Eric mentions in another response to this thread that skb_orphan()
should be called from any ndo_start_xmit() which sends traffic back
into the stack. With that, presumably we would be pushing the
orphaning earlier such that the only way that the skb->sk ref can be
non-NULL around this point in receive would be because it was
specifically set by some kind of tproxy logic?
> Same for the safety check in the forwarding path.
> Netfilter modules need o be audited as well, they might make assumptions
> wrt. skb->sk being inet sockets (set by local stack or early demux).
>
> > However, if I drop these lines then I end up causing sockets to
> > release references too many times. Seems like if we don't orphan the
> > skb here, then later logic assumes that we have one more reference
> > than we actually have, and decrements the count when it shouldn't
> > (perhaps the skb_steal_sock() call in __inet_lookup_skb() which seems
> > to assume we always have a reference to the socket?)
>
> We might be calling the wrong destructor (i.e., the one set by tcp
> receive instead of the one set at tx time)?
Hmm, interesting thought. Sure enough, with a bit of bpftrace
debugging we find it's tcp_wfree():
$ cat ip_rcv.bt
#include <linux/skbuff.h>
kprobe:ip_rcv {
$sk = ((struct sk_buff *)arg0)->sk;
$des = ((struct sk_buff *)arg0)->destructor;
if ($sk) {
if ($des) {
printf("received %s on %s with sk destructor %s
set\n", str(arg0), str(arg1), ksym($des));
@ip4_stacks[kstack] = count();
}
}
}
$ sudo bpftrace ip_rcv.bt
Attaching 1 probe...
received on eth0 with sk destructor tcp_wfree set
^C
@ip4_stacks[
ip_rcv+1
__netif_receive_skb+24
process_backlog+179
net_rx_action+304
__do_softirq+220
do_softirq_own_stack+42
do_softirq.part.17+70
__local_bh_enable_ip+101
ip_finish_output2+421
__ip_finish_output+187
ip_finish_output+44
ip_output+109
ip_local_out+59
__ip_queue_xmit+368
ip_queue_xmit+16
__tcp_transmit_skb+1303
tcp_connect+2758
tcp_v4_connect+1135
__inet_stream_connect+214
inet_stream_connect+59
__sys_connect+237
__x64_sys_connect+26
do_syscall_64+90
entry_SYSCALL_64_after_hwframe+68
]: 1
Is there a solution here where we call the destructor if it's not
sock_efree()? When the socket is later stolen, it will only return the
reference via a call to sock_put(), so presumably at that point in the
stack we already assume that the skb->destructor is not one of these
other destructors (otherwise we wouldn't release the resources
correctly).
^ permalink raw reply
* [PATCH 2/2] net/ipv6: Fix misuse of proc_dointvec "skip_notify_on_dev_down"
From: Eiichi Tsukata @ 2019-06-25 3:08 UTC (permalink / raw)
To: gregkh, jslaby, davem, kuznet, yoshfuji, linux-kernel, netdev
Cc: Eiichi Tsukata
In-Reply-To: <20190625030801.24538-1-devel@etsukata.com>
/proc/sys/net/ipv6/route/skip_notify_on_dev_down assumes given value to be
0 or 1. Use proc_dointvec_minmax instead of proc_dointvec.
Fixes: 7c6bb7d2faaf ("net/ipv6: Add knob to skip DELROUTE message ondevice down")
Signed-off-by: Eiichi Tsukata <devel@etsukata.com>
---
net/ipv6/route.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 11ad62effd56..aade636c6be6 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -5281,7 +5281,7 @@ static struct ctl_table ipv6_route_table_template[] = {
.data = &init_net.ipv6.sysctl.skip_notify_on_dev_down,
.maxlen = sizeof(int),
.mode = 0644,
- .proc_handler = proc_dointvec,
+ .proc_handler = proc_dointvec_minmax,
.extra1 = &zero,
.extra2 = &one,
},
--
2.21.0
^ permalink raw reply related
* [PATCH 1/2] tty: ldisc: Fix misuse of proc_dointvec "ldisc_autoload"
From: Eiichi Tsukata @ 2019-06-25 3:08 UTC (permalink / raw)
To: gregkh, jslaby, davem, kuznet, yoshfuji, linux-kernel, netdev
Cc: Eiichi Tsukata
/proc/sys/dev/tty/ldisc_autoload assumes given value to be 0 or 1. Use
proc_dointvec_minmax instead of proc_dointvec.
Fixes: 7c0cca7c847e "(tty: ldisc: add sysctl to prevent autoloading of ldiscs)"
Signed-off-by: Eiichi Tsukata <devel@etsukata.com>
---
drivers/tty/tty_ldisc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index e38f104db174..a8ea7a35c94e 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -863,7 +863,7 @@ static struct ctl_table tty_table[] = {
.data = &tty_ldisc_autoload,
.maxlen = sizeof(tty_ldisc_autoload),
.mode = 0644,
- .proc_handler = proc_dointvec,
+ .proc_handler = proc_dointvec_minmax,
.extra1 = &zero,
.extra2 = &one,
},
--
2.21.0
^ permalink raw reply related
* [PATCH net-next] xdp: Make __mem_id_disconnect static
From: YueHaibing @ 2019-06-25 2:31 UTC (permalink / raw)
To: davem, ast, daniel, jakub.kicinski, hawk, john.fastabend
Cc: linux-kernel, netdev, xdp-newbies, bpf, YueHaibing
Fix sparse warning:
net/core/xdp.c:88:6: warning:
symbol '__mem_id_disconnect' was not declared. Should it be static?
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
net/core/xdp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/xdp.c b/net/core/xdp.c
index b29d7b5..829377c 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -85,7 +85,7 @@ static void __xdp_mem_allocator_rcu_free(struct rcu_head *rcu)
kfree(xa);
}
-bool __mem_id_disconnect(int id, bool force)
+static bool __mem_id_disconnect(int id, bool force)
{
struct xdp_mem_allocator *xa;
bool safe_to_remove = true;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next] net: link_watch: prevent starvation when processing linkwatch wq
From: Yunsheng Lin @ 2019-06-25 2:28 UTC (permalink / raw)
To: David Miller
Cc: hkallweit1, f.fainelli, netdev, linux-kernel, linuxarm, pbonzini,
rkrcmar, kvm, xuwei (O)
In-Reply-To: <6e9b41c9-6edb-be7f-07ee-5480162a227e@huawei.com>
On 2019/5/29 16:59, Yunsheng Lin wrote:
> On 2019/5/29 14:58, David Miller wrote:
>> From: Yunsheng Lin <linyunsheng@huawei.com>
>> Date: Mon, 27 May 2019 09:47:54 +0800
>>
>>> When user has configured a large number of virtual netdev, such
>>> as 4K vlans, the carrier on/off operation of the real netdev
>>> will also cause it's virtual netdev's link state to be processed
>>> in linkwatch. Currently, the processing is done in a work queue,
>>> which may cause worker starvation problem for other work queue.
>>>
>>> This patch releases the cpu when link watch worker has processed
>>> a fixed number of netdev' link watch event, and schedule the
>>> work queue again when there is still link watch event remaining.
>>>
>>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>>
>> Why not rtnl_unlock(); yield(); rtnl_lock(); every "100" events
>> processed?
>>
>> That seems better than adding all of this overhead to reschedule the
>> workqueue every 100 items.
>
> One minor concern, the above solution does not seem to solve the cpu
> starvation for other normal workqueue which was scheduled on the same
> cpu as linkwatch. Maybe I misunderstand the workqueue or there is other
> consideration here? :)
>
> Anyway, I will implemet it as you suggested and test it before posting V2.
> Thanks.
Hi, David
I stress tested the above solution with a lot of vlan dev and qemu-kvm with
vf passthrongh mode, the linkwatch wq sometimes block the irqfd_inject wq
when they are scheduled on the same cpu, which may cause interrupt delay
problem for vm.
Rescheduling workqueue every 100 items does give irqfd_inject wq to run sooner,
which alleviate the interrupt delay problems for vm.
So It is ok for me to fall back to reschedule the link watch wq every 100 items,
or is there a better way to fix it properly?
>
>>
>> .
>>
>
>
> .
>
^ permalink raw reply
* Re: [PATCH v2 0/3] fix bugs when enable route_localnet
From: Luoshijie (Poincare Lab) @ 2019-06-25 1:50 UTC (permalink / raw)
To: David Miller
Cc: tgraf, dsahern, netdev, liuzhiqiang26, wangxiaogang3, mingfangsen,
zhoukang7
In-Reply-To: <20190624.090325.163853495970223718.davem@davemloft.net>
On 2019/6/25 0:03, David Miller wrote:
> From: luoshijie <luoshijie1@huawei.com>
> Date: Tue, 18 Jun 2019 15:14:02 +0000
>
>> From: Shijie Luo <luoshijie1@huawei.com>
>>
>> When enable route_localnet, route of the 127/8 address is enabled.
>> But in some situations like arp_announce=2, ARP requests or reply
>> work abnormally.
>>
>> This patchset fix some bugs when enable route_localnet.
>>
>> Change History:
>> V2:
>> - Change a single patch to a patchset.
>> - Add bug fix for arp_ignore = 3.
>> - Add a couple of test for enabling route_localnet in selftests.
>
> Series applied to net-next, thanks.
>
Thanks a lot, and I'm truly grateful for advice and help of David Ahern.
^ permalink raw reply
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Takshak Chahande @ 2019-06-25 1:39 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Jakub Kicinski, Andrey Ignatov, Daniel Borkmann,
netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
Stanislav Fomichev
In-Reply-To: <99a92dd0-4914-aeb5-709b-ed4615820aa0@fb.com>
Alexei Starovoitov <ast@fb.com> wrote on Mon [2019-Jun-24 18:23:28 -0700]:
> On 6/24/19 5:57 PM, Jakub Kicinski wrote:
> > On Mon, 24 Jun 2019 17:47:26 -0700, Jakub Kicinski wrote:
> >> I see. The local flag would not an option in getopt_long() sense, what
> >> I was thinking was about adding an "effective" keyword:
> >
> > Something like this, untested:
> >
> > --->8------------
> >
> > The BPF_F_QUERY_EFFECTIVE is a syscall flag, and fits nicely
> > as a subcommand option. We want to move away from global
> > options, anyway.
> >
> > We need a global variable because of nftw limitations.
> > Clean this flag on every invocations in case we run in
> > batch mode.
> >
> > NOTE the argv[1] use on the error path in do_show() looks
> > like a bug on it's own.
> > ---
> > .../bpftool/Documentation/bpftool-cgroup.rst | 24 +++----
> > tools/bpf/bpftool/Documentation/bpftool.rst | 6 +-
> > tools/bpf/bpftool/bash-completion/bpftool | 17 ++---
> > tools/bpf/bpftool/cgroup.c | 62 ++++++++++++-------
> > tools/bpf/bpftool/main.c | 7 +--
> > tools/bpf/bpftool/main.h | 3 +-
> > 6 files changed, 66 insertions(+), 53 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> > index 324df15bf4cc..4fde3dfad395 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> > @@ -12,8 +12,7 @@ SYNOPSIS
> >
> > **bpftool** [*OPTIONS*] **cgroup** *COMMAND*
> >
> > - *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** }
> > - | { **-e** | **--effective** } }
> > + *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
> >
> > *COMMANDS* :=
> > { **show** | **list** | **tree** | **attach** | **detach** | **help** }
> > @@ -21,8 +20,8 @@ SYNOPSIS
> > CGROUP COMMANDS
> > ===============
> >
> > -| **bpftool** **cgroup { show | list }** *CGROUP*
> > -| **bpftool** **cgroup tree** [*CGROUP_ROOT*]
> > +| **bpftool** **cgroup { show | list }** *CGROUP* [**effective**]
> > +| **bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**]
> > | **bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> > | **bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
> > | **bpftool** **cgroup help**
> > @@ -35,13 +34,17 @@ CGROUP COMMANDS
> >
> > DESCRIPTION
> > ===========
> > - **bpftool cgroup { show | list }** *CGROUP*
> > + **bpftool cgroup { show | list }** *CGROUP* [**effective**]
> > List all programs attached to the cgroup *CGROUP*.
> >
> > Output will start with program ID followed by attach type,
> > attach flags and program name.
> >
> > - **bpftool cgroup tree** [*CGROUP_ROOT*]
> > + If **effective** is specified retrieve effective programs that
> > + will execute for events within a cgroup. This includes
> > + inherited along with attached ones.
> > +
> > + **bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**]
> > Iterate over all cgroups in *CGROUP_ROOT* and list all
> > attached programs. If *CGROUP_ROOT* is not specified,
> > bpftool uses cgroup v2 mountpoint.
> > @@ -50,6 +53,10 @@ DESCRIPTION
> > commands: it starts with absolute cgroup path, followed by
> > program ID, attach type, attach flags and program name.
> >
> > + If **effective** is specified retrieve effective programs that
> > + will execute for events within a cgroup. This includes
> > + inherited along with attached ones.
> > +
> > **bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> > Attach program *PROG* to the cgroup *CGROUP* with attach type
> > *ATTACH_TYPE* and optional *ATTACH_FLAGS*.
> > @@ -122,11 +129,6 @@ OPTIONS
> > Print all logs available from libbpf, including debug-level
> > information.
> >
> > - -e, --effective
> > - Retrieve effective programs that will execute for events
> > - within a cgroup. This includes inherited along with attached
> > - ones.
> > -
> > EXAMPLES
> > ========
> > |
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
> > index d2f76b55988d..6a9c52ef84a9 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool.rst
> > @@ -19,7 +19,7 @@ SYNOPSIS
> > *OBJECT* := { **map** | **program** | **cgroup** | **perf** | **net** | **feature** }
> >
> > *OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** }
> > - | { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-e** | **--effective** } }
> > + | { **-j** | **--json** } [{ **-p** | **--pretty** }] }
> >
> > *MAP-COMMANDS* :=
> > { **show** | **list** | **create** | **dump** | **update** | **lookup** | **getnext**
> > @@ -71,10 +71,6 @@ OPTIONS
> > includes logs from libbpf as well as from the verifier, when
> > attempting to load programs.
> >
> > - -e, --effective
> > - Retrieve effective programs that will execute for events
> > - within a cgroup. This includes inherited along with attached ones.
> > -
> > SEE ALSO
> > ========
> > **bpf**\ (2),
> > diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> > index c98cb99867f6..de84ae06ae4e 100644
> > --- a/tools/bpf/bpftool/bash-completion/bpftool
> > +++ b/tools/bpf/bpftool/bash-completion/bpftool
> > @@ -187,7 +187,7 @@ _bpftool()
> >
> > # Deal with options
> > if [[ ${words[cword]} == -* ]]; then
> > - local c='--version --json --pretty --bpffs --mapcompat --debug --effective'
> > + local c='--version --json --pretty --bpffs --mapcompat --debug'
> > COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
> > return 0
> > fi
> > @@ -678,12 +678,15 @@ _bpftool()
> > ;;
> > cgroup)
> > case $command in
> > - show|list)
> > - _filedir
> > - return 0
> > - ;;
> > - tree)
> > - _filedir
> > + show|list|tree)
> > + case $cword in
> > + 3)
> > + _filedir
> > + ;;
> > + 4)
> > + COMPREPLY=( $( compgen -W 'effective' -- "$cur" ) )
> > + ;;
> > + esac
> > return 0
> > ;;
> > attach|detach)
> > diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> > index 1bb2a751107a..88b80616d47b 100644
> > --- a/tools/bpf/bpftool/cgroup.c
> > +++ b/tools/bpf/bpftool/cgroup.c
> > @@ -28,6 +28,8 @@
> > " connect6 | sendmsg4 | sendmsg6 |\n" \
> > " recvmsg4 | recvmsg6 | sysctl }"
> >
> > +static unsigned int query_flags;
> > +
> > static const char * const attach_type_strings[] = {
> > [BPF_CGROUP_INET_INGRESS] = "ingress",
> > [BPF_CGROUP_INET_EGRESS] = "egress",
> > @@ -104,8 +106,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
> > __u32 prog_cnt = 0;
> > int ret;
> >
> > - ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL,
> > - &prog_cnt);
> > + ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL,
> > + NULL, &prog_cnt);
> > if (ret)
> > return -1;
> >
> > @@ -156,20 +158,30 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
> > static int do_show(int argc, char **argv)
> > {
> > enum bpf_attach_type type;
> > + const char *path;
> > int cgroup_fd;
> > int ret = -1;
> >
> > - if (argc < 1) {
> > - p_err("too few parameters for cgroup show");
> > - goto exit;
> > - } else if (argc > 1) {
> > - p_err("too many parameters for cgroup show");
> > - goto exit;
> > + query_flags = 0;
> > +
> > + if (!REQ_ARGS(1))
> > + return -1;
> > + path = GET_ARG();
> > +
> > + while (argc) {
> > + if (is_prefix(*argv, "effective")) {
> > + query_flags |= BPF_F_QUERY_EFFECTIVE;
> > + NEXT_ARG();
> > + } else {
> > + p_err("expected no more arguments, 'effective', got: '%s'?",
> > + *argv);
> > + return -1;
> > + }
> > }
> >
> > - cgroup_fd = open(argv[0], O_RDONLY);
> > + cgroup_fd = open(path, O_RDONLY);
> > if (cgroup_fd < 0) {
> > - p_err("can't open cgroup %s", argv[1]);
> > + p_err("can't open cgroup %s", path);
> > goto exit;
> > }
> >
> > @@ -295,23 +307,29 @@ static int do_show_tree(int argc, char **argv)
> > char *cgroup_root;
> > int ret;
> >
> > - switch (argc) {
> > - case 0:
> > + query_flags = 0;
> > +
> > + if (!argc) {
> > cgroup_root = find_cgroup_root();
> > if (!cgroup_root) {
> > p_err("cgroup v2 isn't mounted");
> > return -1;
> > }
> > - break;
> > - case 1:
> > - cgroup_root = argv[0];
> > - break;
> > - default:
> > - p_err("too many parameters for cgroup tree");
> > - return -1;
> > + } else {
> > + cgroup_root = GET_ARG();
> > +
> > + while (argc) {
> > + if (is_prefix(*argv, "effective")) {
> > + query_flags |= BPF_F_QUERY_EFFECTIVE;
> > + NEXT_ARG();
> > + } else {
> > + p_err("expected no more arguments, 'effective', got: '%s'?",
> > + *argv);
> > + return -1;
> > + }
> > + }
> > }
> >
> > -
> > if (json_output)
> > jsonw_start_array(json_wtr);
> > else
> > @@ -457,8 +475,8 @@ static int do_help(int argc, char **argv)
> > }
> >
> > fprintf(stderr,
> > - "Usage: %s %s { show | list } CGROUP\n"
> > - " %s %s tree [CGROUP_ROOT]\n"
> > + "Usage: %s %s { show | list } CGROUP [**effective**]\n"
> > + " %s %s tree [CGROUP_ROOT] [**effective**]\n"
>
> lgtm.
> Takshak, Andrey, wdyt?
ya, looks fine to me.
--Takshak
^ permalink raw reply
* Re: [PATCH v2 0/3] fix bugs when enable route_localnet
From: Zhiqiang Liu @ 2019-06-25 1:25 UTC (permalink / raw)
To: David Miller, luoshijie1
Cc: tgraf, dsahern, netdev, wangxiaogang3, mingfangsen, zhoukang7
In-Reply-To: <20190624.090325.163853495970223718.davem@davemloft.net>
On 2019/6/25 0:03, David Miller wrote:
> From: luoshijie <luoshijie1@huawei.com>
> Date: Tue, 18 Jun 2019 15:14:02 +0000
>
>> From: Shijie Luo <luoshijie1@huawei.com>
>>
>> When enable route_localnet, route of the 127/8 address is enabled.
>> But in some situations like arp_announce=2, ARP requests or reply
>> work abnormally.
>>
>> This patchset fix some bugs when enable route_localnet.
>>
>> Change History:
>> V2:
>> - Change a single patch to a patchset.
>> - Add bug fix for arp_ignore = 3.
>> - Add a couple of test for enabling route_localnet in selftests.
>
> Series applied to net-next, thanks.
>
Thanks again for you and David Ahern.
^ permalink raw reply
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Alexei Starovoitov @ 2019-06-25 1:23 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
Stanislav Fomichev
In-Reply-To: <20190624175740.5cccea9b@cakuba.netronome.com>
On 6/24/19 5:57 PM, Jakub Kicinski wrote:
> On Mon, 24 Jun 2019 17:47:26 -0700, Jakub Kicinski wrote:
>> I see. The local flag would not an option in getopt_long() sense, what
>> I was thinking was about adding an "effective" keyword:
>
> Something like this, untested:
>
> --->8------------
>
> The BPF_F_QUERY_EFFECTIVE is a syscall flag, and fits nicely
> as a subcommand option. We want to move away from global
> options, anyway.
>
> We need a global variable because of nftw limitations.
> Clean this flag on every invocations in case we run in
> batch mode.
>
> NOTE the argv[1] use on the error path in do_show() looks
> like a bug on it's own.
> ---
> .../bpftool/Documentation/bpftool-cgroup.rst | 24 +++----
> tools/bpf/bpftool/Documentation/bpftool.rst | 6 +-
> tools/bpf/bpftool/bash-completion/bpftool | 17 ++---
> tools/bpf/bpftool/cgroup.c | 62 ++++++++++++-------
> tools/bpf/bpftool/main.c | 7 +--
> tools/bpf/bpftool/main.h | 3 +-
> 6 files changed, 66 insertions(+), 53 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> index 324df15bf4cc..4fde3dfad395 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> @@ -12,8 +12,7 @@ SYNOPSIS
>
> **bpftool** [*OPTIONS*] **cgroup** *COMMAND*
>
> - *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** }
> - | { **-e** | **--effective** } }
> + *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
>
> *COMMANDS* :=
> { **show** | **list** | **tree** | **attach** | **detach** | **help** }
> @@ -21,8 +20,8 @@ SYNOPSIS
> CGROUP COMMANDS
> ===============
>
> -| **bpftool** **cgroup { show | list }** *CGROUP*
> -| **bpftool** **cgroup tree** [*CGROUP_ROOT*]
> +| **bpftool** **cgroup { show | list }** *CGROUP* [**effective**]
> +| **bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**]
> | **bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> | **bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
> | **bpftool** **cgroup help**
> @@ -35,13 +34,17 @@ CGROUP COMMANDS
>
> DESCRIPTION
> ===========
> - **bpftool cgroup { show | list }** *CGROUP*
> + **bpftool cgroup { show | list }** *CGROUP* [**effective**]
> List all programs attached to the cgroup *CGROUP*.
>
> Output will start with program ID followed by attach type,
> attach flags and program name.
>
> - **bpftool cgroup tree** [*CGROUP_ROOT*]
> + If **effective** is specified retrieve effective programs that
> + will execute for events within a cgroup. This includes
> + inherited along with attached ones.
> +
> + **bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**]
> Iterate over all cgroups in *CGROUP_ROOT* and list all
> attached programs. If *CGROUP_ROOT* is not specified,
> bpftool uses cgroup v2 mountpoint.
> @@ -50,6 +53,10 @@ DESCRIPTION
> commands: it starts with absolute cgroup path, followed by
> program ID, attach type, attach flags and program name.
>
> + If **effective** is specified retrieve effective programs that
> + will execute for events within a cgroup. This includes
> + inherited along with attached ones.
> +
> **bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> Attach program *PROG* to the cgroup *CGROUP* with attach type
> *ATTACH_TYPE* and optional *ATTACH_FLAGS*.
> @@ -122,11 +129,6 @@ OPTIONS
> Print all logs available from libbpf, including debug-level
> information.
>
> - -e, --effective
> - Retrieve effective programs that will execute for events
> - within a cgroup. This includes inherited along with attached
> - ones.
> -
> EXAMPLES
> ========
> |
> diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
> index d2f76b55988d..6a9c52ef84a9 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool.rst
> @@ -19,7 +19,7 @@ SYNOPSIS
> *OBJECT* := { **map** | **program** | **cgroup** | **perf** | **net** | **feature** }
>
> *OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** }
> - | { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-e** | **--effective** } }
> + | { **-j** | **--json** } [{ **-p** | **--pretty** }] }
>
> *MAP-COMMANDS* :=
> { **show** | **list** | **create** | **dump** | **update** | **lookup** | **getnext**
> @@ -71,10 +71,6 @@ OPTIONS
> includes logs from libbpf as well as from the verifier, when
> attempting to load programs.
>
> - -e, --effective
> - Retrieve effective programs that will execute for events
> - within a cgroup. This includes inherited along with attached ones.
> -
> SEE ALSO
> ========
> **bpf**\ (2),
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index c98cb99867f6..de84ae06ae4e 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -187,7 +187,7 @@ _bpftool()
>
> # Deal with options
> if [[ ${words[cword]} == -* ]]; then
> - local c='--version --json --pretty --bpffs --mapcompat --debug --effective'
> + local c='--version --json --pretty --bpffs --mapcompat --debug'
> COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
> return 0
> fi
> @@ -678,12 +678,15 @@ _bpftool()
> ;;
> cgroup)
> case $command in
> - show|list)
> - _filedir
> - return 0
> - ;;
> - tree)
> - _filedir
> + show|list|tree)
> + case $cword in
> + 3)
> + _filedir
> + ;;
> + 4)
> + COMPREPLY=( $( compgen -W 'effective' -- "$cur" ) )
> + ;;
> + esac
> return 0
> ;;
> attach|detach)
> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> index 1bb2a751107a..88b80616d47b 100644
> --- a/tools/bpf/bpftool/cgroup.c
> +++ b/tools/bpf/bpftool/cgroup.c
> @@ -28,6 +28,8 @@
> " connect6 | sendmsg4 | sendmsg6 |\n" \
> " recvmsg4 | recvmsg6 | sysctl }"
>
> +static unsigned int query_flags;
> +
> static const char * const attach_type_strings[] = {
> [BPF_CGROUP_INET_INGRESS] = "ingress",
> [BPF_CGROUP_INET_EGRESS] = "egress",
> @@ -104,8 +106,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
> __u32 prog_cnt = 0;
> int ret;
>
> - ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL,
> - &prog_cnt);
> + ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL,
> + NULL, &prog_cnt);
> if (ret)
> return -1;
>
> @@ -156,20 +158,30 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
> static int do_show(int argc, char **argv)
> {
> enum bpf_attach_type type;
> + const char *path;
> int cgroup_fd;
> int ret = -1;
>
> - if (argc < 1) {
> - p_err("too few parameters for cgroup show");
> - goto exit;
> - } else if (argc > 1) {
> - p_err("too many parameters for cgroup show");
> - goto exit;
> + query_flags = 0;
> +
> + if (!REQ_ARGS(1))
> + return -1;
> + path = GET_ARG();
> +
> + while (argc) {
> + if (is_prefix(*argv, "effective")) {
> + query_flags |= BPF_F_QUERY_EFFECTIVE;
> + NEXT_ARG();
> + } else {
> + p_err("expected no more arguments, 'effective', got: '%s'?",
> + *argv);
> + return -1;
> + }
> }
>
> - cgroup_fd = open(argv[0], O_RDONLY);
> + cgroup_fd = open(path, O_RDONLY);
> if (cgroup_fd < 0) {
> - p_err("can't open cgroup %s", argv[1]);
> + p_err("can't open cgroup %s", path);
> goto exit;
> }
>
> @@ -295,23 +307,29 @@ static int do_show_tree(int argc, char **argv)
> char *cgroup_root;
> int ret;
>
> - switch (argc) {
> - case 0:
> + query_flags = 0;
> +
> + if (!argc) {
> cgroup_root = find_cgroup_root();
> if (!cgroup_root) {
> p_err("cgroup v2 isn't mounted");
> return -1;
> }
> - break;
> - case 1:
> - cgroup_root = argv[0];
> - break;
> - default:
> - p_err("too many parameters for cgroup tree");
> - return -1;
> + } else {
> + cgroup_root = GET_ARG();
> +
> + while (argc) {
> + if (is_prefix(*argv, "effective")) {
> + query_flags |= BPF_F_QUERY_EFFECTIVE;
> + NEXT_ARG();
> + } else {
> + p_err("expected no more arguments, 'effective', got: '%s'?",
> + *argv);
> + return -1;
> + }
> + }
> }
>
> -
> if (json_output)
> jsonw_start_array(json_wtr);
> else
> @@ -457,8 +475,8 @@ static int do_help(int argc, char **argv)
> }
>
> fprintf(stderr,
> - "Usage: %s %s { show | list } CGROUP\n"
> - " %s %s tree [CGROUP_ROOT]\n"
> + "Usage: %s %s { show | list } CGROUP [**effective**]\n"
> + " %s %s tree [CGROUP_ROOT] [**effective**]\n"
lgtm.
Takshak, Andrey, wdyt?
^ permalink raw reply
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Alexei Starovoitov @ 2019-06-25 1:22 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
Stanislav Fomichev
In-Reply-To: <20190624174726.2dda122b@cakuba.netronome.com>
On 6/24/19 5:47 PM, Jakub Kicinski wrote:
> On Tue, 25 Jun 2019 00:40:09 +0000, Alexei Starovoitov wrote:
>> On 6/24/19 5:30 PM, Jakub Kicinski wrote:
>>> On Tue, 25 Jun 2019 00:21:57 +0000, Alexei Starovoitov wrote:
>>>> On 6/24/19 5:16 PM, Jakub Kicinski wrote:
>>>>> On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote:
>>>>>> I don't think this patch should be penalized.
>>>>>> I'd rather see we fix them all.
>>>>>
>>>>> So we are going to add this broken option just to remove it?
>>>>> I don't understand.
>>>>> I'm happy to spend the 15 minutes rewriting this if you don't
>>>>> want to penalize Takshak.
>>>>
>>>> hmm. I don't understand the 'broken' part.
>>>> The only issue I see that it could have been local vs global,
>>>> but they all should have been local.
>>>
>>> I don't think all of them. Only --mapcompat and --bpffs. bpffs could
>>> be argued. On mapcompat I must have not read the patch fully, I was
>>> under the impression its a global libbpf flag :(
>>>
>>> --json, --pretty, --nomount, --debug are global because they affect
>>> global behaviour of bpftool. The difference here is that we basically
>>> add a syscall parameter as a global option.
>>
>> sure. I only disagreed about not touching older flags.
>> --effective should be local.
>> If follow up patch means 90% rewrite then revert is better.
>> If it's 10% fixup then it's different story.
>
> I see. The local flag would not an option in getopt_long() sense, what
> I was thinking was about adding an "effective" keyword:
>
> # bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/
>
> becomes:
>
> # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/ effective
>
> That's how we handle flags for update calls for instance..
>
> So I think a revert :(
fair enough.
removed it and force pushed bpf-next.
^ permalink raw reply
* [PATCH net] ipv4: Use return value of inet_iif() for __raw_v4_lookup in the while loop
From: Stephen Suryaputra @ 2019-06-25 0:14 UTC (permalink / raw)
To: netdev; +Cc: dsahern, Stephen Suryaputra
In commit 19e4e768064a8 ("ipv4: Fix raw socket lookup for local
traffic"), the dif argument to __raw_v4_lookup() is coming from the
returned value of inet_iif() but the change was done only for the first
lookup. Subsequent lookups in the while loop still use skb->dev->ifIndex.
Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
---
net/ipv4/raw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index 0b8e06ca75d6..40a6abbc9cf6 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -197,7 +197,7 @@ static int raw_v4_input(struct sk_buff *skb, const struct iphdr *iph, int hash)
}
sk = __raw_v4_lookup(net, sk_next(sk), iph->protocol,
iph->saddr, iph->daddr,
- skb->dev->ifindex, sdif);
+ dif, sdif);
}
out:
read_unlock(&raw_v4_hashinfo.lock);
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 1/1] tc-testing: Restore original behaviour for namespaces in tdc
From: Lucas Bates @ 2019-06-25 1:00 UTC (permalink / raw)
To: davem
Cc: netdev, nicolas.dichtel, jhs, xiyou.wangcong, jiri, mleitner,
vladbu, dcaratti, kernel, Lucas Bates
This patch restores the original behaviour for tdc prior to the
introduction of the plugin system, where the network namespace
functionality was split from the main script.
It introduces the concept of required plugins for testcases,
and will automatically load any plugin that isn't already
enabled when said plugin is required by even one testcase.
Additionally, the -n option for the nsPlugin is deprecated
so the default action is to make use of the namespaces.
Instead, we introduce -N to not use them, but still create
the veth pair.
buildebpfPlugin's -B option is also deprecated.
If a test cases requires the features of a specific plugin
in order to pass, it should instead include a new key/value
pair describing plugin interactions:
"plugins": {
"requires": "buildebpfPlugin"
},
A test case can have more than one required plugin: a list
can be inserted as the value for 'requires'.
Signed-off-by: Lucas Bates <lucasb@mojatatu.com>
---
tools/testing/selftests/tc-testing/README | 22 ++-
.../tc-testing/plugin-lib/buildebpfPlugin.py | 5 +-
.../selftests/tc-testing/plugin-lib/nsPlugin.py | 26 +++-
.../selftests/tc-testing/tc-tests/actions/bpf.json | 6 +
.../selftests/tc-testing/tc-tests/filters/fw.json | 162 +++++++++++++++++++++
.../tc-testing/tc-tests/filters/tests.json | 12 ++
tools/testing/selftests/tc-testing/tdc.py | 78 +++++++++-
tools/testing/selftests/tc-testing/tdc_helper.py | 5 +-
8 files changed, 296 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/tc-testing/README b/tools/testing/selftests/tc-testing/README
index f9281e8..22e5da9 100644
--- a/tools/testing/selftests/tc-testing/README
+++ b/tools/testing/selftests/tc-testing/README
@@ -12,10 +12,10 @@ REQUIREMENTS
* Minimum Python version of 3.4. Earlier 3.X versions may work but are not
guaranteed.
-* The kernel must have network namespace support
+* The kernel must have network namespace support if using nsPlugin
* The kernel must have veth support available, as a veth pair is created
- prior to running the tests.
+ prior to running the tests when using nsPlugin.
* The kernel must have the appropriate infrastructure enabled to run all tdc
unit tests. See the config file in this directory for minimum required
@@ -53,8 +53,12 @@ commands being tested must be run as root. The code that enforces
execution by root uid has been moved into a plugin (see PLUGIN
ARCHITECTURE, below).
-If nsPlugin is linked, all tests are executed inside a network
-namespace to prevent conflicts within the host.
+Tests that use a network device should have nsPlugin.py listed as a
+requirement for that test. nsPlugin executes all commands within a
+network namespace and creates a veth pair which may be used in those test
+cases. To disable execution within the namespace, pass the -N option
+to tdc when starting a test run; the veth pair will still be created
+by the plugin.
Running tdc without any arguments will run all tests. Refer to the section
on command line arguments for more information, or run:
@@ -154,8 +158,8 @@ action:
netns:
options for nsPlugin (run commands in net namespace)
- -n, --namespace
- Run commands in namespace as specified in tdc_config.py
+ -N, --no-namespace
+ Do not run commands in a network namespace.
valgrind:
options for valgrindPlugin (run command under test under Valgrind)
@@ -171,7 +175,8 @@ was in the tdc.py script has been moved into the plugins.
The plugins are in the directory plugin-lib. The are executed from
directory plugins. Put symbolic links from plugins to plugin-lib,
-and name them according to the order you want them to run.
+and name them according to the order you want them to run. This is not
+necessary if a test case being run requires a specific plugin to work.
Example:
@@ -223,7 +228,8 @@ directory:
- rootPlugin.py:
implements the enforcement of running as root
- nsPlugin.py:
- sets up a network namespace and runs all commands in that namespace
+ sets up a network namespace and runs all commands in that namespace,
+ while also setting up dummy devices to be used in testing.
- valgrindPlugin.py
runs each command in the execute stage under valgrind,
and checks for leaks.
diff --git a/tools/testing/selftests/tc-testing/plugin-lib/buildebpfPlugin.py b/tools/testing/selftests/tc-testing/plugin-lib/buildebpfPlugin.py
index 9f0ba10..e98c367 100644
--- a/tools/testing/selftests/tc-testing/plugin-lib/buildebpfPlugin.py
+++ b/tools/testing/selftests/tc-testing/plugin-lib/buildebpfPlugin.py
@@ -34,8 +34,9 @@ class SubPlugin(TdcPlugin):
'buildebpf',
'options for buildebpfPlugin')
self.argparser_group.add_argument(
- '-B', '--buildebpf', action='store_true',
- help='build eBPF programs')
+ '--nobuildebpf', action='store_false', default=True,
+ dest='buildebpf',
+ help='Don\'t build eBPF programs')
return self.argparser
diff --git a/tools/testing/selftests/tc-testing/plugin-lib/nsPlugin.py b/tools/testing/selftests/tc-testing/plugin-lib/nsPlugin.py
index a194b1a..affa7f2 100644
--- a/tools/testing/selftests/tc-testing/plugin-lib/nsPlugin.py
+++ b/tools/testing/selftests/tc-testing/plugin-lib/nsPlugin.py
@@ -18,6 +18,8 @@ class SubPlugin(TdcPlugin):
if self.args.namespace:
self._ns_create()
+ else:
+ self._ports_create()
def post_suite(self, index):
'''run commands after test_runner goes into a test loop'''
@@ -27,6 +29,8 @@ class SubPlugin(TdcPlugin):
if self.args.namespace:
self._ns_destroy()
+ else:
+ self._ports_destroy()
def add_args(self, parser):
super().add_args(parser)
@@ -34,8 +38,8 @@ class SubPlugin(TdcPlugin):
'netns',
'options for nsPlugin(run commands in net namespace)')
self.argparser_group.add_argument(
- '-n', '--namespace', action='store_true',
- help='Run commands in namespace')
+ '-N', '--no-namespace', action='store_false', default=True,
+ dest='namespace', help='Don\'t run commands in namespace')
return self.argparser
def adjust_command(self, stage, command):
@@ -73,20 +77,30 @@ class SubPlugin(TdcPlugin):
print('adjust_command: return command [{}]'.format(command))
return command
+ def _ports_create(self):
+ cmd = 'ip link add $DEV0 type veth peer name $DEV1'
+ self._exec_cmd('pre', cmd)
+ cmd = 'ip link set $DEV0 up'
+ self._exec_cmd('pre', cmd)
+ if not self.args.namespace:
+ cmd = 'ip link set $DEV1 up'
+ self._exec_cmd('pre', cmd)
+
+ def _ports_destroy(self):
+ cmd = 'ip link del $DEV0'
+ self._exec_cmd('post', cmd)
+
def _ns_create(self):
'''
Create the network namespace in which the tests will be run and set up
the required network devices for it.
'''
+ self._ports_create()
if self.args.namespace:
cmd = 'ip netns add {}'.format(self.args.NAMES['NS'])
self._exec_cmd('pre', cmd)
- cmd = 'ip link add $DEV0 type veth peer name $DEV1'
- self._exec_cmd('pre', cmd)
cmd = 'ip link set $DEV1 netns {}'.format(self.args.NAMES['NS'])
self._exec_cmd('pre', cmd)
- cmd = 'ip link set $DEV0 up'
- self._exec_cmd('pre', cmd)
cmd = 'ip -n {} link set $DEV1 up'.format(self.args.NAMES['NS'])
self._exec_cmd('pre', cmd)
if self.args.device:
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/bpf.json b/tools/testing/selftests/tc-testing/tc-tests/actions/bpf.json
index b074ea9..47a3082 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/bpf.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/bpf.json
@@ -54,6 +54,9 @@
"actions",
"bpf"
],
+ "plugins": {
+ "requires": "buildebpfPlugin"
+ },
"setup": [
[
"$TC action flush action bpf",
@@ -78,6 +81,9 @@
"actions",
"bpf"
],
+ "plugins": {
+ "requires": "buildebpfPlugin"
+ },
"setup": [
[
"$TC action flush action bpf",
diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/fw.json b/tools/testing/selftests/tc-testing/tc-tests/filters/fw.json
index 6944b90..5272049 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/filters/fw.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/filters/fw.json
@@ -6,6 +6,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress"
],
@@ -25,6 +28,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress"
],
@@ -44,6 +50,114 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress"
],
@@ -872,6 +986,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: protocol 802_3 prio 3 handle 7 fw action ok"
@@ -892,6 +1009,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: prio 6 handle 2 fw action continue index 5"
@@ -912,6 +1032,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress"
],
@@ -931,6 +1054,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress"
],
@@ -950,6 +1076,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: handle 5 prio 7 fw action pass",
@@ -972,6 +1101,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: handle 5 prio 7 fw action pass",
@@ -994,6 +1126,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: handle 5 prio 7 fw action pass",
@@ -1015,6 +1150,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: handle 1 prio 4 fw action ok",
@@ -1036,6 +1174,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: handle 4 prio 2 chain 13 fw action pipe",
@@ -1057,6 +1198,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: handle 2 prio 4 fw action drop"
@@ -1077,6 +1221,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: handle 3 prio 4 fw action continue"
@@ -1097,6 +1244,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: handle 4 prio 2 protocol arp fw action pipe"
@@ -1117,6 +1267,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: handle 4 prio 2 fw action pipe flowid 45"
@@ -1137,6 +1290,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: handle 1 prio 2 fw action ok"
@@ -1157,6 +1313,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: handle 1 prio 2 fw action ok"
@@ -1177,6 +1336,9 @@
"filter",
"fw"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress",
"$TC filter add dev $DEV1 parent ffff: handle 1 prio 2 fw action ok index 3"
diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
index e2f92ce..8135778 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
@@ -6,6 +6,9 @@
"filter",
"u32"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 ingress"
],
@@ -25,6 +28,9 @@
"filter",
"matchall"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV1 clsact",
"$TC filter add dev $DEV1 protocol all pref 1 ingress handle 0x1234 matchall action ok"
@@ -45,6 +51,9 @@
"filter",
"flower"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV2 ingress",
"./tdc_batch.py $DEV2 $BATCH_FILE --share_action -n 1000000"
@@ -66,6 +75,9 @@
"filter",
"flower"
],
+ "plugins": {
+ "requires": "nsPlugin"
+ },
"setup": [
"$TC qdisc add dev $DEV2 ingress",
"$TC filter add dev $DEV2 protocol ip prio 1 parent ffff: flower dst_mac e4:11:22:11:4a:51 src_mac e4:11:22:11:4a:50 ip_proto tcp src_ip 1.1.1.1 dst_ip 2.2.2.2 action drop"
diff --git a/tools/testing/selftests/tc-testing/tdc.py b/tools/testing/selftests/tc-testing/tdc.py
index 5cee156..678182a 100755
--- a/tools/testing/selftests/tc-testing/tdc.py
+++ b/tools/testing/selftests/tc-testing/tdc.py
@@ -25,6 +25,9 @@ from tdc_helper import *
import TdcPlugin
from TdcResults import *
+class PluginDependencyException(Exception):
+ def __init__(self, missing_pg):
+ self.missing_pg = missing_pg
class PluginMgrTestFail(Exception):
def __init__(self, stage, output, message):
@@ -37,7 +40,7 @@ class PluginMgr:
super().__init__()
self.plugins = {}
self.plugin_instances = []
- self.args = []
+ self.failed_plugins = {}
self.argparser = argparser
# TODO, put plugins in order
@@ -53,6 +56,64 @@ class PluginMgr:
self.plugins[mn] = foo
self.plugin_instances.append(foo.SubPlugin())
+ def load_plugin(self, pgdir, pgname):
+ pgname = pgname[0:-3]
+ foo = importlib.import_module('{}.{}'.format(pgdir, pgname))
+ self.plugins[pgname] = foo
+ self.plugin_instances.append(foo.SubPlugin())
+ self.plugin_instances[-1].check_args(self.args, None)
+
+ def get_required_plugins(self, testlist):
+ '''
+ Get all required plugins from the list of test cases and return
+ all unique items.
+ '''
+ reqs = []
+ for t in testlist:
+ try:
+ if 'requires' in t['plugins']:
+ if isinstance(t['plugins']['requires'], list):
+ reqs.extend(t['plugins']['requires'])
+ else:
+ reqs.append(t['plugins']['requires'])
+ except KeyError:
+ continue
+ reqs = get_unique_item(reqs)
+ return reqs
+
+ def load_required_plugins(self, reqs, parser, args, remaining):
+ '''
+ Get all required plugins from the list of test cases and load any plugin
+ that is not already enabled.
+ '''
+ pgd = ['plugin-lib', 'plugin-lib-custom']
+ pnf = []
+
+ for r in reqs:
+ if r not in self.plugins:
+ fname = '{}.py'.format(r)
+ source_path = []
+ for d in pgd:
+ pgpath = '{}/{}'.format(d, fname)
+ if os.path.isfile(pgpath):
+ source_path.append(pgpath)
+ if len(source_path) == 0:
+ print('ERROR: unable to find required plugin {}'.format(r))
+ pnf.append(fname)
+ continue
+ elif len(source_path) > 1:
+ print('WARNING: multiple copies of plugin {} found, using version found')
+ print('at {}'.format(source_path[0]))
+ pgdir = source_path[0]
+ pgdir = pgdir.split('/')[0]
+ self.load_plugin(pgdir, fname)
+ if len(pnf) > 0:
+ raise PluginDependencyException(pnf)
+
+ parser = self.call_add_args(parser)
+ (args, remaining) = parser.parse_known_args(args=remaining, namespace=args)
+ return args
+
def call_pre_suite(self, testcount, testidlist):
for pgn_inst in self.plugin_instances:
pgn_inst.pre_suite(testcount, testidlist)
@@ -98,6 +159,9 @@ class PluginMgr:
command = pgn_inst.adjust_command(stage, command)
return command
+ def set_args(self, args):
+ self.args = args
+
@staticmethod
def _make_argparser(args):
self.argparser = argparse.ArgumentParser(
@@ -550,6 +614,7 @@ def filter_tests_by_category(args, testlist):
return answer
+
def get_test_cases(args):
"""
If a test case file is specified, retrieve tests from that file.
@@ -611,7 +676,7 @@ def get_test_cases(args):
return allcatlist, allidlist, testcases_by_cats, alltestcases
-def set_operation_mode(pm, args):
+def set_operation_mode(pm, parser, args, remaining):
"""
Load the test case data and process remaining arguments to determine
what the script should do for this run, and call the appropriate
@@ -649,6 +714,12 @@ def set_operation_mode(pm, args):
exit(0)
if len(alltests):
+ req_plugins = pm.get_required_plugins(alltests)
+ try:
+ args = pm.load_required_plugins(req_plugins, parser, args, remaining)
+ except PluginDependencyException as pde:
+ print('The following plugins were not found:')
+ print('{}'.format(pde.missing_pg))
catresults = test_runner(pm, args, alltests)
if args.format == 'none':
print('Test results output suppression requested\n')
@@ -686,11 +757,12 @@ def main():
parser = pm.call_add_args(parser)
(args, remaining) = parser.parse_known_args()
args.NAMES = NAMES
+ pm.set_args(args)
check_default_settings(args, remaining, pm)
if args.verbose > 2:
print('args is {}'.format(args))
- set_operation_mode(pm, args)
+ set_operation_mode(pm, parser, args, remaining)
exit(0)
diff --git a/tools/testing/selftests/tc-testing/tdc_helper.py b/tools/testing/selftests/tc-testing/tdc_helper.py
index 9f35c96..0440d25 100644
--- a/tools/testing/selftests/tc-testing/tdc_helper.py
+++ b/tools/testing/selftests/tc-testing/tdc_helper.py
@@ -17,7 +17,10 @@ def get_categorized_testlist(alltests, ucat):
def get_unique_item(lst):
""" For a list, return a list of the unique items in the list. """
- return list(set(lst))
+ if len(lst) > 1:
+ return list(set(lst))
+ else:
+ return lst
def get_test_categories(alltests):
--
2.7.4
^ permalink raw reply related
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Daniel Santos @ 2019-06-25 0:58 UTC (permalink / raw)
To: René van Dorst, sean.wang, f.fainelli, linux, davem,
matthias.bgg, andrew, vivien.didelot
Cc: frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190624145251.4849-2-opensource@vdorst.com>
On 6/24/19 9:52 AM, René van Dorst wrote:
> Convert mt7530 to PHYLINK API
>
> Signed-off-by: René van Dorst <opensource@vdorst.com>
> ---
> drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
> drivers/net/dsa/mt7530.h | 9 ++
> 2 files changed, 187 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3181e95586d6..9c5e4dd00826 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -13,7 +13,7 @@
> #include <linux/of_mdio.h>
> #include <linux/of_net.h>
> #include <linux/of_platform.h>
> -#include <linux/phy.h>
> +#include <linux/phylink.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> #include <linux/reset.h>
> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset)
> return ARRAY_SIZE(mt7530_mib);
> }
>
> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
> - struct phy_device *phydev)
> -{
> - struct mt7530_priv *priv = ds->priv;
> -
> - if (phy_is_pseudo_fixed_link(phydev)) {
> - dev_dbg(priv->dev, "phy-mode for master device = %x\n",
> - phydev->interface);
> -
> - /* Setup TX circuit incluing relevant PAD and driving */
> - mt7530_pad_clk_setup(ds, phydev->interface);
> -
> - if (priv->id == ID_MT7530) {
> - /* Setup RX circuit, relevant PAD and driving on the
> - * host which must be placed after the setup on the
> - * device side is all finished.
> - */
> - mt7623_pad_clk_setup(ds);
> - }
> - } else {
> - u16 lcl_adv = 0, rmt_adv = 0;
> - u8 flowctrl;
> - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
> -
> - switch (phydev->speed) {
> - case SPEED_1000:
> - mcr |= PMCR_FORCE_SPEED_1000;
> - break;
> - case SPEED_100:
> - mcr |= PMCR_FORCE_SPEED_100;
> - break;
> - }
> -
> - if (phydev->link)
> - mcr |= PMCR_FORCE_LNK;
> -
> - if (phydev->duplex) {
> - mcr |= PMCR_FORCE_FDX;
> -
> - if (phydev->pause)
> - rmt_adv = LPA_PAUSE_CAP;
> - if (phydev->asym_pause)
> - rmt_adv |= LPA_PAUSE_ASYM;
> -
> - lcl_adv = linkmode_adv_to_lcl_adv_t(
> - phydev->advertising);
> - flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
> -
> - if (flowctrl & FLOW_CTRL_TX)
> - mcr |= PMCR_TX_FC_EN;
> - if (flowctrl & FLOW_CTRL_RX)
> - mcr |= PMCR_RX_FC_EN;
> - }
> - mt7530_write(priv, MT7530_PMCR_P(port), mcr);
> - }
> -}
> -
> static int
> mt7530_cpu_port_enable(struct mt7530_priv *priv,
> int port)
> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
> return 0;
> }
>
> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + const struct phylink_link_state *state)
> +{
> + struct mt7530_priv *priv = ds->priv;
> + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
> + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
> +
> + switch (port) {
> + case 0: /* Internal phy */
> + case 1:
> + case 2:
> + case 3:
> + case 4:
> + if (state->interface != PHY_INTERFACE_MODE_GMII)
> + goto unsupported;
> + break;
> + /* case 5: Port 5 is not supported! */
> + case 6: /* 1st cpu port */
> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> + state->interface != PHY_INTERFACE_MODE_TRGMII)
> + goto unsupported;
> +
> + /* Setup TX circuit incluing relevant PAD and driving */
> + mt7530_pad_clk_setup(ds, state->interface);
> +
> + if (priv->id == ID_MT7530) {
> + /* Setup RX circuit, relevant PAD and driving on the
> + * host which must be placed after the setup on the
> + * device side is all finished.
> + */
> + mt7623_pad_clk_setup(ds);
> + }
> + break;
> + default:
> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> + return;
> + }
> +
> + if (!state->an_enabled || mode == MLO_AN_FIXED) {
> + mcr |= PMCR_FORCE_MODE;
> +
> + if (state->speed == SPEED_1000)
> + mcr |= PMCR_FORCE_SPEED_1000;
> + if (state->speed == SPEED_100)
> + mcr |= PMCR_FORCE_SPEED_100;
I would suggest using the defines
#define PMCR_FORCE_SPEED 0x0000000c /* or PMCR_FORCE_SPEED_MASK */
#define PMCR_FORCE_SPEED_10 0x00000000
#define PMCR_FORCE_SPEED_100 0x00000004
#define PMCR_FORCE_SPEED_1000 0x00000008
and a switch statement such as
switch (state->speed) {
case SPEED_10:
mcr |= PMCR_FORCE_SPEED_10;
break;
case SPEED_100:
mcr |= PMCR_FORCE_SPEED_100;
break;
case SPEED_1000:
mcr |= PMCR_FORCE_SPEED_1000;
break;
}
This will compile the same (i.e, the mcr |= 0 will optimize away, etc.),
while alleviating the need to intimately know the hardware in order to
easily understand what the code is doing at a glance. It's also better
form as we're treating the two bits as a composite value, rather than
two separate bits.
> + if (state->duplex == DUPLEX_FULL)
> + mcr |= PMCR_FORCE_FDX;
> + if (state->link || mode == MLO_AN_FIXED)
> + mcr |= PMCR_FORCE_LNK;
> + if (state->pause || phylink_test(state->advertising, Pause))
> + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
> + if (state->pause & MLO_PAUSE_TX)
> + mcr |= PMCR_TX_FC_EN;
> + if (state->pause & MLO_PAUSE_RX)
> + mcr |= PMCR_RX_FC_EN;
> + }
> +
> + mt7530_write(priv, MT7530_PMCR_P(port), mcr);
> +
> + return;
> +
> +unsupported:
> + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
> + __func__, port, state->interface, phy_modes(state->interface));
> +}
> +
> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface)
> +{
> + /* Do nothing */
> +}
> +
> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface,
> + struct phy_device *phydev)
> +{
> + /* Do nothing */
> +}
> +
> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
> + unsigned long *supported,
> + struct phylink_link_state *state)
> +{
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> + switch (port) {
> + case 0: /* Internal phy */
> + case 1:
> + case 2:
> + case 3:
> + case 4:
> + if (state->interface != PHY_INTERFACE_MODE_NA &&
> + state->interface != PHY_INTERFACE_MODE_GMII)
> + goto unsupported;
> + break;
> + /* case 5: Port 5 not supported! */
> + case 6: /* 1st cpu port */
> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> + state->interface != PHY_INTERFACE_MODE_TRGMII)
> + goto unsupported;
> + break;
> + default:
> + linkmode_zero(supported);
> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> + return;
> + }
> +
> + phylink_set(mask, Autoneg);
> + phylink_set(mask, Pause);
> + phylink_set(mask, Asym_Pause);
> + phylink_set(mask, MII);
> +
> + phylink_set(mask, 10baseT_Half);
> + phylink_set(mask, 10baseT_Full);
> + phylink_set(mask, 100baseT_Half);
> + phylink_set(mask, 100baseT_Full);
> + phylink_set(mask, 1000baseT_Full);
> + phylink_set(mask, 1000baseT_Half);
> +
> + linkmode_and(supported, supported, mask);
> + linkmode_and(state->advertising, state->advertising, mask);
> + return;
> +
> +unsupported:
> + linkmode_zero(supported);
> + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
> + __func__, state->interface, phy_modes(state->interface));
> +}
> +
> +static int
> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
> + struct phylink_link_state *state)
> +{
> + struct mt7530_priv *priv = ds->priv;
> + u32 pmsr;
> +
> + if (port < 0 || port >= MT7530_NUM_PORTS)
> + return -EINVAL;
> +
> + pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
> +
> + state->link = (pmsr & PMSR_LINK);
> + state->an_complete = state->link;
> + state->duplex = (pmsr & PMSR_DPX) >> 1;
> +
> + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
> + case 0:
> + state->speed = SPEED_10;
> + break;
> + case PMSR_SPEED_100:
> + state->speed = SPEED_100;
> + break;
> + case PMSR_SPEED_1000:
> + state->speed = SPEED_1000;
> + break;
> + default:
> + state->speed = SPEED_UNKNOWN;
> + break;
> + }
> +
Same as above: add PMSR_SPEED_10, and and with PMSR_SPEED (or
PMSR_SPEED_MASK if you prefer).
> + state->pause = 0;
> + if (pmsr & PMSR_RX_FC)
> + state->pause |= MLO_PAUSE_RX;
> + if (pmsr & PMSR_TX_FC)
> + state->pause |= MLO_PAUSE_TX;
> +
> + return 1;
> +}
> +
> static const struct dsa_switch_ops mt7530_switch_ops = {
> .get_tag_protocol = mtk_get_tag_protocol,
> .setup = mt7530_setup,
> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
> .phy_write = mt7530_phy_write,
> .get_ethtool_stats = mt7530_get_ethtool_stats,
> .get_sset_count = mt7530_get_sset_count,
> - .adjust_link = mt7530_adjust_link,
> .port_enable = mt7530_port_enable,
> .port_disable = mt7530_port_disable,
> .port_stp_state_set = mt7530_stp_state_set,
> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
> .port_vlan_prepare = mt7530_port_vlan_prepare,
> .port_vlan_add = mt7530_port_vlan_add,
> .port_vlan_del = mt7530_port_vlan_del,
> + .phylink_validate = mt7530_phylink_validate,
> + .phylink_mac_link_state = mt7530_phylink_mac_link_state,
> + .phylink_mac_config = mt7530_phylink_mac_config,
> + .phylink_mac_link_down = mt7530_phylink_mac_link_down,
> + .phylink_mac_link_up = mt7530_phylink_mac_link_up,
> };
>
> static const struct of_device_id mt7530_of_match[] = {
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index bfac90f48102..41d9a132ac70 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
> #define PMCR_FORCE_SPEED_100 BIT(2)
> #define PMCR_FORCE_FDX BIT(1)
> #define PMCR_FORCE_LNK BIT(0)
> +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE
> #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
> PMCR_TX_EN | PMCR_RX_EN | \
> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
> PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>
> #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100)
> +#define PMSR_EEE1G BIT(7)
> +#define PMSR_EEE100M BIT(6)
> +#define PMSR_RX_FC BIT(5)
> +#define PMSR_TX_FC BIT(4)
> +#define PMSR_SPEED_1000 BIT(3)
> +#define PMSR_SPEED_100 BIT(2)
> +#define PMSR_DPX BIT(1)
> +#define PMSR_LINK BIT(0)
>
> /* Register for MIB */
> #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100)
Cheers,
Daniel
^ permalink raw reply
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Jakub Kicinski @ 2019-06-25 0:57 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
Stanislav Fomichev
In-Reply-To: <20190624174726.2dda122b@cakuba.netronome.com>
On Mon, 24 Jun 2019 17:47:26 -0700, Jakub Kicinski wrote:
> I see. The local flag would not an option in getopt_long() sense, what
> I was thinking was about adding an "effective" keyword:
Something like this, untested:
--->8------------
The BPF_F_QUERY_EFFECTIVE is a syscall flag, and fits nicely
as a subcommand option. We want to move away from global
options, anyway.
We need a global variable because of nftw limitations.
Clean this flag on every invocations in case we run in
batch mode.
NOTE the argv[1] use on the error path in do_show() looks
like a bug on it's own.
---
.../bpftool/Documentation/bpftool-cgroup.rst | 24 +++----
tools/bpf/bpftool/Documentation/bpftool.rst | 6 +-
tools/bpf/bpftool/bash-completion/bpftool | 17 ++---
tools/bpf/bpftool/cgroup.c | 62 ++++++++++++-------
tools/bpf/bpftool/main.c | 7 +--
tools/bpf/bpftool/main.h | 3 +-
6 files changed, 66 insertions(+), 53 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
index 324df15bf4cc..4fde3dfad395 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
@@ -12,8 +12,7 @@ SYNOPSIS
**bpftool** [*OPTIONS*] **cgroup** *COMMAND*
- *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** }
- | { **-e** | **--effective** } }
+ *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
*COMMANDS* :=
{ **show** | **list** | **tree** | **attach** | **detach** | **help** }
@@ -21,8 +20,8 @@ SYNOPSIS
CGROUP COMMANDS
===============
-| **bpftool** **cgroup { show | list }** *CGROUP*
-| **bpftool** **cgroup tree** [*CGROUP_ROOT*]
+| **bpftool** **cgroup { show | list }** *CGROUP* [**effective**]
+| **bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**]
| **bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
| **bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
| **bpftool** **cgroup help**
@@ -35,13 +34,17 @@ CGROUP COMMANDS
DESCRIPTION
===========
- **bpftool cgroup { show | list }** *CGROUP*
+ **bpftool cgroup { show | list }** *CGROUP* [**effective**]
List all programs attached to the cgroup *CGROUP*.
Output will start with program ID followed by attach type,
attach flags and program name.
- **bpftool cgroup tree** [*CGROUP_ROOT*]
+ If **effective** is specified retrieve effective programs that
+ will execute for events within a cgroup. This includes
+ inherited along with attached ones.
+
+ **bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**]
Iterate over all cgroups in *CGROUP_ROOT* and list all
attached programs. If *CGROUP_ROOT* is not specified,
bpftool uses cgroup v2 mountpoint.
@@ -50,6 +53,10 @@ DESCRIPTION
commands: it starts with absolute cgroup path, followed by
program ID, attach type, attach flags and program name.
+ If **effective** is specified retrieve effective programs that
+ will execute for events within a cgroup. This includes
+ inherited along with attached ones.
+
**bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
Attach program *PROG* to the cgroup *CGROUP* with attach type
*ATTACH_TYPE* and optional *ATTACH_FLAGS*.
@@ -122,11 +129,6 @@ OPTIONS
Print all logs available from libbpf, including debug-level
information.
- -e, --effective
- Retrieve effective programs that will execute for events
- within a cgroup. This includes inherited along with attached
- ones.
-
EXAMPLES
========
|
diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
index d2f76b55988d..6a9c52ef84a9 100644
--- a/tools/bpf/bpftool/Documentation/bpftool.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool.rst
@@ -19,7 +19,7 @@ SYNOPSIS
*OBJECT* := { **map** | **program** | **cgroup** | **perf** | **net** | **feature** }
*OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** }
- | { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-e** | **--effective** } }
+ | { **-j** | **--json** } [{ **-p** | **--pretty** }] }
*MAP-COMMANDS* :=
{ **show** | **list** | **create** | **dump** | **update** | **lookup** | **getnext**
@@ -71,10 +71,6 @@ OPTIONS
includes logs from libbpf as well as from the verifier, when
attempting to load programs.
- -e, --effective
- Retrieve effective programs that will execute for events
- within a cgroup. This includes inherited along with attached ones.
-
SEE ALSO
========
**bpf**\ (2),
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index c98cb99867f6..de84ae06ae4e 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -187,7 +187,7 @@ _bpftool()
# Deal with options
if [[ ${words[cword]} == -* ]]; then
- local c='--version --json --pretty --bpffs --mapcompat --debug --effective'
+ local c='--version --json --pretty --bpffs --mapcompat --debug'
COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
return 0
fi
@@ -678,12 +678,15 @@ _bpftool()
;;
cgroup)
case $command in
- show|list)
- _filedir
- return 0
- ;;
- tree)
- _filedir
+ show|list|tree)
+ case $cword in
+ 3)
+ _filedir
+ ;;
+ 4)
+ COMPREPLY=( $( compgen -W 'effective' -- "$cur" ) )
+ ;;
+ esac
return 0
;;
attach|detach)
diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
index 1bb2a751107a..88b80616d47b 100644
--- a/tools/bpf/bpftool/cgroup.c
+++ b/tools/bpf/bpftool/cgroup.c
@@ -28,6 +28,8 @@
" connect6 | sendmsg4 | sendmsg6 |\n" \
" recvmsg4 | recvmsg6 | sysctl }"
+static unsigned int query_flags;
+
static const char * const attach_type_strings[] = {
[BPF_CGROUP_INET_INGRESS] = "ingress",
[BPF_CGROUP_INET_EGRESS] = "egress",
@@ -104,8 +106,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
__u32 prog_cnt = 0;
int ret;
- ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL,
- &prog_cnt);
+ ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL,
+ NULL, &prog_cnt);
if (ret)
return -1;
@@ -156,20 +158,30 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
static int do_show(int argc, char **argv)
{
enum bpf_attach_type type;
+ const char *path;
int cgroup_fd;
int ret = -1;
- if (argc < 1) {
- p_err("too few parameters for cgroup show");
- goto exit;
- } else if (argc > 1) {
- p_err("too many parameters for cgroup show");
- goto exit;
+ query_flags = 0;
+
+ if (!REQ_ARGS(1))
+ return -1;
+ path = GET_ARG();
+
+ while (argc) {
+ if (is_prefix(*argv, "effective")) {
+ query_flags |= BPF_F_QUERY_EFFECTIVE;
+ NEXT_ARG();
+ } else {
+ p_err("expected no more arguments, 'effective', got: '%s'?",
+ *argv);
+ return -1;
+ }
}
- cgroup_fd = open(argv[0], O_RDONLY);
+ cgroup_fd = open(path, O_RDONLY);
if (cgroup_fd < 0) {
- p_err("can't open cgroup %s", argv[1]);
+ p_err("can't open cgroup %s", path);
goto exit;
}
@@ -295,23 +307,29 @@ static int do_show_tree(int argc, char **argv)
char *cgroup_root;
int ret;
- switch (argc) {
- case 0:
+ query_flags = 0;
+
+ if (!argc) {
cgroup_root = find_cgroup_root();
if (!cgroup_root) {
p_err("cgroup v2 isn't mounted");
return -1;
}
- break;
- case 1:
- cgroup_root = argv[0];
- break;
- default:
- p_err("too many parameters for cgroup tree");
- return -1;
+ } else {
+ cgroup_root = GET_ARG();
+
+ while (argc) {
+ if (is_prefix(*argv, "effective")) {
+ query_flags |= BPF_F_QUERY_EFFECTIVE;
+ NEXT_ARG();
+ } else {
+ p_err("expected no more arguments, 'effective', got: '%s'?",
+ *argv);
+ return -1;
+ }
+ }
}
-
if (json_output)
jsonw_start_array(json_wtr);
else
@@ -457,8 +475,8 @@ static int do_help(int argc, char **argv)
}
fprintf(stderr,
- "Usage: %s %s { show | list } CGROUP\n"
- " %s %s tree [CGROUP_ROOT]\n"
+ "Usage: %s %s { show | list } CGROUP [**effective**]\n"
+ " %s %s tree [CGROUP_ROOT] [**effective**]\n"
" %s %s attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]\n"
" %s %s detach CGROUP ATTACH_TYPE PROG\n"
" %s %s help\n"
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 42e9ddfbbbe0..4879f6395c7e 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -27,7 +27,6 @@ bool json_output;
bool show_pinned;
bool block_mount;
bool verifier_logs;
-unsigned int query_flags;
int bpf_flags;
struct pinned_obj_table prog_table;
struct pinned_obj_table map_table;
@@ -328,7 +327,6 @@ int main(int argc, char **argv)
{ "mapcompat", no_argument, NULL, 'm' },
{ "nomount", no_argument, NULL, 'n' },
{ "debug", no_argument, NULL, 'd' },
- { "effective", no_argument, NULL, 'e' },
{ 0 }
};
int opt, ret;
@@ -344,7 +342,7 @@ int main(int argc, char **argv)
hash_init(map_table.table);
opterr = 0;
- while ((opt = getopt_long(argc, argv, "Vhpjfmnde",
+ while ((opt = getopt_long(argc, argv, "Vhpjfmnd",
options, NULL)) >= 0) {
switch (opt) {
case 'V':
@@ -378,9 +376,6 @@ int main(int argc, char **argv)
libbpf_set_print(print_all_levels);
verifier_logs = true;
break;
- case 'e':
- query_flags = BPF_F_QUERY_EFFECTIVE;
- break;
default:
p_err("unrecognized option '%s'", argv[optind - 1]);
if (json_output)
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index fddec15c454a..28a2a5857e14 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -45,7 +45,7 @@
"PROG := { id PROG_ID | pinned FILE | tag PROG_TAG }"
#define HELP_SPEC_OPTIONS \
"OPTIONS := { {-j|--json} [{-p|--pretty}] | {-f|--bpffs} |\n" \
- "\t {-m|--mapcompat} | {-n|--nomount} | {-e|--effective} }"
+ "\t {-m|--mapcompat} | {-n|--nomount} }"
#define HELP_SPEC_MAP \
"MAP := { id MAP_ID | pinned FILE }"
@@ -92,7 +92,6 @@ extern bool json_output;
extern bool show_pinned;
extern bool block_mount;
extern bool verifier_logs;
-extern unsigned int query_flags;
extern int bpf_flags;
extern struct pinned_obj_table prog_table;
extern struct pinned_obj_table map_table;
--
2.21.0
^ permalink raw reply related
* [PATCH v2] samples: bpf: make the use of xdp samples consistent
From: Daniel T. Lee @ 2019-06-25 0:55 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov; +Cc: bpf, netdev
Currently, each xdp samples are inconsistent in the use.
Most of the samples fetch the interface with it's name.
(ex. xdp1, xdp2skb, xdp_redirect_cpu, xdp_sample_pkts, etc.)
But some of the xdp samples are fetching the interface with
ifindex by command argument.
This commit enables xdp samples to fetch interface with it's name
without changing the original index interface fetching.
(<ifname|ifindex> fetching in the same way as xdp_sample_pkts_user.c does.)
Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
Changes in v2:
- added xdp_redirect_user.c, xdp_redirect_map_user.c
samples/bpf/xdp_adjust_tail_user.c | 12 ++++++++++--
samples/bpf/xdp_redirect_map_user.c | 15 +++++++++++----
samples/bpf/xdp_redirect_user.c | 15 +++++++++++----
samples/bpf/xdp_tx_iptunnel_user.c | 12 ++++++++++--
4 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/samples/bpf/xdp_adjust_tail_user.c b/samples/bpf/xdp_adjust_tail_user.c
index 586ff751aba9..a3596b617c4c 100644
--- a/samples/bpf/xdp_adjust_tail_user.c
+++ b/samples/bpf/xdp_adjust_tail_user.c
@@ -13,6 +13,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <net/if.h>
#include <sys/resource.h>
#include <arpa/inet.h>
#include <netinet/ether.h>
@@ -69,7 +70,7 @@ static void usage(const char *cmd)
printf("Start a XDP prog which send ICMP \"packet too big\" \n"
"messages if ingress packet is bigger then MAX_SIZE bytes\n");
printf("Usage: %s [...]\n", cmd);
- printf(" -i <ifindex> Interface Index\n");
+ printf(" -i <ifname|ifindex> Interface\n");
printf(" -T <stop-after-X-seconds> Default: 0 (forever)\n");
printf(" -S use skb-mode\n");
printf(" -N enforce native mode\n");
@@ -102,7 +103,9 @@ int main(int argc, char **argv)
switch (opt) {
case 'i':
- ifindex = atoi(optarg);
+ ifindex = if_nametoindex(optarg);
+ if (!ifindex)
+ ifindex = atoi(optarg);
break;
case 'T':
kill_after_s = atoi(optarg);
@@ -136,6 +139,11 @@ int main(int argc, char **argv)
return 1;
}
+ if (!ifindex) {
+ fprintf(stderr, "Invalid ifname\n");
+ return 1;
+ }
+
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
prog_load_attr.file = filename;
diff --git a/samples/bpf/xdp_redirect_map_user.c b/samples/bpf/xdp_redirect_map_user.c
index 15bb6f67f9c3..f70ee33907fd 100644
--- a/samples/bpf/xdp_redirect_map_user.c
+++ b/samples/bpf/xdp_redirect_map_user.c
@@ -10,6 +10,7 @@
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
+#include <net/if.h>
#include <unistd.h>
#include <libgen.h>
#include <sys/resource.h>
@@ -85,7 +86,7 @@ static void poll_stats(int interval, int ifindex)
static void usage(const char *prog)
{
fprintf(stderr,
- "usage: %s [OPTS] IFINDEX_IN IFINDEX_OUT\n\n"
+ "usage: %s [OPTS] <IFNAME|IFINDEX>_IN <IFNAME|IFINDEX>_OUT\n\n"
"OPTS:\n"
" -S use skb-mode\n"
" -N enforce native mode\n"
@@ -127,7 +128,7 @@ int main(int argc, char **argv)
}
if (optind == argc) {
- printf("usage: %s IFINDEX_IN IFINDEX_OUT\n", argv[0]);
+ printf("usage: %s <IFNAME|IFINDEX>_IN <IFNAME|IFINDEX>_OUT\n", argv[0]);
return 1;
}
@@ -136,8 +137,14 @@ int main(int argc, char **argv)
return 1;
}
- ifindex_in = strtoul(argv[optind], NULL, 0);
- ifindex_out = strtoul(argv[optind + 1], NULL, 0);
+ ifindex_in = if_nametoindex(argv[optind]);
+ if (!ifindex_in)
+ ifindex_in = strtoul(argv[optind], NULL, 0);
+
+ ifindex_out = if_nametoindex(argv[optind + 1]);
+ if (!ifindex_out)
+ ifindex_out = strtoul(argv[optind + 1], NULL, 0);
+
printf("input: %d output: %d\n", ifindex_in, ifindex_out);
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
diff --git a/samples/bpf/xdp_redirect_user.c b/samples/bpf/xdp_redirect_user.c
index ce71be187205..39de06f3ec25 100644
--- a/samples/bpf/xdp_redirect_user.c
+++ b/samples/bpf/xdp_redirect_user.c
@@ -10,6 +10,7 @@
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
+#include <net/if.h>
#include <unistd.h>
#include <libgen.h>
#include <sys/resource.h>
@@ -85,7 +86,7 @@ static void poll_stats(int interval, int ifindex)
static void usage(const char *prog)
{
fprintf(stderr,
- "usage: %s [OPTS] IFINDEX_IN IFINDEX_OUT\n\n"
+ "usage: %s [OPTS] <IFNAME|IFINDEX>_IN <IFNAME|IFINDEX>_OUT\n\n"
"OPTS:\n"
" -S use skb-mode\n"
" -N enforce native mode\n"
@@ -128,7 +129,7 @@ int main(int argc, char **argv)
}
if (optind == argc) {
- printf("usage: %s IFINDEX_IN IFINDEX_OUT\n", argv[0]);
+ printf("usage: %s <IFNAME|IFINDEX>_IN <IFNAME|IFINDEX>_OUT\n", argv[0]);
return 1;
}
@@ -137,8 +138,14 @@ int main(int argc, char **argv)
return 1;
}
- ifindex_in = strtoul(argv[optind], NULL, 0);
- ifindex_out = strtoul(argv[optind + 1], NULL, 0);
+ ifindex_in = if_nametoindex(argv[optind]);
+ if (!ifindex_in)
+ ifindex_in = strtoul(argv[optind], NULL, 0);
+
+ ifindex_out = if_nametoindex(argv[optind + 1]);
+ if (!ifindex_out)
+ ifindex_out = strtoul(argv[optind + 1], NULL, 0);
+
printf("input: %d output: %d\n", ifindex_in, ifindex_out);
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
diff --git a/samples/bpf/xdp_tx_iptunnel_user.c b/samples/bpf/xdp_tx_iptunnel_user.c
index 394896430712..dfb68582e243 100644
--- a/samples/bpf/xdp_tx_iptunnel_user.c
+++ b/samples/bpf/xdp_tx_iptunnel_user.c
@@ -9,6 +9,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <net/if.h>
#include <sys/resource.h>
#include <arpa/inet.h>
#include <netinet/ether.h>
@@ -83,7 +84,7 @@ static void usage(const char *cmd)
"in an IPv4/v6 header and XDP_TX it out. The dst <VIP:PORT>\n"
"is used to select packets to encapsulate\n\n");
printf("Usage: %s [...]\n", cmd);
- printf(" -i <ifindex> Interface Index\n");
+ printf(" -i <ifname|ifindex> Interface\n");
printf(" -a <vip-service-address> IPv4 or IPv6\n");
printf(" -p <vip-service-port> A port range (e.g. 433-444) is also allowed\n");
printf(" -s <source-ip> Used in the IPTunnel header\n");
@@ -181,7 +182,9 @@ int main(int argc, char **argv)
switch (opt) {
case 'i':
- ifindex = atoi(optarg);
+ ifindex = if_nametoindex(optarg);
+ if (!ifindex)
+ ifindex = atoi(optarg);
break;
case 'a':
vip.family = parse_ipstr(optarg, vip.daddr.v6);
@@ -253,6 +256,11 @@ int main(int argc, char **argv)
return 1;
}
+ if (!ifindex) {
+ fprintf(stderr, "Invalid ifname\n");
+ return 1;
+ }
+
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
prog_load_attr.file = filename;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Jakub Kicinski @ 2019-06-25 0:47 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
Stanislav Fomichev
In-Reply-To: <01c2c76b-5a45-aab0-e698-b5a66ab6c2e7@fb.com>
On Tue, 25 Jun 2019 00:40:09 +0000, Alexei Starovoitov wrote:
> On 6/24/19 5:30 PM, Jakub Kicinski wrote:
> > On Tue, 25 Jun 2019 00:21:57 +0000, Alexei Starovoitov wrote:
> >> On 6/24/19 5:16 PM, Jakub Kicinski wrote:
> >>> On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote:
> >>>> I don't think this patch should be penalized.
> >>>> I'd rather see we fix them all.
> >>>
> >>> So we are going to add this broken option just to remove it?
> >>> I don't understand.
> >>> I'm happy to spend the 15 minutes rewriting this if you don't
> >>> want to penalize Takshak.
> >>
> >> hmm. I don't understand the 'broken' part.
> >> The only issue I see that it could have been local vs global,
> >> but they all should have been local.
> >
> > I don't think all of them. Only --mapcompat and --bpffs. bpffs could
> > be argued. On mapcompat I must have not read the patch fully, I was
> > under the impression its a global libbpf flag :(
> >
> > --json, --pretty, --nomount, --debug are global because they affect
> > global behaviour of bpftool. The difference here is that we basically
> > add a syscall parameter as a global option.
>
> sure. I only disagreed about not touching older flags.
> --effective should be local.
> If follow up patch means 90% rewrite then revert is better.
> If it's 10% fixup then it's different story.
I see. The local flag would not an option in getopt_long() sense, what
I was thinking was about adding an "effective" keyword:
# bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/
becomes:
# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/ effective
That's how we handle flags for update calls for instance..
So I think a revert :(
> Takshak,
> could you check which way is cleaner? Revert and new patch or follow up fix?
> But bpftool doesn't have a way to do local, no?
> so it's kinda new feature and other flags should become local too.
> hence it feels more like follow up. Just my .02
^ permalink raw reply
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Alexei Starovoitov @ 2019-06-25 0:40 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
Stanislav Fomichev
In-Reply-To: <20190624173005.06430163@cakuba.netronome.com>
On 6/24/19 5:30 PM, Jakub Kicinski wrote:
> On Tue, 25 Jun 2019 00:21:57 +0000, Alexei Starovoitov wrote:
>> On 6/24/19 5:16 PM, Jakub Kicinski wrote:
>>> On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote:
>>>> I don't think this patch should be penalized.
>>>> I'd rather see we fix them all.
>>>
>>> So we are going to add this broken option just to remove it?
>>> I don't understand.
>>> I'm happy to spend the 15 minutes rewriting this if you don't
>>> want to penalize Takshak.
>>
>> hmm. I don't understand the 'broken' part.
>> The only issue I see that it could have been local vs global,
>> but they all should have been local.
>
> I don't think all of them. Only --mapcompat and --bpffs. bpffs could
> be argued. On mapcompat I must have not read the patch fully, I was
> under the impression its a global libbpf flag :(
>
> --json, --pretty, --nomount, --debug are global because they affect
> global behaviour of bpftool. The difference here is that we basically
> add a syscall parameter as a global option.
sure. I only disagreed about not touching older flags.
--effective should be local.
If follow up patch means 90% rewrite then revert is better.
If it's 10% fixup then it's different story.
Takshak,
could you check which way is cleaner? Revert and new patch or follow up fix?
But bpftool doesn't have a way to do local, no?
so it's kinda new feature and other flags should become local too.
hence it feels more like follow up. Just my .02
^ permalink raw reply
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Jakub Kicinski @ 2019-06-25 0:30 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande,
netdev@vger.kernel.org, ast@kernel.org, Kernel Team,
Stanislav Fomichev
In-Reply-To: <6d44d265-7133-d191-beeb-c22dde73993f@fb.com>
On Tue, 25 Jun 2019 00:21:57 +0000, Alexei Starovoitov wrote:
> On 6/24/19 5:16 PM, Jakub Kicinski wrote:
> > On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote:
> >> I don't think this patch should be penalized.
> >> I'd rather see we fix them all.
> >
> > So we are going to add this broken option just to remove it?
> > I don't understand.
> > I'm happy to spend the 15 minutes rewriting this if you don't
> > want to penalize Takshak.
>
> hmm. I don't understand the 'broken' part.
> The only issue I see that it could have been local vs global,
> but they all should have been local.
I don't think all of them. Only --mapcompat and --bpffs. bpffs could
be argued. On mapcompat I must have not read the patch fully, I was
under the impression its a global libbpf flag :(
--json, --pretty, --nomount, --debug are global because they affect
global behaviour of bpftool. The difference here is that we basically
add a syscall parameter as a global option.
^ 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