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

From: Joe Perches
Date: Wed Dec 09 2015 - 14:28:49 EST


On Wed, 2015-12-09 at 14:03 +0200, Andy Shevchenko wrote:
> On Wed, Dec 9, 2015 at 2:03 AM, Joe Perches <joe@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/