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

[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

[PATCH 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.47.1

[PATCH 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.

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.47.1

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

[Jacob Keller]

On 1/13/2025 2:17 PM, Nick Child wrote:
> 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.
> 
> 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
> @@ -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);
>  	}

This is nicer to read and a bit more flexible than print_hex_dump. Neat.
Strictly you don't need the {} here, but i think its more readable given
all the arguments you have to pass to the for_each_macro over multiple
lines.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  
>  	/* Sanity checks */

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
> 
>