[PATCH 0/4] Hexdump enhancements

[PATCH 0/4] Hexdump enhancements

[Alastair D'Silva]

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.

Alastair D'Silva (4):
  lib/hexdump.c: Allow 64 bytes per line
  lib/hexdump.c: Optionally suppress lines of filler 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 '|'

 drivers/gpu/drm/i915/intel_engine_cs.c        |   2 +-
 drivers/isdn/hardware/mISDN/mISDNisar.c       |   6 +-
 drivers/mailbox/mailbox-test.c                |   2 +-
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c      |   2 +-
 .../net/ethernet/synopsys/dwc-xlgmac-common.c |   2 +-
 drivers/net/wireless/ath/ath10k/debug.c       |   3 +-
 .../net/wireless/intel/iwlegacy/3945-mac.c    |   2 +-
 drivers/platform/chrome/wilco_ec/debugfs.c    |   3 +-
 drivers/scsi/scsi_logging.c                   |   8 +-
 drivers/staging/fbtft/fbtft-core.c            |   2 +-
 fs/seq_file.c                                 |   3 +-
 include/linux/printk.h                        |  38 ++++-
 lib/hexdump.c                                 | 143 ++++++++++++++----
 lib/test_hexdump.c                            |   5 +-
 14 files changed, 168 insertions(+), 53 deletions(-)

-- 
2.20.1

[PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line

[Alastair D'Silva]

From: Alastair D'Silva <alastair@d-silva.org>

With modern high resolution screens, we can display more data, which makes
life a bit easier when debugging.

Allow 64 bytes to be dumped per line.

Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
 lib/hexdump.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/hexdump.c b/lib/hexdump.c
index 81b70ed37209..b8a164814744 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -80,14 +80,14 @@ 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 16, 32 or 64
  * @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.
+ * 16, 32 or 64 bytes of input data converted to hex + ASCII output.
  *
  * 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,7 +116,7 @@ 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)
+	if (rowsize != 16 && rowsize != 32 && rowsize != 64)
 		rowsize = 16;
 
 	if (len > rowsize)		/* limit to one line at a time */
@@ -216,7 +216,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 16, 32 or 64
  * @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
@@ -227,7 +227,7 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
  * 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.
+ * 16, 32 or 64 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.
  *
@@ -246,9 +246,9 @@ 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[64 * 3 + 2 + 64 + 1];
 
-	if (rowsize != 16 && rowsize != 32)
+	if (rowsize != 16 && rowsize != 32 && rowsize != 64)
 		rowsize = 16;
 
 	for (i = 0; i < len; i += rowsize) {
-- 
2.20.1

[PATCH 2/4] lib/hexdump.c: Optionally suppress lines of filler bytes

[Alastair D'Silva]

From: Alastair D'Silva <alastair@d-silva.org>

Some buffers may only be partially filled with useful data, while the rest
is padded (typically with 0x00 or 0xff).

This patch introduces flags which allow lines of padding bytes to be
suppressed, making the output easier to interpret: HEXDUMP_SUPPRESS_0X00,
HEXDUMP_SUPPRESS_0XFF

The first and last lines are not suppressed by default, so the function
always outputs something. This behaviour can be further controlled with
the HEXDUMP_SUPPRESS_FIRST & HEXDUMP_SUPPRESS_LAST flags.

An inline wrapper function is provided for backwards compatibility with
existing code, which maintains the original behaviour.

Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
 include/linux/printk.h | 38 ++++++++++++++++++----
 lib/hexdump.c          | 72 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 89 insertions(+), 21 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index d7c77ed1a4cb..c014e5573665 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -479,13 +479,26 @@ enum {
 	DUMP_PREFIX_ADDRESS,
 	DUMP_PREFIX_OFFSET
 };
+
+#define HEXDUMP_ASCII		(1 << 0)
+#define HEXDUMP_SUPPRESS_0X00	(1 << 1)
+#define HEXDUMP_SUPPRESS_0XFF	(1 << 2)
+#define HEXDUMP_SUPPRESS_FIRST	(1 << 3)
+#define HEXDUMP_SUPPRESS_LAST	(1 << 4)
+
+#define HEXDUMP_QUIET		(HEXDUMP_SUPPRESS_0X00 | \
+				HEXDUMP_SUPPRESS_0XFF | \
+				HEXDUMP_SUPPRESS_FIRST | \
+				HEXDUMP_SUPPRESS_LAST)
+
 extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
 			      int groupsize, char *linebuf, size_t linebuflen,
 			      bool ascii);
+
 #ifdef CONFIG_PRINTK
-extern void print_hex_dump(const char *level, const char *prefix_str,
-			   int prefix_type, int rowsize, int groupsize,
-			   const void *buf, size_t len, bool ascii);
+extern void print_hex_dump_ext(const char *level, const char *prefix_str,
+			       int prefix_type, int rowsize, int groupsize,
+			       const void *buf, size_t len, u64 flags);
 #if defined(CONFIG_DYNAMIC_DEBUG)
 #define print_hex_dump_bytes(prefix_str, prefix_type, buf, len)	\
 	dynamic_hex_dump(prefix_str, prefix_type, 16, 1, buf, len, true)
@@ -494,18 +507,29 @@ extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
 				 const void *buf, size_t len);
 #endif /* defined(CONFIG_DYNAMIC_DEBUG) */
 #else
-static inline void print_hex_dump(const char *level, const char *prefix_str,
-				  int prefix_type, int rowsize, int groupsize,
-				  const void *buf, size_t len, bool ascii)
+static inline void print_hex_dump_ext(const char *level, const char *prefix_str,
+				      int prefix_type, int rowsize,
+				      int groupsize, const void *buf,
+				      size_t len, u64 flags)
 {
 }
 static inline void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
 					const void *buf, size_t len)
 {
 }
-
 #endif
 
+static __always_inline void print_hex_dump(const char *level,
+					   const char *prefix_str,
+					   int prefix_type, int rowsize,
+					   int groupsize, const void *buf,
+					   size_t len, bool ascii)
+{
+	print_hex_dump_ext(level, prefix_str, prefix_type, rowsize, groupsize,
+			buf, len, ascii ? HEXDUMP_ASCII : 0);
+}
+
+
 #if defined(CONFIG_DYNAMIC_DEBUG)
 #define print_hex_dump_debug(prefix_str, prefix_type, rowsize,	\
 			     groupsize, buf, len, ascii)	\
diff --git a/lib/hexdump.c b/lib/hexdump.c
index b8a164814744..2f3bafb55a44 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -209,8 +209,21 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 EXPORT_SYMBOL(hex_dump_to_buffer);
 
 #ifdef CONFIG_PRINTK
+
+static bool buf_is_all(const u8 *buf, size_t len, u8 val)
+{
+	size_t i;
+
+	for (i = 0; i < len; i++) {
+		if (buf[i] != val)
+			return false;
+	}
+
+	return true;
+}
+
 /**
- * print_hex_dump - print a text hex dump to syslog for a binary blob of data
+ * print_hex_dump_ext: dump a binary blob of data to syslog in hexadecimal
  * @level: kernel log level (e.g. KERN_DEBUG)
  * @prefix_str: string to prefix each line with;
  *  caller supplies trailing spaces for alignment if desired
@@ -221,42 +234,73 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
  * @buf: data blob to dump
  * @len: number of bytes in the @buf
  * @ascii: include ASCII after the hex output
+ * @flags: A bitwise OR of the following flags:
+ *	HEXDUMP_ASCII:		include ASCII after the hex output
+ *	HEXDUMP_SUPPRESS_0X00:	suppress lines that are all 0x00
+ *				(other than first or last)
+ *	HEXDUMP_SUPPRESS_0XFF:	suppress lines that are all 0xff
+ *				(other than first or last)
+ *	HEXDUMP_SUPPRESS_FIRST:	allows the first line to be suppressed
+ *	HEXDUMP_SUPPRESS_LAST:	allows the last line to be suppressed
+ *				If the first and last line may be suppressed,
+ *				an empty buffer will not produce any output
  *
  * 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
  * leading prefix.
  *
- * print_hex_dump() works on one "line" of output at a time, i.e.,
+ * print_hex_dump_ext() works on one "line" of output at a time, i.e.,
  * 16, 32 or 64 bytes of input data converted to hex + ASCII output.
- * print_hex_dump() iterates over the entire input @buf, breaking it into
+ * print_hex_dump_ext() iterates over the entire input @buf, breaking it into
  * "line size" chunks to format and print.
  *
  * E.g.:
- *   print_hex_dump(KERN_DEBUG, "raw data: ", DUMP_PREFIX_ADDRESS,
- *		    16, 1, frame->data, frame->len, true);
+ *   print_hex_dump_ext(KERN_DEBUG, "raw data: ", DUMP_PREFIX_ADDRESS,
+ *		    16, 1, frame->data, frame->len, HEXDUMP_ASCII);
  *
  * Example output using %DUMP_PREFIX_OFFSET and 1-byte mode:
  * 0009ab42: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f  @ABCDEFGHIJKLMNO
  * Example output using %DUMP_PREFIX_ADDRESS and 4-byte mode:
  * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c  pqrstuvwxyz{|}~.
  */
-void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
-		    int rowsize, int groupsize,
-		    const void *buf, size_t len, bool ascii)
+void print_hex_dump_ext(const char *level, const char *prefix_str,
+			int prefix_type, int rowsize, int groupsize,
+			const void *buf, size_t len, u64 flags)
 {
 	const u8 *ptr = buf;
-	int i, linelen, remaining = len;
+	int i, remaining = len;
 	unsigned char linebuf[64 * 3 + 2 + 64 + 1];
+	bool first_line = true;
 
 	if (rowsize != 16 && rowsize != 32 && rowsize != 64)
 		rowsize = 16;
 
 	for (i = 0; i < len; i += rowsize) {
-		linelen = min(remaining, rowsize);
+		bool skip = false;
+		int linelen = min(remaining, rowsize);
+
 		remaining -= rowsize;
 
+		if (flags & HEXDUMP_SUPPRESS_0X00)
+			skip = buf_is_all(ptr + i, linelen, 0x00);
+
+		if (!skip && (flags & HEXDUMP_SUPPRESS_0XFF))
+			skip = buf_is_all(ptr + i, linelen, 0xff);
+
+		if (first_line && !(flags & HEXDUMP_SUPPRESS_FIRST))
+			skip = false;
+
+		if (remaining <= 0 && !(flags & HEXDUMP_SUPPRESS_LAST))
+			skip = false;
+
+		if (skip)
+			continue;
+
+		first_line = false;
+
 		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
-				   linebuf, sizeof(linebuf), ascii);
+				   linebuf, sizeof(linebuf),
+				   flags & HEXDUMP_ASCII);
 
 		switch (prefix_type) {
 		case DUMP_PREFIX_ADDRESS:
@@ -272,7 +316,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 		}
 	}
 }
-EXPORT_SYMBOL(print_hex_dump);
+EXPORT_SYMBOL(print_hex_dump_ext);
 
 #if !defined(CONFIG_DYNAMIC_DEBUG)
 /**
@@ -290,8 +334,8 @@ EXPORT_SYMBOL(print_hex_dump);
 void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
 			  const void *buf, size_t len)
 {
-	print_hex_dump(KERN_DEBUG, prefix_str, prefix_type, 16, 1,
-		       buf, len, true);
+	print_hex_dump_ext(KERN_DEBUG, prefix_str, prefix_type, 16, 1,
+		       buf, len, HEXDUMP_ASCII);
 }
 EXPORT_SYMBOL(print_hex_dump_bytes);
 #endif /* !defined(CONFIG_DYNAMIC_DEBUG) */
-- 
2.20.1

[PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags

[Alastair D'Silva]

From: Alastair D'Silva <alastair@d-silva.org>

In order to support additional features in hex_dump_to_buffer, replace
the ascii bool parameter with flags.

Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
 drivers/gpu/drm/i915/intel_engine_cs.c            |  2 +-
 drivers/isdn/hardware/mISDN/mISDNisar.c           |  6 ++++--
 drivers/mailbox/mailbox-test.c                    |  2 +-
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c          |  2 +-
 drivers/net/ethernet/synopsys/dwc-xlgmac-common.c |  2 +-
 drivers/net/wireless/ath/ath10k/debug.c           |  3 ++-
 drivers/net/wireless/intel/iwlegacy/3945-mac.c    |  2 +-
 drivers/platform/chrome/wilco_ec/debugfs.c        |  3 ++-
 drivers/scsi/scsi_logging.c                       |  8 +++-----
 drivers/staging/fbtft/fbtft-core.c                |  2 +-
 fs/seq_file.c                                     |  3 ++-
 include/linux/printk.h                            |  2 +-
 lib/hexdump.c                                     | 15 ++++++++-------
 lib/test_hexdump.c                                |  5 +++--
 14 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 49fa43ff02ba..fb133e729f9a 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1318,7 +1318,7 @@ 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));
+						0) >= 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 386731ec2489..f13f34db6c17 100644
--- a/drivers/isdn/hardware/mISDN/mISDNisar.c
+++ b/drivers/isdn/hardware/mISDN/mISDNisar.c
@@ -84,7 +84,8 @@ send_mbox(struct isar_hw *isar, u8 his, u8 creg, u8 len, u8 *msg)
 
 			while (l < (int)len) {
 				hex_dump_to_buffer(msg + l, len - l, 32, 1,
-						   isar->log, 256, 1);
+						   isar->log, 256,
+						   HEXDUMP_ASCII);
 				pr_debug("%s: %s %02x: %s\n", isar->name,
 					 __func__, l, isar->log);
 				l += 32;
@@ -113,7 +114,8 @@ rcv_mbox(struct isar_hw *isar, u8 *msg)
 
 			while (l < (int)isar->clsb) {
 				hex_dump_to_buffer(msg + l, isar->clsb - l, 32,
-						   1, isar->log, 256, 1);
+						   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 4e4ac4be6423..2f9a094d0259 100644
--- a/drivers/mailbox/mailbox-test.c
+++ b/drivers/mailbox/mailbox-test.c
@@ -213,7 +213,7 @@ static ssize_t mbox_test_message_read(struct file *filp, char __user *userbuf,
 		hex_dump_to_buffer(ptr,
 				   MBOX_BYTES_PER_LINE,
 				   MBOX_BYTES_PER_LINE, 1, touser + l,
-				   MBOX_HEXDUMP_LINE_LEN, true);
+				   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 0cc911f928b1..e954a31cee0c 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), 0);
 		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..b80adfa1f890 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), 0);
 		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..4c99ea03226d 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -2662,7 +2662,8 @@ void ath10k_dbg_dump(struct ath10k *ar,
 						(unsigned int)(ptr - buf));
 			hex_dump_to_buffer(ptr, len - (ptr - buf), 16, 1,
 					   linebuf + linebuflen,
-					   sizeof(linebuf) - linebuflen, true);
+					   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 271977f7fbb0..acbe26d22c34 100644
--- a/drivers/net/wireless/intel/iwlegacy/3945-mac.c
+++ b/drivers/net/wireless/intel/iwlegacy/3945-mac.c
@@ -3247,7 +3247,7 @@ il3945_show_measurement(struct device *d, struct device_attribute *attr,
 
 	while (size && PAGE_SIZE - len) {
 		hex_dump_to_buffer(data + ofs, size, 16, 1, buf + len,
-				   PAGE_SIZE - len, true);
+				   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 c090db2cd5be..238ede4a26d8 100644
--- a/drivers/platform/chrome/wilco_ec/debugfs.c
+++ b/drivers/platform/chrome/wilco_ec/debugfs.c
@@ -174,7 +174,8 @@ static ssize_t raw_read(struct file *file, char __user *user_buf, size_t count,
 		fmt_len = hex_dump_to_buffer(debug_info->raw_data,
 					     debug_info->response_size,
 					     16, 1, debug_info->formatted_data,
-					     FORMATTED_BUFFER_SIZE, true);
+					     FORMATTED_BUFFER_SIZE,
+					     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 bd70339c1242..fce542bb40e6 100644
--- a/drivers/scsi/scsi_logging.c
+++ b/drivers/scsi/scsi_logging.c
@@ -263,7 +263,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, 0);
 			}
 			dev_printk(KERN_INFO, &cmd->device->sdev_gendev, "%s",
 				   logbuf);
@@ -274,8 +274,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, 0);
 	}
 out_printk:
 	dev_printk(KERN_INFO, &cmd->device->sdev_gendev, "%s", logbuf);
@@ -354,8 +353,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, 0);
 		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..2e5df5cc9d61 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, 0);
 
 	if (len > 32)
 		dev_info(dev, "%s ...\n", text);
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1dea7a8a5255..a0213637af3e 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -873,7 +873,8 @@ 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);
+					 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 c014e5573665..82975853c400 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -493,7 +493,7 @@ enum {
 
 extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
 			      int groupsize, char *linebuf, size_t linebuflen,
-			      bool ascii);
+			      u64 flags);
 
 #ifdef CONFIG_PRINTK
 extern void print_hex_dump_ext(const char *level, const char *prefix_str,
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 2f3bafb55a44..79db784577e7 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -84,7 +84,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, i.e.,
  * 16, 32 or 64 bytes of input data converted to hex + ASCII output.
@@ -95,7 +96,7 @@ EXPORT_SYMBOL(bin2hex);
  *
  * E.g.:
  *   hex_dump_to_buffer(frame->data, frame->len, 16, 1,
- *			linebuf, sizeof(linebuf), true);
+ *			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
@@ -107,7 +108,7 @@ EXPORT_SYMBOL(bin2hex);
  * 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)
+		       char *linebuf, size_t linebuflen, u64 flags)
 {
 	const u8 *ptr = buf;
 	int ngroups;
@@ -184,7 +185,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) {
@@ -204,7 +205,8 @@ 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);
 
@@ -299,8 +301,7 @@ void print_hex_dump_ext(const char *level, const char *prefix_str,
 		first_line = false;
 
 		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
-				   linebuf, sizeof(linebuf),
-				   flags & HEXDUMP_ASCII);
+				   linebuf, sizeof(linebuf), flags);
 
 		switch (prefix_type) {
 		case DUMP_PREFIX_ADDRESS:
diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 5144899d3c6b..16bdb483114d 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -132,7 +132,7 @@ static void __init test_hexdump(size_t len, int rowsize, int groupsize,
 
 	memset(real, FILL_CHAR, sizeof(real));
 	hex_dump_to_buffer(data_b, len, rowsize, groupsize, real, sizeof(real),
-			   ascii);
+			   ascii ? HEXDUMP_ASCII : 0);
 
 	memset(test, FILL_CHAR, sizeof(test));
 	test_hexdump_prepare_test(len, rowsize, groupsize, test, sizeof(test),
@@ -171,7 +171,8 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
 
 	memset(buf, FILL_CHAR, sizeof(buf));
 
-	r = hex_dump_to_buffer(data_b, len, rs, gs, buf, buflen, ascii);
+	r = hex_dump_to_buffer(data_b, len, rs, gs, buf, buflen,
+			ascii ? HEXDUMP_ASCII : 0);
 
 	/*
 	 * Caller must provide the data length multiple of groupsize. The
-- 
2.20.1

[PATCH 4/4] lib/hexdump.c: Allow multiple groups to be separated by lines '|'

[Alastair D'Silva]

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          | 50 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 82975853c400..d9e407e59059 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -485,6 +485,9 @@ enum {
 #define HEXDUMP_SUPPRESS_0XFF	(1 << 2)
 #define HEXDUMP_SUPPRESS_FIRST	(1 << 3)
 #define HEXDUMP_SUPPRESS_LAST	(1 << 4)
+#define HEXDUMP_2_GRP_LINES	(1 << 5)
+#define HEXDUMP_4_GRP_LINES	(1 << 6)
+#define HEXDUMP_8_GRP_LINES	(1 << 7)
 
 #define HEXDUMP_QUIET		(HEXDUMP_SUPPRESS_0X00 | \
 				HEXDUMP_SUPPRESS_0XFF | \
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 79db784577e7..d42f34b93b2c 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -76,6 +76,20 @@ char *bin2hex(char *dst, const void *src, size_t count)
 }
 EXPORT_SYMBOL(bin2hex);
 
+static const char *group_separator(int group, u64 flags)
+{
+	if ((flags & HEXDUMP_8_GRP_LINES) && ((group - 1) % 8))
+		return "|";
+
+	if ((flags & HEXDUMP_4_GRP_LINES) && ((group - 1) % 4))
+		return "|";
+
+	if ((flags & HEXDUMP_2_GRP_LINES) && ((group - 1) % 2))
+		return "|";
+
+	return " ";
+}
+
 /**
  * hex_dump_to_buffer - convert a blob of data to "hex ASCII" in memory
  * @buf: data blob to dump
@@ -86,6 +100,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, i.e.,
  * 16, 32 or 64 bytes of input data converted to hex + ASCII output.
@@ -116,6 +133,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 	int j, lx = 0;
 	int ascii_column;
 	int ret;
+	int line_chars = 0;
 
 	if (rowsize != 16 && rowsize != 32 && rowsize != 64)
 		rowsize = 16;
@@ -141,7 +159,8 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 
 		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;
@@ -152,7 +171,8 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 
 		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;
@@ -163,7 +183,8 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 
 		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;
@@ -193,11 +214,26 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 			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';
@@ -205,7 +241,8 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 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);
@@ -246,6 +283,9 @@ static bool buf_is_all(const u8 *buf, size_t len, u8 val)
  *	HEXDUMP_SUPPRESS_LAST:	allows the last line to be suppressed
  *				If the first and last line may be suppressed,
  *				an empty buffer will not produce any 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
  *
  * 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
@@ -271,7 +311,7 @@ void print_hex_dump_ext(const char *level, const char *prefix_str,
 {
 	const u8 *ptr = buf;
 	int i, remaining = len;
-	unsigned char linebuf[64 * 3 + 2 + 64 + 1];
+	unsigned char linebuf[64 * 3 + 2 + 64 + 31 + 1];
 	bool first_line = true;
 
 	if (rowsize != 16 && rowsize != 32 && rowsize != 64)
-- 
2.20.1

Re: [PATCH 2/4] lib/hexdump.c: Optionally suppress lines of filler bytes

[Alastair D'Silva]

On Wed, 2019-04-10 at 13:17 +1000, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> Some buffers may only be partially filled with useful data, while the
> rest
> is padded (typically with 0x00 or 0xff).
> 
This patch introduces flags which allow lines of padding bytes to be
> suppressed, making the output easier to interpret:
> HEXDUMP_SUPPRESS_0X00,
> HEXDUMP_SUPPRESS_0XFF
> 
> The first and last lines are not suppressed by default, so the
> function
> always outputs something. This behaviour can be further controlled
> with
> the HEXDUMP_SUPPRESS_FIRST & HEXDUMP_SUPPRESS_LAST flags.
> 
> An inline wrapper function is provided for backwards compatibility
> with
> existing code, which maintains the original behaviour.
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
> 
<snip>

> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index b8a164814744..2f3bafb55a44 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -209,8 +209,21 @@ int hex_dump_to_buffer(const void *buf, size_t
> len, int rowsize, int groupsize,
>  EXPORT_SYMBOL(hex_dump_to_buffer);
>  
>  #ifdef CONFIG_PRINTK
> +
> +static bool buf_is_all(const u8 *buf, size_t len, u8 val)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < len; i++) {
> +		if (buf[i] != val)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  /**
> - * print_hex_dump - print a text hex dump to syslog for a binary
> blob of data
> + * print_hex_dump_ext: dump a binary blob of data to syslog in
> hexadecimal
>   * @level: kernel log level (e.g. KERN_DEBUG)
>   * @prefix_str: string to prefix each line with;
>   *  caller supplies trailing spaces for alignment if desired
> @@ -221,42 +234,73 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>   * @buf: data blob to dump
>   * @len: number of bytes in the @buf
>   * @ascii: include ASCII after the hex output
This line should have been removed. I'll address it in V2.

> + * @flags: A bitwise OR of the following flags:
> + *	HEXDUMP_ASCII:		include ASCII after the hex output
> + *	HEXDUMP_SUPPRESS_0X00:	suppress lines that are all 0x00
> + *				(other than first or last)
> + *	HEXDUMP_SUPPRESS_0XFF:	suppress lines that are all 0xff
> + *				(other than first or last)
> + *	HEXDUMP_SUPPRESS_FIRST:	allows the first line to be
> suppressed
> + *	HEXDUMP_SUPPRESS_LAST:	allows the last line to be suppressed
> + *				If the first and last line may be
> suppressed,
> + *				an empty buffer will not produce any
> output
>   *
>   * 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
>   * leading prefix.
>   *
> - * print_hex_dump() works on one "line" of output at a time, i.e.,
> + * print_hex_dump_ext() works on one "line" of output at a time,
> i.e.,
>   * 16, 32 or 64 bytes of input data converted to hex + ASCII output.
> - * print_hex_dump() iterates over the entire input @buf, breaking it
> into
> + * print_hex_dump_ext() iterates over the entire input @buf,
> breaking it into
>   * "line size" chunks to format and print.
>   *
>   * E.g.:
> - *   print_hex_dump(KERN_DEBUG, "raw data: ", DUMP_PREFIX_ADDRESS,
> - *		    16, 1, frame->data, frame->len, true);
> + *   print_hex_dump_ext(KERN_DEBUG, "raw data: ",
> DUMP_PREFIX_ADDRESS,
> + *		    16, 1, frame->data, frame->len, HEXDUMP_ASCII);
>   *
>   * Example output using %DUMP_PREFIX_OFFSET and 1-byte mode:
>   * 0009ab42: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e
> 4f  @ABCDEFGHIJKLMNO
>   * Example output using %DUMP_PREFIX_ADDRESS and 4-byte mode:
>   * ffffffff88089af0: 73727170 77767574 7b7a7978
> 7f7e7d7c  pqrstuvwxyz{|}~.
>   */

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva    
Twitter: @EvilDeece
blog: http://alastair.d-silva.org

Re: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags

[Dan Carpenter]

On Wed, Apr 10, 2019 at 01:17:19PM +1000, Alastair D'Silva wrote:
> @@ -107,7 +108,7 @@ EXPORT_SYMBOL(bin2hex);
>   * 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)
> +		       char *linebuf, size_t linebuflen, u64 flags)
>  {
>  	const u8 *ptr = buf;
>  	int ngroups;
> @@ -184,7 +185,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)
            ^^^^^^^^^^^^^^^^^^^^^^
This is a precedence bug.  It should be if (!(flags & HEXDUMP_ASCII)).


>  		goto nil;
>  

regards,
dan carpenter

RE: [PATCH 4/4] lib/hexdump.c: Allow multiple groups to be separated by lines '|'

[David Laight]

From: Alastair D'Silva
> Sent: 10 April 2019 04:17
> 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  ........|........

Ugg, that is just horrid.
It is enough to add an extra space if you really need the columns
to be more easily counted.

I'm not even sure that is needed if you are printing 32bit words.
OTOH 32bit words makes 64bit values really stupid on LE systems.
Bytes with extra spaces every 4 bytes is the format I prefer
even for long lines.

Oh, and if you are using hexdump() a lot you want a version
that never uses snprintf().

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 4/4] lib/hexdump.c: Allow multiple groups to be separated by lines '|'

[Sergey Senozhatsky]

On (04/10/19 13:17), Alastair D'Silva wrote:
> 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  ........|........

What if the output had |-s in it?  |||||||||

	-ss

RE: [PATCH 4/4] lib/hexdump.c: Allow multiple groups to be separated by lines '|'

[Alastair D'Silva]

> -----Original Message-----
> From: David Laight <David.Laight@ACULAB.COM>
> Sent: Wednesday, 10 April 2019 6:45 PM
> To: 'Alastair D'Silva' <alastair@au1.ibm.com>; alastair@d-silva.org
> Cc: Jani Nikula <jani.nikula@linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Rodrigo Vivi <rodrigo.vivi@intel.com>;
> David Airlie <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; Karsten Keil
> <isdn@linux-pingi.de>; Jassi Brar <jassisinghbrar@gmail.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; David S. Miller <davem@davemloft.net>;
> Jose Abreu <Jose.Abreu@synopsys.com>; Kalle Valo
> <kvalo@codeaurora.org>; Stanislaw Gruszka <sgruszka@redhat.com>;
> Benson Leung <bleung@chromium.org>; Enric Balletbo i Serra
> <enric.balletbo@collabora.com>; James E.J. Bottomley
> <jejb@linux.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com>;
> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Alexander Viro
> <viro@zeniv.linux.org.uk>; Petr Mladek <pmladek@suse.com>; Sergey
> Senozhatsky <sergey.senozhatsky@gmail.com>; Steven Rostedt
> <rostedt@goodmis.org>; Andrew Morton <akpm@linux-foundation.org>;
> intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org;
> ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux-
> scsi@vger.kernel.org; linux-fbdev@vger.kernel.org;
> devel@driverdev.osuosl.org; linux-fsdevel@vger.kernel.org
> Subject: RE: [PATCH 4/4] lib/hexdump.c: Allow multiple groups to be
> separated by lines '|'
> 
> From: Alastair D'Silva
> > Sent: 10 April 2019 04:17
> > 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  ........|........
> 
> Ugg, that is just horrid.
> It is enough to add an extra space if you really need the columns to be more
> easily counted.
>

I did consider that, but it would be a more invasive change, as the buffer length required would differ based on the flags.
 
> I'm not even sure that is needed if you are printing 32bit words.
> OTOH 32bit words makes 64bit values really stupid on LE systems.
> Bytes with extra spaces every 4 bytes is the format I prefer even for long
> lines.
> 
> Oh, and if you are using hexdump() a lot you want a version that never uses
> snprintf().
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK Registration No: 1397386 (Wales)
> 
> 
> ---
> This email has been checked for viruses by AVG.
> https://www.avg.com

Re: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line

[Petr Mladek]

On Wed 2019-04-10 13:17:17, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> With modern high resolution screens, we can display more data, which makes
> life a bit easier when debugging.

I have quite some doubts about this feature.

We are talking about more than 256 characters per-line. I wonder
if such a long line is really easier to read for a human.

I am not expert but there is a reason why the standard is 80
characters per-line. I guess that anything above 100 characters
is questionable. https://en.wikipedia.org/wiki/Line_length
somehow confirms that.

Second, if we take 8 pixels per-character. Then we need
2048 to show the 256 characters. It is more than HD.
IMHO, there is still huge number of people that even do
not have HD display, especially on a notebook.


I am sorry but I am strongly against having this hardcoded
anywhere. And I doubt that there will be enough users
to complicate the code and make it configurable.

Best Regards,
Petr

Re: [PATCH 2/4] lib/hexdump.c: Optionally suppress lines of filler bytes

[Petr Mladek]

On Wed 2019-04-10 13:17:18, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> Some buffers may only be partially filled with useful data, while the rest
> is padded (typically with 0x00 or 0xff).
> 
> This patch introduces flags which allow lines of padding bytes to be
> suppressed, making the output easier to interpret: HEXDUMP_SUPPRESS_0X00,
> HEXDUMP_SUPPRESS_0XFF
> 
> The first and last lines are not suppressed by default, so the function
> always outputs something. This behaviour can be further controlled with
> the HEXDUMP_SUPPRESS_FIRST & HEXDUMP_SUPPRESS_LAST flags.
> 
> An inline wrapper function is provided for backwards compatibility with
> existing code, which maintains the original behaviour.
> 

> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index b8a164814744..2f3bafb55a44 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> +void print_hex_dump_ext(const char *level, const char *prefix_str,
> +			int prefix_type, int rowsize, int groupsize,
> +			const void *buf, size_t len, u64 flags)
>  {
>  	const u8 *ptr = buf;
> -	int i, linelen, remaining = len;
> +	int i, remaining = len;
>  	unsigned char linebuf[64 * 3 + 2 + 64 + 1];
> +	bool first_line = true;
>  
>  	if (rowsize != 16 && rowsize != 32 && rowsize != 64)
>  		rowsize = 16;
>  
>  	for (i = 0; i < len; i += rowsize) {
> -		linelen = min(remaining, rowsize);
> +		bool skip = false;
> +		int linelen = min(remaining, rowsize);
> +
>  		remaining -= rowsize;
>  
> +		if (flags & HEXDUMP_SUPPRESS_0X00)
> +			skip = buf_is_all(ptr + i, linelen, 0x00);
> +
> +		if (!skip && (flags & HEXDUMP_SUPPRESS_0XFF))
> +			skip = buf_is_all(ptr + i, linelen, 0xff);
> +
> +		if (first_line && !(flags & HEXDUMP_SUPPRESS_FIRST))
> +			skip = false;
> +
> +		if (remaining <= 0 && !(flags & HEXDUMP_SUPPRESS_LAST))
> +			skip = false;
> +
> +		if (skip)
> +			continue;

IMHO, quietly skipping lines could cause a lot of confusion,
espcially when the address is not printed.

I wonder how it would look like when we print something like:

    --- skipped XX lines full of 0x00 ---

Then we might even remove the SUPPRESS_FIRST, SUPPRESS_LAST
and the ambiguous QUIET flags.

> +
> +		first_line = false;

This should be above the if (skip).

> +
>  		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> -				   linebuf, sizeof(linebuf), ascii);
> +				   linebuf, sizeof(linebuf),
> +				   flags & HEXDUMP_ASCII);
>  
>  		switch (prefix_type) {
>  		case DUMP_PREFIX_ADDRESS:
> @@ -272,7 +316,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>  		}
>  	}
>  }
> -EXPORT_SYMBOL(print_hex_dump);
> +EXPORT_SYMBOL(print_hex_dump_ext);

We should still export even the original function that
is still used everywhere.

Best Regards,
Petr

Re: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags

[Petr Mladek]

On Wed 2019-04-10 13:17:19, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> In order to support additional features in hex_dump_to_buffer, replace
> the ascii bool parameter with flags.
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c            |  2 +-
>  drivers/isdn/hardware/mISDN/mISDNisar.c           |  6 ++++--
>  drivers/mailbox/mailbox-test.c                    |  2 +-
>  drivers/net/ethernet/amd/xgbe/xgbe-drv.c          |  2 +-
>  drivers/net/ethernet/synopsys/dwc-xlgmac-common.c |  2 +-
>  drivers/net/wireless/ath/ath10k/debug.c           |  3 ++-
>  drivers/net/wireless/intel/iwlegacy/3945-mac.c    |  2 +-
>  drivers/platform/chrome/wilco_ec/debugfs.c        |  3 ++-
>  drivers/scsi/scsi_logging.c                       |  8 +++-----
>  drivers/staging/fbtft/fbtft-core.c                |  2 +-
>  fs/seq_file.c                                     |  3 ++-
>  include/linux/printk.h                            |  2 +-
>  lib/hexdump.c                                     | 15 ++++++++-------
>  lib/test_hexdump.c                                |  5 +++--
>  14 files changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 49fa43ff02ba..fb133e729f9a 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1318,7 +1318,7 @@ 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));
> +						0) >= sizeof(line));

It might be more clear when we define:

#define HEXDUMP_BINARY 0

>  		drm_printf(m, "[%04zx] %s\n", pos, line);
>  
>  		prev = buf + pos;
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index c014e5573665..82975853c400 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -493,7 +493,7 @@ enum {
>  
>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>  			      int groupsize, char *linebuf, size_t linebuflen,
> -			      bool ascii);
> +			      u64 flags);

I wonder how fancy hex_dump could be. IMHO, u32 should be enough.
The last famous words ;-)

Best Regards,
Petr

Re: [Intel-gfx] [PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags

[Tvrtko Ursulin]

On 10/04/2019 04:17, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> In order to support additional features in hex_dump_to_buffer, replace
> the ascii bool parameter with flags.
> 
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
>   drivers/gpu/drm/i915/intel_engine_cs.c            |  2 +-
>   drivers/isdn/hardware/mISDN/mISDNisar.c           |  6 ++++--
>   drivers/mailbox/mailbox-test.c                    |  2 +-
>   drivers/net/ethernet/amd/xgbe/xgbe-drv.c          |  2 +-
>   drivers/net/ethernet/synopsys/dwc-xlgmac-common.c |  2 +-
>   drivers/net/wireless/ath/ath10k/debug.c           |  3 ++-
>   drivers/net/wireless/intel/iwlegacy/3945-mac.c    |  2 +-
>   drivers/platform/chrome/wilco_ec/debugfs.c        |  3 ++-
>   drivers/scsi/scsi_logging.c                       |  8 +++-----
>   drivers/staging/fbtft/fbtft-core.c                |  2 +-
>   fs/seq_file.c                                     |  3 ++-
>   include/linux/printk.h                            |  2 +-
>   lib/hexdump.c                                     | 15 ++++++++-------
>   lib/test_hexdump.c                                |  5 +++--
>   14 files changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 49fa43ff02ba..fb133e729f9a 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1318,7 +1318,7 @@ 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));
> +						0) >= sizeof(line));
>   		drm_printf(m, "[%04zx] %s\n", pos, line);
>   
>   		prev = buf + pos;

i915 code here actually does something I think is more interesting than
HEXDUMP_SUPPRESS_0X**. It replaces any N repeating lines with a single star
marker. For example:

    00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
    *
    00000040 00000001 00000000 00000018 00000002 00000001 00000000 00000018 00000000
    00000060 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000003
    00000080 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
    *
    000000c0 00000002 00000000 00000000 00000000 00000000 00000000 00000000 00000000
    000000e0 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
    *

If or when you end up with this feature in your series you can therefore
replace the whole implementation of hexdump in the above file.

Regards,

Tvrtko

RE: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line

[Alastair D'Silva]

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Friday, 12 April 2019 11:48 PM
> To: Alastair D'Silva <alastair@au1.ibm.com>
> Cc: alastair@d-silva.org; Jani Nikula <jani.nikula@linux.intel.com>;
Joonas
> Lahtinen <joonas.lahtinen@linux.intel.com>; Rodrigo Vivi
> <rodrigo.vivi@intel.com>; David Airlie <airlied@linux.ie>; Daniel Vetter
> <daniel@ffwll.ch>; Karsten Keil <isdn@linux-pingi.de>; Jassi Brar
> <jassisinghbrar@gmail.com>; Tom Lendacky <thomas.lendacky@amd.com>;
> David S. Miller <davem@davemloft.net>; Jose Abreu
> <Jose.Abreu@synopsys.com>; Kalle Valo <kvalo@codeaurora.org>;
> Stanislaw Gruszka <sgruszka@redhat.com>; Benson Leung
> <bleung@chromium.org>; Enric Balletbo i Serra
> <enric.balletbo@collabora.com>; James E.J. Bottomley
> <jejb@linux.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com>;
> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Alexander Viro
> <viro@zeniv.linux.org.uk>; Sergey Senozhatsky
> <sergey.senozhatsky@gmail.com>; Steven Rostedt <rostedt@goodmis.org>;
> Andrew Morton <akpm@linux-foundation.org>; intel-
> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org;
> ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux-
> scsi@vger.kernel.org; linux-fbdev@vger.kernel.org;
> devel@driverdev.osuosl.org; linux-fsdevel@vger.kernel.org
> Subject: Re: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line
> 
> On Wed 2019-04-10 13:17:17, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > With modern high resolution screens, we can display more data, which
> > makes life a bit easier when debugging.
> 
> I have quite some doubts about this feature.
> 
> We are talking about more than 256 characters per-line. I wonder if such a
> long line is really easier to read for a human.

It's basically 2 separate panes of information side by side, the hexdump and
the ASCII version.

I'm using this myself when dealing with the pmem labels, and it works quite
nicely.

> 
> I am not expert but there is a reason why the standard is 80 characters
per-
> line. I guess that anything above 100 characters is questionable.
> https://en.wikipedia.org/wiki/Line_length
> somehow confirms that.
> 
> Second, if we take 8 pixels per-character. Then we need
> 2048 to show the 256 characters. It is more than HD.
> IMHO, there is still huge number of people that even do not have HD
display,
> especially on a notebook.

The intent is to make debugging easier when dealing with large chunks of
binary data. I don't expect end users to see this output.

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva     msn: alastair@d-silva.org
blog: http://alastair.d-silva.org    Twitter: @EvilDeece

RE: [PATCH 2/4] lib/hexdump.c: Optionally suppress lines of filler bytes

[Alastair D'Silva]

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Saturday, 13 April 2019 12:04 AM
> To: Alastair D'Silva <alastair@au1.ibm.com>
> Cc: alastair@d-silva.org; Jani Nikula <jani.nikula@linux.intel.com>;
Joonas
> Lahtinen <joonas.lahtinen@linux.intel.com>; Rodrigo Vivi
> <rodrigo.vivi@intel.com>; David Airlie <airlied@linux.ie>; Daniel Vetter
> <daniel@ffwll.ch>; Karsten Keil <isdn@linux-pingi.de>; Jassi Brar
> <jassisinghbrar@gmail.com>; Tom Lendacky <thomas.lendacky@amd.com>;
> David S. Miller <davem@davemloft.net>; Jose Abreu
> <Jose.Abreu@synopsys.com>; Kalle Valo <kvalo@codeaurora.org>;
> Stanislaw Gruszka <sgruszka@redhat.com>; Benson Leung
> <bleung@chromium.org>; Enric Balletbo i Serra
> <enric.balletbo@collabora.com>; James E.J. Bottomley
> <jejb@linux.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com>;
> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Alexander Viro
> <viro@zeniv.linux.org.uk>; Sergey Senozhatsky
> <sergey.senozhatsky@gmail.com>; Steven Rostedt <rostedt@goodmis.org>;
> Andrew Morton <akpm@linux-foundation.org>; intel-
> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org;
> ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux-
> scsi@vger.kernel.org; linux-fbdev@vger.kernel.org;
> devel@driverdev.osuosl.org; linux-fsdevel@vger.kernel.org
> Subject: Re: [PATCH 2/4] lib/hexdump.c: Optionally suppress lines of
filler
> bytes
> 
> On Wed 2019-04-10 13:17:18, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > Some buffers may only be partially filled with useful data, while the
> > rest is padded (typically with 0x00 or 0xff).
> >
> > This patch introduces flags which allow lines of padding bytes to be
> > suppressed, making the output easier to interpret:
> > HEXDUMP_SUPPRESS_0X00, HEXDUMP_SUPPRESS_0XFF
> >
> > The first and last lines are not suppressed by default, so the
> > function always outputs something. This behaviour can be further
> > controlled with the HEXDUMP_SUPPRESS_FIRST &
> HEXDUMP_SUPPRESS_LAST flags.
> >
> > An inline wrapper function is provided for backwards compatibility
> > with existing code, which maintains the original behaviour.
> >
> 
> > diff --git a/lib/hexdump.c b/lib/hexdump.c index
> > b8a164814744..2f3bafb55a44 100644
> > --- a/lib/hexdump.c
> > +++ b/lib/hexdump.c
> > +void print_hex_dump_ext(const char *level, const char *prefix_str,
> > +			int prefix_type, int rowsize, int groupsize,
> > +			const void *buf, size_t len, u64 flags)
> >  {
> >  	const u8 *ptr = buf;
> > -	int i, linelen, remaining = len;
> > +	int i, remaining = len;
> >  	unsigned char linebuf[64 * 3 + 2 + 64 + 1];
> > +	bool first_line = true;
> >
> >  	if (rowsize != 16 && rowsize != 32 && rowsize != 64)
> >  		rowsize = 16;
> >
> >  	for (i = 0; i < len; i += rowsize) {
> > -		linelen = min(remaining, rowsize);
> > +		bool skip = false;
> > +		int linelen = min(remaining, rowsize);
> > +
> >  		remaining -= rowsize;
> >
> > +		if (flags & HEXDUMP_SUPPRESS_0X00)
> > +			skip = buf_is_all(ptr + i, linelen, 0x00);
> > +
> > +		if (!skip && (flags & HEXDUMP_SUPPRESS_0XFF))
> > +			skip = buf_is_all(ptr + i, linelen, 0xff);
> > +
> > +		if (first_line && !(flags & HEXDUMP_SUPPRESS_FIRST))
> > +			skip = false;
> > +
> > +		if (remaining <= 0 && !(flags & HEXDUMP_SUPPRESS_LAST))
> > +			skip = false;
> > +
> > +		if (skip)
> > +			continue;
> 
> IMHO, quietly skipping lines could cause a lot of confusion, espcially
when
> the address is not printed.
>
It's up to the caller to decide how they want it displayed.

> I wonder how it would look like when we print something like:
> 
>     --- skipped XX lines full of 0x00 ---

This could be added as a later enhancement, with a new flag (eg.
HEXDUMP_SUPPRESS_VERBOSE).

> 
> Then we might even remove the SUPPRESS_FIRST, SUPPRESS_LAST and the
> ambiguous QUIET flags.
> 
> > +
> > +		first_line = false;
> 
> This should be above the if (skip).
> 
> > +
> >  		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> > -				   linebuf, sizeof(linebuf), ascii);
> > +				   linebuf, sizeof(linebuf),
> > +				   flags & HEXDUMP_ASCII);
> >
> >  		switch (prefix_type) {
> >  		case DUMP_PREFIX_ADDRESS:
> > @@ -272,7 +316,7 @@ void print_hex_dump(const char *level, const char
> *prefix_str, int prefix_type,
> >  		}
> >  	}
> >  }
> > -EXPORT_SYMBOL(print_hex_dump);
> > +EXPORT_SYMBOL(print_hex_dump_ext);
> 
> We should still export even the original function that is still used
everywhere.

It's replaced with an inline wrapper function, there's no need to export it.

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva     msn: alastair@d-silva.org
blog: http://alastair.d-silva.org    Twitter: @EvilDeece

RE: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags

[Alastair D'Silva]

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Saturday, 13 April 2019 12:12 AM
> To: Alastair D'Silva <alastair@au1.ibm.com>
> Cc: alastair@d-silva.org; Jani Nikula <jani.nikula@linux.intel.com>;
Joonas
> Lahtinen <joonas.lahtinen@linux.intel.com>; Rodrigo Vivi
> <rodrigo.vivi@intel.com>; David Airlie <airlied@linux.ie>; Daniel Vetter
> <daniel@ffwll.ch>; Karsten Keil <isdn@linux-pingi.de>; Jassi Brar
> <jassisinghbrar@gmail.com>; Tom Lendacky <thomas.lendacky@amd.com>;
> David S. Miller <davem@davemloft.net>; Jose Abreu
> <Jose.Abreu@synopsys.com>; Kalle Valo <kvalo@codeaurora.org>;
> Stanislaw Gruszka <sgruszka@redhat.com>; Benson Leung
> <bleung@chromium.org>; Enric Balletbo i Serra
> <enric.balletbo@collabora.com>; James E.J. Bottomley
> <jejb@linux.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com>;
> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Alexander Viro
> <viro@zeniv.linux.org.uk>; Sergey Senozhatsky
> <sergey.senozhatsky@gmail.com>; Steven Rostedt <rostedt@goodmis.org>;
> Andrew Morton <akpm@linux-foundation.org>; intel-
> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org;
> ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux-
> scsi@vger.kernel.org; linux-fbdev@vger.kernel.org;
> devel@driverdev.osuosl.org; linux-fsdevel@vger.kernel.org
> Subject: Re: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in
> hex_dump_to_buffer with flags
> 
> On Wed 2019-04-10 13:17:19, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > In order to support additional features in hex_dump_to_buffer, replace
> > the ascii bool parameter with flags.
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> >  drivers/gpu/drm/i915/intel_engine_cs.c            |  2 +-
> >  drivers/isdn/hardware/mISDN/mISDNisar.c           |  6 ++++--
> >  drivers/mailbox/mailbox-test.c                    |  2 +-
> >  drivers/net/ethernet/amd/xgbe/xgbe-drv.c          |  2 +-
> >  drivers/net/ethernet/synopsys/dwc-xlgmac-common.c |  2 +-
> >  drivers/net/wireless/ath/ath10k/debug.c           |  3 ++-
> >  drivers/net/wireless/intel/iwlegacy/3945-mac.c    |  2 +-
> >  drivers/platform/chrome/wilco_ec/debugfs.c        |  3 ++-
> >  drivers/scsi/scsi_logging.c                       |  8 +++-----
> >  drivers/staging/fbtft/fbtft-core.c                |  2 +-
> >  fs/seq_file.c                                     |  3 ++-
> >  include/linux/printk.h                            |  2 +-
> >  lib/hexdump.c                                     | 15 ++++++++-------
> >  lib/test_hexdump.c                                |  5 +++--
> >  14 files changed, 31 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> > b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index 49fa43ff02ba..fb133e729f9a 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -1318,7 +1318,7 @@ 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));
> > +						0) >= sizeof(line));
> 
> It might be more clear when we define:
> 
> #define HEXDUMP_BINARY 0

This feels unnecessary, and weird. Omitting the flag won't disable the hex
output (as expected), and if you don't want hex output, why call hexdump in
the first place?

> >  		drm_printf(m, "[%04zx] %s\n", pos, line);
> >
> >  		prev = buf + pos;
> > diff --git a/include/linux/printk.h b/include/linux/printk.h index
> > c014e5573665..82975853c400 100644
> > --- a/include/linux/printk.h
> > +++ b/include/linux/printk.h
> > @@ -493,7 +493,7 @@ enum {
> >
> >  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> >  			      int groupsize, char *linebuf, size_t
linebuflen,
> > -			      bool ascii);
> > +			      u64 flags);
> 
> I wonder how fancy hex_dump could be. IMHO, u32 should be enough.
> The last famous words ;-)
> 
> Best Regards,
> Petr
> 
> 
> ---
> This email has been checked for viruses by AVG.
> https://www.avg.com

Re: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line

[Petr Mladek]

On Sat 2019-04-13 09:22:05, Alastair D'Silva wrote:
> > -----Original Message-----
> > From: Petr Mladek <pmladek@suse.com>
> > Sent: Friday, 12 April 2019 11:48 PM
> > To: Alastair D'Silva <alastair@au1.ibm.com>
> > Cc: alastair@d-silva.org; Jani Nikula <jani.nikula@linux.intel.com>;
> Joonas
> > Lahtinen <joonas.lahtinen@linux.intel.com>; Rodrigo Vivi
> > <rodrigo.vivi@intel.com>; David Airlie <airlied@linux.ie>; Daniel Vetter
> > <daniel@ffwll.ch>; Karsten Keil <isdn@linux-pingi.de>; Jassi Brar
> > <jassisinghbrar@gmail.com>; Tom Lendacky <thomas.lendacky@amd.com>;
> > David S. Miller <davem@davemloft.net>; Jose Abreu
> > <Jose.Abreu@synopsys.com>; Kalle Valo <kvalo@codeaurora.org>;
> > Stanislaw Gruszka <sgruszka@redhat.com>; Benson Leung
> > <bleung@chromium.org>; Enric Balletbo i Serra
> > <enric.balletbo@collabora.com>; James E.J. Bottomley
> > <jejb@linux.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com>;
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Alexander Viro
> > <viro@zeniv.linux.org.uk>; Sergey Senozhatsky
> > <sergey.senozhatsky@gmail.com>; Steven Rostedt <rostedt@goodmis.org>;
> > Andrew Morton <akpm@linux-foundation.org>; intel-
> > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> > kernel@vger.kernel.org; netdev@vger.kernel.org;
> > ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux-
> > scsi@vger.kernel.org; linux-fbdev@vger.kernel.org;
> > devel@driverdev.osuosl.org; linux-fsdevel@vger.kernel.org
> > Subject: Re: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line
> > 
> > On Wed 2019-04-10 13:17:17, Alastair D'Silva wrote:
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > >
> > > With modern high resolution screens, we can display more data, which
> > > makes life a bit easier when debugging.
> > 
> > I have quite some doubts about this feature.
> > 
> > We are talking about more than 256 characters per-line. I wonder if such a
> > long line is really easier to read for a human.
> 
> It's basically 2 separate panes of information side by side, the hexdump and
> the ASCII version.
> 
> I'm using this myself when dealing with the pmem labels, and it works quite
> nicely.

I am sure that it works for you. But I do not believe that it
would be useful in general.


> > I am not expert but there is a reason why the standard is 80 characters
> per-
> > line. I guess that anything above 100 characters is questionable.
> > https://en.wikipedia.org/wiki/Line_length
> > somehow confirms that.
> > 
> > Second, if we take 8 pixels per-character. Then we need
> > 2048 to show the 256 characters. It is more than HD.
> > IMHO, there is still huge number of people that even do not have HD
> display,
> > especially on a notebook.
> 
> The intent is to make debugging easier when dealing with large chunks of
> binary data. I don't expect end users to see this output.

How is it supposed to be used then? Only by your temporary patches?

Best Regards,
Petr

Re: [PATCH 2/4] lib/hexdump.c: Optionally suppress lines of filler bytes

[Petr Mladek]

On Sat 2019-04-13 09:28:03, Alastair D'Silva wrote:
> > -----Original Message-----
> > From: Petr Mladek <pmladek@suse.com>
> > Sent: Saturday, 13 April 2019 12:04 AM
> > To: Alastair D'Silva <alastair@au1.ibm.com>
> > Cc: alastair@d-silva.org; Jani Nikula <jani.nikula@linux.intel.com>;
> Joonas
> > Lahtinen <joonas.lahtinen@linux.intel.com>; Rodrigo Vivi
> > <rodrigo.vivi@intel.com>; David Airlie <airlied@linux.ie>; Daniel Vetter
> > <daniel@ffwll.ch>; Karsten Keil <isdn@linux-pingi.de>; Jassi Brar
> > <jassisinghbrar@gmail.com>; Tom Lendacky <thomas.lendacky@amd.com>;
> > David S. Miller <davem@davemloft.net>; Jose Abreu
> > <Jose.Abreu@synopsys.com>; Kalle Valo <kvalo@codeaurora.org>;
> > Stanislaw Gruszka <sgruszka@redhat.com>; Benson Leung
> > <bleung@chromium.org>; Enric Balletbo i Serra
> > <enric.balletbo@collabora.com>; James E.J. Bottomley
> > <jejb@linux.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com>;
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Alexander Viro
> > <viro@zeniv.linux.org.uk>; Sergey Senozhatsky
> > <sergey.senozhatsky@gmail.com>; Steven Rostedt <rostedt@goodmis.org>;
> > Andrew Morton <akpm@linux-foundation.org>; intel-
> > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> > kernel@vger.kernel.org; netdev@vger.kernel.org;
> > ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux-
> > scsi@vger.kernel.org; linux-fbdev@vger.kernel.org;
> > devel@driverdev.osuosl.org; linux-fsdevel@vger.kernel.org
> > Subject: Re: [PATCH 2/4] lib/hexdump.c: Optionally suppress lines of
> filler
> > bytes
> > 
> > On Wed 2019-04-10 13:17:18, Alastair D'Silva wrote:
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > >
> > > Some buffers may only be partially filled with useful data, while the
> > > rest is padded (typically with 0x00 or 0xff).
> > >
> > > This patch introduces flags which allow lines of padding bytes to be
> > > suppressed, making the output easier to interpret:
> > > HEXDUMP_SUPPRESS_0X00, HEXDUMP_SUPPRESS_0XFF
> > >
> > > The first and last lines are not suppressed by default, so the
> > > function always outputs something. This behaviour can be further
> > > controlled with the HEXDUMP_SUPPRESS_FIRST &
> > HEXDUMP_SUPPRESS_LAST flags.
> > >
> > > An inline wrapper function is provided for backwards compatibility
> > > with existing code, which maintains the original behaviour.
> > >
> > 
> > > diff --git a/lib/hexdump.c b/lib/hexdump.c index
> > > b8a164814744..2f3bafb55a44 100644
> > > --- a/lib/hexdump.c
> > > +++ b/lib/hexdump.c
> > > +void print_hex_dump_ext(const char *level, const char *prefix_str,
> > > +			int prefix_type, int rowsize, int groupsize,
> > > +			const void *buf, size_t len, u64 flags)
> > >  {
> > >  	const u8 *ptr = buf;
> > > -	int i, linelen, remaining = len;
> > > +	int i, remaining = len;
> > >  	unsigned char linebuf[64 * 3 + 2 + 64 + 1];
> > > +	bool first_line = true;
> > >
> > >  	if (rowsize != 16 && rowsize != 32 && rowsize != 64)
> > >  		rowsize = 16;
> > >
> > >  	for (i = 0; i < len; i += rowsize) {
> > > -		linelen = min(remaining, rowsize);
> > > +		bool skip = false;
> > > +		int linelen = min(remaining, rowsize);
> > > +
> > >  		remaining -= rowsize;
> > >
> > > +		if (flags & HEXDUMP_SUPPRESS_0X00)
> > > +			skip = buf_is_all(ptr + i, linelen, 0x00);
> > > +
> > > +		if (!skip && (flags & HEXDUMP_SUPPRESS_0XFF))
> > > +			skip = buf_is_all(ptr + i, linelen, 0xff);
> > > +
> > > +		if (first_line && !(flags & HEXDUMP_SUPPRESS_FIRST))
> > > +			skip = false;
> > > +
> > > +		if (remaining <= 0 && !(flags & HEXDUMP_SUPPRESS_LAST))
> > > +			skip = false;
> > > +
> > > +		if (skip)
> > > +			continue;
> > 
> > IMHO, quietly skipping lines could cause a lot of confusion, espcially
> when the address is not printed.
> >
> It's up to the caller to decide how they want it displayed.

I wonder who would want to quietly skip some data values.
Are you using it yourself? Could you please provide an
example?

I do not see why we would need to complicate the API and code
by this.

The behavior proposed by Tvrtko Ursulin makes much more
sense. I mean
https://lkml.kernel.org/r/929244ed-cc7f-b0f3-b5ac-50e798e83188@linux.intel.com


> > I wonder how it would look like when we print something like:
> > 
> >     --- skipped XX lines full of 0x00 ---
> 
> This could be added as a later enhancement, with a new flag (eg.
> HEXDUMP_SUPPRESS_VERBOSE).

Who will add this later? Frankly, this looks like a half baked
feature that it good enough for you. If you want it upstream,
it must behave reasonably and it must be worth it.

Best Regards,
Petr

Re: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags

[Petr Mladek]

On Sat 2019-04-13 09:31:27, Alastair D'Silva wrote:
> > -----Original Message-----
> > From: Petr Mladek <pmladek@suse.com>
> > Sent: Saturday, 13 April 2019 12:12 AM
> > To: Alastair D'Silva <alastair@au1.ibm.com>
> > Cc: alastair@d-silva.org; Jani Nikula <jani.nikula@linux.intel.com>;
> Joonas
> > Lahtinen <joonas.lahtinen@linux.intel.com>; Rodrigo Vivi
> > <rodrigo.vivi@intel.com>; David Airlie <airlied@linux.ie>; Daniel Vetter
> > <daniel@ffwll.ch>; Karsten Keil <isdn@linux-pingi.de>; Jassi Brar
> > <jassisinghbrar@gmail.com>; Tom Lendacky <thomas.lendacky@amd.com>;
> > David S. Miller <davem@davemloft.net>; Jose Abreu
> > <Jose.Abreu@synopsys.com>; Kalle Valo <kvalo@codeaurora.org>;
> > Stanislaw Gruszka <sgruszka@redhat.com>; Benson Leung
> > <bleung@chromium.org>; Enric Balletbo i Serra
> > <enric.balletbo@collabora.com>; James E.J. Bottomley
> > <jejb@linux.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com>;
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Alexander Viro
> > <viro@zeniv.linux.org.uk>; Sergey Senozhatsky
> > <sergey.senozhatsky@gmail.com>; Steven Rostedt <rostedt@goodmis.org>;
> > Andrew Morton <akpm@linux-foundation.org>; intel-
> > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> > kernel@vger.kernel.org; netdev@vger.kernel.org;
> > ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux-
> > scsi@vger.kernel.org; linux-fbdev@vger.kernel.org;
> > devel@driverdev.osuosl.org; linux-fsdevel@vger.kernel.org
> > Subject: Re: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in
> > hex_dump_to_buffer with flags
> > 
> > On Wed 2019-04-10 13:17:19, Alastair D'Silva wrote:
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > >
> > > In order to support additional features in hex_dump_to_buffer, replace
> > > the ascii bool parameter with flags.
> > >
> > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > > ---
> > >  drivers/gpu/drm/i915/intel_engine_cs.c            |  2 +-
> > >  drivers/isdn/hardware/mISDN/mISDNisar.c           |  6 ++++--
> > >  drivers/mailbox/mailbox-test.c                    |  2 +-
> > >  drivers/net/ethernet/amd/xgbe/xgbe-drv.c          |  2 +-
> > >  drivers/net/ethernet/synopsys/dwc-xlgmac-common.c |  2 +-
> > >  drivers/net/wireless/ath/ath10k/debug.c           |  3 ++-
> > >  drivers/net/wireless/intel/iwlegacy/3945-mac.c    |  2 +-
> > >  drivers/platform/chrome/wilco_ec/debugfs.c        |  3 ++-
> > >  drivers/scsi/scsi_logging.c                       |  8 +++-----
> > >  drivers/staging/fbtft/fbtft-core.c                |  2 +-
> > >  fs/seq_file.c                                     |  3 ++-
> > >  include/linux/printk.h                            |  2 +-
> > >  lib/hexdump.c                                     | 15 ++++++++-------
> > >  lib/test_hexdump.c                                |  5 +++--
> > >  14 files changed, 31 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> > > b/drivers/gpu/drm/i915/intel_engine_cs.c
> > > index 49fa43ff02ba..fb133e729f9a 100644
> > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > > @@ -1318,7 +1318,7 @@ 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));
> > > +						0) >= sizeof(line));
> > 
> > It might be more clear when we define:
> > 
> > #define HEXDUMP_BINARY 0
> 
> This feels unnecessary, and weird. Omitting the flag won't disable the hex
> output (as expected), and if you don't want hex output why call hexdump in
> the first place?

Why do we have HEXDUMP_ASCII then?
Why is the above funtion not using HEXDUMP_ASCII?
Who would call it with (HEXDUMP_ASCII | HEXDUMP_BINARY)?

Best Regards,
Petr

RE: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags

[Alastair D'Silva]

> -----Original Message-----
> From: Petr Mladek <pmladek@suse.com>
> Sent: Monday, 15 April 2019 7:24 PM
> To: Alastair D'Silva <alastair@d-silva.org>
> Cc: 'Alastair D'Silva' <alastair@au1.ibm.com>; 'Jani Nikula'
> <jani.nikula@linux.intel.com>; 'Joonas Lahtinen'
> <joonas.lahtinen@linux.intel.com>; 'Rodrigo Vivi'
<rodrigo.vivi@intel.com>;
> 'David Airlie' <airlied@linux.ie>; 'Daniel Vetter' <daniel@ffwll.ch>;
'Karsten
> Keil' <isdn@linux-pingi.de>; 'Jassi Brar' <jassisinghbrar@gmail.com>; 'Tom
> Lendacky' <thomas.lendacky@amd.com>; 'David S. Miller'
> <davem@davemloft.net>; 'Jose Abreu' <Jose.Abreu@synopsys.com>; 'Kalle
> Valo' <kvalo@codeaurora.org>; 'Stanislaw Gruszka' <sgruszka@redhat.com>;
> 'Benson Leung' <bleung@chromium.org>; 'Enric Balletbo i Serra'
> <enric.balletbo@collabora.com>; 'James E.J. Bottomley'
> <jejb@linux.ibm.com>; 'Martin K. Petersen' <martin.petersen@oracle.com>;
> 'Greg Kroah-Hartman' <gregkh@linuxfoundation.org>; 'Alexander Viro'
> <viro@zeniv.linux.org.uk>; 'Sergey Senozhatsky'
> <sergey.senozhatsky@gmail.com>; 'Steven Rostedt'
> <rostedt@goodmis.org>; 'Andrew Morton' <akpm@linux-foundation.org>;
> intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org;
> ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux-
> scsi@vger.kernel.org; linux-fbdev@vger.kernel.org;
> devel@driverdev.osuosl.org; linux-fsdevel@vger.kernel.org
> Subject: Re: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in
> hex_dump_to_buffer with flags
> 
> On Sat 2019-04-13 09:31:27, Alastair D'Silva wrote:
> > > -----Original Message-----
> > > From: Petr Mladek <pmladek@suse.com>
> > > Sent: Saturday, 13 April 2019 12:12 AM
> > > To: Alastair D'Silva <alastair@au1.ibm.com>
> > > Cc: alastair@d-silva.org; Jani Nikula <jani.nikula@linux.intel.com>;
> > Joonas
> > > Lahtinen <joonas.lahtinen@linux.intel.com>; Rodrigo Vivi
> > > <rodrigo.vivi@intel.com>; David Airlie <airlied@linux.ie>; Daniel
> > > Vetter <daniel@ffwll.ch>; Karsten Keil <isdn@linux-pingi.de>; Jassi
> > > Brar <jassisinghbrar@gmail.com>; Tom Lendacky
> > > <thomas.lendacky@amd.com>; David S. Miller
> <davem@davemloft.net>;
> > > Jose Abreu <Jose.Abreu@synopsys.com>; Kalle Valo
> > > <kvalo@codeaurora.org>; Stanislaw Gruszka <sgruszka@redhat.com>;
> > > Benson Leung <bleung@chromium.org>; Enric Balletbo i Serra
> > > <enric.balletbo@collabora.com>; James E.J. Bottomley
> > > <jejb@linux.ibm.com>; Martin K. Petersen
> > > <martin.petersen@oracle.com>; Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org>; Alexander Viro
> > > <viro@zeniv.linux.org.uk>; Sergey Senozhatsky
> > > <sergey.senozhatsky@gmail.com>; Steven Rostedt
> > > <rostedt@goodmis.org>; Andrew Morton <akpm@linux-
> foundation.org>;
> > > intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> > > linux- kernel@vger.kernel.org; netdev@vger.kernel.org;
> > > ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux-
> > > scsi@vger.kernel.org; linux-fbdev@vger.kernel.org;
> > > devel@driverdev.osuosl.org; linux-fsdevel@vger.kernel.org
> > > Subject: Re: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in
> > > hex_dump_to_buffer with flags
> > >
> > > On Wed 2019-04-10 13:17:19, Alastair D'Silva wrote:
> > > > From: Alastair D'Silva <alastair@d-silva.org>
> > > >
> > > > In order to support additional features in hex_dump_to_buffer,
> > > > replace the ascii bool parameter with flags.
> > > >
> > > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_engine_cs.c            |  2 +-
> > > >  drivers/isdn/hardware/mISDN/mISDNisar.c           |  6 ++++--
> > > >  drivers/mailbox/mailbox-test.c                    |  2 +-
> > > >  drivers/net/ethernet/amd/xgbe/xgbe-drv.c          |  2 +-
> > > >  drivers/net/ethernet/synopsys/dwc-xlgmac-common.c |  2 +-
> > > >  drivers/net/wireless/ath/ath10k/debug.c           |  3 ++-
> > > >  drivers/net/wireless/intel/iwlegacy/3945-mac.c    |  2 +-
> > > >  drivers/platform/chrome/wilco_ec/debugfs.c        |  3 ++-
> > > >  drivers/scsi/scsi_logging.c                       |  8 +++-----
> > > >  drivers/staging/fbtft/fbtft-core.c                |  2 +-
> > > >  fs/seq_file.c                                     |  3 ++-
> > > >  include/linux/printk.h                            |  2 +-
> > > >  lib/hexdump.c                                     | 15
++++++++-------
> > > >  lib/test_hexdump.c                                |  5 +++--
> > > >  14 files changed, 31 insertions(+), 26 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> > > > b/drivers/gpu/drm/i915/intel_engine_cs.c
> > > > index 49fa43ff02ba..fb133e729f9a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > > > @@ -1318,7 +1318,7 @@ 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));
> > > > +						0) >= sizeof(line));
> > >
> > > It might be more clear when we define:
> > >
> > > #define HEXDUMP_BINARY 0
> >
> > This feels unnecessary, and weird. Omitting the flag won't disable the
> > hex output (as expected), and if you don't want hex output why call
> > hexdump in the first place?
> 
> Why do we have HEXDUMP_ASCII then?
> Why is the above funtion not using HEXDUMP_ASCII?
> Who would call it with (HEXDUMP_ASCII | HEXDUMP_BINARY)?

The ASCII flag controls the optional ASCII output, and replaces the boolean
that existed prior. A user would enable it in the same situations where they
currently pass true for the ascii parameter.

In the above example the author only wants the hex output, while in other
situations, both hex & ASCII output is desirable. If you just want ASCII
output, the caller should just use a printk or one of it's wrappers.

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva     msn: alastair@d-silva.org
blog: http://alastair.d-silva.org    Twitter: @EvilDeece

RE: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags

[David Laight]

From: Alastair D'Silva
> Sent: 15 April 2019 11:07
...
> In the above example the author only wants the hex output, while in other
> situations, both hex & ASCII output is desirable. If you just want ASCII
> output, the caller should just use a printk or one of it's wrappers.

Hexdump will 'sanitise' the ASCII.

Although I think you'd want a 'no hex' flag to suppress the hex.

Probably more useful flags are ones to suppress the address column.
I've also used flags to enable (or disable) suppression of multiple
lines of zeros of constant bytes.
In that case you may want hexdump to return the flags for the
next call when a large buffer is being dumped in fragments.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line

[Alastair D'Silva]

<snip>
> > > On Wed 2019-04-10 13:17:17, Alastair D'Silva wrote:
> > > > From: Alastair D'Silva <alastair@d-silva.org>
> > > >
> > > > With modern high resolution screens, we can display more data,
> > > > which makes life a bit easier when debugging.
> > >
> > > I have quite some doubts about this feature.
> > >
> > > We are talking about more than 256 characters per-line. I wonder if
> > > such a long line is really easier to read for a human.
> >
> > It's basically 2 separate panes of information side by side, the
> > hexdump and the ASCII version.
> >
> > I'm using this myself when dealing with the pmem labels, and it works
> > quite nicely.
> 
> I am sure that it works for you. But I do not believe that it would be
useful in
> general.

I do, and I believe the choice of the output length should be in the hands
of the caller.

On further thought, it would make more sense to remove the hardcoded list of
sizes and just enforce a power of 2. The function shouldn't dictate what the
caller can and can't do beyond the technical limits of it's implementation.

Other print/debug functions don't restrict the output size, and I can't see
a good justification why hexdump should either.

> > > I am not expert but there is a reason why the standard is 80
> > > characters
> > per-
> > > line. I guess that anything above 100 characters is questionable.
> > > https://en.wikipedia.org/wiki/Line_length
> > > somehow confirms that.
> > >
> > > Second, if we take 8 pixels per-character. Then we need
> > > 2048 to show the 256 characters. It is more than HD.
> > > IMHO, there is still huge number of people that even do not have HD
> > display,
> > > especially on a notebook.
> >
> > The intent is to make debugging easier when dealing with large chunks
> > of binary data. I don't expect end users to see this output.
> 
> How is it supposed to be used then? Only by your temporary patches?

Let me rephrase that, I don't expect end users to *use* this data.

Current usage of the hexdump functions are predominantly centred around
logging and debugging, and clearly targeted at someone intimately familiar
with the relevant subsystem. I expect future use would be similar.

Debugging may be as part of active development, or from a log supplied from
an end user. In either case, it should be up to the author (as a
representative for the consumers of the data) to decide how it should be
formatted.

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva     msn: alastair@d-silva.org
blog: http://alastair.d-silva.org    Twitter: @EvilDeece

RE: [PATCH 2/4] lib/hexdump.c: Optionally suppress lines of filler bytes

[Alastair D'Silva]

> > > On Wed 2019-04-10 13:17:18, Alastair D'Silva wrote:
> > > > From: Alastair D'Silva <alastair@d-silva.org>
> > > >
> > > > Some buffers may only be partially filled with useful data, while
> > > > the rest is padded (typically with 0x00 or 0xff).
> > > >
> > > > This patch introduces flags which allow lines of padding bytes to
> > > > be suppressed, making the output easier to interpret:
> > > > HEXDUMP_SUPPRESS_0X00, HEXDUMP_SUPPRESS_0XFF
> > > >
> > > > The first and last lines are not suppressed by default, so the
> > > > function always outputs something. This behaviour can be further
> > > > controlled with the HEXDUMP_SUPPRESS_FIRST &
> > > HEXDUMP_SUPPRESS_LAST flags.
> > > >
> > > > An inline wrapper function is provided for backwards compatibility
> > > > with existing code, which maintains the original behaviour.
> > > >
> > >
> > > > diff --git a/lib/hexdump.c b/lib/hexdump.c index
> > > > b8a164814744..2f3bafb55a44 100644
> > > > --- a/lib/hexdump.c
> > > > +++ b/lib/hexdump.c
> > > > +void print_hex_dump_ext(const char *level, const char *prefix_str,
> > > > +			int prefix_type, int rowsize, int groupsize,
> > > > +			const void *buf, size_t len, u64 flags)
> > > >  {
> > > >  	const u8 *ptr = buf;
> > > > -	int i, linelen, remaining = len;
> > > > +	int i, remaining = len;
> > > >  	unsigned char linebuf[64 * 3 + 2 + 64 + 1];
> > > > +	bool first_line = true;
> > > >
> > > >  	if (rowsize != 16 && rowsize != 32 && rowsize != 64)
> > > >  		rowsize = 16;
> > > >
> > > >  	for (i = 0; i < len; i += rowsize) {
> > > > -		linelen = min(remaining, rowsize);
> > > > +		bool skip = false;
> > > > +		int linelen = min(remaining, rowsize);
> > > > +
> > > >  		remaining -= rowsize;
> > > >
> > > > +		if (flags & HEXDUMP_SUPPRESS_0X00)
> > > > +			skip = buf_is_all(ptr + i, linelen, 0x00);
> > > > +
> > > > +		if (!skip && (flags & HEXDUMP_SUPPRESS_0XFF))
> > > > +			skip = buf_is_all(ptr + i, linelen, 0xff);
> > > > +
> > > > +		if (first_line && !(flags & HEXDUMP_SUPPRESS_FIRST))
> > > > +			skip = false;
> > > > +
> > > > +		if (remaining <= 0 && !(flags &
HEXDUMP_SUPPRESS_LAST))
> > > > +			skip = false;
> > > > +
> > > > +		if (skip)
> > > > +			continue;
> > >
> > > IMHO, quietly skipping lines could cause a lot of confusion,
> > > espcially
> > when the address is not printed.
> > >
> > It's up to the caller to decide how they want it displayed.
> 
> I wonder who would want to quietly skip some data values.
> Are you using it yourself? Could you please provide an example?

Yes, but I don't have the content with me at the moment, so I can't share
it. I'm dumping persistent memory labels, which are 64kB long, but only the
first few hundred bytes are populated.
 
> I do not see why we would need to complicate the API and code by this.
> 
> The behavior proposed by Tvrtko Ursulin makes much more sense. I mean
> https://lkml.kernel.org/r/929244ed-cc7f-b0f3-b5ac-
> 50e798e83188@linux.intel.com

I agree that is better, I'll add that to V2.

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva     msn: alastair@d-silva.org
blog: http://alastair.d-silva.org    Twitter: @EvilDeece

RE: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags

[Alastair D'Silva]

> -----Original Message-----
> From: David Laight <David.Laight@ACULAB.COM>
> Sent: Monday, 15 April 2019 8:21 PM
> To: 'Alastair D'Silva' <alastair@d-silva.org>; 'Petr Mladek'
> <pmladek@suse.com>
> Cc: 'Alastair D'Silva' <alastair@au1.ibm.com>; 'Jani Nikula'
> <jani.nikula@linux.intel.com>; 'Joonas Lahtinen'
> <joonas.lahtinen@linux.intel.com>; 'Rodrigo Vivi' <rodrigo.vivi@intel.com>;
> 'David Airlie' <airlied@linux.ie>; 'Daniel Vetter' <daniel@ffwll.ch>; 'Karsten
> Keil' <isdn@linux-pingi.de>; 'Jassi Brar' <jassisinghbrar@gmail.com>; 'Tom
> Lendacky' <thomas.lendacky@amd.com>; 'David S. Miller'
> <davem@davemloft.net>; 'Jose Abreu' <Jose.Abreu@synopsys.com>; 'Kalle
> Valo' <kvalo@codeaurora.org>; 'Stanislaw Gruszka' <sgruszka@redhat.com>;
> 'Benson Leung' <bleung@chromium.org>; 'Enric Balletbo i Serra'
> <enric.balletbo@collabora.com>; 'James E.J. Bottomley'
> <jejb@linux.ibm.com>; 'Martin K. Petersen' <martin.petersen@oracle.com>;
> 'Greg Kroah-Hartman' <gregkh@linuxfoundation.org>; 'Alexander Viro'
> <viro@zeniv.linux.org.uk>; 'Sergey Senozhatsky'
> <sergey.senozhatsky@gmail.com>; 'Steven Rostedt'
> <rostedt@goodmis.org>; 'Andrew Morton' <akpm@linux-foundation.org>;
> intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org;
> ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux-
> scsi@vger.kernel.org; linux-fbdev@vger.kernel.org;
> devel@driverdev.osuosl.org; linux-fsdevel@vger.kernel.org
> Subject: RE: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in
> hex_dump_to_buffer with flags
> 
> From: Alastair D'Silva
> > Sent: 15 April 2019 11:07
> ...
> > In the above example the author only wants the hex output, while in
> > other situations, both hex & ASCII output is desirable. If you just
> > want ASCII output, the caller should just use a printk or one of it's
> wrappers.
> 
> Hexdump will 'sanitise' the ASCII.
> 

This is functionality that doesn't exist in the current hexdump implementation (you always get the hex version).

I think a better option would be to factor out the sanitisation and expose that as a separate function.

> Although I think you'd want a 'no hex' flag to suppress the hex.
> 
> Probably more useful flags are ones to suppress the address column.

This is already supported by the prefix_type parameter - are you proposing that we eliminate the parameter & combine it with flags?

> I've also used flags to enable (or disable) suppression of multiple lines of
> zeros of constant bytes.
> In that case you may want hexdump to return the flags for the next call when
> a large buffer is being dumped in fragments.
 
I'm afraid I don't quite follow here, hex_dump_to_buffer doesn't alter the flags, so the caller already knows it. 

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva     msn: alastair@d-silva.org
blog: http://alastair.d-silva.org    Twitter: @EvilDeece

RE: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line

[David Laight]

From: Alastair D'Silva
> Sent: 15 April 2019 11:29
...
> I do, and I believe the choice of the output length should be in the hands
> of the caller.
> 
> On further thought, it would make more sense to remove the hardcoded list of
> sizes and just enforce a power of 2. The function shouldn't dictate what the
> caller can and can't do beyond the technical limits of it's implementation.

Why powers of two?
You may want the length to match sizeof (struct foo).
You might even want the address increment to be larger
that the number of lines dumped.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line

[Alastair D'Silva]

> From: Alastair D'Silva
> > Sent: 15 April 2019 11:29
> ...
> > I do, and I believe the choice of the output length should be in the
> > hands of the caller.
> >
> > On further thought, it would make more sense to remove the hardcoded
> > list of sizes and just enforce a power of 2. The function shouldn't
> > dictate what the caller can and can't do beyond the technical limits of it's
> implementation.
> 
> Why powers of two?
> You may want the length to match sizeof (struct foo).
> You might even want the address increment to be larger that the number of
> lines dumped.

Good point, the base requirement is that it should be a multiple of groupsize.

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva     msn: alastair@d-silva.org
blog: http://alastair.d-silva.org    Twitter: @EvilDeece

RE: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags

[David Laight]

From: Alastair D'Silva
> Sent: 15 April 2019 11:45
...
> > Although I think you'd want a 'no hex' flag to suppress the hex.
> >
> > Probably more useful flags are ones to suppress the address column.
> 
> This is already supported by the prefix_type parameter - are you proposing that we eliminate the
> parameter & combine it with flags?

I was looking at the flags on one of my hexdump() functions...

> > I've also used flags to enable (or disable) suppression of multiple lines of
> > zeros of constant bytes.
> > In that case you may want hexdump to return the flags for the next call when
> > a large buffer is being dumped in fragments.
> 
> I'm afraid I don't quite follow here, hex_dump_to_buffer doesn't alter the flags,
> so the caller already knows it.

If you are suppressing lines of zeros and dumping a buffer in several blocks
then subsequent calls need to know that the last line of the previous call
was suppressed zeros - and carry on with the same suppressed block.

I've not looked to see if it does support suppressing lines of zeros/0xff.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags

[Alastair D'Silva]

> -----Original Message-----
> From: David Laight <David.Laight@ACULAB.COM>
> Sent: Monday, 15 April 2019 9:04 PM
> To: 'Alastair D'Silva' <alastair@d-silva.org>; 'Petr Mladek'
> <pmladek@suse.com>
> Cc: 'Alastair D'Silva' <alastair@au1.ibm.com>; 'Jani Nikula'
> <jani.nikula@linux.intel.com>; 'Joonas Lahtinen'
> <joonas.lahtinen@linux.intel.com>; 'Rodrigo Vivi' <rodrigo.vivi@intel.com>;
> 'David Airlie' <airlied@linux.ie>; 'Daniel Vetter' <daniel@ffwll.ch>; 'Karsten
> Keil' <isdn@linux-pingi.de>; 'Jassi Brar' <jassisinghbrar@gmail.com>; 'Tom
> Lendacky' <thomas.lendacky@amd.com>; 'David S. Miller'
> <davem@davemloft.net>; 'Jose Abreu' <Jose.Abreu@synopsys.com>; 'Kalle
> Valo' <kvalo@codeaurora.org>; 'Stanislaw Gruszka' <sgruszka@redhat.com>;
> 'Benson Leung' <bleung@chromium.org>; 'Enric Balletbo i Serra'
> <enric.balletbo@collabora.com>; 'James E.J. Bottomley'
> <jejb@linux.ibm.com>; 'Martin K. Petersen' <martin.petersen@oracle.com>;
> 'Greg Kroah-Hartman' <gregkh@linuxfoundation.org>; 'Alexander Viro'
> <viro@zeniv.linux.org.uk>; 'Sergey Senozhatsky'
> <sergey.senozhatsky@gmail.com>; 'Steven Rostedt'
> <rostedt@goodmis.org>; 'Andrew Morton' <akpm@linux-foundation.org>;
> intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org;
> ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux-
> scsi@vger.kernel.org; linux-fbdev@vger.kernel.org;
> devel@driverdev.osuosl.org; linux-fsdevel@vger.kernel.org
> Subject: RE: [PATCH 3/4] lib/hexdump.c: Replace ascii bool in
> hex_dump_to_buffer with flags
> 
> From: Alastair D'Silva
> > Sent: 15 April 2019 11:45
> ...
> > > Although I think you'd want a 'no hex' flag to suppress the hex.
> > >
> > > Probably more useful flags are ones to suppress the address column.
> >
> > This is already supported by the prefix_type parameter - are you
> > proposing that we eliminate the parameter & combine it with flags?
> 
> I was looking at the flags on one of my hexdump() functions...
> 
> > > I've also used flags to enable (or disable) suppression of multiple
> > > lines of zeros of constant bytes.
> > > In that case you may want hexdump to return the flags for the next
> > > call when a large buffer is being dumped in fragments.
> >
> > I'm afraid I don't quite follow here, hex_dump_to_buffer doesn't alter
> > the flags, so the caller already knows it.
> 
> If you are suppressing lines of zeros and dumping a buffer in several blocks
> then subsequent calls need to know that the last line of the previous call was
> suppressed zeros - and carry on with the same suppressed block.

Why wouldn't you do this with a single call to print_hex_dump? (that is where the repeated lines are suppressed)

That will already take chunks of the buffer until the whole thing is output, in what situation do you see a caller chunking the access themselves?

-- 
Alastair D'Silva           mob: 0423 762 819
skype: alastair_dsilva     msn: alastair@d-silva.org
blog: http://alastair.d-silva.org    Twitter: @EvilDeece