[PATCH v3 0/5] fs/seq_file: introduce seq_hex_dump() helper

[PATCH v3 0/5] fs/seq_file: introduce seq_hex_dump() helper

[Andy Shevchenko]

This introduces a new helper and switches current users to use it.

parisc and s390 weren't tested anyhow, the others are compile tested.

Changelog v3:
- append Mauro's Ack
- rebase on top of recent linux-next

Changelog v2:
- append Acked-by and Reviewed-by tags
- update commit messages in patches 3/5. and 5/5
- update line size to be 32 bytes instead of 16 in patch 3/5
- Joe found that output is changed in patch 4/5, thus I update commit message
  there

Andy Shevchenko (5):
  seq_file: provide an analogue of print_hex_dump()
  saa7164: convert to seq_hex_dump()
  crypto: qat - use seq_hex_dump() to dump buffers
  parisc: use seq_hex_dump() to dump buffers
  [S390] zcrypt: use seq_hex_dump() to dump buffers

 .../crypto/qat/qat_common/adf_transport_debug.c    | 16 ++--------
 drivers/media/pci/saa7164/saa7164-core.c           | 31 +++----------------
 drivers/parisc/ccio-dma.c                          | 14 ++-------
 drivers/parisc/sba_iommu.c                         | 11 ++-----
 drivers/s390/crypto/zcrypt_api.c                   | 10 +------
 fs/seq_file.c                                      | 35 ++++++++++++++++++++++
 include/linux/seq_file.h                           |  4 +++
 7 files changed, 52 insertions(+), 69 deletions(-)

-- 
2.1.0

[PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()

[Andy Shevchenko]

The new seq_hex_dump() is a complete analogue of print_hex_dump().

We have few users of this functionality already. It allows to reduce their
codebase.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 fs/seq_file.c            | 35 +++++++++++++++++++++++++++++++++++
 include/linux/seq_file.h |  4 ++++
 2 files changed, 39 insertions(+)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3857b72..fec4a6b 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/cred.h>
 #include <linux/mm.h>
+#include <linux/printk.h>
 
 #include <asm/uaccess.h>
 #include <asm/page.h>
@@ -794,6 +795,40 @@ void seq_pad(struct seq_file *m, char c)
 }
 EXPORT_SYMBOL(seq_pad);
 
+/* Analogue of print_hex_dump() */
+void seq_hex_dump(struct seq_file *m, 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];
+
+	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:
+			seq_printf(m, "%s%p: %s\n", prefix_str, ptr + i, linebuf);
+			break;
+		case DUMP_PREFIX_OFFSET:
+			seq_printf(m, "%s%.8x: %s\n", prefix_str, i, linebuf);
+			break;
+		default:
+			seq_printf(m, "%s%s\n", prefix_str, linebuf);
+			break;
+		}
+	}
+}
+EXPORT_SYMBOL(seq_hex_dump);
+
 struct list_head *seq_list_start(struct list_head *head, loff_t pos)
 {
 	struct list_head *lh;
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 52e0097..6a8be4c 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -107,6 +107,10 @@ int seq_write(struct seq_file *seq, const void *data, size_t len);
 __printf(2, 3) int seq_printf(struct seq_file *, const char *, ...);
 __printf(2, 0) int seq_vprintf(struct seq_file *, const char *, va_list args);
 
+void seq_hex_dump(struct seq_file *m, const char *prefix_str, int prefix_type,
+		  int rowsize, int groupsize, const void *buf, size_t len,
+		  bool ascii);
+
 int seq_path(struct seq_file *, const struct path *, const char *);
 int seq_dentry(struct seq_file *, struct dentry *, const char *);
 int seq_path_root(struct seq_file *m, const struct path *path,
-- 
2.1.0

[PATCH v3 2/5] saa7164: convert to seq_hex_dump()

[Andy Shevchenko]

Instead of custom approach let's use recently added seq_hex_dump() helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Steven Toth <stoth@kernellabs.com>
Acked-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
 drivers/media/pci/saa7164/saa7164-core.c | 31 ++++---------------------------
 1 file changed, 4 insertions(+), 27 deletions(-)

diff --git a/drivers/media/pci/saa7164/saa7164-core.c b/drivers/media/pci/saa7164/saa7164-core.c
index 1bf0697..6f81584 100644
--- a/drivers/media/pci/saa7164/saa7164-core.c
+++ b/drivers/media/pci/saa7164/saa7164-core.c
@@ -1065,7 +1065,6 @@ static int saa7164_proc_show(struct seq_file *m, void *v)
 	struct saa7164_dev *dev;
 	struct tmComResBusInfo *b;
 	struct list_head *list;
-	int i, c;
 
 	if (saa7164_devcount == 0)
 		return 0;
@@ -1089,35 +1088,13 @@ static int saa7164_proc_show(struct seq_file *m, void *v)
 
 		seq_printf(m, " .m_pdwGetReadPos  = 0x%x (0x%08x)\n",
 			b->m_dwGetWritePos, saa7164_readl(b->m_dwGetWritePos));
-		c = 0;
 		seq_printf(m, "\n  Set Ring:\n");
-		seq_printf(m, "\n addr  00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f\n");
-		for (i = 0; i < b->m_dwSizeSetRing; i++) {
-			if (c == 0)
-				seq_printf(m, " %04x:", i);
+		seq_hex_dump(m, " ", DUMP_PREFIX_OFFSET, 16, 1,
+			     b->m_pdwSetRing, b->m_dwSizeSetRing, false);
 
-			seq_printf(m, " %02x", *(b->m_pdwSetRing + i));
-
-			if (++c == 16) {
-				seq_printf(m, "\n");
-				c = 0;
-			}
-		}
-
-		c = 0;
 		seq_printf(m, "\n  Get Ring:\n");
-		seq_printf(m, "\n addr  00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f\n");
-		for (i = 0; i < b->m_dwSizeGetRing; i++) {
-			if (c == 0)
-				seq_printf(m, " %04x:", i);
-
-			seq_printf(m, " %02x", *(b->m_pdwGetRing + i));
-
-			if (++c == 16) {
-				seq_printf(m, "\n");
-				c = 0;
-			}
-		}
+		seq_hex_dump(m, " ", DUMP_PREFIX_OFFSET, 16, 1,
+			     b->m_pdwGetRing, b->m_dwSizeGetRing, false);
 
 		mutex_unlock(&b->lock);
 
-- 
2.1.0

[PATCH v3 3/5] crypto: qat - use seq_hex_dump() to dump buffers

[Andy Shevchenko]

Instead of custom approach let's use recently introduced seq_hex_dump() helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 drivers/crypto/qat/qat_common/adf_transport_debug.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/adf_transport_debug.c b/drivers/crypto/qat/qat_common/adf_transport_debug.c
index 6b69745..5f438a1 100644
--- a/drivers/crypto/qat/qat_common/adf_transport_debug.c
+++ b/drivers/crypto/qat/qat_common/adf_transport_debug.c
@@ -86,9 +86,7 @@ static int adf_ring_show(struct seq_file *sfile, void *v)
 {
 	struct adf_etr_ring_data *ring = sfile->private;
 	struct adf_etr_bank_data *bank = ring->bank;
-	uint32_t *msg = v;
 	void __iomem *csr = ring->bank->csr_addr;
-	int i, x;
 
 	if (v == SEQ_START_TOKEN) {
 		int head, tail, empty;
@@ -111,18 +109,8 @@ static int adf_ring_show(struct seq_file *sfile, void *v)
 		seq_puts(sfile, "----------- Ring data ------------\n");
 		return 0;
 	}
-	seq_printf(sfile, "%p:", msg);
-	x = 0;
-	i = 0;
-	for (; i < (ADF_MSG_SIZE_TO_BYTES(ring->msg_size) >> 2); i++) {
-		seq_printf(sfile, " %08X", *(msg + i));
-		if ((ADF_MSG_SIZE_TO_BYTES(ring->msg_size) >> 2) != i + 1 &&
-		    (++x == 8)) {
-			seq_printf(sfile, "\n%p:", msg + i + 1);
-			x = 0;
-		}
-	}
-	seq_puts(sfile, "\n");
+	seq_hex_dump(sfile, "", DUMP_PREFIX_ADDRESS, 32, 4,
+		     v, ADF_MSG_SIZE_TO_BYTES(ring->msg_size), false);
 	return 0;
 }
 
-- 
2.1.0

[PATCH v3 4/5] parisc: use seq_hex_dump() to dump buffers

[Andy Shevchenko]

Instead of custom approach let's use recently introduced seq_hex_dump() helper.

In one case it changes the output from
	1111111122222222333333334444444455555555666666667777777788888888
to
	11111111 22222222 33333333 44444444 55555555 66666666 77777777 88888888

though it seems it prints same data (by meaning) in both cases. I decide to
choose to use the space divided one.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/parisc/ccio-dma.c  | 14 +++-----------
 drivers/parisc/sba_iommu.c | 11 +++--------
 2 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/parisc/ccio-dma.c b/drivers/parisc/ccio-dma.c
index 8b490d7..9d353d2 100644
--- a/drivers/parisc/ccio-dma.c
+++ b/drivers/parisc/ccio-dma.c
@@ -1101,20 +1101,12 @@ static const struct file_operations ccio_proc_info_fops = {
 
 static int ccio_proc_bitmap_info(struct seq_file *m, void *p)
 {
-	int len = 0;
 	struct ioc *ioc = ioc_list;
 
 	while (ioc != NULL) {
-		u32 *res_ptr = (u32 *)ioc->res_map;
-		int j;
-
-		for (j = 0; j < (ioc->res_size / sizeof(u32)); j++) {
-			if ((j & 7) == 0)
-				len += seq_puts(m, "\n   ");
-			len += seq_printf(m, "%08x", *res_ptr);
-			res_ptr++;
-		}
-		len += seq_puts(m, "\n\n");
+		seq_hex_dump(m, "   ", DUMP_PREFIX_NONE, 32, 4, ioc->res_map,
+			     ioc->res_size, false);
+		seq_putc(m, '\n');
 		ioc = ioc->next;
 		break; /* XXX - remove me */
 	}
diff --git a/drivers/parisc/sba_iommu.c b/drivers/parisc/sba_iommu.c
index 1ff1b67..fbc4db9 100644
--- a/drivers/parisc/sba_iommu.c
+++ b/drivers/parisc/sba_iommu.c
@@ -1857,15 +1857,10 @@ sba_proc_bitmap_info(struct seq_file *m, void *p)
 {
 	struct sba_device *sba_dev = sba_list;
 	struct ioc *ioc = &sba_dev->ioc[0];	/* FIXME: Multi-IOC support! */
-	unsigned int *res_ptr = (unsigned int *)ioc->res_map;
-	int i, len = 0;
 
-	for (i = 0; i < (ioc->res_size/sizeof(unsigned int)); ++i, ++res_ptr) {
-		if ((i & 7) == 0)
-			len += seq_printf(m, "\n   ");
-		len += seq_printf(m, " %08x", *res_ptr);
-	}
-	len += seq_printf(m, "\n");
+	seq_hex_dump(m, "   ", DUMP_PREFIX_NONE, 32, 4, ioc->res_map,
+		     ioc->res_size, false);
+	seq_printf(m, "\n");
 
 	return 0;
 }
-- 
2.1.0

[PATCH v3 5/5] [S390] zcrypt: use seq_hex_dump() to dump buffers

[Andy Shevchenko]

Instead of custom approach let's use recently introduced seq_hex_dump() helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/s390/crypto/zcrypt_api.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
index 0e18c5d..d1f9983 100644
--- a/drivers/s390/crypto/zcrypt_api.c
+++ b/drivers/s390/crypto/zcrypt_api.c
@@ -1203,16 +1203,8 @@ static void sprinthx(unsigned char *title, struct seq_file *m,
 static void sprinthx4(unsigned char *title, struct seq_file *m,
 		      unsigned int *array, unsigned int len)
 {
-	int r;
-
 	seq_printf(m, "\n%s\n", title);
-	for (r = 0; r < len; r++) {
-		if ((r % 8) == 0)
-			seq_printf(m, "    ");
-		seq_printf(m, "%08X ", array[r]);
-		if ((r % 8) == 7)
-			seq_putc(m, '\n');
-	}
+	seq_hex_dump(m, "    ", DUMP_PREFIX_NONE, 32, 4, array, len, false);
 	seq_putc(m, '\n');
 }
 
-- 
2.1.0

Re: [PATCH v3 4/5] parisc: use seq_hex_dump() to dump buffers

[Helge Deller]

Hi Andy,

On 08/25/2014 11:03 AM, Andy Shevchenko wrote:
> Instead of custom approach let's use recently introduced seq_hex_dump() helper.
>
> In one case it changes the output from
> 	1111111122222222333333334444444455555555666666667777777788888888
> to
> 	11111111 22222222 33333333 44444444 55555555 66666666 77777777 88888888
>
> though it seems it prints same data (by meaning) in both cases. I decide to
> choose to use the space divided one.

That's OK.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I compile-tested the patch on parisc and runtime-checked the changes to the
sba_iommu.c driver. Everything seems OK. Please add my Acked-by.

Acked-by: Helge Deller <deller@gmx.de>

Thanks!
Helge

> ---
>   drivers/parisc/ccio-dma.c  | 14 +++-----------
>   drivers/parisc/sba_iommu.c | 11 +++--------
>   2 files changed, 6 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/parisc/ccio-dma.c b/drivers/parisc/ccio-dma.c
> index 8b490d7..9d353d2 100644
> --- a/drivers/parisc/ccio-dma.c
> +++ b/drivers/parisc/ccio-dma.c
> @@ -1101,20 +1101,12 @@ static const struct file_operations ccio_proc_info_fops = {
>
>   static int ccio_proc_bitmap_info(struct seq_file *m, void *p)
>   {
> -	int len = 0;
>   	struct ioc *ioc = ioc_list;
>
>   	while (ioc != NULL) {
> -		u32 *res_ptr = (u32 *)ioc->res_map;
> -		int j;
> -
> -		for (j = 0; j < (ioc->res_size / sizeof(u32)); j++) {
> -			if ((j & 7) == 0)
> -				len += seq_puts(m, "\n   ");
> -			len += seq_printf(m, "%08x", *res_ptr);
> -			res_ptr++;
> -		}
> -		len += seq_puts(m, "\n\n");
> +		seq_hex_dump(m, "   ", DUMP_PREFIX_NONE, 32, 4, ioc->res_map,
> +			     ioc->res_size, false);
> +		seq_putc(m, '\n');
>   		ioc = ioc->next;
>   		break; /* XXX - remove me */
>   	}
> diff --git a/drivers/parisc/sba_iommu.c b/drivers/parisc/sba_iommu.c
> index 1ff1b67..fbc4db9 100644
> --- a/drivers/parisc/sba_iommu.c
> +++ b/drivers/parisc/sba_iommu.c
> @@ -1857,15 +1857,10 @@ sba_proc_bitmap_info(struct seq_file *m, void *p)
>   {
>   	struct sba_device *sba_dev = sba_list;
>   	struct ioc *ioc = &sba_dev->ioc[0];	/* FIXME: Multi-IOC support! */
> -	unsigned int *res_ptr = (unsigned int *)ioc->res_map;
> -	int i, len = 0;
>
> -	for (i = 0; i < (ioc->res_size/sizeof(unsigned int)); ++i, ++res_ptr) {
> -		if ((i & 7) == 0)
> -			len += seq_printf(m, "\n   ");
> -		len += seq_printf(m, " %08x", *res_ptr);
> -	}
> -	len += seq_printf(m, "\n");
> +	seq_hex_dump(m, "   ", DUMP_PREFIX_NONE, 32, 4, ioc->res_map,
> +		     ioc->res_size, false);
> +	seq_printf(m, "\n");
>
>   	return 0;
>   }
>

Re: [PATCH v3 5/5] [S390] zcrypt: use seq_hex_dump() to dump buffers

[Ingo Tuchscherer]

[-- Attachment #1: Type: text/plain, Size: 4879 bytes --]


I have verified and tested the patch for the zcrypt part.
OK from my side. You can add my Acked-by.
Acked-by: Ingo Tuchscherer <ingo.tuchscherer@de.ibm.com>

Thanks

Mit freundlichen Grüßen / Kind regards

Ingo Tuchscherer

Software Development - Linux on System z
IBM Systems &Technology Group
                                                                       
                                                                       
                                                                       
                                                                       
                                                                       
 Phone:            +49-7031-16-1986                 IBM Deutschland                          (Embedded
                                                                                           image moved
                                                                                              to file:
                                                                                         pic53004.gif)
                                                                       
 E-Mail:           ingo.tuchscherer@de.ibm.com      Schoenaicher Str. 220
                                                                       
                                                    71032 Boeblingen   
                                                                       
                                                    Germany            
                                                                       
                                                                       
                                                                       
                                                                       
                                                                       
 IBM Deutschland                                                       
 Research &                                                            
 Development                                                           
 GmbH /                                                                
 Vorsitzender des                                                      
 Aufsichtsrats:                                                        
 Martina Koederitz                                                     
                                                                       
 Geschäftsführung:                                                 
 Dirk Wittkopp                                                         
 Sitz der                                                              
 Gesellschaft:                                                         
 Böblingen /                                                         
 Registergericht:                                                      
 Amtsgericht                                                           
 Stuttgart, HRB                                                        
 243294                                                                
                                                                       





From:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:	Tadeusz Struk <tadeusz.struk@intel.com>, Herbert Xu
            <herbert@gondor.apana.org.au>, Mauro Carvalho Chehab
            <m.chehab@samsung.com>, Helge Deller <deller@gmx.de>, Ingo
            Tuchscherer/Germany/IBM@IBMDE, Alexander Viro
            <viro@zeniv.linux.org.uk>, linux-kernel@vger.kernel.org, Joe
            Perches <joe@perches.com>, Marek Vasut <marex@denx.de>,
Cc:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date:	08/25/2014 11:03 AM
Subject:	[PATCH v3 5/5] [S390] zcrypt: use seq_hex_dump() to dump
            buffers



Instead of custom approach let's use recently introduced seq_hex_dump()
helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/s390/crypto/zcrypt_api.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_api.c
b/drivers/s390/crypto/zcrypt_api.c
index 0e18c5d..d1f9983 100644
--- a/drivers/s390/crypto/zcrypt_api.c
+++ b/drivers/s390/crypto/zcrypt_api.c
@@ -1203,16 +1203,8 @@ static void sprinthx(unsigned char *title, struct
seq_file *m,
 static void sprinthx4(unsigned char *title, struct seq_file *m,
 		 		       unsigned int *array, unsigned int len)
 {
-		 int r;
-
 		 seq_printf(m, "\n%s\n", title);
-		 for (r = 0; r < len; r++) {
-		 		 if ((r % 8) == 0)
-		 		 		 seq_printf(m, "    ");
-		 		 seq_printf(m, "%08X ", array[r]);
-		 		 if ((r % 8) == 7)
-		 		 		 seq_putc(m, '\n');
-		 }
+		 seq_hex_dump(m, "    ", DUMP_PREFIX_NONE, 32, 4, array, len,
false);
 		 seq_putc(m, '\n');
 }

--
2.1.0


[-- Attachment #2: pic53004.gif --]
[-- Type: image/gif, Size: 1851 bytes --]

Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()

[Al Viro]

On Mon, Aug 25, 2014 at 12:03:11PM +0300, Andy Shevchenko wrote:
> The new seq_hex_dump() is a complete analogue of print_hex_dump().
> 
> We have few users of this functionality already. It allows to reduce their
> codebase.

I really don't like the stack footprint.

> +	unsigned char linebuf[32 * 3 + 2 + 32 + 1];

... and extra copying for no good reason.  Why not check that we have
enough space in buffer and generate directly into it?  See what e.g.
seq_escape() is doing...

Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()

[Andy Shevchenko]

On Sat, 2014-08-30 at 23:54 +0100, Al Viro wrote:
> On Mon, Aug 25, 2014 at 12:03:11PM +0300, Andy Shevchenko wrote:
> > The new seq_hex_dump() is a complete analogue of print_hex_dump().
> > 
> > We have few users of this functionality already. It allows to reduce their
> > codebase.
> 
> I really don't like the stack footprint.
> 
> > +	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
> 
> ... and extra copying for no good reason.  Why not check that we have
> enough space in buffer and generate directly into it?  See what e.g.
> seq_escape() is doing...

What about this variant?


@@ -794,6 +795,47 @@ void seq_pad(struct seq_file *m, char c)
 }
 EXPORT_SYMBOL(seq_pad);
 
+/* Analogue of print_hex_dump() */
+void seq_hex_dump(struct seq_file *m, 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;
+       int ret;
+
+       if (rowsize != 16 && rowsize != 32)
+               rowsize = 16;
+
+       for (i = 0; i < len; i += rowsize) {
+               linelen = min(remaining, rowsize);
+               remaining -= rowsize;
+
+               ret = seq_printf(m, "%s", prefix_str);
+               if (ret < 0)
+                       break;
+
+               if (prefix_type == DUMP_PREFIX_ADDRESS)
+                       ret = seq_printf(m, "%p: ", ptr + i);
+               else if (prefix_type == DUMP_PREFIX_OFFSET)
+                       ret = seq_printf(m, "%.8x: ", i);
+               if (ret < 0)
+                       break;
+
+               if (m->size < m->count + groupsize * 2 + 1)
+                       break;
+
+               hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
+                                  m->buf + m->count, m->size - m->count,
+                                  ascii);
+
+               ret = seq_putc(m, '\n');
+               if (ret < 0)
+                       break;
+       }
+}
+EXPORT_SYMBOL(seq_hex_dump);



-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()

[Geert Uytterhoeven]

Hi Andy,

On Mon, Sep 1, 2014 at 10:36 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>> ... and extra copying for no good reason.  Why not check that we have
>> enough space in buffer and generate directly into it?  See what e.g.
>> seq_escape() is doing...
>
> What about this variant?

I think it needs a call to seq_set_overflow() in case the buffer is too small,
so the caller will retry with a bigger buffer.

>
> @@ -794,6 +795,47 @@ void seq_pad(struct seq_file *m, char c)
>  }
>  EXPORT_SYMBOL(seq_pad);
>
> +/* Analogue of print_hex_dump() */
> +void seq_hex_dump(struct seq_file *m, 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;
> +       int ret;
> +
> +       if (rowsize != 16 && rowsize != 32)
> +               rowsize = 16;
> +
> +       for (i = 0; i < len; i += rowsize) {
> +               linelen = min(remaining, rowsize);
> +               remaining -= rowsize;
> +
> +               ret = seq_printf(m, "%s", prefix_str);
> +               if (ret < 0)
> +                       break;
> +
> +               if (prefix_type == DUMP_PREFIX_ADDRESS)
> +                       ret = seq_printf(m, "%p: ", ptr + i);
> +               else if (prefix_type == DUMP_PREFIX_OFFSET)
> +                       ret = seq_printf(m, "%.8x: ", i);
> +               if (ret < 0)
> +                       break;
> +
> +               if (m->size < m->count + groupsize * 2 + 1)
> +                       break;
> +
> +               hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> +                                  m->buf + m->count, m->size - m->count,
> +                                  ascii);
> +
> +               ret = seq_putc(m, '\n');
> +               if (ret < 0)
> +                       break;
> +       }
> +}
> +EXPORT_SYMBOL(seq_hex_dump);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()

[Andy Shevchenko]

On Mon, 2014-09-01 at 10:58 +0200, Geert Uytterhoeven wrote:
> Hi Andy,
> 
> On Mon, Sep 1, 2014 at 10:36 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >> ... and extra copying for no good reason.  Why not check that we have
> >> enough space in buffer and generate directly into it?  See what e.g.
> >> seq_escape() is doing...
> >
> > What about this variant?
> 
> I think it needs a call to seq_set_overflow() in case the buffer is too small,
> so the caller will retry with a bigger buffer.

Yes, in two places it would be useful to do.

But what the condition for "buffer is too small", the same groupsize * 2
+ 1 or you mean something else?

> 
> >
> > @@ -794,6 +795,47 @@ void seq_pad(struct seq_file *m, char c)
> >  }
> >  EXPORT_SYMBOL(seq_pad);
> >
> > +/* Analogue of print_hex_dump() */
> > +void seq_hex_dump(struct seq_file *m, 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;
> > +       int ret;
> > +
> > +       if (rowsize != 16 && rowsize != 32)
> > +               rowsize = 16;
> > +
> > +       for (i = 0; i < len; i += rowsize) {
> > +               linelen = min(remaining, rowsize);
> > +               remaining -= rowsize;
> > +
> > +               ret = seq_printf(m, "%s", prefix_str);
> > +               if (ret < 0)
> > +                       break;
> > +
> > +               if (prefix_type == DUMP_PREFIX_ADDRESS)
> > +                       ret = seq_printf(m, "%p: ", ptr + i);
> > +               else if (prefix_type == DUMP_PREFIX_OFFSET)
> > +                       ret = seq_printf(m, "%.8x: ", i);
> > +               if (ret < 0)
> > +                       break;
> > +
> > +               if (m->size < m->count + groupsize * 2 + 1)
> > +                       break;
> > +
> > +               hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> > +                                  m->buf + m->count, m->size - m->count,
> > +                                  ascii);
> > +
> > +               ret = seq_putc(m, '\n');
> > +               if (ret < 0)
> > +                       break;
> > +       }
> > +}
> > +EXPORT_SYMBOL(seq_hex_dump);
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()

[Geert Uytterhoeven]

Hi Andy,

On Mon, Sep 1, 2014 at 11:09 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, 2014-09-01 at 10:58 +0200, Geert Uytterhoeven wrote:
>> On Mon, Sep 1, 2014 at 10:36 AM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>> >> ... and extra copying for no good reason.  Why not check that we have
>> >> enough space in buffer and generate directly into it?  See what e.g.
>> >> seq_escape() is doing...
>> >
>> > What about this variant?
>>
>> I think it needs a call to seq_set_overflow() in case the buffer is too small,
>> so the caller will retry with a bigger buffer.
>
> Yes, in two places it would be useful to do.

Two places? I see only one, just before calling hex_dump_to_buffer.

> But what the condition for "buffer is too small", the same groupsize * 2
> + 1 or you mean something else?

"groupsize * 2 + 1" is not the amount of bytes hex_dump_to_buffer() wants
to write. It's only the size for one word.

You could check if there are at least "32 * 3 + 2 + 32 + 1" bytes (your
old linebuf size) available.

However, to protect against overflows if hex_dump_to_buffer() ever changes,
I think it would be better to let hex_dump_to_buffer() indicate if the
passed buffer was to small (it already checks the passed linebuflen).
Then you can just check for that.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()

[Andy Shevchenko]

On Mon, Sep 1, 2014 at 12:25 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Andy,
>
> On Mon, Sep 1, 2014 at 11:09 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Mon, 2014-09-01 at 10:58 +0200, Geert Uytterhoeven wrote:
>>> On Mon, Sep 1, 2014 at 10:36 AM, Andy Shevchenko
>>> <andriy.shevchenko@linux.intel.com> wrote:
>>> >> ... and extra copying for no good reason.  Why not check that we have
>>> >> enough space in buffer and generate directly into it?  See what e.g.
>>> >> seq_escape() is doing...
>>> >
>>> > What about this variant?
>>>
>>> I think it needs a call to seq_set_overflow() in case the buffer is too small,
>>> so the caller will retry with a bigger buffer.
>>
>> Yes, in two places it would be useful to do.
>
> Two places? I see only one, just before calling hex_dump_to_buffer.

seq_putc doesn't set it as I can see.

>> But what the condition for "buffer is too small", the same groupsize * 2
>> + 1 or you mean something else?
>
> "groupsize * 2 + 1" is not the amount of bytes hex_dump_to_buffer() wants
> to write. It's only the size for one word.
>
> You could check if there are at least "32 * 3 + 2 + 32 + 1" bytes (your
> old linebuf size) available.

This is a good question why this number? What if we have to print only
one byte (as different groupsize)?
I think the requirement for one groupsize is quite okay.

> However, to protect against overflows if hex_dump_to_buffer() ever changes,
> I think it would be better to let hex_dump_to_buffer() indicate if the
> passed buffer was to small (it already checks the passed linebuflen).
> Then you can just check for that.

I thought about that. We may introduce either new call and make
current one the user of it or change all occurrences.
Nevertheless, currently it will print only one groupsize if there is
enough room for it but for two or more.
Thus, I prefer to keep the behaviour "print until we can".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()

[Geert Uytterhoeven]

Hi Andy,

On Mon, Sep 1, 2014 at 12:15 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>>>> I think it needs a call to seq_set_overflow() in case the buffer is too small,
>>>> so the caller will retry with a bigger buffer.
>>>
>>> Yes, in two places it would be useful to do.
>>
>> Two places? I see only one, just before calling hex_dump_to_buffer.
>
> seq_putc doesn't set it as I can see.
>
>>> But what the condition for "buffer is too small", the same groupsize * 2
>>> + 1 or you mean something else?
>>
>> "groupsize * 2 + 1" is not the amount of bytes hex_dump_to_buffer() wants
>> to write. It's only the size for one word.
>>
>> You could check if there are at least "32 * 3 + 2 + 32 + 1" bytes (your
>> old linebuf size) available.
>
> This is a good question why this number? What if we have to print only
> one byte (as different groupsize)?

I don't think complaining about a too-small buffer prematurely hurts.

> I think the requirement for one groupsize is quite okay.

Then you will loose data if the buffer is too small.

>> However, to protect against overflows if hex_dump_to_buffer() ever changes,
>> I think it would be better to let hex_dump_to_buffer() indicate if the
>> passed buffer was to small (it already checks the passed linebuflen).
>> Then you can just check for that.
>
> I thought about that. We may introduce either new call and make
> current one the user of it or change all occurrences.
> Nevertheless, currently it will print only one groupsize if there is
> enough room for it but for two or more.
> Thus, I prefer to keep the behaviour "print until we can".

The idea of seq_*() is that it will retry with a bigger bufsize if there's
not enough space.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()

[Andy Shevchenko]

On Mon, 2014-09-01 at 12:59 +0200, Geert Uytterhoeven wrote:
> Hi Andy,
> 
> On Mon, Sep 1, 2014 at 12:15 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >>>> I think it needs a call to seq_set_overflow() in case the buffer is too small,
> >>>> so the caller will retry with a bigger buffer.
> >>>
> >>> Yes, in two places it would be useful to do.
> >>
> >> Two places? I see only one, just before calling hex_dump_to_buffer.
> >
> > seq_putc doesn't set it as I can see.
> >
> >>> But what the condition for "buffer is too small", the same groupsize * 2
> >>> + 1 or you mean something else?
> >>
> >> "groupsize * 2 + 1" is not the amount of bytes hex_dump_to_buffer() wants
> >> to write. It's only the size for one word.
> >>
> >> You could check if there are at least "32 * 3 + 2 + 32 + 1" bytes (your
> >> old linebuf size) available.
> >
> > This is a good question why this number? What if we have to print only
> > one byte (as different groupsize)?
> 
> I don't think complaining about a too-small buffer prematurely hurts.
> 
> > I think the requirement for one groupsize is quite okay.
> 
> Then you will loose data if the buffer is too small.
> 
> >> However, to protect against overflows if hex_dump_to_buffer() ever changes,
> >> I think it would be better to let hex_dump_to_buffer() indicate if the
> >> passed buffer was to small (it already checks the passed linebuflen).
> >> Then you can just check for that.
> >
> > I thought about that. We may introduce either new call and make
> > current one the user of it or change all occurrences.
> > Nevertheless, currently it will print only one groupsize if there is
> > enough room for it but for two or more.
> > Thus, I prefer to keep the behaviour "print until we can".
> 
> The idea of seq_*() is that it will retry with a bigger bufsize if there's
> not enough space.

Fair enough.

So, summarize above, the check before hex_dump_to_buffer() should care
about maximum possible buffer needed for one line. But we have already
rowsize calculated (we actually can't rely on groupsize in this case).

Do you agree with formula rowsize * 3 + 2 + rowsize + 1?

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()

[Geert Uytterhoeven]

Hi Andy,

On Mon, Sep 1, 2014 at 1:33 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>> >> However, to protect against overflows if hex_dump_to_buffer() ever changes,
>> >> I think it would be better to let hex_dump_to_buffer() indicate if the
>> >> passed buffer was to small (it already checks the passed linebuflen).
>> >> Then you can just check for that.
>> >
>> > I thought about that. We may introduce either new call and make
>> > current one the user of it or change all occurrences.
>> > Nevertheless, currently it will print only one groupsize if there is
>> > enough room for it but for two or more.
>> > Thus, I prefer to keep the behaviour "print until we can".
>>
>> The idea of seq_*() is that it will retry with a bigger bufsize if there's
>> not enough space.
>
> Fair enough.
>
> So, summarize above, the check before hex_dump_to_buffer() should care
> about maximum possible buffer needed for one line. But we have already
> rowsize calculated (we actually can't rely on groupsize in this case).
>
> Do you agree with formula rowsize * 3 + 2 + rowsize + 1?

That's the _current_ formula. The day hex_dump_to_buffer() is changed,
it will break silently.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 1/5] seq_file: provide an analogue of print_hex_dump()

[Andy Shevchenko]

On Mon, Sep 1, 2014 at 2:49 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Andy,
>
> On Mon, Sep 1, 2014 at 1:33 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>> >> However, to protect against overflows if hex_dump_to_buffer() ever changes,
>>> >> I think it would be better to let hex_dump_to_buffer() indicate if the
>>> >> passed buffer was to small (it already checks the passed linebuflen).
>>> >> Then you can just check for that.
>>> >
>>> > I thought about that. We may introduce either new call and make
>>> > current one the user of it or change all occurrences.
>>> > Nevertheless, currently it will print only one groupsize if there is
>>> > enough room for it but for two or more.
>>> > Thus, I prefer to keep the behaviour "print until we can".
>>>
>>> The idea of seq_*() is that it will retry with a bigger bufsize if there's
>>> not enough space.
>>
>> Fair enough.
>>
>> So, summarize above, the check before hex_dump_to_buffer() should care
>> about maximum possible buffer needed for one line. But we have already
>> rowsize calculated (we actually can't rely on groupsize in this case).
>>
>> Do you agree with formula rowsize * 3 + 2 + rowsize + 1?
>
> That's the _current_ formula. The day hex_dump_to_buffer() is changed,
> it will break silently.

Do we have any other option except modify hex_dump_to_buffer() to
return an error when buffer is not enough?
In this case we require hex_dump_to_buffer() to follow this agreement
even if it's changed once.

-- 
With Best Regards,
Andy Shevchenko