All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] seq_buf: Add printing formatted hex dumps
@ 2019-11-06  6:27 Piotr Maziarz
  2019-11-06  6:27 ` [PATCH 2/2] tracing: Use seq_buf_hex_dump() to dump buffers Piotr Maziarz
  2019-11-06  8:53 ` [PATCH 1/2] seq_buf: Add printing formatted hex dumps Steven Rostedt
  0 siblings, 2 replies; 6+ messages in thread
From: Piotr Maziarz @ 2019-11-06  6:27 UTC (permalink / raw)
  To: linux-kernel
  Cc: rostedt, mingo, andriy.shevchenko, cezary.rojewski,
	gustaw.lewandowski, Piotr Maziarz

Provided function is an analogue of print_hex_dump().

Implementing this function in seq_buf allows using for multiple
purposes (e.g. for tracing) and therefore prevents from code duplication
in every layer that uses seq_buf.

print_hex_dump() is an essential part of logging data to dmesg. Adding
similar capability for other purposes is beneficial to all users.

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/linux/seq_buf.h |  3 +++
 lib/seq_buf.c           | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
index aa5deb0..fb0205d 100644
--- a/include/linux/seq_buf.h
+++ b/include/linux/seq_buf.h
@@ -125,6 +125,9 @@ extern int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len);
 extern int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
 			      unsigned int len);
 extern int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc);
+extern int seq_buf_hex_dump(struct seq_buf *s, const char *prefix_str,
+			    int prefix_type, int rowsize, int groupsize,
+			    const void *buf, size_t len, bool ascii);
 
 #ifdef CONFIG_BINARY_PRINTF
 extern int
diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index bd807f5..0509706 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -328,3 +328,41 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)
 	s->readpos += cnt;
 	return cnt;
 }
+
+int seq_buf_hex_dump(struct seq_buf *s, const char *prefix_str, int prefix_type,
+		     int rowsize, int groupsize,
+		     const void *buf, size_t len, bool ascii)
+{
+	const u8 *ptr = buf;
+	int i, linelen, remaining = len;
+	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
+	int ret;
+
+	if (rowsize != 16 && rowsize != 32)
+		rowsize = 16;
+
+	for (i = 0; i < len; i += rowsize) {
+		linelen = min(remaining, rowsize);
+		remaining -= rowsize;
+
+		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
+				   linebuf, sizeof(linebuf), ascii);
+
+		switch (prefix_type) {
+		case DUMP_PREFIX_ADDRESS:
+			ret = seq_buf_printf(s, "%s%p: %s\n",
+			       prefix_str, ptr + i, linebuf);
+			break;
+		case DUMP_PREFIX_OFFSET:
+			ret = seq_buf_printf(s, "%s%.8x: %s\n",
+					     prefix_str, i, linebuf);
+			break;
+		default:
+			ret = seq_buf_printf(s, "%s%s\n", prefix_str, linebuf);
+			break;
+		}
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] tracing: Use seq_buf_hex_dump() to dump buffers
  2019-11-06  6:27 [PATCH 1/2] seq_buf: Add printing formatted hex dumps Piotr Maziarz
@ 2019-11-06  6:27 ` Piotr Maziarz
  2019-11-06  8:55   ` Steven Rostedt
  2019-11-06  8:53 ` [PATCH 1/2] seq_buf: Add printing formatted hex dumps Steven Rostedt
  1 sibling, 1 reply; 6+ messages in thread
From: Piotr Maziarz @ 2019-11-06  6:27 UTC (permalink / raw)
  To: linux-kernel
  Cc: rostedt, mingo, andriy.shevchenko, cezary.rojewski,
	gustaw.lewandowski, Piotr Maziarz

Without this, buffers can be printed with __print_array macro that has
no formatting options and can be hard to read. The other way is to
mimic formatting capability with multiple calls of trace event with one
call per row which gives performance impact and different timestamp in
each row.

Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/linux/trace_events.h |  5 +++++
 include/linux/trace_seq.h    |  4 ++++
 include/trace/trace_events.h |  6 ++++++
 kernel/trace/trace_output.c  | 15 +++++++++++++++
 kernel/trace/trace_seq.c     | 30 ++++++++++++++++++++++++++++++
 5 files changed, 60 insertions(+)

diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 30a8cdc..60a41b7 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -45,6 +45,11 @@ const char *trace_print_array_seq(struct trace_seq *p,
 				   const void *buf, int count,
 				   size_t el_size);
 
+const char *
+trace_print_hex_dump_seq(struct trace_seq *p, const char *prefix_str,
+			 int prefix_type, int rowsize, int groupsize,
+			 const void *buf, size_t len, bool ascii);
+
 struct trace_iterator;
 struct trace_event;
 
diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h
index 6609b39a..6c30508 100644
--- a/include/linux/trace_seq.h
+++ b/include/linux/trace_seq.h
@@ -92,6 +92,10 @@ extern int trace_seq_path(struct trace_seq *s, const struct path *path);
 extern void trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
 			     int nmaskbits);
 
+extern int trace_seq_hex_dump(struct trace_seq *s, const char *prefix_str,
+			      int prefix_type, int rowsize, int groupsize,
+			      const void *buf, size_t len, bool ascii);
+
 #else /* CONFIG_TRACING */
 static inline void trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
 {
diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
index 4ecdfe2..7089760 100644
--- a/include/trace/trace_events.h
+++ b/include/trace/trace_events.h
@@ -340,6 +340,12 @@ TRACE_MAKE_SYSTEM_STR();
 		trace_print_array_seq(p, array, count, el_size);	\
 	})
 
+#undef __print_hex_dump
+#define __print_hex_dump(prefix_str, prefix_type,			\
+			 rowsize, groupsize, buf, len, ascii)		\
+	trace_print_hex_dump_seq(p, prefix_str, prefix_type,		\
+				 rowsize, groupsize, buf, len, ascii)
+
 #undef DECLARE_EVENT_CLASS
 #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print)	\
 static notrace enum print_line_t					\
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index d54ce25..d9b4b7c 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -274,6 +274,21 @@ trace_print_array_seq(struct trace_seq *p, const void *buf, int count,
 }
 EXPORT_SYMBOL(trace_print_array_seq);
 
+const char *
+trace_print_hex_dump_seq(struct trace_seq *p, const char *prefix_str,
+			 int prefix_type, int rowsize, int groupsize,
+			 const void *buf, size_t len, bool ascii)
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+
+	trace_seq_putc(p, '\n');
+	trace_seq_hex_dump(p, prefix_str, prefix_type,
+			   rowsize, groupsize, buf, len, ascii);
+	trace_seq_putc(p, 0);
+	return ret;
+}
+EXPORT_SYMBOL(trace_print_hex_dump_seq);
+
 int trace_raw_output_prep(struct trace_iterator *iter,
 			  struct trace_event *trace_event)
 {
diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c
index 6b1c562..344e4c1 100644
--- a/kernel/trace/trace_seq.c
+++ b/kernel/trace/trace_seq.c
@@ -376,3 +376,33 @@ int trace_seq_to_user(struct trace_seq *s, char __user *ubuf, int cnt)
 	return seq_buf_to_user(&s->seq, ubuf, cnt);
 }
 EXPORT_SYMBOL_GPL(trace_seq_to_user);
+
+int trace_seq_hex_dump(struct trace_seq *s, const char *prefix_str,
+		       int prefix_type, int rowsize, int groupsize,
+		       const void *buf, size_t len, bool ascii)
+{
+		unsigned int save_len = s->seq.len;
+
+	if (s->full)
+		return 0;
+
+	__trace_seq_init(s);
+
+	if (TRACE_SEQ_BUF_LEFT(s) < 1) {
+		s->full = 1;
+		return 0;
+	}
+
+	seq_buf_hex_dump(&(s->seq), prefix_str,
+		   prefix_type, rowsize, groupsize,
+		   buf, len, ascii);
+
+	if (unlikely(seq_buf_has_overflowed(&s->seq))) {
+		s->seq.len = save_len;
+		s->full = 1;
+		return 0;
+	}
+
+	return 1;
+}
+EXPORT_SYMBOL(trace_seq_hex_dump);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] seq_buf: Add printing formatted hex dumps
  2019-11-06  6:27 [PATCH 1/2] seq_buf: Add printing formatted hex dumps Piotr Maziarz
  2019-11-06  6:27 ` [PATCH 2/2] tracing: Use seq_buf_hex_dump() to dump buffers Piotr Maziarz
@ 2019-11-06  8:53 ` Steven Rostedt
  2019-11-06  9:34   ` Andy Shevchenko
  1 sibling, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2019-11-06  8:53 UTC (permalink / raw)
  To: Piotr Maziarz
  Cc: linux-kernel, mingo, andriy.shevchenko, cezary.rojewski,
	gustaw.lewandowski

On Wed,  6 Nov 2019 07:27:39 +0100
Piotr Maziarz <piotrx.maziarz@linux.intel.com> wrote:

> Provided function is an analogue of print_hex_dump().
> 
> Implementing this function in seq_buf allows using for multiple
> purposes (e.g. for tracing) and therefore prevents from code duplication
> in every layer that uses seq_buf.
> 
> print_hex_dump() is an essential part of logging data to dmesg. Adding
> similar capability for other purposes is beneficial to all users.

Can you add to the change log an example output of print_hex_dump().

It makes it easier for reviewers to know if what is implemented matches
what is expected.

> 
> Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/linux/seq_buf.h |  3 +++
>  lib/seq_buf.c           | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> index aa5deb0..fb0205d 100644
> --- a/include/linux/seq_buf.h
> +++ b/include/linux/seq_buf.h
> @@ -125,6 +125,9 @@ extern int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len);
>  extern int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
>  			      unsigned int len);
>  extern int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc);
> +extern int seq_buf_hex_dump(struct seq_buf *s, const char *prefix_str,
> +			    int prefix_type, int rowsize, int groupsize,
> +			    const void *buf, size_t len, bool ascii);
>  
>  #ifdef CONFIG_BINARY_PRINTF
>  extern int
> diff --git a/lib/seq_buf.c b/lib/seq_buf.c
> index bd807f5..0509706 100644
> --- a/lib/seq_buf.c
> +++ b/lib/seq_buf.c
> @@ -328,3 +328,41 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)
>  	s->readpos += cnt;
>  	return cnt;
>  }
> +

Requires a kernel doc header here.

> +int seq_buf_hex_dump(struct seq_buf *s, const char *prefix_str, int prefix_type,
> +		     int rowsize, int groupsize,
> +		     const void *buf, size_t len, bool ascii)
> +{
> +	const u8 *ptr = buf;
> +	int i, linelen, remaining = len;
> +	unsigned char linebuf[32 * 3 + 2 + 32 + 1];

What do the above magic numbers mean? Should have a comment explaining
why you picked those numbers and created the length this way.

Also the preferred method of declarations is to order it by longest
first. That is, linebuf, followed by the ints, followed by ptr.


> +	int ret;
> +
> +	if (rowsize != 16 && rowsize != 32)
> +		rowsize = 16;
> +
> +	for (i = 0; i < len; i += rowsize) {
> +		linelen = min(remaining, rowsize);
> +		remaining -= rowsize;

Probably should make the above:

		remaining -= linelen;

Yeah, what you have works, but it makes a reviewer worry about using
remaining later and having it negative.

> +
> +		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> +				   linebuf, sizeof(linebuf), ascii);
> +
> +		switch (prefix_type) {
> +		case DUMP_PREFIX_ADDRESS:

I'm curious to know what uses the above type? By default, today,
pointers are pretty much obfuscated, and that will show up here too.

-- Steve

> +			ret = seq_buf_printf(s, "%s%p: %s\n",
> +			       prefix_str, ptr + i, linebuf);
> +			break;
> +		case DUMP_PREFIX_OFFSET:
> +			ret = seq_buf_printf(s, "%s%.8x: %s\n",
> +					     prefix_str, i, linebuf);
> +			break;
> +		default:
> +			ret = seq_buf_printf(s, "%s%s\n", prefix_str, linebuf);
> +			break;
> +		}
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] tracing: Use seq_buf_hex_dump() to dump buffers
  2019-11-06  6:27 ` [PATCH 2/2] tracing: Use seq_buf_hex_dump() to dump buffers Piotr Maziarz
@ 2019-11-06  8:55   ` Steven Rostedt
  2019-11-07 12:18     ` Piotr Maziarz
  0 siblings, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2019-11-06  8:55 UTC (permalink / raw)
  To: Piotr Maziarz
  Cc: linux-kernel, mingo, andriy.shevchenko, cezary.rojewski,
	gustaw.lewandowski

On Wed,  6 Nov 2019 07:27:40 +0100
Piotr Maziarz <piotrx.maziarz@linux.intel.com> wrote:

> Without this, buffers can be printed with __print_array macro that has
> no formatting options and can be hard to read. The other way is to
> mimic formatting capability with multiple calls of trace event with one
> call per row which gives performance impact and different timestamp in
> each row.
> 
> Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/linux/trace_events.h |  5 +++++
>  include/linux/trace_seq.h    |  4 ++++
>  include/trace/trace_events.h |  6 ++++++
>  kernel/trace/trace_output.c  | 15 +++++++++++++++
>  kernel/trace/trace_seq.c     | 30 ++++++++++++++++++++++++++++++
>  5 files changed, 60 insertions(+)
> 

I'd like to see in the patch series (patch 3?) a use case of these
added functionality. Or at least a link to what would be using it.

Thanks!

-- Steve

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] seq_buf: Add printing formatted hex dumps
  2019-11-06  8:53 ` [PATCH 1/2] seq_buf: Add printing formatted hex dumps Steven Rostedt
@ 2019-11-06  9:34   ` Andy Shevchenko
  0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2019-11-06  9:34 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Piotr Maziarz, linux-kernel, mingo, cezary.rojewski, gustaw.lewandowski

On Wed, Nov 06, 2019 at 03:53:17AM -0500, Steven Rostedt wrote:
> On Wed,  6 Nov 2019 07:27:39 +0100
> Piotr Maziarz <piotrx.maziarz@linux.intel.com> wrote:

> > +	for (i = 0; i < len; i += rowsize) {
> > +		linelen = min(remaining, rowsize);
> > +		remaining -= rowsize;
> 
> Probably should make the above:
> 
> 		remaining -= linelen;
> 
> Yeah, what you have works, but it makes a reviewer worry about using
> remaining later and having it negative.

OTOH, the original function and followers (like seq_hex_dump() one) are using
exactly above form. Maybe for the sake of consistency we may do the same and
then fix all at once. Or other way around, amend the rest first.

> > +		case DUMP_PREFIX_ADDRESS:
> 
> I'm curious to know what uses the above type? By default, today,
> pointers are pretty much obfuscated, and that will show up here too.

Good question. Current users are:

arch/microblaze/kernel/traps.c
arch/x86/kernel/mpparse.c
drivers/crypto/axis/artpec6_crypto.c
drivers/crypto/caam/...
drivers/crypto/ccree/cc_driver.c
drivers/crypto/qat/qat_common/adf_transport_debug.c
drivers/dma/xgene-dma.c
drivers/mailbox/mailbox-test.c
drivers/net/can/usb/ucan.c
drivers/net/ethernet/cadence/macb_main.c
drivers/net/ethernet/cavium/liquidio/octeon_droq.c
drivers/net/ethernet/intel/...
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
drivers/usb/gadget/function/f_ncm.c
fs/ext4/super.c
fs/jfs/...
mm/page_poison.c
mm/slub.c

Not many.

My understanding that it's still useful in conjunction with some other messages
where pointers are printed and developer, who is reading the logs, may match
them and do some conclusions.

> > +	}

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] tracing: Use seq_buf_hex_dump() to dump buffers
  2019-11-06  8:55   ` Steven Rostedt
@ 2019-11-07 12:18     ` Piotr Maziarz
  0 siblings, 0 replies; 6+ messages in thread
From: Piotr Maziarz @ 2019-11-07 12:18 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-kernel, mingo, andriy.shevchenko, cezary.rojewski,
	gustaw.lewandowski

On 2019-11-06 9:55, Steven Rostedt wrote:
> On Wed,  6 Nov 2019 07:27:40 +0100
> Piotr Maziarz <piotrx.maziarz@linux.intel.com> wrote:
> 
>> Without this, buffers can be printed with __print_array macro that has
>> no formatting options and can be hard to read. The other way is to
>> mimic formatting capability with multiple calls of trace event with one
>> call per row which gives performance impact and different timestamp in
>> each row.
>>
>> Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   include/linux/trace_events.h |  5 +++++
>>   include/linux/trace_seq.h    |  4 ++++
>>   include/trace/trace_events.h |  6 ++++++
>>   kernel/trace/trace_output.c  | 15 +++++++++++++++
>>   kernel/trace/trace_seq.c     | 30 ++++++++++++++++++++++++++++++
>>   5 files changed, 60 insertions(+)
>>
> 
> I'd like to see in the patch series (patch 3?) a use case of these
> added functionality. Or at least a link to what would be using it.
> 
> Thanks!
> 
> -- Steve
> 

ASoC: Intel is an initial recipient for this feature. I have a patch for 
this, but it should be also sent to alsa-devel mailing list, and since 
hex_dump tracing isn't there yet I'm not sure how this patch series 
should be sent.

Best regards,
Piotr Maziarz

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-07 12:18 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06  6:27 [PATCH 1/2] seq_buf: Add printing formatted hex dumps Piotr Maziarz
2019-11-06  6:27 ` [PATCH 2/2] tracing: Use seq_buf_hex_dump() to dump buffers Piotr Maziarz
2019-11-06  8:55   ` Steven Rostedt
2019-11-07 12:18     ` Piotr Maziarz
2019-11-06  8:53 ` [PATCH 1/2] seq_buf: Add printing formatted hex dumps Steven Rostedt
2019-11-06  9:34   ` Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.