[PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Masahiro Yamada]

Trivial changes suggested by checkpatch.pl.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Fix printk(KERN_DEBUG ...) too

 drivers/of/address.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 9582c57..65fafbb 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
 #ifdef DEBUG
 static void of_dump_addr(const char *s, const __be32 *addr, int na)
 {
-	printk(KERN_DEBUG "%s", s);
+	pr_debug("%s", s);
 	while (na--)
 		printk(" %08x", be32_to_cpu(*(addr++)));
 	printk("\n");
@@ -597,7 +597,7 @@ static u64 __of_translate_address(struct device_node *dev,
 		pbus = of_match_bus(parent);
 		pbus->count_cells(dev, &pna, &pns);
 		if (!OF_CHECK_COUNTS(pna, pns)) {
-			printk(KERN_ERR "prom_parse: Bad cell count for %s\n",
+			pr_err("prom_parse: Bad cell count for %s\n",
 			       of_node_full_name(dev));
 			break;
 		}
-- 
1.9.1

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Joe Perches]

On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
> Trivial changes suggested by checkpatch.pl.
[]
> diff --git a/drivers/of/address.c b/drivers/of/address.c
[]
> @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
>  #ifdef DEBUG
>  static void of_dump_addr(const char *s, const __be32 *addr, int na)
>  {
> -	printk(KERN_DEBUG "%s", s);
> +	pr_debug("%s", s);
>  	while (na--)
>  		printk(" %08x", be32_to_cpu(*(addr++)));
>  	printk("\n");

mixing pr_debug and printk doesn't make much sense.

It might be nicer to use

static void of_dumpaddr(const char *s, const __be32 *addr, int na)
{
#ifdef DEBUG
	...
#endif
}

to avoid the other static declaration

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Joe Perches]

On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
> On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
> > Trivial changes suggested by checkpatch.pl.
> []
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> []
> > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
> >  #ifdef DEBUG
> >  static void of_dump_addr(const char *s, const __be32 *addr, int na)
> >  {
> > -	printk(KERN_DEBUG "%s", s);
> > +	pr_debug("%s", s);
> >  	while (na--)
> >  		printk(" %08x", be32_to_cpu(*(addr++)));
> >  	printk("\n");
> 
> mixing pr_debug and printk doesn't make much sense.
> 
> It might be nicer to use
> 
> static void of_dumpaddr(const char *s, const __be32 *addr, int na)
> {
> #ifdef DEBUG
> 	...
> #endif
> }
> 
> to avoid the other static declaration

Or more simply:

static void of_dumpaddr(const char *s, const __be32 *addr, int na)
{
	print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
			     addr, na * sizeof(__be32), false);
}

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Rob Herring]

On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
>> On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
>> > Trivial changes suggested by checkpatch.pl.
>> []
>> > diff --git a/drivers/of/address.c b/drivers/of/address.c
>> []
>> > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
>> >  #ifdef DEBUG
>> >  static void of_dump_addr(const char *s, const __be32 *addr, int na)
>> >  {
>> > -   printk(KERN_DEBUG "%s", s);
>> > +   pr_debug("%s", s);
>> >     while (na--)
>> >             printk(" %08x", be32_to_cpu(*(addr++)));
>> >     printk("\n");
>>
>> mixing pr_debug and printk doesn't make much sense.
>>
>> It might be nicer to use
>>
>> static void of_dumpaddr(const char *s, const __be32 *addr, int na)
>> {
>> #ifdef DEBUG
>>       ...
>> #endif
>> }
>>
>> to avoid the other static declaration
>
> Or more simply:
>
> static void of_dumpaddr(const char *s, const __be32 *addr, int na)
> {
>         print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
>                              addr, na * sizeof(__be32), false);
> }

Except that it doesn't do the endian swapping. Looking closer at this,
we should just drop this hunk. So I will take v1.

Rob

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Joe Perches]

On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote:
> On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
> > > On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
> > > > Trivial changes suggested by checkpatch.pl.
> > > []
> > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > []
> > > > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
> > > >  #ifdef DEBUG
> > > >  static void of_dump_addr(const char *s, const __be32 *addr, int na)
> > > >  {
> > > > -   printk(KERN_DEBUG "%s", s);
> > > > +   pr_debug("%s", s);
> > > >     while (na--)
> > > >             printk(" %08x", be32_to_cpu(*(addr++)));
> > > >     printk("\n");
[]
> > static void of_dumpaddr(const char *s, const __be32 *addr, int na)
> > {
> >         print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
> >                              addr, na * sizeof(__be32), false);
> > }
> 
> Except that it doesn't do the endian swapping. Looking closer at this,
> we should just drop this hunk. So I will take v1.

Maybe endian conversions should be added to hex_dump_debug like:
(probably doesn't apply.  Evolution 3.18 is horrible)
---
 include/linux/printk.h |  7 +++++++
 lib/hexdump.c          | 56 +++++++++++++++++++++++++++++++++++---------------
 2 files changed, 47 insertions(+), 16 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 9729565..4be190c 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -424,6 +424,13 @@ enum {
 	DUMP_PREFIX_ADDRESS,
 	DUMP_PREFIX_OFFSET
 };
+
+enum {
+	DUMP_TYPE_CPU = 0,
+	DUMP_TYPE_LE = BIT(30),
+	DUMP_TYPE_BE = BIT(31)
+};
+
 extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
 			      int groupsize, char *linebuf, size_t linebuflen,
 			      bool ascii);
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 992457b..49113aa 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
  * @len: number of bytes in the @buf
  * @rowsize: number of bytes to print per line; must be 16 or 32
  * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ *             OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
  * @linebuf: where to put the converted data
  * @linebuflen: total size of @linebuf, including space for terminating NUL
  * @ascii: include ASCII after the hex output
@@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 	int j, lx = 0;
 	int ascii_column;
 	int ret;
+	int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
 
 	if (rowsize != 16 && rowsize != 32)
 		rowsize = 16;
 
 	if (len > rowsize)		/* limit to one line at a time */
 		len = rowsize;
-	if (!is_power_of_2(groupsize) || groupsize > 8)
-		groupsize = 1;
-	if ((len % groupsize) != 0)	/* no mixed size output */
-		groupsize = 1;
+	if (!is_power_of_2(actual_groupsize) || actual_groupsize > 8)
+		actual_groupsize = 1;
+	if ((len % actual_groupsize) != 0)	/* no mixed size output */
+		actual_groupsize = 1;
 
-	ngroups = len / groupsize;
-	ascii_column = rowsize * 2 + rowsize / groupsize + 1;
+	ngroups = len / actual_groupsize;
+	ascii_column = rowsize * 2 + rowsize / actual_groupsize + 1;
 
 	if (!linebuflen)
 		goto overflow1;
@@ -134,35 +136,56 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 	if (!len)
 		goto nil;
 
-	if (groupsize == 8) {
+	if (actual_groupsize == 8) {
 		const u64 *ptr8 = buf;
 
 		for (j = 0; j < ngroups; j++) {
+			u64 val;
+
+			if (groupsize & DUMP_TYPE_LE)
+				val = get_unaligned_le64(ptr8 + j);
+			else if (groupsize & DUMP_TYPE_BE)
+				val = get_unaligned_be64(ptr8 + j);
+			else
+				val = get_unaligned(ptr8 + j);
 			ret = snprintf(linebuf + lx, linebuflen - lx,
-				       "%s%16.16llx", j ? " " : "",
-				       get_unaligned(ptr8 + j));
+				       "%s%16.16llx", j ? " " : "", val);
 			if (ret >= linebuflen - lx)
 				goto overflow1;
 			lx += ret;
 		}
-	} else if (groupsize == 4) {
+	} else if (actual_groupsize == 4) {
 		const u32 *ptr4 = buf;
 
 		for (j = 0; j < ngroups; j++) {
+			u32 val;
+
+			if (groupsize & DUMP_TYPE_LE)
+				val = get_unaligned_le32(ptr4 + j);
+			else if (groupsize & DUMP_TYPE_BE)
+				val = get_unaligned_be32(ptr4 + j);
+			else
+				val = get_unaligned(ptr4 + j);
 			ret = snprintf(linebuf + lx, linebuflen - lx,
-				       "%s%8.8x", j ? " " : "",
-				       get_unaligned(ptr4 + j));
+				       "%s%8.8x", j ? " " : "", val);
 			if (ret >= linebuflen - lx)
 				goto overflow1;
 			lx += ret;
 		}
-	} else if (groupsize == 2) {
+	} else if (actual_groupsize == 2) {
 		const u16 *ptr2 = buf;
 
 		for (j = 0; j < ngroups; j++) {
+			u16 val;
+
+			if (groupsize & DUMP_TYPE_LE)
+				val = get_unaligned_le16(ptr2 + j);
+			else if (groupsize & DUMP_TYPE_BE)
+				val = get_unaligned_be16(ptr2 + j);
+			else
+				val = get_unaligned(ptr2 + j);
 			ret = snprintf(linebuf + lx, linebuflen - lx,
-				       "%s%4.4x", j ? " " : "",
-				       get_unaligned(ptr2 + j));
+				       "%s%4.4x", j ? " " : "", val);
 			if (ret >= linebuflen - lx)
 				goto overflow1;
 			lx += ret;
@@ -203,7 +226,8 @@ nil:
 overflow2:
 	linebuf[lx++] = '\0';
 overflow1:
-	return ascii ? ascii_column + len : (groupsize * 2 + 1) * ngroups - 1;
+	return ascii ? ascii_column + len
+		     : (actual_groupsize * 2 + 1) * ngroups - 1;
 }
 EXPORT_SYMBOL(hex_dump_to_buffer);
 

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Andy Shevchenko]

On Wed, Dec 9, 2015 at 2:03 AM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote:
>> On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe@perches.com> wrote:
>> > On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
>> > > On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
>> > > > Trivial changes suggested by checkpatch.pl.
>> > > []
>> > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
>> > > []
>> > > > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
>> > > >  #ifdef DEBUG
>> > > >  static void of_dump_addr(const char *s, const __be32 *addr, int na)
>> > > >  {
>> > > > -   printk(KERN_DEBUG "%s", s);
>> > > > +   pr_debug("%s", s);
>> > > >     while (na--)
>> > > >             printk(" %08x", be32_to_cpu(*(addr++)));
>> > > >     printk("\n");
> []
>> > static void of_dumpaddr(const char *s, const __be32 *addr, int na)
>> > {
>> >         print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
>> >                              addr, na * sizeof(__be32), false);
>> > }
>>
>> Except that it doesn't do the endian swapping. Looking closer at this,
>> we should just drop this hunk. So I will take v1.
>
> Maybe endian conversions should be added to hex_dump_debug like:
> (probably doesn't apply.  Evolution 3.18 is horrible)

Overall the idea is not bad I think, though few comments below.
Besides that, please, update test_hexdump (wrt my update to the test
suite [1]) to go through BE test cases.

[1] http://www.spinics.net/lists/kernel/msg2139337.html

> ---
>  include/linux/printk.h |  7 +++++++
>  lib/hexdump.c          | 56 +++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 47 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 9729565..4be190c 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -424,6 +424,13 @@ enum {
>         DUMP_PREFIX_ADDRESS,
>         DUMP_PREFIX_OFFSET
>  };
> +
> +enum {
> +       DUMP_TYPE_CPU = 0,

Do we need explicit enum for that? Documentation update anyway is required.

> +       DUMP_TYPE_LE = BIT(30),
> +       DUMP_TYPE_BE = BIT(31)
> +};
> +
>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>                               int groupsize, char *linebuf, size_t linebuflen,
>                               bool ascii);
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 992457b..49113aa 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
>   * @len: number of bytes in the @buf
>   * @rowsize: number of bytes to print per line; must be 16 or 32
>   * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + *             OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
>   * @linebuf: where to put the converted data
>   * @linebuflen: total size of @linebuf, including space for terminating NUL
>   * @ascii: include ASCII after the hex output
> @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>         int j, lx = 0;
>         int ascii_column;
>         int ret;
> +       int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);

I would rather prefer to have function parameter to be renamed.

E.g. gsflags ?

>
>         if (rowsize != 16 && rowsize != 32)
>                 rowsize = 16;
>
>         if (len > rowsize)              /* limit to one line at a time */
>                 len = rowsize;
> -       if (!is_power_of_2(groupsize) || groupsize > 8)
> -               groupsize = 1;
> -       if ((len % groupsize) != 0)     /* no mixed size output */
> -               groupsize = 1;
> +       if (!is_power_of_2(actual_groupsize) || actual_groupsize > 8)
> +               actual_groupsize = 1;
> +       if ((len % actual_groupsize) != 0)      /* no mixed size output */
> +               actual_groupsize = 1;
>
> -       ngroups = len / groupsize;
> -       ascii_column = rowsize * 2 + rowsize / groupsize + 1;
> +       ngroups = len / actual_groupsize;
> +       ascii_column = rowsize * 2 + rowsize / actual_groupsize + 1;
>
>         if (!linebuflen)
>                 goto overflow1;
> @@ -134,35 +136,56 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>         if (!len)
>                 goto nil;
>
> -       if (groupsize == 8) {
> +       if (actual_groupsize == 8) {
>                 const u64 *ptr8 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u64 val;
> +
> +                       if (groupsize & DUMP_TYPE_LE)
> +                               val = get_unaligned_le64(ptr8 + j);
> +                       else if (groupsize & DUMP_TYPE_BE)
> +                               val = get_unaligned_be64(ptr8 + j);
> +                       else
> +                               val = get_unaligned(ptr8 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%16.16llx", j ? " " : "",
> -                                      get_unaligned(ptr8 + j));
> +                                      "%s%16.16llx", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
>                 }
> -       } else if (groupsize == 4) {
> +       } else if (actual_groupsize == 4) {
>                 const u32 *ptr4 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u32 val;
> +
> +                       if (groupsize & DUMP_TYPE_LE)
> +                               val = get_unaligned_le32(ptr4 + j);
> +                       else if (groupsize & DUMP_TYPE_BE)
> +                               val = get_unaligned_be32(ptr4 + j);
> +                       else
> +                               val = get_unaligned(ptr4 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%8.8x", j ? " " : "",
> -                                      get_unaligned(ptr4 + j));
> +                                      "%s%8.8x", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
>                 }
> -       } else if (groupsize == 2) {
> +       } else if (actual_groupsize == 2) {
>                 const u16 *ptr2 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u16 val;
> +
> +                       if (groupsize & DUMP_TYPE_LE)
> +                               val = get_unaligned_le16(ptr2 + j);
> +                       else if (groupsize & DUMP_TYPE_BE)
> +                               val = get_unaligned_be16(ptr2 + j);
> +                       else
> +                               val = get_unaligned(ptr2 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%4.4x", j ? " " : "",
> -                                      get_unaligned(ptr2 + j));
> +                                      "%s%4.4x", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
> @@ -203,7 +226,8 @@ nil:
>  overflow2:
>         linebuf[lx++] = '\0';
>  overflow1:
> -       return ascii ? ascii_column + len : (groupsize * 2 + 1) * ngroups - 1;
> +       return ascii ? ascii_column + len
> +                    : (actual_groupsize * 2 + 1) * ngroups - 1;
>  }
>  EXPORT_SYMBOL(hex_dump_to_buffer);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Andy Shevchenko]

On Wed, Dec 9, 2015 at 2:03 AM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote:
>> On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> > On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
>> > > On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
>> > > > Trivial changes suggested by checkpatch.pl.
>> > > []
>> > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
>> > > []
>> > > > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
>> > > >  #ifdef DEBUG
>> > > >  static void of_dump_addr(const char *s, const __be32 *addr, int na)
>> > > >  {
>> > > > -   printk(KERN_DEBUG "%s", s);
>> > > > +   pr_debug("%s", s);
>> > > >     while (na--)
>> > > >             printk(" %08x", be32_to_cpu(*(addr++)));
>> > > >     printk("\n");
> []
>> > static void of_dumpaddr(const char *s, const __be32 *addr, int na)
>> > {
>> >         print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
>> >                              addr, na * sizeof(__be32), false);
>> > }
>>
>> Except that it doesn't do the endian swapping. Looking closer at this,
>> we should just drop this hunk. So I will take v1.
>
> Maybe endian conversions should be added to hex_dump_debug like:
> (probably doesn't apply.  Evolution 3.18 is horrible)

Overall the idea is not bad I think, though few comments below.
Besides that, please, update test_hexdump (wrt my update to the test
suite [1]) to go through BE test cases.

[1] http://www.spinics.net/lists/kernel/msg2139337.html

> ---
>  include/linux/printk.h |  7 +++++++
>  lib/hexdump.c          | 56 +++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 47 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 9729565..4be190c 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -424,6 +424,13 @@ enum {
>         DUMP_PREFIX_ADDRESS,
>         DUMP_PREFIX_OFFSET
>  };
> +
> +enum {
> +       DUMP_TYPE_CPU = 0,

Do we need explicit enum for that? Documentation update anyway is required.

> +       DUMP_TYPE_LE = BIT(30),
> +       DUMP_TYPE_BE = BIT(31)
> +};
> +
>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>                               int groupsize, char *linebuf, size_t linebuflen,
>                               bool ascii);
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 992457b..49113aa 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
>   * @len: number of bytes in the @buf
>   * @rowsize: number of bytes to print per line; must be 16 or 32
>   * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + *             OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
>   * @linebuf: where to put the converted data
>   * @linebuflen: total size of @linebuf, including space for terminating NUL
>   * @ascii: include ASCII after the hex output
> @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>         int j, lx = 0;
>         int ascii_column;
>         int ret;
> +       int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);

I would rather prefer to have function parameter to be renamed.

E.g. gsflags ?

>
>         if (rowsize != 16 && rowsize != 32)
>                 rowsize = 16;
>
>         if (len > rowsize)              /* limit to one line at a time */
>                 len = rowsize;
> -       if (!is_power_of_2(groupsize) || groupsize > 8)
> -               groupsize = 1;
> -       if ((len % groupsize) != 0)     /* no mixed size output */
> -               groupsize = 1;
> +       if (!is_power_of_2(actual_groupsize) || actual_groupsize > 8)
> +               actual_groupsize = 1;
> +       if ((len % actual_groupsize) != 0)      /* no mixed size output */
> +               actual_groupsize = 1;
>
> -       ngroups = len / groupsize;
> -       ascii_column = rowsize * 2 + rowsize / groupsize + 1;
> +       ngroups = len / actual_groupsize;
> +       ascii_column = rowsize * 2 + rowsize / actual_groupsize + 1;
>
>         if (!linebuflen)
>                 goto overflow1;
> @@ -134,35 +136,56 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>         if (!len)
>                 goto nil;
>
> -       if (groupsize == 8) {
> +       if (actual_groupsize == 8) {
>                 const u64 *ptr8 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u64 val;
> +
> +                       if (groupsize & DUMP_TYPE_LE)
> +                               val = get_unaligned_le64(ptr8 + j);
> +                       else if (groupsize & DUMP_TYPE_BE)
> +                               val = get_unaligned_be64(ptr8 + j);
> +                       else
> +                               val = get_unaligned(ptr8 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%16.16llx", j ? " " : "",
> -                                      get_unaligned(ptr8 + j));
> +                                      "%s%16.16llx", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
>                 }
> -       } else if (groupsize == 4) {
> +       } else if (actual_groupsize == 4) {
>                 const u32 *ptr4 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u32 val;
> +
> +                       if (groupsize & DUMP_TYPE_LE)
> +                               val = get_unaligned_le32(ptr4 + j);
> +                       else if (groupsize & DUMP_TYPE_BE)
> +                               val = get_unaligned_be32(ptr4 + j);
> +                       else
> +                               val = get_unaligned(ptr4 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%8.8x", j ? " " : "",
> -                                      get_unaligned(ptr4 + j));
> +                                      "%s%8.8x", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
>                 }
> -       } else if (groupsize == 2) {
> +       } else if (actual_groupsize == 2) {
>                 const u16 *ptr2 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u16 val;
> +
> +                       if (groupsize & DUMP_TYPE_LE)
> +                               val = get_unaligned_le16(ptr2 + j);
> +                       else if (groupsize & DUMP_TYPE_BE)
> +                               val = get_unaligned_be16(ptr2 + j);
> +                       else
> +                               val = get_unaligned(ptr2 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%4.4x", j ? " " : "",
> -                                      get_unaligned(ptr2 + j));
> +                                      "%s%4.4x", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
> @@ -203,7 +226,8 @@ nil:
>  overflow2:
>         linebuf[lx++] = '\0';
>  overflow1:
> -       return ascii ? ascii_column + len : (groupsize * 2 + 1) * ngroups - 1;
> +       return ascii ? ascii_column + len
> +                    : (actual_groupsize * 2 + 1) * ngroups - 1;
>  }
>  EXPORT_SYMBOL(hex_dump_to_buffer);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Joe Perches]

On Wed, 2015-12-09 at 14:03 +0200, Andy Shevchenko wrote:
> On Wed, Dec 9, 2015 at 2:03 AM, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote:
> > > On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe@perches.com> wrote:
> > > > On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
> > > > > On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
> > > > > > Trivial changes suggested by checkpatch.pl.
> > > > > []
> > > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > > > []
> > > > > > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
> > > > > >  #ifdef DEBUG
> > > > > >  static void of_dump_addr(const char *s, const __be32 *addr, int na)
> > > > > >  {
> > > > > > -   printk(KERN_DEBUG "%s", s);
> > > > > > +   pr_debug("%s", s);
> > > > > >     while (na--)
> > > > > >             printk(" %08x", be32_to_cpu(*(addr++)));
> > > > > >     printk("\n");
> > []
> > > > static void of_dumpaddr(const char *s, const __be32 *addr, int na)
> > > > {
> > > >         print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
> > > >                              addr, na * sizeof(__be32), false);
> > > > }
> > > 
> > > Except that it doesn't do the endian swapping. Looking closer at this,
> > > we should just drop this hunk. So I will take v1.
> > 
> > Maybe endian conversions should be added to hex_dump_debug like:
> > (probably doesn't apply.  Evolution 3.18 is horrible)
> 
> Overall the idea is not bad I think, though few comments below.
> Besides that, please, update test_hexdump (wrt my update to the test
> suite [1]) to go through BE test cases.
> 
> [1] http://www.spinics.net/lists/kernel/msg2139337.html

I don't want to have to learn that code as it seems a bit tricky.
Maybe you could do it.

> > diff --git a/lib/hexdump.c b/lib/hexdump.c
> > index 992457b..49113aa 100644
> > --- a/lib/hexdump.c
> > +++ b/lib/hexdump.c
> > @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
> >   * @len: number of bytes in the @buf
> >   * @rowsize: number of bytes to print per line; must be 16 or 32
> >   * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + *             OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
> >   * @linebuf: where to put the converted data
> >   * @linebuflen: total size of @linebuf, including space for terminating NUL
> >   * @ascii: include ASCII after the hex output
> > @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> >         int j, lx = 0;
> >         int ascii_column;
> >         int ret;
> > +       int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
> 
> I would rather prefer to have function parameter to be renamed.
> 
> E.g. gsflags ?
> 

Well, it's a bit simpler changelog.
---
 include/linux/printk.h |  7 +++++++
 lib/hexdump.c          | 39 +++++++++++++++++++++++++++++++--------
 2 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 9729565..4be190c 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -424,6 +424,13 @@ enum {
 	DUMP_PREFIX_ADDRESS,
 	DUMP_PREFIX_OFFSET
 };
+
+enum {
+	DUMP_TYPE_CPU = 0,
+	DUMP_TYPE_LE = BIT(30),
+	DUMP_TYPE_BE = BIT(31)
+};
+
 extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
 			      int groupsize, char *linebuf, size_t linebuflen,
 			      bool ascii);
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 992457b..5b1eda70 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -80,7 +80,8 @@ EXPORT_SYMBOL(bin2hex);
  * @buf: data blob to dump
  * @len: number of bytes in the @buf
  * @rowsize: number of bytes to print per line; must be 16 or 32
- * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ *              OR with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
  * @linebuf: where to put the converted data
  * @linebuflen: total size of @linebuf, including space for terminating NUL
  * @ascii: include ASCII after the hex output
@@ -105,7 +106,7 @@ EXPORT_SYMBOL(bin2hex);
  * (excluding the terminating NUL) which would have been written to the final
  * string if enough space had been available.
  */
-int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
+int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupflags,
 		       char *linebuf, size_t linebuflen, bool ascii)
 {
 	const u8 *ptr = buf;
@@ -114,6 +115,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 	int j, lx = 0;
 	int ascii_column;
 	int ret;
+	int groupsize = groupflags & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
 
 	if (rowsize != 16 && rowsize != 32)
 		rowsize = 16;
@@ -138,9 +140,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 		const u64 *ptr8 = buf;
 
 		for (j = 0; j < ngroups; j++) {
+			u64 val;
+
+			if (groupflags & DUMP_TYPE_LE)
+				val = get_unaligned_le64(ptr8 + j);
+			else if (groupflags & DUMP_TYPE_BE)
+				val = get_unaligned_be64(ptr8 + j);
+			else
+				val = get_unaligned(ptr8 + j);
 			ret = snprintf(linebuf + lx, linebuflen - lx,
-				       "%s%16.16llx", j ? " " : "",
-				       get_unaligned(ptr8 + j));
+				       "%s%16.16llx", j ? " " : "", val);
 			if (ret >= linebuflen - lx)
 				goto overflow1;
 			lx += ret;
@@ -149,9 +158,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 		const u32 *ptr4 = buf;
 
 		for (j = 0; j < ngroups; j++) {
+			u32 val;
+
+			if (groupflags & DUMP_TYPE_LE)
+				val = get_unaligned_le32(ptr4 + j);
+			else if (groupflags & DUMP_TYPE_BE)
+				val = get_unaligned_be32(ptr4 + j);
+			else
+				val = get_unaligned(ptr4 + j);
 			ret = snprintf(linebuf + lx, linebuflen - lx,
-				       "%s%8.8x", j ? " " : "",
-				       get_unaligned(ptr4 + j));
+				       "%s%8.8x", j ? " " : "", val);
 			if (ret >= linebuflen - lx)
 				goto overflow1;
 			lx += ret;
@@ -160,9 +176,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 		const u16 *ptr2 = buf;
 
 		for (j = 0; j < ngroups; j++) {
+			u16 val;
+
+			if (groupflags & DUMP_TYPE_LE)
+				val = get_unaligned_le16(ptr2 + j);
+			else if (groupflags & DUMP_TYPE_BE)
+				val = get_unaligned_be16(ptr2 + j);
+			else
+				val = get_unaligned(ptr2 + j);
 			ret = snprintf(linebuf + lx, linebuflen - lx,
-				       "%s%4.4x", j ? " " : "",
-				       get_unaligned(ptr2 + j));
+				       "%s%4.4x", j ? " " : "", val);
 			if (ret >= linebuflen - lx)
 				goto overflow1;
 			lx += ret;

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Andy Shevchenko]

On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2015-12-09 at 14:03 +0200, Andy Shevchenko wrote:
>> On Wed, Dec 9, 2015 at 2:03 AM, Joe Perches <joe@perches.com> wrote:
>> > On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote:
>> > > On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe@perches.com> wrote:
>> > > > On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
>> > > > > On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
>> > > > > > Trivial changes suggested by checkpatch.pl.
>> > > > > []
>> > > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
>> > > > > []
>> > > > > > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
>> > > > > >  #ifdef DEBUG
>> > > > > >  static void of_dump_addr(const char *s, const __be32 *addr, int na)
>> > > > > >  {
>> > > > > > -   printk(KERN_DEBUG "%s", s);
>> > > > > > +   pr_debug("%s", s);
>> > > > > >     while (na--)
>> > > > > >             printk(" %08x", be32_to_cpu(*(addr++)));
>> > > > > >     printk("\n");
>> > []
>> > > > static void of_dumpaddr(const char *s, const __be32 *addr, int na)
>> > > > {
>> > > >         print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
>> > > >                              addr, na * sizeof(__be32), false);
>> > > > }
>> > >
>> > > Except that it doesn't do the endian swapping. Looking closer at this,
>> > > we should just drop this hunk. So I will take v1.
>> >
>> > Maybe endian conversions should be added to hex_dump_debug like:
>> > (probably doesn't apply.  Evolution 3.18 is horrible)
>>
>> Overall the idea is not bad I think, though few comments below.
>> Besides that, please, update test_hexdump (wrt my update to the test
>> suite [1]) to go through BE test cases.
>>
>> [1] http://www.spinics.net/lists/kernel/msg2139337.html
>
> I don't want to have to learn that code as it seems a bit tricky.
> Maybe you could do it.

Okay, I'm fine if it at least passes existing test cases. Good to
mention this in commit message then.

>
>> > diff --git a/lib/hexdump.c b/lib/hexdump.c
>> > index 992457b..49113aa 100644
>> > --- a/lib/hexdump.c
>> > +++ b/lib/hexdump.c
>> > @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
>> >   * @len: number of bytes in the @buf
>> >   * @rowsize: number of bytes to print per line; must be 16 or 32
>> >   * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>> > + *             OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
>> >   * @linebuf: where to put the converted data
>> >   * @linebuflen: total size of @linebuf, including space for terminating NUL
>> >   * @ascii: include ASCII after the hex output
>> > @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>> >         int j, lx = 0;
>> >         int ascii_column;
>> >         int ret;
>> > +       int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
>>
>> I would rather prefer to have function parameter to be renamed.
>>
>> E.g. gsflags ?
>>
>
> Well, it's a bit simpler changelog.

Generally I'm fine with this version, though take into account above and below.

> ---
>  include/linux/printk.h |  7 +++++++
>  lib/hexdump.c          | 39 +++++++++++++++++++++++++++++++--------
>  2 files changed, 38 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 9729565..4be190c 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -424,6 +424,13 @@ enum {
>         DUMP_PREFIX_ADDRESS,
>         DUMP_PREFIX_OFFSET
>  };
> +
> +enum {
> +       DUMP_TYPE_CPU = 0,

And still open this, do we need it? I think you may just mention in
the documentation that default behaviour is CPU like.

> +       DUMP_TYPE_LE = BIT(30),
> +       DUMP_TYPE_BE = BIT(31)
> +};
> +
>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>                               int groupsize, char *linebuf, size_t linebuflen,

Here as well to change.

>                               bool ascii);
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 992457b..5b1eda70 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -80,7 +80,8 @@ EXPORT_SYMBOL(bin2hex);
>   * @buf: data blob to dump
>   * @len: number of bytes in the @buf
>   * @rowsize: number of bytes to print per line; must be 16 or 32
> - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + *              OR with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions

Maybe specify "bitwise OR with.." ?

%DUMP_…

>   * @linebuf: where to put the converted data
>   * @linebuflen: total size of @linebuf, including space for terminating NUL
>   * @ascii: include ASCII after the hex output
> @@ -105,7 +106,7 @@ EXPORT_SYMBOL(bin2hex);
>   * (excluding the terminating NUL) which would have been written to the final
>   * string if enough space had been available.
>   */
> -int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> +int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupflags,
>                        char *linebuf, size_t linebuflen, bool ascii)
>  {
>         const u8 *ptr = buf;
> @@ -114,6 +115,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>         int j, lx = 0;
>         int ascii_column;
>         int ret;
> +       int groupsize = groupflags & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
>
>         if (rowsize != 16 && rowsize != 32)
>                 rowsize = 16;
> @@ -138,9 +140,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>                 const u64 *ptr8 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u64 val;
> +
> +                       if (groupflags & DUMP_TYPE_LE)
> +                               val = get_unaligned_le64(ptr8 + j);
> +                       else if (groupflags & DUMP_TYPE_BE)
> +                               val = get_unaligned_be64(ptr8 + j);
> +                       else
> +                               val = get_unaligned(ptr8 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%16.16llx", j ? " " : "",
> -                                      get_unaligned(ptr8 + j));
> +                                      "%s%16.16llx", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
> @@ -149,9 +158,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>                 const u32 *ptr4 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u32 val;
> +
> +                       if (groupflags & DUMP_TYPE_LE)
> +                               val = get_unaligned_le32(ptr4 + j);
> +                       else if (groupflags & DUMP_TYPE_BE)
> +                               val = get_unaligned_be32(ptr4 + j);
> +                       else
> +                               val = get_unaligned(ptr4 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%8.8x", j ? " " : "",
> -                                      get_unaligned(ptr4 + j));
> +                                      "%s%8.8x", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
> @@ -160,9 +176,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>                 const u16 *ptr2 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u16 val;
> +
> +                       if (groupflags & DUMP_TYPE_LE)
> +                               val = get_unaligned_le16(ptr2 + j);
> +                       else if (groupflags & DUMP_TYPE_BE)
> +                               val = get_unaligned_be16(ptr2 + j);
> +                       else
> +                               val = get_unaligned(ptr2 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%4.4x", j ? " " : "",
> -                                      get_unaligned(ptr2 + j));
> +                                      "%s%4.4x", j ? " " : "", val);



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Andy Shevchenko]

On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Wed, 2015-12-09 at 14:03 +0200, Andy Shevchenko wrote:
>> On Wed, Dec 9, 2015 at 2:03 AM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> > On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote:
>> > > On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> > > > On Tue, 2015-12-08 at 08:16 -0800, Joe Perches wrote:
>> > > > > On Wed, 2015-12-09 at 01:07 +0900, Masahiro Yamada wrote:
>> > > > > > Trivial changes suggested by checkpatch.pl.
>> > > > > []
>> > > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
>> > > > > []
>> > > > > > @@ -23,7 +23,7 @@ static int __of_address_to_resource(struct device_node *dev,
>> > > > > >  #ifdef DEBUG
>> > > > > >  static void of_dump_addr(const char *s, const __be32 *addr, int na)
>> > > > > >  {
>> > > > > > -   printk(KERN_DEBUG "%s", s);
>> > > > > > +   pr_debug("%s", s);
>> > > > > >     while (na--)
>> > > > > >             printk(" %08x", be32_to_cpu(*(addr++)));
>> > > > > >     printk("\n");
>> > []
>> > > > static void of_dumpaddr(const char *s, const __be32 *addr, int na)
>> > > > {
>> > > >         print_hex_dump_debug(s, DUMP_PREFIX_NONE, 16, 1,
>> > > >                              addr, na * sizeof(__be32), false);
>> > > > }
>> > >
>> > > Except that it doesn't do the endian swapping. Looking closer at this,
>> > > we should just drop this hunk. So I will take v1.
>> >
>> > Maybe endian conversions should be added to hex_dump_debug like:
>> > (probably doesn't apply.  Evolution 3.18 is horrible)
>>
>> Overall the idea is not bad I think, though few comments below.
>> Besides that, please, update test_hexdump (wrt my update to the test
>> suite [1]) to go through BE test cases.
>>
>> [1] http://www.spinics.net/lists/kernel/msg2139337.html
>
> I don't want to have to learn that code as it seems a bit tricky.
> Maybe you could do it.

Okay, I'm fine if it at least passes existing test cases. Good to
mention this in commit message then.

>
>> > diff --git a/lib/hexdump.c b/lib/hexdump.c
>> > index 992457b..49113aa 100644
>> > --- a/lib/hexdump.c
>> > +++ b/lib/hexdump.c
>> > @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
>> >   * @len: number of bytes in the @buf
>> >   * @rowsize: number of bytes to print per line; must be 16 or 32
>> >   * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>> > + *             OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
>> >   * @linebuf: where to put the converted data
>> >   * @linebuflen: total size of @linebuf, including space for terminating NUL
>> >   * @ascii: include ASCII after the hex output
>> > @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>> >         int j, lx = 0;
>> >         int ascii_column;
>> >         int ret;
>> > +       int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
>>
>> I would rather prefer to have function parameter to be renamed.
>>
>> E.g. gsflags ?
>>
>
> Well, it's a bit simpler changelog.

Generally I'm fine with this version, though take into account above and below.

> ---
>  include/linux/printk.h |  7 +++++++
>  lib/hexdump.c          | 39 +++++++++++++++++++++++++++++++--------
>  2 files changed, 38 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 9729565..4be190c 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -424,6 +424,13 @@ enum {
>         DUMP_PREFIX_ADDRESS,
>         DUMP_PREFIX_OFFSET
>  };
> +
> +enum {
> +       DUMP_TYPE_CPU = 0,

And still open this, do we need it? I think you may just mention in
the documentation that default behaviour is CPU like.

> +       DUMP_TYPE_LE = BIT(30),
> +       DUMP_TYPE_BE = BIT(31)
> +};
> +
>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>                               int groupsize, char *linebuf, size_t linebuflen,

Here as well to change.

>                               bool ascii);
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 992457b..5b1eda70 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -80,7 +80,8 @@ EXPORT_SYMBOL(bin2hex);
>   * @buf: data blob to dump
>   * @len: number of bytes in the @buf
>   * @rowsize: number of bytes to print per line; must be 16 or 32
> - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + *              OR with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions

Maybe specify "bitwise OR with.." ?

%DUMP_…

>   * @linebuf: where to put the converted data
>   * @linebuflen: total size of @linebuf, including space for terminating NUL
>   * @ascii: include ASCII after the hex output
> @@ -105,7 +106,7 @@ EXPORT_SYMBOL(bin2hex);
>   * (excluding the terminating NUL) which would have been written to the final
>   * string if enough space had been available.
>   */
> -int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> +int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupflags,
>                        char *linebuf, size_t linebuflen, bool ascii)
>  {
>         const u8 *ptr = buf;
> @@ -114,6 +115,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>         int j, lx = 0;
>         int ascii_column;
>         int ret;
> +       int groupsize = groupflags & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
>
>         if (rowsize != 16 && rowsize != 32)
>                 rowsize = 16;
> @@ -138,9 +140,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>                 const u64 *ptr8 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u64 val;
> +
> +                       if (groupflags & DUMP_TYPE_LE)
> +                               val = get_unaligned_le64(ptr8 + j);
> +                       else if (groupflags & DUMP_TYPE_BE)
> +                               val = get_unaligned_be64(ptr8 + j);
> +                       else
> +                               val = get_unaligned(ptr8 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%16.16llx", j ? " " : "",
> -                                      get_unaligned(ptr8 + j));
> +                                      "%s%16.16llx", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
> @@ -149,9 +158,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>                 const u32 *ptr4 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u32 val;
> +
> +                       if (groupflags & DUMP_TYPE_LE)
> +                               val = get_unaligned_le32(ptr4 + j);
> +                       else if (groupflags & DUMP_TYPE_BE)
> +                               val = get_unaligned_be32(ptr4 + j);
> +                       else
> +                               val = get_unaligned(ptr4 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%8.8x", j ? " " : "",
> -                                      get_unaligned(ptr4 + j));
> +                                      "%s%8.8x", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
> @@ -160,9 +176,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>                 const u16 *ptr2 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u16 val;
> +
> +                       if (groupflags & DUMP_TYPE_LE)
> +                               val = get_unaligned_le16(ptr2 + j);
> +                       else if (groupflags & DUMP_TYPE_BE)
> +                               val = get_unaligned_be16(ptr2 + j);
> +                       else
> +                               val = get_unaligned(ptr2 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%4.4x", j ? " " : "",
> -                                      get_unaligned(ptr2 + j));
> +                                      "%s%4.4x", j ? " " : "", val);



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Joe Perches]

On Wed, 2015-12-09 at 22:02 +0200, Andy Shevchenko wrote:
> On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe@perches.com> wrote:
[]
> > > > diff --git a/lib/hexdump.c b/lib/hexdump.c
> > > > index 992457b..49113aa 100644
> > > > --- a/lib/hexdump.c
> > > > +++ b/lib/hexdump.c
> > > > @@ -81,6 +81,7 @@ EXPORT_SYMBOL(bin2hex);
> > > >   * @len: number of bytes in the @buf
> > > >   * @rowsize: number of bytes to print per line; must be 16 or 32
> > > >   * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > > > + *             OR'd with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
> > > >   * @linebuf: where to put the converted data
> > > >   * @linebuflen: total size of @linebuf, including space for terminating NUL
> > > >   * @ascii: include ASCII after the hex output
> > > > @@ -114,19 +115,20 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> > > >         int j, lx = 0;
> > > >         int ascii_column;
> > > >         int ret;
> > > > +       int actual_groupsize = groupsize & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
> > > 
> > > I would rather prefer to have function parameter to be renamed.
> > > 
> > > E.g. gsflags ?
> > > 
> > 
> > Well, it's a bit simpler changelog.
> 
> Generally I'm fine with this version, though take into account above and below.
> 
> > ---
> >  include/linux/printk.h |  7 +++++++
> >  lib/hexdump.c          | 39 +++++++++++++++++++++++++++++++--------
> >  2 files changed, 38 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > index 9729565..4be190c 100644
> > --- a/include/linux/printk.h
> > +++ b/include/linux/printk.h
> > @@ -424,6 +424,13 @@ enum {
> >         DUMP_PREFIX_ADDRESS,
> >         DUMP_PREFIX_OFFSET
> >  };
> > +
> > +enum {
> > +       DUMP_TYPE_CPU = 0,
> 
> And still open this, do we need it? I think you may just mention in
> the documentation that default behaviour is CPU like.

The only documentation I'm aware of is the kernel-doc

> > +       DUMP_TYPE_LE = BIT(30),
> > +       DUMP_TYPE_BE = BIT(31)
> > +};
> > +
> >  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> >                               int groupsize, char *linebuf, size_t linebuflen,
> 
> Here as well to change.

Right, thanks.

> >                               bool ascii);
> > diff --git a/lib/hexdump.c b/lib/hexdump.c
> > index 992457b..5b1eda70 100644
> > --- a/lib/hexdump.c
> > +++ b/lib/hexdump.c
> > @@ -80,7 +80,8 @@ EXPORT_SYMBOL(bin2hex);
> >   * @buf: data blob to dump
> >   * @len: number of bytes in the @buf
> >   * @rowsize: number of bytes to print per line; must be 16 or 32
> > - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + *              OR with DUMP_TYPE_BE or DUMP_TYPE_LE for endian conversions
> 
> Maybe specify "bitwise OR with.." ?

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Rasmus Villemoes]

On Wed, Dec 09 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe@perches.com> wrote:
>> ---
>>  include/linux/printk.h |  7 +++++++
>>  lib/hexdump.c          | 39 +++++++++++++++++++++++++++++++--------
>>  2 files changed, 38 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> index 9729565..4be190c 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -424,6 +424,13 @@ enum {
>>         DUMP_PREFIX_ADDRESS,
>>         DUMP_PREFIX_OFFSET
>>  };
>> +
>> +enum {
>> +       DUMP_TYPE_CPU = 0,
>
> And still open this, do we need it? I think you may just mention in
> the documentation that default behaviour is CPU like.

I think it's best to have a name for the default. In this case it's
unlikely to ever be relevant, but in general one could imagine stuff
like

#ifdef THIS_OR_THAT
#define MY_DUMP_TYPE DUMP_TYPE_LE
#else
#define MY_DUMP_TYPE DUMP_TYPE_CPU
#endif

which is a lot more readable than if the latter was a naked 0.

The feature seems fine to me. It's always been somewhat annoying that
grouping changes the order of the byte values on little-endian machines
(so grouping is actually a wrong name for it; it's more like "treat the
bytes an array of u16/u32/u64"), and one can now get around that by
specifying DUMP_TYPE_BE.

Rasmus

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Rasmus Villemoes]

On Wed, Dec 09 2015, Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> ---
>>  include/linux/printk.h |  7 +++++++
>>  lib/hexdump.c          | 39 +++++++++++++++++++++++++++++++--------
>>  2 files changed, 38 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> index 9729565..4be190c 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -424,6 +424,13 @@ enum {
>>         DUMP_PREFIX_ADDRESS,
>>         DUMP_PREFIX_OFFSET
>>  };
>> +
>> +enum {
>> +       DUMP_TYPE_CPU = 0,
>
> And still open this, do we need it? I think you may just mention in
> the documentation that default behaviour is CPU like.

I think it's best to have a name for the default. In this case it's
unlikely to ever be relevant, but in general one could imagine stuff
like

#ifdef THIS_OR_THAT
#define MY_DUMP_TYPE DUMP_TYPE_LE
#else
#define MY_DUMP_TYPE DUMP_TYPE_CPU
#endif

which is a lot more readable than if the latter was a naked 0.

The feature seems fine to me. It's always been somewhat annoying that
grouping changes the order of the byte values on little-endian machines
(so grouping is actually a wrong name for it; it's more like "treat the
bytes an array of u16/u32/u64"), and one can now get around that by
specifying DUMP_TYPE_BE.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Andy Shevchenko]

On Wed, Dec 9, 2015 at 10:17 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On Wed, Dec 09 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches <joe@perches.com> wrote:

>>> +enum {
>>> +       DUMP_TYPE_CPU = 0,
>>
>> And still open this, do we need it? I think you may just mention in
>> the documentation that default behaviour is CPU like.
>
> I think it's best to have a name for the default. In this case it's
> unlikely to ever be relevant, but in general one could imagine stuff
> like
>
> #ifdef THIS_OR_THAT
> #define MY_DUMP_TYPE DUMP_TYPE_LE
> #else
> #define MY_DUMP_TYPE DUMP_TYPE_CPU
> #endif
>
> which is a lot more readable than if the latter was a naked 0.

Point taken.

Though _CPU suggests user to think if it's equivalent to BE or LE. I'm
wondering if _NATIVE is better?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err()

[Joe Perches]

On Wed, 2015-12-09 at 22:36 +0200, Andy Shevchenko wrote:
[]
> wondering if _NATIVE is better?

I think CPU is better myself and it's already
in use like:

include/linux/iio/iio.h-enum iio_endian {
include/linux/iio/iio.h:	IIO_CPU,
include/linux/iio/iio.h-	IIO_BE,
include/linux/iio/iio.h-	IIO_LE,

[PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions

[Joe Perches]

There are use cases for dumping buffers with specific endian types
for 2, 4, and 8 byte variables in arrays.

Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.

Rename groupsize to groupflags in the functions and add documentation
to the kernel-doc to describe the use of the DUMP_TYPE_ enum.

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/linux/printk.h | 23 +++++++++++++++--------
 lib/hexdump.c          | 49 ++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 53 insertions(+), 19 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 9729565..aac2943 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -424,12 +424,19 @@ enum {
 	DUMP_PREFIX_ADDRESS,
 	DUMP_PREFIX_OFFSET
 };
+
+enum {
+	DUMP_TYPE_CPU = 0,
+	DUMP_TYPE_LE = BIT(30),
+	DUMP_TYPE_BE = BIT(31)
+};
+
 extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
-			      int groupsize, char *linebuf, size_t linebuflen,
+			      int groupflags, char *linebuf, size_t linebuflen,
 			      bool ascii);
 #ifdef CONFIG_PRINTK
 extern void print_hex_dump(const char *level, const char *prefix_str,
-			   int prefix_type, int rowsize, int groupsize,
+			   int prefix_type, int rowsize, int groupflags,
 			   const void *buf, size_t len, bool ascii);
 #if defined(CONFIG_DYNAMIC_DEBUG)
 #define print_hex_dump_bytes(prefix_str, prefix_type, buf, len)	\
@@ -440,7 +447,7 @@ extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
 #endif /* defined(CONFIG_DYNAMIC_DEBUG) */
 #else
 static inline void print_hex_dump(const char *level, const char *prefix_str,
-				  int prefix_type, int rowsize, int groupsize,
+				  int prefix_type, int rowsize, int groupflags,
 				  const void *buf, size_t len, bool ascii)
 {
 }
@@ -453,17 +460,17 @@ static inline void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
 
 #if defined(CONFIG_DYNAMIC_DEBUG)
 #define print_hex_dump_debug(prefix_str, prefix_type, rowsize,	\
-			     groupsize, buf, len, ascii)	\
+			     groupflags, buf, len, ascii)	\
 	dynamic_hex_dump(prefix_str, prefix_type, rowsize,	\
-			 groupsize, buf, len, ascii)
+			 groupflags, buf, len, ascii)
 #elif defined(DEBUG)
 #define print_hex_dump_debug(prefix_str, prefix_type, rowsize,		\
-			     groupsize, buf, len, ascii)		\
+			     groupflags, buf, len, ascii)		\
 	print_hex_dump(KERN_DEBUG, prefix_str, prefix_type, rowsize,	\
-		       groupsize, buf, len, ascii)
+		       groupflags, buf, len, ascii)
 #else
 static inline void print_hex_dump_debug(const char *prefix_str, int prefix_type,
-					int rowsize, int groupsize,
+					int rowsize, int groupflags,
 					const void *buf, size_t len, bool ascii)
 {
 }
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 992457b..f44608e 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -80,7 +80,9 @@ EXPORT_SYMBOL(bin2hex);
  * @buf: data blob to dump
  * @len: number of bytes in the @buf
  * @rowsize: number of bytes to print per line; must be 16 or 32
- * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ *  use a bitwise OR with %DUMP_TYPE_BE or %DUMP_TYPE_LE for endian conversions
+ *  of @buf, otherwise %DUMP_TYPE_CPU is used
  * @linebuf: where to put the converted data
  * @linebuflen: total size of @linebuf, including space for terminating NUL
  * @ascii: include ASCII after the hex output
@@ -105,7 +107,7 @@ EXPORT_SYMBOL(bin2hex);
  * (excluding the terminating NUL) which would have been written to the final
  * string if enough space had been available.
  */
-int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
+int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupflags,
 		       char *linebuf, size_t linebuflen, bool ascii)
 {
 	const u8 *ptr = buf;
@@ -114,6 +116,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 	int j, lx = 0;
 	int ascii_column;
 	int ret;
+	int groupsize = groupflags & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
 
 	if (rowsize != 16 && rowsize != 32)
 		rowsize = 16;
@@ -138,9 +141,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 		const u64 *ptr8 = buf;
 
 		for (j = 0; j < ngroups; j++) {
+			u64 val;
+
+			if (groupflags & DUMP_TYPE_LE)
+				val = get_unaligned_le64(ptr8 + j);
+			else if (groupflags & DUMP_TYPE_BE)
+				val = get_unaligned_be64(ptr8 + j);
+			else
+				val = get_unaligned(ptr8 + j);
 			ret = snprintf(linebuf + lx, linebuflen - lx,
-				       "%s%16.16llx", j ? " " : "",
-				       get_unaligned(ptr8 + j));
+				       "%s%16.16llx", j ? " " : "", val);
 			if (ret >= linebuflen - lx)
 				goto overflow1;
 			lx += ret;
@@ -149,9 +159,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 		const u32 *ptr4 = buf;
 
 		for (j = 0; j < ngroups; j++) {
+			u32 val;
+
+			if (groupflags & DUMP_TYPE_LE)
+				val = get_unaligned_le32(ptr4 + j);
+			else if (groupflags & DUMP_TYPE_BE)
+				val = get_unaligned_be32(ptr4 + j);
+			else
+				val = get_unaligned(ptr4 + j);
 			ret = snprintf(linebuf + lx, linebuflen - lx,
-				       "%s%8.8x", j ? " " : "",
-				       get_unaligned(ptr4 + j));
+				       "%s%8.8x", j ? " " : "", val);
 			if (ret >= linebuflen - lx)
 				goto overflow1;
 			lx += ret;
@@ -160,9 +177,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 		const u16 *ptr2 = buf;
 
 		for (j = 0; j < ngroups; j++) {
+			u16 val;
+
+			if (groupflags & DUMP_TYPE_LE)
+				val = get_unaligned_le16(ptr2 + j);
+			else if (groupflags & DUMP_TYPE_BE)
+				val = get_unaligned_be16(ptr2 + j);
+			else
+				val = get_unaligned(ptr2 + j);
 			ret = snprintf(linebuf + lx, linebuflen - lx,
-				       "%s%4.4x", j ? " " : "",
-				       get_unaligned(ptr2 + j));
+				       "%s%4.4x", j ? " " : "", val);
 			if (ret >= linebuflen - lx)
 				goto overflow1;
 			lx += ret;
@@ -216,7 +240,10 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
  * @prefix_type: controls whether prefix of an offset, address, or none
  *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
  * @rowsize: number of bytes to print per line; must be 16 or 32
- * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ *  use a bitwise OR with %DUMP_TYPE_BE or %DUMP_TYPE_LE for endian conversions
+ *  of @buf, otherwise %DUMP_TYPE_CPU is used
  * @buf: data blob to dump
  * @len: number of bytes in the @buf
  * @ascii: include ASCII after the hex output
@@ -240,7 +267,7 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
  * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c  pqrstuvwxyz{|}~.
  */
 void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
-		    int rowsize, int groupsize,
+		    int rowsize, int groupflags,
 		    const void *buf, size_t len, bool ascii)
 {
 	const u8 *ptr = buf;
@@ -254,7 +281,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 		linelen = min(remaining, rowsize);
 		remaining -= rowsize;
 
-		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
+		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupflags,
 				   linebuf, sizeof(linebuf), ascii);
 
 		switch (prefix_type) {
-- 
2.6.3.368.gf34be46

Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions

[Andrew Morton]

On Wed,  9 Dec 2015 13:59:30 -0800 Joe Perches <joe@perches.com> wrote:

> There are use cases for dumping buffers with specific endian types
> for 2, 4, and 8 byte variables in arrays.
> 
> Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
> 
> Rename groupsize to groupflags in the functions and add documentation
> to the kernel-doc to describe the use of the DUMP_TYPE_ enum.

What are these use cases?  Will patches be forthcoming to convert
them?

Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions

[Joe Perches]

On Wed, 2015-12-09 at 14:09 -0800, Andrew Morton wrote:
> On Wed,  9 Dec 2015 13:59:30 -0800 Joe Perches <joe@perches.com> wrote:
> 
> > There are use cases for dumping buffers with specific endian types
> > for 2, 4, and 8 byte variables in arrays.
> > 
> > Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
> > 
> > Rename groupsize to groupflags in the functions and add documentation
> > to the kernel-doc to describe the use of the DUMP_TYPE_ enum.
> 
> What are these use cases?

Buffers that contain LE or BE arrays that want
to be emitted by print_hex_dump.

> Will patches be forthcoming to convert them?

Sure, after a settling period for Masahiro's patch
to be applied upstream.

https://lkml.org/lkml/2015/12/8/480

Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions

[Andrew Morton]

On Wed, 09 Dec 2015 14:16:36 -0800 Joe Perches <joe@perches.com> wrote:

> On Wed, 2015-12-09 at 14:09 -0800, Andrew Morton wrote:
> > On Wed,____9 Dec 2015 13:59:30 -0800 Joe Perches <joe@perches.com> wrote:
> > 
> > > There are use cases for dumping buffers with specific endian types
> > > for 2, 4, and 8 byte variables in arrays.
> > > 
> > > Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
> > > 
> > > Rename groupsize to groupflags in the functions and add documentation
> > > to the kernel-doc to describe the use of the DUMP_TYPE_ enum.
> > 
> > What are these use cases?
> 
> Buffers that contain LE or BE arrays that want
> to be emitted by print_hex_dump.

Make that "where are".

> > Will patches be forthcoming to convert them?
> 
> Sure, after a settling period for Masahiro's patch
> to be applied upstream.
> 
> https://lkml.org/lkml/2015/12/8/480

One single callsite under #ifdef DEBUG?  Doesn't seem to warrant adding
code and complexity to core library functions.

Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions

[Joe Perches]

On Wed, 2015-12-09 at 14:45 -0800, Andrew Morton wrote:
> On Wed, 09 Dec 2015 14:16:36 -0800 Joe Perches <joe@perches.com> wrote:
> > > Will patches be forthcoming to convert them?
> > Sure, after a settling period for Masahiro's patch
> > to be applied upstream.
> > 
> > https://lkml.org/lkml/2015/12/8/480
> 
> One single callsite under #ifdef DEBUG?  Doesn't seem to warrant adding
> code and complexity to core library functions.

It was just a for-instance.

It's pretty trivial code and it's quite likely there
are other buffers that could be emitted that way.

e.g.: look at code like:

drivers/message/fusion/mptdebug.h
drivers/infiniband/hw/mthca/mthca_mr.c

Dunno, I didn't look very hard.

Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions

[Andy Shevchenko]

On Wed, Dec 9, 2015 at 11:59 PM, Joe Perches <joe@perches.com> wrote:
> There are use cases for dumping buffers with specific endian types
> for 2, 4, and 8 byte variables in arrays.
>
> Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
>
> Rename groupsize to groupflags in the functions and add documentation
> to the kernel-doc to describe the use of the DUMP_TYPE_ enum.

One comment below and my tag
Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  include/linux/printk.h | 23 +++++++++++++++--------
>  lib/hexdump.c          | 49 ++++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 53 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h

> index 9729565..aac2943 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -424,12 +424,19 @@ enum {
>         DUMP_PREFIX_ADDRESS,
>         DUMP_PREFIX_OFFSET
>  };
> +
> +enum {
> +       DUMP_TYPE_CPU = 0,
> +       DUMP_TYPE_LE = BIT(30),
> +       DUMP_TYPE_BE = BIT(31)
> +};
> +
>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> -                             int groupsize, char *linebuf, size_t linebuflen,
> +                             int groupflags, char *linebuf, size_t linebuflen,
>                               bool ascii);
>  #ifdef CONFIG_PRINTK
>  extern void print_hex_dump(const char *level, const char *prefix_str,
> -                          int prefix_type, int rowsize, int groupsize,
> +                          int prefix_type, int rowsize, int groupflags,
>                            const void *buf, size_t len, bool ascii);
>  #if defined(CONFIG_DYNAMIC_DEBUG)
>  #define print_hex_dump_bytes(prefix_str, prefix_type, buf, len)        \
> @@ -440,7 +447,7 @@ extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
>  #endif /* defined(CONFIG_DYNAMIC_DEBUG) */
>  #else
>  static inline void print_hex_dump(const char *level, const char *prefix_str,
> -                                 int prefix_type, int rowsize, int groupsize,
> +                                 int prefix_type, int rowsize, int groupflags,
>                                   const void *buf, size_t len, bool ascii)
>  {
>  }
> @@ -453,17 +460,17 @@ static inline void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
>
>  #if defined(CONFIG_DYNAMIC_DEBUG)
>  #define print_hex_dump_debug(prefix_str, prefix_type, rowsize, \
> -                            groupsize, buf, len, ascii)        \
> +                            groupflags, buf, len, ascii)       \
>         dynamic_hex_dump(prefix_str, prefix_type, rowsize,      \
> -                        groupsize, buf, len, ascii)
> +                        groupflags, buf, len, ascii)
>  #elif defined(DEBUG)
>  #define print_hex_dump_debug(prefix_str, prefix_type, rowsize,         \
> -                            groupsize, buf, len, ascii)                \
> +                            groupflags, buf, len, ascii)               \
>         print_hex_dump(KERN_DEBUG, prefix_str, prefix_type, rowsize,    \
> -                      groupsize, buf, len, ascii)
> +                      groupflags, buf, len, ascii)
>  #else
>  static inline void print_hex_dump_debug(const char *prefix_str, int prefix_type,
> -                                       int rowsize, int groupsize,
> +                                       int rowsize, int groupflags,
>                                         const void *buf, size_t len, bool ascii)
>  {
>  }
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 992457b..f44608e 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -80,7 +80,9 @@ EXPORT_SYMBOL(bin2hex);
>   * @buf: data blob to dump
>   * @len: number of bytes in the @buf
>   * @rowsize: number of bytes to print per line; must be 16 or 32
> - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + *  use a bitwise OR with %DUMP_TYPE_BE or %DUMP_TYPE_LE for endian conversions
> + *  of @buf, otherwise %DUMP_TYPE_CPU is used
>   * @linebuf: where to put the converted data
>   * @linebuflen: total size of @linebuf, including space for terminating NUL
>   * @ascii: include ASCII after the hex output
> @@ -105,7 +107,7 @@ EXPORT_SYMBOL(bin2hex);
>   * (excluding the terminating NUL) which would have been written to the final
>   * string if enough space had been available.
>   */
> -int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
> +int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupflags,
>                        char *linebuf, size_t linebuflen, bool ascii)
>  {
>         const u8 *ptr = buf;
> @@ -114,6 +116,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>         int j, lx = 0;
>         int ascii_column;
>         int ret;
> +       int groupsize = groupflags & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);
>
>         if (rowsize != 16 && rowsize != 32)
>                 rowsize = 16;
> @@ -138,9 +141,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>                 const u64 *ptr8 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u64 val;
> +
> +                       if (groupflags & DUMP_TYPE_LE)
> +                               val = get_unaligned_le64(ptr8 + j);
> +                       else if (groupflags & DUMP_TYPE_BE)
> +                               val = get_unaligned_be64(ptr8 + j);
> +                       else
> +                               val = get_unaligned(ptr8 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%16.16llx", j ? " " : "",
> -                                      get_unaligned(ptr8 + j));
> +                                      "%s%16.16llx", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
> @@ -149,9 +159,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>                 const u32 *ptr4 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u32 val;
> +
> +                       if (groupflags & DUMP_TYPE_LE)
> +                               val = get_unaligned_le32(ptr4 + j);
> +                       else if (groupflags & DUMP_TYPE_BE)
> +                               val = get_unaligned_be32(ptr4 + j);
> +                       else
> +                               val = get_unaligned(ptr4 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%8.8x", j ? " " : "",
> -                                      get_unaligned(ptr4 + j));
> +                                      "%s%8.8x", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
> @@ -160,9 +177,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>                 const u16 *ptr2 = buf;
>
>                 for (j = 0; j < ngroups; j++) {
> +                       u16 val;
> +
> +                       if (groupflags & DUMP_TYPE_LE)
> +                               val = get_unaligned_le16(ptr2 + j);
> +                       else if (groupflags & DUMP_TYPE_BE)
> +                               val = get_unaligned_be16(ptr2 + j);
> +                       else
> +                               val = get_unaligned(ptr2 + j);
>                         ret = snprintf(linebuf + lx, linebuflen - lx,
> -                                      "%s%4.4x", j ? " " : "",
> -                                      get_unaligned(ptr2 + j));
> +                                      "%s%4.4x", j ? " " : "", val);
>                         if (ret >= linebuflen - lx)
>                                 goto overflow1;
>                         lx += ret;
> @@ -216,7 +240,10 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>   * @prefix_type: controls whether prefix of an offset, address, or none
>   *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
>   * @rowsize: number of bytes to print per line; must be 16 or 32
> - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)

> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)

Duplicate.

> + *  use a bitwise OR with %DUMP_TYPE_BE or %DUMP_TYPE_LE for endian conversions
> + *  of @buf, otherwise %DUMP_TYPE_CPU is used
>   * @buf: data blob to dump
>   * @len: number of bytes in the @buf
>   * @ascii: include ASCII after the hex output
> @@ -240,7 +267,7 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>   * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c  pqrstuvwxyz{|}~.
>   */
>  void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> -                   int rowsize, int groupsize,
> +                   int rowsize, int groupflags,
>                     const void *buf, size_t len, bool ascii)
>  {
>         const u8 *ptr = buf;
> @@ -254,7 +281,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>                 linelen = min(remaining, rowsize);
>                 remaining -= rowsize;
>
> -               hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> +               hex_dump_to_buffer(ptr + i, linelen, rowsize, groupflags,
>                                    linebuf, sizeof(linebuf), ascii);
>
>                 switch (prefix_type) {
> --
> 2.6.3.368.gf34be46
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions

[Joe Perches]

On Thu, 2015-12-10 at 00:11 +0200, Andy Shevchenko wrote:
> On Wed, Dec 9, 2015 at 11:59 PM, Joe Perches <joe@perches.com> wrote:
[]
> > diff --git a/lib/hexdump.c b/lib/hexdump.c
[]
> > @@ -216,7 +240,10 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
> >   * @prefix_type: controls whether prefix of an offset, address, or none
> >   *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
> >   * @rowsize: number of bytes to print per line; must be 16 or 32
> > - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> > + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
> 
> Duplicate.

Ick.  Andrew, can you delete one line please or do you want a resend?

Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions

[Andy Shevchenko]

On Thu, Dec 10, 2015 at 12:11 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Dec 9, 2015 at 11:59 PM, Joe Perches <joe@perches.com> wrote:
>> There are use cases for dumping buffers with specific endian types
>> for 2, 4, and 8 byte variables in arrays.
>>
>> Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
>>
>> Rename groupsize to groupflags in the functions and add documentation
>> to the kernel-doc to describe the use of the DUMP_TYPE_ enum.
>
> One comment below and my tag
> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Couple of additional notes. (Tag still in power)

1. There is similar to print_hex_dump() function in seq_file. Separate patch?
2. What to do with ASCII part? Shall we print it as byte stream?

And one more style comment below.

>
>>
>> Signed-off-by: Joe Perches <joe@perches.com>
>> ---
>>  include/linux/printk.h | 23 +++++++++++++++--------
>>  lib/hexdump.c          | 49 ++++++++++++++++++++++++++++++++++++++-----------
>>  2 files changed, 53 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>
>> index 9729565..aac2943 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -424,12 +424,19 @@ enum {
>>         DUMP_PREFIX_ADDRESS,
>>         DUMP_PREFIX_OFFSET
>>  };
>> +
>> +enum {
>> +       DUMP_TYPE_CPU = 0,
>> +       DUMP_TYPE_LE = BIT(30),
>> +       DUMP_TYPE_BE = BIT(31)
>> +};
>> +
>>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>> -                             int groupsize, char *linebuf, size_t linebuflen,
>> +                             int groupflags, char *linebuf, size_t linebuflen,
>>                               bool ascii);
>>  #ifdef CONFIG_PRINTK
>>  extern void print_hex_dump(const char *level, const char *prefix_str,
>> -                          int prefix_type, int rowsize, int groupsize,
>> +                          int prefix_type, int rowsize, int groupflags,
>>                            const void *buf, size_t len, bool ascii);
>>  #if defined(CONFIG_DYNAMIC_DEBUG)
>>  #define print_hex_dump_bytes(prefix_str, prefix_type, buf, len)        \
>> @@ -440,7 +447,7 @@ extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
>>  #endif /* defined(CONFIG_DYNAMIC_DEBUG) */
>>  #else
>>  static inline void print_hex_dump(const char *level, const char *prefix_str,
>> -                                 int prefix_type, int rowsize, int groupsize,
>> +                                 int prefix_type, int rowsize, int groupflags,
>>                                   const void *buf, size_t len, bool ascii)
>>  {
>>  }
>> @@ -453,17 +460,17 @@ static inline void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
>>
>>  #if defined(CONFIG_DYNAMIC_DEBUG)
>>  #define print_hex_dump_debug(prefix_str, prefix_type, rowsize, \
>> -                            groupsize, buf, len, ascii)        \
>> +                            groupflags, buf, len, ascii)       \
>>         dynamic_hex_dump(prefix_str, prefix_type, rowsize,      \
>> -                        groupsize, buf, len, ascii)
>> +                        groupflags, buf, len, ascii)
>>  #elif defined(DEBUG)
>>  #define print_hex_dump_debug(prefix_str, prefix_type, rowsize,         \
>> -                            groupsize, buf, len, ascii)                \
>> +                            groupflags, buf, len, ascii)               \
>>         print_hex_dump(KERN_DEBUG, prefix_str, prefix_type, rowsize,    \
>> -                      groupsize, buf, len, ascii)
>> +                      groupflags, buf, len, ascii)
>>  #else
>>  static inline void print_hex_dump_debug(const char *prefix_str, int prefix_type,
>> -                                       int rowsize, int groupsize,
>> +                                       int rowsize, int groupflags,
>>                                         const void *buf, size_t len, bool ascii)
>>  {
>>  }
>> diff --git a/lib/hexdump.c b/lib/hexdump.c
>> index 992457b..f44608e 100644
>> --- a/lib/hexdump.c
>> +++ b/lib/hexdump.c
>> @@ -80,7 +80,9 @@ EXPORT_SYMBOL(bin2hex);
>>   * @buf: data blob to dump
>>   * @len: number of bytes in the @buf
>>   * @rowsize: number of bytes to print per line; must be 16 or 32
>> - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>> + *  use a bitwise OR with %DUMP_TYPE_BE or %DUMP_TYPE_LE for endian conversions
>> + *  of @buf, otherwise %DUMP_TYPE_CPU is used
>>   * @linebuf: where to put the converted data
>>   * @linebuflen: total size of @linebuf, including space for terminating NUL
>>   * @ascii: include ASCII after the hex output
>> @@ -105,7 +107,7 @@ EXPORT_SYMBOL(bin2hex);
>>   * (excluding the terminating NUL) which would have been written to the final
>>   * string if enough space had been available.
>>   */
>> -int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>> +int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupflags,
>>                        char *linebuf, size_t linebuflen, bool ascii)
>>  {
>>         const u8 *ptr = buf;
>> @@ -114,6 +116,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>>         int j, lx = 0;
>>         int ascii_column;
>>         int ret;
>> +       int groupsize = groupflags & ~(DUMP_TYPE_LE | DUMP_TYPE_BE);

Can you move this upper to first or second position in the definition block?

>>
>>         if (rowsize != 16 && rowsize != 32)
>>                 rowsize = 16;
>> @@ -138,9 +141,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>>                 const u64 *ptr8 = buf;
>>
>>                 for (j = 0; j < ngroups; j++) {
>> +                       u64 val;
>> +
>> +                       if (groupflags & DUMP_TYPE_LE)
>> +                               val = get_unaligned_le64(ptr8 + j);
>> +                       else if (groupflags & DUMP_TYPE_BE)
>> +                               val = get_unaligned_be64(ptr8 + j);
>> +                       else
>> +                               val = get_unaligned(ptr8 + j);
>>                         ret = snprintf(linebuf + lx, linebuflen - lx,
>> -                                      "%s%16.16llx", j ? " " : "",
>> -                                      get_unaligned(ptr8 + j));
>> +                                      "%s%16.16llx", j ? " " : "", val);
>>                         if (ret >= linebuflen - lx)
>>                                 goto overflow1;
>>                         lx += ret;
>> @@ -149,9 +159,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>>                 const u32 *ptr4 = buf;
>>
>>                 for (j = 0; j < ngroups; j++) {
>> +                       u32 val;
>> +
>> +                       if (groupflags & DUMP_TYPE_LE)
>> +                               val = get_unaligned_le32(ptr4 + j);
>> +                       else if (groupflags & DUMP_TYPE_BE)
>> +                               val = get_unaligned_be32(ptr4 + j);
>> +                       else
>> +                               val = get_unaligned(ptr4 + j);
>>                         ret = snprintf(linebuf + lx, linebuflen - lx,
>> -                                      "%s%8.8x", j ? " " : "",
>> -                                      get_unaligned(ptr4 + j));
>> +                                      "%s%8.8x", j ? " " : "", val);
>>                         if (ret >= linebuflen - lx)
>>                                 goto overflow1;
>>                         lx += ret;
>> @@ -160,9 +177,16 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
>>                 const u16 *ptr2 = buf;
>>
>>                 for (j = 0; j < ngroups; j++) {
>> +                       u16 val;
>> +
>> +                       if (groupflags & DUMP_TYPE_LE)
>> +                               val = get_unaligned_le16(ptr2 + j);
>> +                       else if (groupflags & DUMP_TYPE_BE)
>> +                               val = get_unaligned_be16(ptr2 + j);
>> +                       else
>> +                               val = get_unaligned(ptr2 + j);
>>                         ret = snprintf(linebuf + lx, linebuflen - lx,
>> -                                      "%s%4.4x", j ? " " : "",
>> -                                      get_unaligned(ptr2 + j));
>> +                                      "%s%4.4x", j ? " " : "", val);
>>                         if (ret >= linebuflen - lx)
>>                                 goto overflow1;
>>                         lx += ret;
>> @@ -216,7 +240,10 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>>   * @prefix_type: controls whether prefix of an offset, address, or none
>>   *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
>>   * @rowsize: number of bytes to print per line; must be 16 or 32
>> - * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>
>> + * @groupflags: number of bytes to print at a time (1, 2, 4, 8; default = 1)
>
> Duplicate.
>
>> + *  use a bitwise OR with %DUMP_TYPE_BE or %DUMP_TYPE_LE for endian conversions
>> + *  of @buf, otherwise %DUMP_TYPE_CPU is used
>>   * @buf: data blob to dump
>>   * @len: number of bytes in the @buf
>>   * @ascii: include ASCII after the hex output
>> @@ -240,7 +267,7 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>>   * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c  pqrstuvwxyz{|}~.
>>   */
>>  void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>> -                   int rowsize, int groupsize,
>> +                   int rowsize, int groupflags,
>>                     const void *buf, size_t len, bool ascii)
>>  {
>>         const u8 *ptr = buf;
>> @@ -254,7 +281,7 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>>                 linelen = min(remaining, rowsize);
>>                 remaining -= rowsize;
>>
>> -               hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
>> +               hex_dump_to_buffer(ptr + i, linelen, rowsize, groupflags,
>>                                    linebuf, sizeof(linebuf), ascii);
>>
>>                 switch (prefix_type) {
>> --
>> 2.6.3.368.gf34be46
>>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions

[Joe Perches]

On Thu, 2015-12-10 at 01:09 +0200, Andy Shevchenko wrote:
> On Thu, Dec 10, 2015 at 12:11 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Dec 9, 2015 at 11:59 PM, Joe Perches <joe@perches.com> wrote:
> > > There are use cases for dumping buffers with specific endian types
> > > for 2, 4, and 8 byte variables in arrays.
> > > 
> > > Add an enum for DUMP_TYPE_(LE|BE|CPU) to enable emitting them as such.
> > > 
> > > Rename groupsize to groupflags in the functions and add documentation
> > > to the kernel-doc to describe the use of the DUMP_TYPE_ enum.
> > 
> > One comment below and my tag
> > Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> Couple of additional notes. (Tag still in power)
> 
> 1. There is similar to print_hex_dump() function in seq_file. Separate patch?

Let's see if anyone wants this one first.

> 2. What to do with ASCII part? Shall we print it as byte stream?

It's still emitted as a byte stream.

> And one more style comment below.

That one's a nit I don't care about.

Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions

[kbuild test robot]

Hi Joe,

[auto build test WARNING on v4.4-rc4]
[also build test WARNING on next-20151209]

url:    https://github.com/0day-ci/linux/commits/Joe-Perches/hexdump-Add-ability-to-do-endian-conversions-in-print_hex_dump-functions/20151210-060244
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   include/linux/compiler.h:228:8: sparse: attribute 'no_sanitize_address': unknown attribute
   include/linux/printk.h:430:24: sparse: undefined identifier 'BIT'
>> include/linux/printk.h:430:27: sparse: bad constant expression type
   include/linux/printk.h:431:24: sparse: undefined identifier 'BIT'
   include/linux/printk.h:431:27: sparse: bad constant expression type
   In file included from lib/decompress.c:19:0:
   include/linux/printk.h:430:17: error: implicit declaration of function 'BIT' [-Werror=implicit-function-declaration]
     DUMP_TYPE_LE = BIT(30),
                    ^
   include/linux/printk.h:430:2: error: enumerator value for 'DUMP_TYPE_LE' is not an integer constant
     DUMP_TYPE_LE = BIT(30),
     ^
   include/linux/printk.h:432:1: error: enumerator value for 'DUMP_TYPE_BE' is not an integer constant
    };
    ^
   cc1: some warnings being treated as errors

vim +430 include/linux/printk.h

   414		printk_ratelimited(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
   415	#else
   416	#define pr_debug_ratelimited(fmt, ...) \
   417		no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
   418	#endif
   419	
   420	extern const struct file_operations kmsg_fops;
   421	
   422	enum {
   423		DUMP_PREFIX_NONE,
   424		DUMP_PREFIX_ADDRESS,
   425		DUMP_PREFIX_OFFSET
   426	};
   427	
   428	enum {
   429		DUMP_TYPE_CPU = 0,
 > 430		DUMP_TYPE_LE = BIT(30),
   431		DUMP_TYPE_BE = BIT(31)
   432	};
   433	
   434	extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
   435				      int groupflags, char *linebuf, size_t linebuflen,
   436				      bool ascii);
   437	#ifdef CONFIG_PRINTK
   438	extern void print_hex_dump(const char *level, const char *prefix_str,

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH] hexdump: Add ability to do endian conversions in print_hex_dump functions

[Joe Perches]

On Thu, 2015-12-10 at 13:55 +0800, kbuild test robot wrote:
> Hi Joe,

Hello Fengguang.

Thanks for the report.  I'll fix and resubmit.

> > > include/linux/printk.h:430:24: sparse: undefined identifier 'BIT'
> > > include/linux/printk.h:430:27: sparse: bad constant expression type