[PATCH v2 0/3] Use new for_each macro to create hexdumps

[PATCH v2 0/3] Use new for_each macro to create hexdumps

[Nick Child]

Apologies, not sure what mailing list/tree to target. First 2 patches look
like *-next and last patch should go to net-next.

Currently, obtaining a hexdump can be done through one of the following:
 1. hex_dump_to_buffer - takes at most 32 bytes of a buffer and returns a 
     hexdump string representation
 2. print_hex_dump - prints output of hex_dump_to_buffer iteratively over
    a large buffer

There is no functionality for iterating over a large buffer and receiving
the string representation. It seems most users of hex_dump_to_buffer are
calling hex_dump_to_buffer within the body of a loop which iterates
through a buffer.

This patchset creates a for_each macro that accepts a buffer and fills
out an output string with the converted hexdump. This loops over the
buffer and takes care of incrementing pointers. Hopefully this makes
writing sequential calls to hex_dump_to_buffer more straightforward.

From a users perspective there should be no difference in output.

The inspiration here was I wanted to use print_hex_dump in ibmvnic code
but I wanted to print through netdevice printing functions to maintain
formatting. Looking at other users of hex_dump_to_buffer it seems they had
similar intentions.

Side question:
  hex_dump_to_buffer automatically sets groupsize to 1 if user given
  groupsize is not a multiple of len. When printing large buffers this
  makes non-uniform output. For example, this is a 31 byte 8 groupsize
  buffer:
   ibmvnic 30000003 env3: 6c6774732e737561 2e6d62692e736261
   ibmvnic 30000003 env3: 63 6f 6d 00 03 00 05 65 6e 76 33 00 00 00 00
  Since the second line is only 15 bytes, the group size is set to 1. I
  have written a patch which keeps groupsize so output would be:
   ibmvnic 30000003 env3: 6c6774732e737561 2e6d62692e736261
   ibmvnic 30000003 env3: 636f6d0003000565 6e763300000000
  But since I am not sure if this would break some dependency for someone,
  and my justification for change is purely pedantic, I chose to omit
  that patch in this patchset. Let me know if there is any interest and
  I will send a different patchset for that. 

Thanks for your consideration/review. And thanks to Simon and Jacob for v1
review!

Changes since v1:
 - add Jacob's Reviewed-by
 - fix kernel doc typo in patch 1 noted by Simon

v1: https://lore.kernel.org/lkml/20250113221721.362093-1-nnac123@linux.ibm.com/

Nick Child (3):
  hexdump: Implement macro for converting large buffers
  hexdump: Use for_each macro in print_hex_dump
  ibmvnic: Print data buffers with kernel API's

 drivers/net/ethernet/ibm/ibmvnic.c | 23 ++++++++++++++---------
 include/linux/printk.h             | 21 +++++++++++++++++++++
 lib/hexdump.c                      | 11 +++--------
 3 files changed, 38 insertions(+), 17 deletions(-)

-- 
2.48.0

[PATCH 1/3] hexdump: Implement macro for converting large buffers

[Nick Child]

Define for_each_line_in_hex_dump which loops over a buffer and calls
hex_dump_to_buffer for each segment in the buffer. This allows the
caller to decide what to do with the resulting string and is not
limited by a specific printing format like print_hex_dump.

Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---
 include/linux/printk.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 4217a9f412b2..559d4bfe0645 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -755,6 +755,27 @@ enum {
 extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
 			      int groupsize, char *linebuf, size_t linebuflen,
 			      bool ascii);
+/**
+ * for_each_line_in_hex_dump - iterate over buffer, converting into hex ASCII
+ * @i: offset in @buff
+ * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @linebuf: where to put the converted data
+ * @linebuflen: total size of @linebuf, including space for terminating NUL
+ *		IOW >= (@rowsize * 2) + ((@rowsize - 1 / @groupsize)) + 1
+ * @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
+ */
+ #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
+				   buf, len) \
+	for ((i) = 0;							\
+	     (i) < (len) &&						\
+	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
+				min((len) - (i), rowsize),		\
+				(rowsize), (groupsize), (linebuf),	\
+				(linebuflen), false);			\
+	     (i) += (rowsize) == 16 || (rowsize) == 32 ? (rowsize) : 16	\
+	    )
 #ifdef CONFIG_PRINTK
 extern void print_hex_dump(const char *level, const char *prefix_str,
 			   int prefix_type, int rowsize, int groupsize,
-- 
2.48.0

Re: [PATCH 1/3] hexdump: Implement macro for converting large buffers

[Nick Child]

Hi Dave,

Thanks for reviewing,

On 2/14/25 12:00 PM, Dave Marquardt wrote:
> Nick Child <nnac123@linux.ibm.com> writes:
>
>> +	     (i) += (rowsize) == 16 || (rowsize) == 32 ? (rowsize) : 16	\
> Nit: If you left out the (rowsize) == 16 check here you'd still add 16
> to (i).

I was trying to have this translate into "if invalid rowsize was used 
then default to 16" since

hex_dump_to_buffer has a very similar conditional. But I agree, 
logically it looks strange.

If I send a v3 (I also foolishly forgot the v2 tag in this patch), I 
will change this like to

+	     (i) += (rowsize) == 32 ? 32 : 16	\

Re: [PATCH 1/3] hexdump: Implement macro for converting large buffers

[Simon Horman]

+ David Laight

On Fri, Feb 14, 2025 at 10:24:34AM -0600, Nick Child wrote:
> Define for_each_line_in_hex_dump which loops over a buffer and calls
> hex_dump_to_buffer for each segment in the buffer. This allows the
> caller to decide what to do with the resulting string and is not
> limited by a specific printing format like print_hex_dump.
> 
> Signed-off-by: Nick Child <nnac123@linux.ibm.com>
> ---
>  include/linux/printk.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 4217a9f412b2..559d4bfe0645 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -755,6 +755,27 @@ enum {
>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>  			      int groupsize, char *linebuf, size_t linebuflen,
>  			      bool ascii);
> +/**
> + * for_each_line_in_hex_dump - iterate over buffer, converting into hex ASCII
> + * @i: offset in @buff
> + * @rowsize: number of bytes to print per line; must be 16 or 32
> + * @linebuf: where to put the converted data
> + * @linebuflen: total size of @linebuf, including space for terminating NUL
> + *		IOW >= (@rowsize * 2) + ((@rowsize - 1 / @groupsize)) + 1
> + * @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
> + */
> + #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
> +				   buf, len) \
> +	for ((i) = 0;							\
> +	     (i) < (len) &&						\
> +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> +				min((len) - (i), rowsize),		\
> +				(rowsize), (groupsize), (linebuf),	\
> +				(linebuflen), false);			\
> +	     (i) += (rowsize) == 16 || (rowsize) == 32 ? (rowsize) : 16	\
> +	    )
>  #ifdef CONFIG_PRINTK
>  extern void print_hex_dump(const char *level, const char *prefix_str,
>  			   int prefix_type, int rowsize, int groupsize,

Hi Nick,

When compiling with gcc 7.5.0 (old, but still supported AFAIK) on x86_64
with patch 2/3 (and 1/3) applied I see this:

  CC      lib/hexdump.o
In file included from <command-line>:0:0:
lib/hexdump.c: In function 'print_hex_dump':
././include/linux/compiler_types.h:542:38: error: call to '__compiletime_assert_11' declared with attribute error: min((len) - (i), rowsize) signedness error
  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
                                      ^
././include/linux/compiler_types.h:523:4: note: in definition of macro '__compiletime_assert'
    prefix ## suffix();    \
    ^~~~~~
././include/linux/compiler_types.h:542:2: note: in expansion of macro '_compiletime_assert'
  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
  ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
 #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                     ^~~~~~~~~~~~~~~~~~
./include/linux/minmax.h:93:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
  BUILD_BUG_ON_MSG(!__types_ok(ux, uy),  \
  ^~~~~~~~~~~~~~~~
./include/linux/minmax.h:98:2: note: in expansion of macro '__careful_cmp_once'
  __careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
  ^~~~~~~~~~~~~~~~~~
./include/linux/minmax.h:105:19: note: in expansion of macro '__careful_cmp'
 #define min(x, y) __careful_cmp(min, x, y)
                   ^~~~~~~~~~~~~
./include/linux/printk.h:774:5: note: in expansion of macro 'min'
     min((len) - (i), rowsize),  \
     ^~~
lib/hexdump.c:272:2: note: in expansion of macro 'for_each_line_in_hex_dump'
  for_each_line_in_hex_dump(i, rowsize, linebuf, sizeof(linebuf),
  ^~~~~~~~~~~~~~~~~~~~~~~~~

Highlighting the min line in the macro for context, it looks like this:

	min((len) - (i), rowsize)

And in this case the types involved are:

	size_t len
	int i
	int rowsize

This is not a proposal, but I made a quick hack changing they type of rowsize
to size_t and the problem goes away. So I guess it is the type missmatch
between the two arguments to min that needs to be resolved somehow.


FWIIW, you should be able to reproduce this problem fairly easily using
the toolchain here:
https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/7.5.0/

Re: [PATCH 1/3] hexdump: Implement macro for converting large buffers

[David Laight]

On Sat, 15 Feb 2025 16:36:12 +0000
Simon Horman <horms@kernel.org> wrote:

> + David Laight
> 
> On Fri, Feb 14, 2025 at 10:24:34AM -0600, Nick Child wrote:
> > Define for_each_line_in_hex_dump which loops over a buffer and calls
> > hex_dump_to_buffer for each segment in the buffer. This allows the
> > caller to decide what to do with the resulting string and is not
> > limited by a specific printing format like print_hex_dump.
> > 
> > Signed-off-by: Nick Child <nnac123@linux.ibm.com>
> > ---
> >  include/linux/printk.h | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > index 4217a9f412b2..559d4bfe0645 100644
> > --- a/include/linux/printk.h
> > +++ b/include/linux/printk.h
> > @@ -755,6 +755,27 @@ enum {
> >  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> >  			      int groupsize, char *linebuf, size_t linebuflen,
> >  			      bool ascii);
> > +/**
> > + * for_each_line_in_hex_dump - iterate over buffer, converting into hex ASCII
> > + * @i: offset in @buff
> > + * @rowsize: number of bytes to print per line; must be 16 or 32
> > + * @linebuf: where to put the converted data
> > + * @linebuflen: total size of @linebuf, including space for terminating NUL
> > + *		IOW >= (@rowsize * 2) + ((@rowsize - 1 / @groupsize)) + 1
> > + * @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
> > + */
> > + #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
> > +				   buf, len) \
> > +	for ((i) = 0;							\
> > +	     (i) < (len) &&						\
> > +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> > +				min((len) - (i), rowsize),		\
> > +				(rowsize), (groupsize), (linebuf),	\
> > +				(linebuflen), false);			\
> > +	     (i) += (rowsize) == 16 || (rowsize) == 32 ? (rowsize) : 16	\

WTF?
I'm not sure why there is a restriction on 'rowsize' by that increment
makes no sense at all.

> > +	    )
> >  #ifdef CONFIG_PRINTK
> >  extern void print_hex_dump(const char *level, const char *prefix_str,
> >  			   int prefix_type, int rowsize, int groupsize,  
> 
> Hi Nick,
> 
> When compiling with gcc 7.5.0 (old, but still supported AFAIK) on x86_64
> with patch 2/3 (and 1/3) applied I see this:
> 
>   CC      lib/hexdump.o
> In file included from <command-line>:0:0:
> lib/hexdump.c: In function 'print_hex_dump':
> ././include/linux/compiler_types.h:542:38: error: call to '__compiletime_assert_11' declared with attribute error: min((len) - (i), rowsize) signedness error
...
> Highlighting the min line in the macro for context, it looks like this:
> 
> 	min((len) - (i), rowsize)
> 
> And in this case the types involved are:
> 
> 	size_t len
> 	int i
> 	int rowsize

Yep, that should fail for all versions of gcc.
Both 'i' and 'rowsize' should be unsigned types.
In fact all three can be 'unsigned int'.

	David

> 
> This is not a proposal, but I made a quick hack changing they type of rowsize
> to size_t and the problem goes away. So I guess it is the type missmatch
> between the two arguments to min that needs to be resolved somehow.
> 
> 
> FWIIW, you should be able to reproduce this problem fairly easily using
> the toolchain here:
> https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/7.5.0/

Re: [PATCH 1/3] hexdump: Implement macro for converting large buffers

[David Laight]

On Sat, 15 Feb 2025 17:40:39 +0000
David Laight <david.laight.linux@gmail.com> wrote:

> On Sat, 15 Feb 2025 16:36:12 +0000
> Simon Horman <horms@kernel.org> wrote:
> 
> > + David Laight
> > 
> > On Fri, Feb 14, 2025 at 10:24:34AM -0600, Nick Child wrote:  
> > > Define for_each_line_in_hex_dump which loops over a buffer and calls
> > > hex_dump_to_buffer for each segment in the buffer. This allows the
> > > caller to decide what to do with the resulting string and is not
> > > limited by a specific printing format like print_hex_dump.
> > > 
> > > Signed-off-by: Nick Child <nnac123@linux.ibm.com>
> > > ---
> > >  include/linux/printk.h | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > > index 4217a9f412b2..559d4bfe0645 100644
> > > --- a/include/linux/printk.h
> > > +++ b/include/linux/printk.h
> > > @@ -755,6 +755,27 @@ enum {
> > >  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> > >  			      int groupsize, char *linebuf, size_t linebuflen,
> > >  			      bool ascii);
> > > +/**
> > > + * for_each_line_in_hex_dump - iterate over buffer, converting into hex ASCII
> > > + * @i: offset in @buff
> > > + * @rowsize: number of bytes to print per line; must be 16 or 32
> > > + * @linebuf: where to put the converted data
> > > + * @linebuflen: total size of @linebuf, including space for terminating NUL
> > > + *		IOW >= (@rowsize * 2) + ((@rowsize - 1 / @groupsize)) + 1
> > > + * @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
> > > + */
> > > + #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
> > > +				   buf, len) \
> > > +	for ((i) = 0;							\
> > > +	     (i) < (len) &&						\
> > > +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> > > +				min((len) - (i), rowsize),		\
> > > +				(rowsize), (groupsize), (linebuf),	\
> > > +				(linebuflen), false);			\
> > > +	     (i) += (rowsize) == 16 || (rowsize) == 32 ? (rowsize) : 16	\  
> > > +	    )
> > >  #ifdef CONFIG_PRINTK
> > >  extern void print_hex_dump(const char *level, const char *prefix_str,
> > >  			   int prefix_type, int rowsize, int groupsize,    
> > 
> > Hi Nick,
> > 
> > When compiling with gcc 7.5.0 (old, but still supported AFAIK) on x86_64
> > with patch 2/3 (and 1/3) applied I see this:
> > 
> >   CC      lib/hexdump.o
> > In file included from <command-line>:0:0:
> > lib/hexdump.c: In function 'print_hex_dump':
> > ././include/linux/compiler_types.h:542:38: error: call to '__compiletime_assert_11' declared with attribute error: min((len) - (i), rowsize) signedness error  
> ...
> > Highlighting the min line in the macro for context, it looks like this:
> > 
> > 	min((len) - (i), rowsize)
> > 
> > And in this case the types involved are:
> > 
> > 	size_t len
> > 	int i
> > 	int rowsize  
> 
> Yep, that should fail for all versions of gcc.
> Both 'i' and 'rowsize' should be unsigned types.
> In fact all three can be 'unsigned int'.

Thinking a bit more.
If the compiler can determine that 'rowsize >= 0' then the test will pass.
More modern compilers do better value tracking so that might be enough
to stop the compiler 'bleating'.

	David

> 
> 	David
> 
> > 
> > This is not a proposal, but I made a quick hack changing they type of rowsize
> > to size_t and the problem goes away. So I guess it is the type missmatch
> > between the two arguments to min that needs to be resolved somehow.
> > 
> > 
> > FWIIW, you should be able to reproduce this problem fairly easily using
> > the toolchain here:
> > https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/7.5.0/  
> 

Re: [PATCH 1/3] hexdump: Implement macro for converting large buffers

[Simon Horman]

On Sat, Feb 15, 2025 at 05:46:35PM +0000, David Laight wrote:
> On Sat, 15 Feb 2025 17:40:39 +0000
> David Laight <david.laight.linux@gmail.com> wrote:
> 
> > On Sat, 15 Feb 2025 16:36:12 +0000
> > Simon Horman <horms@kernel.org> wrote:
> > 
> > > + David Laight
> > > 
> > > On Fri, Feb 14, 2025 at 10:24:34AM -0600, Nick Child wrote:  
> > > > Define for_each_line_in_hex_dump which loops over a buffer and calls
> > > > hex_dump_to_buffer for each segment in the buffer. This allows the
> > > > caller to decide what to do with the resulting string and is not
> > > > limited by a specific printing format like print_hex_dump.
> > > > 
> > > > Signed-off-by: Nick Child <nnac123@linux.ibm.com>
> > > > ---
> > > >  include/linux/printk.h | 21 +++++++++++++++++++++
> > > >  1 file changed, 21 insertions(+)
> > > > 
> > > > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > > > index 4217a9f412b2..559d4bfe0645 100644
> > > > --- a/include/linux/printk.h
> > > > +++ b/include/linux/printk.h
> > > > @@ -755,6 +755,27 @@ enum {
> > > >  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> > > >  			      int groupsize, char *linebuf, size_t linebuflen,
> > > >  			      bool ascii);
> > > > +/**
> > > > + * for_each_line_in_hex_dump - iterate over buffer, converting into hex ASCII
> > > > + * @i: offset in @buff
> > > > + * @rowsize: number of bytes to print per line; must be 16 or 32
> > > > + * @linebuf: where to put the converted data
> > > > + * @linebuflen: total size of @linebuf, including space for terminating NUL
> > > > + *		IOW >= (@rowsize * 2) + ((@rowsize - 1 / @groupsize)) + 1
> > > > + * @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
> > > > + */
> > > > + #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
> > > > +				   buf, len) \
> > > > +	for ((i) = 0;							\
> > > > +	     (i) < (len) &&						\
> > > > +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> > > > +				min((len) - (i), rowsize),		\
> > > > +				(rowsize), (groupsize), (linebuf),	\
> > > > +				(linebuflen), false);			\
> > > > +	     (i) += (rowsize) == 16 || (rowsize) == 32 ? (rowsize) : 16	\  
> > > > +	    )
> > > >  #ifdef CONFIG_PRINTK
> > > >  extern void print_hex_dump(const char *level, const char *prefix_str,
> > > >  			   int prefix_type, int rowsize, int groupsize,    
> > > 
> > > Hi Nick,
> > > 
> > > When compiling with gcc 7.5.0 (old, but still supported AFAIK) on x86_64
> > > with patch 2/3 (and 1/3) applied I see this:
> > > 
> > >   CC      lib/hexdump.o
> > > In file included from <command-line>:0:0:
> > > lib/hexdump.c: In function 'print_hex_dump':
> > > ././include/linux/compiler_types.h:542:38: error: call to '__compiletime_assert_11' declared with attribute error: min((len) - (i), rowsize) signedness error  
> > ...
> > > Highlighting the min line in the macro for context, it looks like this:
> > > 
> > > 	min((len) - (i), rowsize)
> > > 
> > > And in this case the types involved are:
> > > 
> > > 	size_t len
> > > 	int i
> > > 	int rowsize  
> > 
> > Yep, that should fail for all versions of gcc.
> > Both 'i' and 'rowsize' should be unsigned types.
> > In fact all three can be 'unsigned int'.

To give a bit more context, a complication changing the types is that the
type of len and rowsise (but not i) is in the signature of the calling
function, print_hex_dump(). And I believe that function is widely used
throughout the tree.

> 
> Thinking a bit more.
> If the compiler can determine that 'rowsize >= 0' then the test will pass.
> More modern compilers do better value tracking so that might be enough
> to stop the compiler 'bleating'.

FTR, I did not see this with GCC 14.2.0 (or clang 19.1.7).

Re: [PATCH 1/3] hexdump: Implement macro for converting large buffers

[David Laight]

On Sun, 16 Feb 2025 09:32:04 +0000
Simon Horman <horms@kernel.org> wrote:

>...
> > > Yep, that should fail for all versions of gcc.
> > > Both 'i' and 'rowsize' should be unsigned types.
> > > In fact all three can be 'unsigned int'.  
> 
> To give a bit more context, a complication changing the types is that the
> type of len and rowsise (but not i) is in the signature of the calling
> function, print_hex_dump(). And I believe that function is widely used
> throughout the tree.

Doesn't matter, nothing with assign the address of the function to a
variable so changing the types (to unsigned) doesn't affect any callers.
The values better be positive!

I just changed the prototypes (include/linux/printk.h) to make both
rowsize and groupsize 'unsigned int'.
The same change in lib/hexdump.c + changing the local 'i, linelen, remaining'
to unsigned int and it all compiled.

FWIW that hexdump code is pretty horrid (especially if groupsize != 1).

	David

Re: [PATCH 1/3] hexdump: Implement macro for converting large buffers

[Nick Child]

Thank you David and Simon for testing and review!

On Sun, Feb 16, 2025 at 11:24:30AM +0000, David Laight wrote:
> 
> I just changed the prototypes (include/linux/printk.h) to make both
> rowsize and groupsize 'unsigned int'.
> The same change in lib/hexdump.c + changing the local 'i, linelen, remaining'
> to unsigned int and it all compiled.
> 
> FWIW that hexdump code is pretty horrid (especially if groupsize != 1).
> 

Given this discussion and my own head-scratching, I think I will take a
closer look at hex_dump_to_buffer.

I was trying to avoid editing this function due the number of callers it
has across the kernel. But I think we can get away with keeping the
API (but change args to uint's) and editing the body of the function
to always iterate byte-by-byte, addding space chars where necessary. At the
cost of a few more cycles, this will allow for more dynamic values
for rowsize and groupsize and shorten the code up a bit. This would also
address the "Side question" in my cover letter. Will send a v3
regardless if I can figure that out or not.

The return value of hex_dump_to_buffer on error still irks me a bit but
I don't think that can easily be changed.

Thanks again!

Re: [PATCH 1/3] hexdump: Implement macro for converting large buffers

[Paolo Abeni]

On 2/17/25 4:09 PM, Nick Child wrote:
> Thank you David and Simon for testing and review!
> 
> On Sun, Feb 16, 2025 at 11:24:30AM +0000, David Laight wrote:
>>
>> I just changed the prototypes (include/linux/printk.h) to make both
>> rowsize and groupsize 'unsigned int'.
>> The same change in lib/hexdump.c + changing the local 'i, linelen, remaining'
>> to unsigned int and it all compiled.
>>
>> FWIW that hexdump code is pretty horrid (especially if groupsize != 1).
>>
> 
> Given this discussion and my own head-scratching, I think I will take a
> closer look at hex_dump_to_buffer.
> 
> I was trying to avoid editing this function due the number of callers it
> has across the kernel. But I think we can get away with keeping the
> API (but change args to uint's) and editing the body of the function
> to always iterate byte-by-byte, addding space chars where necessary. At the
> cost of a few more cycles, this will allow for more dynamic values
> for rowsize and groupsize and shorten the code up a bit. This would also
> address the "Side question" in my cover letter. Will send a v3
> regardless if I can figure that out or not.
> 
> The return value of hex_dump_to_buffer on error still irks me a bit but
> I don't think that can easily be changed.

For the new versions, please:

- respect the 24h grace period:

https://elixir.bootlin.com/linux/v6.11.8/source/Documentation/process/maintainer-netdev.rst#L15

- add the target tree in the subj prefix (in this case 'net-next')

- ensure all the patches in the series have consistent subj prefix,
otherwise patchwork will be fooled.

I think it would be better to avoid modifying hex_dump_to_buffer(), if
not necessary, and I think you could do so either changing the type used
in patch 2 or even dropping such patch.

/P

[PATCH v2 2/3] hexdump: Use for_each macro in print_hex_dump

[Nick Child]

The looping logic in print_hex_dump can be handled by the macro
for_each_line_in_hex_dump.

Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---
 lib/hexdump.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/lib/hexdump.c b/lib/hexdump.c
index c3db7c3a7643..181b82dfe40d 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -263,19 +263,14 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 		    const void *buf, size_t len, bool ascii)
 {
 	const u8 *ptr = buf;
-	int i, linelen, remaining = len;
+	int i;
 	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
 
 	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);
-
+	for_each_line_in_hex_dump(i, rowsize, linebuf, sizeof(linebuf),
+				  groupsize, buf, len) {
 		switch (prefix_type) {
 		case DUMP_PREFIX_ADDRESS:
 			printk("%s%s%p: %s\n",
-- 
2.48.0

[PATCH v2 3/3] ibmvnic: Print data buffers with kernel API's

[Nick Child]

Previously, data buffers that were to be printed were cast to 8 byte
integers and printed. This can lead to buffer overflow if the length
of the buffer is not a multiple of 8.

Simplify and safeguard printing by using kernel provided functions
to print these data blobs.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index e95ae0d39948..a8f1feb9a2e7 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -4834,6 +4834,7 @@ static int send_login(struct ibmvnic_adapter *adapter)
 	struct device *dev = &adapter->vdev->dev;
 	struct vnic_login_client_data *vlcd;
 	dma_addr_t rsp_buffer_token;
+	unsigned char hex_str[16 * 3];
 	dma_addr_t buffer_token;
 	size_t rsp_buffer_size;
 	union ibmvnic_crq crq;
@@ -4937,9 +4938,9 @@ static int send_login(struct ibmvnic_adapter *adapter)
 	vnic_add_client_data(adapter, vlcd);
 
 	netdev_dbg(adapter->netdev, "Login Buffer:\n");
-	for (i = 0; i < (adapter->login_buf_sz - 1) / 8 + 1; i++) {
-		netdev_dbg(adapter->netdev, "%016lx\n",
-			   ((unsigned long *)(adapter->login_buf))[i]);
+	for_each_line_in_hex_dump(i, 16, hex_str, sizeof(hex_str), 8,
+				  adapter->login_buf, adapter->login_buf_sz) {
+		netdev_dbg(adapter->netdev, "%s\n", hex_str);
 	}
 
 	memset(&crq, 0, sizeof(crq));
@@ -5317,15 +5318,17 @@ static void handle_query_ip_offload_rsp(struct ibmvnic_adapter *adapter)
 {
 	struct device *dev = &adapter->vdev->dev;
 	struct ibmvnic_query_ip_offload_buffer *buf = &adapter->ip_offload_buf;
+	unsigned char hex_str[16 * 3];
 	int i;
 
 	dma_unmap_single(dev, adapter->ip_offload_tok,
 			 sizeof(adapter->ip_offload_buf), DMA_FROM_DEVICE);
 
 	netdev_dbg(adapter->netdev, "Query IP Offload Buffer:\n");
-	for (i = 0; i < (sizeof(adapter->ip_offload_buf) - 1) / 8 + 1; i++)
-		netdev_dbg(adapter->netdev, "%016lx\n",
-			   ((unsigned long *)(buf))[i]);
+	for_each_line_in_hex_dump(i, 16, hex_str, sizeof(hex_str), 8, buf,
+				  sizeof(adapter->ip_offload_buf)) {
+		netdev_dbg(adapter->netdev, "%s\n", hex_str);
+	}
 
 	netdev_dbg(adapter->netdev, "ipv4_chksum = %d\n", buf->ipv4_chksum);
 	netdev_dbg(adapter->netdev, "ipv6_chksum = %d\n", buf->ipv6_chksum);
@@ -5518,6 +5521,7 @@ static int handle_login_rsp(union ibmvnic_crq *login_rsp_crq,
 	struct net_device *netdev = adapter->netdev;
 	struct ibmvnic_login_rsp_buffer *login_rsp = adapter->login_rsp_buf;
 	struct ibmvnic_login_buffer *login = adapter->login_buf;
+	unsigned char hex_str[16 * 3];
 	u64 *tx_handle_array;
 	u64 *rx_handle_array;
 	int num_tx_pools;
@@ -5556,9 +5560,10 @@ static int handle_login_rsp(union ibmvnic_crq *login_rsp_crq,
 	netdev->mtu = adapter->req_mtu - ETH_HLEN;
 
 	netdev_dbg(adapter->netdev, "Login Response Buffer:\n");
-	for (i = 0; i < (adapter->login_rsp_buf_sz - 1) / 8 + 1; i++) {
-		netdev_dbg(adapter->netdev, "%016lx\n",
-			   ((unsigned long *)(adapter->login_rsp_buf))[i]);
+	for_each_line_in_hex_dump(i, 16, hex_str, sizeof(hex_str), 8,
+				  adapter->login_rsp_buf,
+				  adapter->login_rsp_buf_sz) {
+		netdev_dbg(adapter->netdev, "%s\n", hex_str);
 	}
 
 	/* Sanity checks */
-- 
2.48.0

[PATCH 0/3] Use new for_each macro to create hexdumps

[Nick Child]

Apologies, not sure what mailing list/tree to target. First 2 patches look
like *-next and last patch should go to net-next.

Currently, obtaining a hexdump can be done through one of the following:
 1. hex_dump_to_buffer - takes at most 32 bytes of a buffer and returns a 
     hexdump string representation
 2. print_hex_dump - prints output of hex_dump_to_buffer iteratively over
    a large buffer

There is no functionality for iterating over a large buffer and receiving
the string representation. It seems most users of hex_dump_to_buffer are
calling hex_dump_to_buffer within the body of a loop which iterates
through a buffer.

This patchset creates a for_each macro that accepts a buffer and fills
out an output string with the converted hexdump. This loops over the
buffer and takes care of incrementing pointers. Hopefully this makes
writing sequential calls to hex_dump_to_buffer more straightforward.

From a users perspective there should be no difference in output.

The inspiration here was I wanted to use print_hex_dump in ibmvnic code
but I wanted to print through netdevice printing functions to maintain
formatting. Looking at other users of hex_dump_to_buffer it seems they had
similar intentions.

Side question:
  hex_dump_to_buffer automatically sets groupsize to 1 if user given
  groupsize is not a multiple of len. When printing large buffers this
  makes non-uniform output. For example, this is a 31 byte 8 groupsize
  buffer:
   ibmvnic 30000003 env3: 6c6774732e737561 2e6d62692e736261
   ibmvnic 30000003 env3: 63 6f 6d 00 03 00 05 65 6e 76 33 00 00 00 00
  Since the second line is only 15 bytes, the group size is set to 1. I
  have written a patch which keeps groupsize so output would be:
   ibmvnic 30000003 env3: 6c6774732e737561 2e6d62692e736261
   ibmvnic 30000003 env3: 636f6d0003000565 6e763300000000
  But since I am not sure if this would break some dependency for someone,
  and my justification for change is purely pedantic, I chose to omit
  that patch in this patchset. Let me know if there is any interest and
  I will send a different patchset for that. 

Thanks for your consideration/review.

Nick Child (3):
  hexdump: Implement macro for converting large buffers
  hexdump: Use for_each macro in print_hex_dump
  ibmvnic: Print data buffers with kernel API's

 drivers/net/ethernet/ibm/ibmvnic.c | 23 ++++++++++++++---------
 include/linux/printk.h             | 21 +++++++++++++++++++++
 lib/hexdump.c                      | 11 +++--------
 3 files changed, 38 insertions(+), 17 deletions(-)

-- 
2.47.1

[PATCH 1/3] hexdump: Implement macro for converting large buffers

[Nick Child]

Define for_each_line_in_hex_dump which loops over a buffer and calls
hex_dump_to_buffer for each segment in the buffer. This allows the
caller to decide what to do with the resulting string and is not
limited by a specific printing format like print_hex_dump.

Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---
 include/linux/printk.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 4217a9f412b2..d55968f7ac10 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -755,6 +755,27 @@ enum {
 extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
 			      int groupsize, char *linebuf, size_t linebuflen,
 			      bool ascii);
+/**
+ * for_each_line_in_hex_dump - iterate over buffer, converting into hex ASCII
+ * @i - offset in @buff
+ * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @linebuf: where to put the converted data
+ * @linebuflen: total size of @linebuf, including space for terminating NUL
+ *		IOW >= (@rowsize * 2) + ((@rowsize - 1 / @groupsize)) + 1
+ * @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
+ */
+ #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
+				   buf, len) \
+	for ((i) = 0;							\
+	     (i) < (len) &&						\
+	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
+				min((len) - (i), rowsize),		\
+				(rowsize), (groupsize), (linebuf),	\
+				(linebuflen), false);			\
+	     (i) += (rowsize) == 16 || (rowsize) == 32 ? (rowsize) : 16	\
+	    )
 #ifdef CONFIG_PRINTK
 extern void print_hex_dump(const char *level, const char *prefix_str,
 			   int prefix_type, int rowsize, int groupsize,
-- 
2.47.1

Re: [PATCH 1/3] hexdump: Implement macro for converting large buffers

[Simon Horman]

On Mon, Jan 13, 2025 at 04:17:19PM -0600, Nick Child wrote:
> Define for_each_line_in_hex_dump which loops over a buffer and calls
> hex_dump_to_buffer for each segment in the buffer. This allows the
> caller to decide what to do with the resulting string and is not
> limited by a specific printing format like print_hex_dump.
> 
> Signed-off-by: Nick Child <nnac123@linux.ibm.com>
> ---
>  include/linux/printk.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 4217a9f412b2..d55968f7ac10 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -755,6 +755,27 @@ enum {
>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>  			      int groupsize, char *linebuf, size_t linebuflen,
>  			      bool ascii);
> +/**
> + * for_each_line_in_hex_dump - iterate over buffer, converting into hex ASCII
> + * @i - offset in @buff

nit: scripts/kernel-doc would like this to be "@i: ..." 

> + * @rowsize: number of bytes to print per line; must be 16 or 32
> + * @linebuf: where to put the converted data
> + * @linebuflen: total size of @linebuf, including space for terminating NUL
> + *		IOW >= (@rowsize * 2) + ((@rowsize - 1 / @groupsize)) + 1
> + * @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
> + */
> + #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
> +				   buf, len) \
> +	for ((i) = 0;							\
> +	     (i) < (len) &&						\
> +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> +				min((len) - (i), rowsize),		\
> +				(rowsize), (groupsize), (linebuf),	\
> +				(linebuflen), false);			\
> +	     (i) += (rowsize) == 16 || (rowsize) == 32 ? (rowsize) : 16	\
> +	    )
>  #ifdef CONFIG_PRINTK
>  extern void print_hex_dump(const char *level, const char *prefix_str,
>  			   int prefix_type, int rowsize, int groupsize,
> -- 
> 2.47.1
> 
>