From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753878AbbLIUCa (ORCPT ); Wed, 9 Dec 2015 15:02:30 -0500 Received: from mail-vk0-f54.google.com ([209.85.213.54]:33435 "EHLO mail-vk0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752291AbbLIUC1 convert rfc822-to-8bit (ORCPT ); Wed, 9 Dec 2015 15:02:27 -0500 MIME-Version: 1.0 In-Reply-To: <1449689319.25389.28.camel@perches.com> References: <1449590868-19070-1-git-send-email-yamada.masahiro@socionext.com> <1449591386.3315.2.camel@perches.com> <1449594231.13275.2.camel@perches.com> <1449619439.18646.20.camel@perches.com> <1449689319.25389.28.camel@perches.com> Date: Wed, 9 Dec 2015 22:02:26 +0200 Message-ID: Subject: Re: [PATCH v2] of/address: replace printk() with pr_debug() / pr_err() From: Andy Shevchenko To: Joe Perches Cc: Rob Herring , Andrew Morton , Masahiro Yamada , "devicetree@vger.kernel.org" , Frank Rowand , "linux-kernel@vger.kernel.org" , Grant Likely , Rasmus Villemoes Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 9, 2015 at 9:28 PM, Joe Perches wrote: > On Wed, 2015-12-09 at 14:03 +0200, Andy Shevchenko wrote: >> On Wed, Dec 9, 2015 at 2:03 AM, Joe Perches wrote: >> > On Tue, 2015-12-08 at 15:28 -0600, Rob Herring wrote: >> > > On Tue, Dec 8, 2015 at 11:03 AM, Joe Perches 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