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